Try to break resolve into more isolated parts #63400

petrochenkov · 2019-08-09T01:31:15Z

Some small step towards resolve librarification.

"Late resolution" is the pass that resolves most of names in a crate beside imports and macros.
It runs when the crate is fully expanded and its module structure is fully built.
So we just walk through the crate and resolve all the expressions, types, etc.

This pass is pretty self-contained, but it was previously done by implementing Visitor on the whole Resolver (which is used for many other tasks), and fields specific to this pass were indiscernible from the global Resolver state.

This PR moves the late resolution pass into a separate visitor and a separate file, fields specific to this visitor are moved from Resolver as well.

I'm especially happy about current_module being removed from Resolver.
It was used even for operations not related to visiting and changing the current_module position in process.
It was also used as an implicit argument for some functions used in this style

let orig_current_module = mem::replace(&mut self.current_module, module);
self.resolve_ident_somewhere();
self.current_module = orig_current_module;

and having effects on e.g. privacy checking somewhere deeply inside resolve_ident_somewhere.
Now we explicitly pass a ParentScope to those functions instead, which includes the module and some other data describing our position in the crate relatively to which we resolve names.

Rustdoc was one of the users of current_module, it set it for resolving intra-doc links.
Now it passes it explicitly as an argument as well (I also supported resolving paths from rustdoc in unnamed blocks as a drive-by fix).

Visibility resolution is also changed to use early resolution (which is correct because it's used during the work of BuildReducedGraphVisitor, i.e. integration of a new AST fragment into the existing partially built module structures.) instead of untimely late resolution (which worked only due to restrictions on paths in visibilities like inability to refer to anything except ancestor modules).
This slightly regresses its diagnostics because late resolution has a more systematic error detection and recovery currently.
Due to changes in current_module and visibilities BuildReducedGraphVisitor ended up almost as heavily affected by this refactoring as late resolution.

Fixes #63223 (due to visibility resolution changes).

petrochenkov · 2019-08-10T01:12:41Z

Actually, let's r? @matklad @Xanewok on this, nobody knows the nuances of this code anyway and this PR doesn't do many nuanced things beside perhaps some of the current_module changes.

@rust-lang/compiler @davidtwco are also welcome to review.

The PR is better reviewed in per-commit fashion.
Some of the commits are large because they move code between files and impls, but they don't do much else.

eddyb · 2019-08-10T06:21:01Z

src/librustc_resolve/build_reduced_graph.rs

@@ -395,7 +400,7 @@ impl<'a> Resolver<'a> {
                });
                self.potentially_unused_imports.push(directive);
                let imported_binding = self.import(binding, directive);
-                if ptr::eq(self.current_module, self.graph_root) {
+                if ptr::eq(parent, self.graph_root) {


Random aside, is there any chance we could compare modules by DefId or something?
Also, the way they are used (for block scopes as well), they feel a bit more like "(def) scopes" than "modules" (but we also have the other kind of scope, the Ribs, which can contain runtime variables).

Module in resolve is either a mod, or enum, or trait, or an ast::Block.
The naming seems ancient, I don't know why it was chosen, perhaps there was only mod initially, but then it got extended.

Modules and many other things (including those without IDs) are allocated on the resolver arena, so they use referential equality pretty regularly.
In this case ptr::eq is just simpler, I guess, than extracting the optional DefIds and comparing them if they exist.

Oh, wait, two different Modules can have the same DefId in cross-crate scenarios, see #57181.
I think that case cannot apply to self.graph_root specifically, though.

eddyb · 2019-08-10T06:26:05Z

@petrochenkov I'm a bit curious, what "inputs" does late resolution have, i.e. what does it use from the Resolver, and could that information be saved globally after expansion finishes?
I know early resolution and expansion are intertwined, but maybe we can make late resolution a query sooner than we otherwise could (without the early/late split).

eddyb · 2019-08-10T06:39:24Z

src/librustc_resolve/late.rs

+            (PathSource::Pat, false) | (PathSource::TupleStruct, false) => "E0531",
+            (PathSource::TraitItem(..), true) => "E0575",
+            (PathSource::TraitItem(..), false) => "E0576",
+        }


@nikomatsakis This sort of error code juggling feels very silly tbh.

eddyb · 2019-08-10T06:42:44Z

@bors r+

bors · 2019-08-10T06:42:45Z

📌 Commit 4b564f1da0fe91df0bc1022f967b556c5663bfcb has been approved by eddyb

petrochenkov · 2019-08-10T09:59:27Z

@bors p=1 (this PR conflicts with other resolve changes in the queue and is hard to rebase)

bors · 2019-08-10T10:01:49Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout resplit (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self resplit --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging src/librustc_resolve/lib.rs
CONFLICT (content): Merge conflict in src/librustc_resolve/lib.rs
Automatic merge failed; fix conflicts and then commit the result.

bors · 2019-08-10T10:13:22Z

☔ The latest upstream changes (presumably #63428) made this pull request unmergeable. Please resolve the merge conflicts.

Move `Resolver` fields specific to late resolution to the new visitor. The `current_module` field from `Resolver` is replaced with two `current_module`s in `LateResolutionVisitor` and `BuildReducedGraphVisitor`. Outside of those visitors `current_module` is replaced by passing `parent_scope` to more functions and using the parent module from it. Visibility resolution no longer have access to later resolution methods and has to use early resolution, so its diagnostics in case of errors regress slightly.

Instead of tracking current module and other components separately. (`ParentScope` includes the module as a component.)

It's now immediately clear what fields belong to the global resolver state and what are specific to passes/visitors.

Rename it to `report_error` and move into `diagnostics.rs` Also turn `check_unused` into a method on `Resolver`

Move methods logically belonging to build-reduced-graph into `impl BuildReducedGraphVisitor` and `build_reduced_graph.rs` Move types mostly specific to late resolution closer to the late resolution visitor

Cleanup some surrounding code. Support resolution of intra doc links in unnamed block scopes. (Paths from rustdoc now use early resolution and no longer need results of late resolution like all the built ribs.) Fix one test hitting file path limits on Windows.

Make the `is_import` flag in `ScopeSet` independent from namespace Fix rebase

petrochenkov · 2019-08-10T10:49:01Z

@bors r=eddyb

bors · 2019-08-10T10:49:02Z

📌 Commit 319f0de has been approved by eddyb

Centril · 2019-08-10T11:36:41Z

@bors p=20

petrochenkov · 2019-08-10T11:53:16Z

@eddyb

I'm a bit curious, what "inputs" does late resolution have, i.e. what does it use from the Resolver

Most of the resolver fields, I think?
Many of them for diagnostics only (e.g. if you want to suggest println!(...) instead of println(...) you need the macro-related tables).
The primary ones are probably Module and NameBinding data in Resolver::arenas + Resolver::module_map containing the module structure of the crate and names registered in those modules.

A few of the Resolver fields are pass-specific and can can be moved into pass-specific structures + some fields are "output only" (e.g. label_res_map), but by the most part the purpose of Resolver as a structure is to hoard data so it can then answer queries from late resolution, HIR lowering, or rustdoc.

Try to break resolve into more isolated parts Some small step towards resolve librarification. "Late resolution" is the pass that resolves most of names in a crate beside imports and macros. It runs when the crate is fully expanded and its module structure is fully built. So we just walk through the crate and resolve all the expressions, types, etc. This pass is pretty self-contained, but it was previously done by implementing `Visitor` on the whole `Resolver` (which is used for many other tasks), and fields specific to this pass were indiscernible from the global `Resolver` state. This PR moves the late resolution pass into a separate visitor and a separate file, fields specific to this visitor are moved from `Resolver` as well. I'm especially happy about `current_module` being removed from `Resolver`. It was used even for operations not related to visiting and changing the `current_module` position in process. It was also used as an implicit argument for some functions used in this style ```rust let orig_current_module = mem::replace(&mut self.current_module, module); self.resolve_ident_somewhere(); self.current_module = orig_current_module; ``` and having effects on e.g. privacy checking somewhere deeply inside `resolve_ident_somewhere`. Now we explicitly pass a `ParentScope` to those functions instead, which includes the module and some other data describing our position in the crate relatively to which we resolve names. Rustdoc was one of the users of `current_module`, it set it for resolving intra-doc links. Now it passes it explicitly as an argument as well (I also supported resolving paths from rustdoc in unnamed blocks as a drive-by fix). Visibility resolution is also changed to use early resolution (which is correct because it's used during the work of `BuildReducedGraphVisitor`, i.e. integration of a new AST fragment into the existing partially built module structures.) instead of untimely late resolution (which worked only due to restrictions on paths in visibilities like inability to refer to anything except ancestor modules). This slightly regresses its diagnostics because late resolution has a more systematic error detection and recovery currently. Due to changes in `current_module` and visibilities `BuildReducedGraphVisitor` ended up almost as heavily affected by this refactoring as late resolution. Fixes rust-lang#63223 (due to visibility resolution changes).

@ghost

Rollup of 4 pull requests Successful merges: - #63400 (Try to break resolve into more isolated parts) - #63425 (Cleanup historical stability comments) - #63429 (.gitignore: Readd `/tmp/`) - #63432 (Cleanup & Simplify stuff in lowering) Failed merges: r? @ghost

bors · 2019-08-10T17:19:18Z

⌛ Testing commit 319f0de with merge be3fb0c...

matklad · 2019-08-12T09:28:16Z

I love this so much, thank you! Could you perhaps add a short comment at the top of late.rs describing what it is? I feel like I understand this from the issue description, but, if I were just reading the code, I would be left wondering what this thing is and why there's no early.rs :)

matklad · 2019-08-12T09:33:49Z

also cc @vlad20012: this new setup might serve as an inspiration for rewriting IntelliJ Rust's name resolution :)

petrochenkov · 2019-08-13T21:28:33Z

Some file-level docs are added in #63535.

rustdoc: Add test for fixed issue rust-lang#61732 was almost certainly fixed by rust-lang#63400. Closes rust-lang#61732

rust-highfive assigned matthewjasper Aug 9, 2019

petrochenkov mentioned this pull request Aug 9, 2019

Compiler panic on a macro invocation combined with invalid pub declaration #63223

Closed

petrochenkov force-pushed the resplit branch 2 times, most recently from f5bf56d to 4b564f1 Compare August 10, 2019 00:32

petrochenkov assigned Xanewok and unassigned matthewjasper Aug 10, 2019

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2019

eddyb reviewed Aug 10, 2019

View reviewed changes

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2019

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 10, 2019

petrochenkov added 8 commits August 10, 2019 13:15

resolve: Move late resolution visitor into a separate file

ff85d1c

resolve: Track whole parent scope in the visitors

df03e42

Instead of tracking current module and other components separately. (`ParentScope` includes the module as a component.)

resolve: Remove Deref<Target=Resolver> implementations

6a347f3

It's now immediately clear what fields belong to the global resolver state and what are specific to passes/visitors.

resolve: Turn resolve_error into a method on Resolver

9c86ce7

Rename it to `report_error` and move into `diagnostics.rs` Also turn `check_unused` into a method on `Resolver`

resolve: Move some more code around

f360d79

Move methods logically belonging to build-reduced-graph into `impl BuildReducedGraphVisitor` and `build_reduced_graph.rs` Move types mostly specific to late resolution closer to the late resolution visitor

resolve: Address FIXME from the previous commit

319f0de

Make the `is_import` flag in `ScopeSet` independent from namespace Fix rebase

petrochenkov force-pushed the resplit branch from 4b564f1 to 319f0de Compare August 10, 2019 10:48

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 Aug 10, 2019

This was referenced Aug 10, 2019

resolve: Populate external modules in more automatic and lazy way #63149

Merged

Suggest using a qualified path in patterns with inconsistent bindings #63406

Merged

diagnostics: Describe crate root modules in DefKind::Mod as "crate" #63250

Merged

Centril mentioned this pull request Aug 10, 2019

Rollup of 4 pull requests #63437

Merged

bors merged commit 319f0de into rust-lang:master Aug 10, 2019

mark-i-m mentioned this pull request Aug 12, 2019

Fix broken link rust-lang/rustc-dev-guide#421

Merged

This was referenced Aug 25, 2019

Crater runs for Rust 1.38.0 #63628

Closed

[1.38] Regression: expected module, found unresolved item crate::mod #63891

Closed

Skepfyr mentioned this pull request Oct 4, 2019

src/librustc/hir/def.rs:271: attempted .def_id() on invalid def: Err #61732

Closed

This was referenced Dec 4, 2019

ICE when cargo doc on lexical-core: attempted .def_id() on invalid res: Err #64705

Closed

rustdoc: Add test for fixed issue #67008

Merged

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 6, 2019

Rollup merge of rust-lang#67008 - ollie27:rustdoc_issue_61732, r=Centril

0df1609

rustdoc: Add test for fixed issue rust-lang#61732 was almost certainly fixed by rust-lang#63400. Closes rust-lang#61732

jyn514 added A-resolve Area: Name resolution T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to break resolve into more isolated parts #63400

Try to break resolve into more isolated parts #63400

petrochenkov commented Aug 9, 2019 •

edited

Loading

petrochenkov commented Aug 10, 2019

eddyb Aug 10, 2019

petrochenkov Aug 10, 2019

petrochenkov Aug 10, 2019 •

edited

Loading

eddyb commented Aug 10, 2019

eddyb Aug 10, 2019

eddyb commented Aug 10, 2019

bors commented Aug 10, 2019

petrochenkov commented Aug 10, 2019

bors commented Aug 10, 2019

bors commented Aug 10, 2019

petrochenkov commented Aug 10, 2019

bors commented Aug 10, 2019

Centril commented Aug 10, 2019

petrochenkov commented Aug 10, 2019

bors commented Aug 10, 2019

matklad commented Aug 12, 2019

matklad commented Aug 12, 2019 •

edited

Loading

petrochenkov commented Aug 13, 2019

Try to break resolve into more isolated parts #63400

Try to break resolve into more isolated parts #63400

Conversation

petrochenkov commented Aug 9, 2019 • edited Loading

petrochenkov commented Aug 10, 2019

eddyb Aug 10, 2019

Choose a reason for hiding this comment

petrochenkov Aug 10, 2019

Choose a reason for hiding this comment

petrochenkov Aug 10, 2019 • edited Loading

Choose a reason for hiding this comment

eddyb commented Aug 10, 2019

eddyb Aug 10, 2019

Choose a reason for hiding this comment

eddyb commented Aug 10, 2019

bors commented Aug 10, 2019

petrochenkov commented Aug 10, 2019

bors commented Aug 10, 2019

bors commented Aug 10, 2019

petrochenkov commented Aug 10, 2019

bors commented Aug 10, 2019

Centril commented Aug 10, 2019

petrochenkov commented Aug 10, 2019

bors commented Aug 10, 2019

matklad commented Aug 12, 2019

matklad commented Aug 12, 2019 • edited Loading

petrochenkov commented Aug 13, 2019

petrochenkov commented Aug 9, 2019 •

edited

Loading

petrochenkov Aug 10, 2019 •

edited

Loading

matklad commented Aug 12, 2019 •

edited

Loading