Some comments and documentation for name resolution crate #48693

vorner · 2018-03-03T10:20:35Z

Hello

I'm trying to get a grasp of how the name resolution crate works, as part of helping with rust-lang/rustc-dev-guide#16. Not that I'd be succeeding much, but as I was reading the code, I started to put some notes into it, to help me understand.

I guess I didn't get very far yet, but I'd like to share what I have, in case it might be useful for someone else. I hope these are correct (even if incomplete), but I'll be glad for a fast check in case I put something misleading there.

rust-highfive · 2018-03-03T10:20:46Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

pietroalbini · 2018-03-12T10:45:23Z

Thanks for this PR @vorner! We'll make sure someone from @rust-lang/compiler reviews this PR!

(@pnkfelix, ping from triage!)

petrochenkov · 2018-03-12T10:50:28Z

Sorry, missed this. Will review.

oli-obk · 2018-03-12T10:47:53Z

src/librustc_resolve/lib.rs

@@ -662,6 +670,7 @@ impl<'tcx> Visitor<'tcx> for UsePlacementFinder {
    }
 }

+// This thing walks the whole crate in DFS manner, visiting each item, resolving names as it goes.


You can use doc comments. They'll show up on the impl in the docs.

oli-obk · 2018-03-12T10:49:08Z

src/librustc_resolve/lib.rs

+/// Different [rib kinds](enum.RibKind) are transparent for different names.
+///
+/// The resolution keeps a separate stack of ribs as it traverses the AST for each namespace. When
+/// resolving, the name is looked from inside out.


nit: looked up

oli-obk · 2018-03-12T10:50:35Z

src/librustc_resolve/lib.rs

@@ -1122,7 +1152,8 @@ impl<'a> NameBinding<'a> {
        resolver.get_macro(self.def_ignoring_ambiguity())
    }

-    // We sometimes need to treat variants as `pub` for backwards compatibility
+    // We sometimes need to treat variants as `pub` for backwards compatibility (eg. in enum
+    // variants)


This change doesn't seem to clarify more. variants are already mentioned

oli-obk · 2018-03-12T10:50:58Z

src/librustc_resolve/lib.rs

    }
 }

+// This is the interface through which the rest of the compiler asks about name resolution after
+// the whole AST has been indexed.


again, this can be a doc comment

vorner · 2018-03-12T20:21:47Z

Good to know I can put a comment on trait implementation, I always thought it shows the description of the trait.

Should I squash the fixup once it looks good?

oli-obk · 2018-03-12T21:10:58Z

Yes, please squash the fixup. Lgtm now

jseyfried · 2018-03-12T23:02:45Z

src/librustc_resolve/lib.rs

@@ -909,7 +935,11 @@ enum PathResult<'a> {
 }

 enum ModuleKind {
+    /// Inline `mod something { ... }`.


ModuleKind::Block is an anonymous module, i.e. a block:

fn main() { fn f() {} // (1) { f(); // This resolves to (2) fn f() {} // (2) } }

Whether a module is inline or not has no effect on name resolution.

jseyfried · 2018-03-12T23:04:01Z

src/librustc_resolve/lib.rs

    Block(NodeId),
+    /// Module from another file.
+    ///
+    /// Also called a normal module in the following code.


A normal module is a mod module (inline or not). ModuleKind::Def could also be a trait or an enum.

jseyfried · 2018-03-12T23:04:43Z

src/librustc_resolve/lib.rs

@@ -1359,6 +1394,7 @@ pub struct Resolver<'a> {
    injected_crate: Option<Module<'a>>,
 }

+/// Nothing really interesting here, it just provides memory for the rest of the crate.


nit: memory -> arenas

I think it is obvious it contains arenas, my point was that these arenas are there to allocate memory. Should I still change it?

jseyfried · 2018-03-12T23:11:54Z

src/librustc_resolve/lib.rs

    }
 }

+/// This is the interface through which the rest of the compiler asks about name resolution after
+/// the whole AST has been indexed.


hir::lowering::Resolver is used to resolve HIR paths generated due to desugaring during AST->HIR lowering, and to look up the resolution AST paths during lowering (get_resolution). After lowering, resolution information is embedded in the HIR; this is what the rest of the compiler uses. The Resolver is destroyed shortly after lowering.

petrochenkov · 2018-03-14T18:05:07Z

src/librustc_resolve/lib.rs

@@ -864,6 +875,16 @@ enum RibKind<'a> {
 }

 /// One local scope.
+///
+/// A rib represents a scope names can live in. Note that these appear in many places, not just
+/// around braces. At any place where the list of accessible names (of the given namespace)


It's probably better to say it more explicitly - the list of accessible names changes or new restrictions on name accessibility are added.

petrochenkov · 2018-03-14T18:08:06Z

r=me after addressing the comments

shepmaster · 2018-03-23T15:17:56Z

Ping from triage @vorner ! You have pending feedback to address; will you be able to address it soon?

vorner · 2018-03-23T15:50:30Z

I know, I keep track of it. It seems the comments will need some more research on my side, so I postponed it. I hope to find the time during this weekend.

vorner · 2018-03-24T16:09:20Z

OK, added one more fixup commit.

petrochenkov · 2018-03-25T22:51:14Z

@bors r+ rollup

bors · 2018-03-25T22:51:15Z

📌 Commit 433a03e has been approved by petrochenkov

…chenkov Some comments and documentation for name resolution crate Hello I'm trying to get a grasp of how the name resolution crate works, as part of helping with rust-lang/rustc-dev-guide#16. Not that I'd be succeeding much, but as I was reading the code, I started to put some notes into it, to help me understand. I guess I didn't get very far yet, but I'd like to share what I have, in case it might be useful for someone else. I hope these are correct (even if incomplete), but I'll be glad for a fast check in case I put something misleading there.

Rollup of 7 pull requests - Successful merges: #48693, #48932, #49103, #49170, #49187, #49346, #49353 - Failed merges:

rust-highfive assigned pnkfelix Mar 3, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 3, 2018

petrochenkov assigned petrochenkov and unassigned pnkfelix Mar 12, 2018

oli-obk requested changes Mar 12, 2018

View reviewed changes

oli-obk approved these changes Mar 12, 2018

View reviewed changes

Some comments and documentation for name resolution crate

68d6edd

jseyfried reviewed Mar 12, 2018

View reviewed changes

petrochenkov reviewed Mar 14, 2018

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2018

fixup! Some comments and documentation for name resolution crate

433a03e

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 25, 2018

kennytm mentioned this pull request Mar 26, 2018

Rollup of 8 pull requests #49374

Closed

TimNN mentioned this pull request Mar 26, 2018

Rollup of 7 pull requests #49379

Merged

bors added a commit that referenced this pull request Mar 26, 2018

Auto merge of #49379 - TimNN:rollup, r=TimNN

ab8b961

Rollup of 7 pull requests - Successful merges: #48693, #48932, #49103, #49170, #49187, #49346, #49353 - Failed merges:

bors merged commit 433a03e into rust-lang:master Mar 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some comments and documentation for name resolution crate #48693

Some comments and documentation for name resolution crate #48693

vorner commented Mar 3, 2018

rust-highfive commented Mar 3, 2018

pietroalbini commented Mar 12, 2018

petrochenkov commented Mar 12, 2018

oli-obk Mar 12, 2018

oli-obk Mar 12, 2018

oli-obk Mar 12, 2018

oli-obk Mar 12, 2018

vorner commented Mar 12, 2018

oli-obk commented Mar 12, 2018

jseyfried Mar 12, 2018 •

edited

Loading

jseyfried Mar 12, 2018

jseyfried Mar 12, 2018

vorner Mar 24, 2018

jseyfried Mar 12, 2018

petrochenkov Mar 14, 2018

petrochenkov commented Mar 14, 2018

shepmaster commented Mar 23, 2018

vorner commented Mar 23, 2018

vorner commented Mar 24, 2018

petrochenkov commented Mar 25, 2018

bors commented Mar 25, 2018

Some comments and documentation for name resolution crate #48693

Some comments and documentation for name resolution crate #48693

Conversation

vorner commented Mar 3, 2018

rust-highfive commented Mar 3, 2018

pietroalbini commented Mar 12, 2018

petrochenkov commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vorner commented Mar 12, 2018

oli-obk commented Mar 12, 2018

jseyfried Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Mar 14, 2018

shepmaster commented Mar 23, 2018

vorner commented Mar 23, 2018

vorner commented Mar 24, 2018

petrochenkov commented Mar 25, 2018

bors commented Mar 25, 2018

jseyfried Mar 12, 2018 •

edited

Loading