Refactor resolve_ident_in_module to use scopes #142547

b-naber · 2025-06-15T19:35:01Z

Refactoring that should help with introducing name resolution for namespaced crates.

r? @petrochenkov

compiler/rustc_resolve/src/ident.rs

eholk · 2025-06-17T18:44:16Z

I gave this a quick read and it seems cleaner than before!

compiler/rustc_resolve/src/lib.rs

compiler/rustc_resolve/src/ident.rs

petrochenkov · 2025-06-19T17:53:12Z

compiler/rustc_resolve/src/ident.rs

-        // However, it causes resolution/expansion to stuck too often (#53144), so, to make
-        // progress, we have to ignore those potential unresolved invocations from other modules
-        // and prohibit access to macro-expanded `macro_export` macros instead (unless restricted
-        // shadowing is enabled, see `macro_expanded_macro_export_errors`).


Could you make sure none of the comments is lost, like in this case?

compiler/rustc_resolve/src/ident.rs

petrochenkov · 2025-06-19T18:04:09Z

compiler/rustc_resolve/src/ident.rs

+            }
+        }
+
+        // Forbid expanded shadowing to avoid time travel.


This also indicates that the conversion to scopes wasn't done properly.
Ideally MoreExpandedVsOuter should be reported here in a similar way to how it's done in early_resolve_ident_in_lexical_scope.

Can you elaborate on what you expect this logic to look like with the change to NameResolution?

I really need to experiment on this myself to give a good answer.
Basically, this logic should be subsumed by the innermost_result checking logic in early_resolve_ident_in_lexical_scope.

And the AmbiguityKind::GlobVsExpanded logic in try_define should be subsumed by it as well.
cc #gsoc > Project: Parallel Macro Expansion @ 💬

petrochenkov · 2025-06-19T18:14:00Z

compiler/rustc_resolve/src/lib.rs

@@ -130,6 +130,13 @@ enum Scope<'ra> {
    BuiltinTypes,
 }

+/// Scopes used for resolving an `Ident` in a `Module`.
+#[derive(Debug, Copy, Clone, PartialEq)]
+enum ModuleScope {


Instead of this, I think enum Scope above should also get two variants ModuleNonGlobs and ModuleGlobs instead of one Module.

Then finding any resolution inside a module would be done using a new ScopeSet variant including just two scopes, but other ScopeSet would just iterate over ModuleNonGlobs and ModuleGlobs as a part of the common loop.

This would also be a much larger refactoring.

hm while I do think it would be more consistent to have those two scopes in the Scope enum, it would also add more complexity to it. I kind of like that the resolution of an Ident in a module is isolated with the current resolve_ident_in_module, and using a separate ModuleScope for that method simplifies things imo.

The logic in early_resolve_ident_in_lexical_scope should see the explicit resolution and the glob resolution in a module as two separate steps.
So either Scope::Module should be split into two scopes, or resolve_ident_in_module_unadjusted should somehow produce the both resolutions so that early_resolve_ident_in_lexical_scope could process them as if they were from two separate scopes.

I'm open to try to implement this, but I still don't understand why this is necessary. Can you explain in more detail, please?

Pretty much the same answer as in #142547 (comment).
Sorry, I'll try to produce some more details later.

compiler/rustc_resolve/src/ident.rs

petrochenkov · 2025-06-19T18:19:51Z

I think this is a small step in the right direction, but it probably doesn't move us much closer to implementing the open namespace proposal, splitting modules into scopes properly is a large refactoring that only partially intersects with open namespaces.

Also not sure if landing just this part of the refactoring is beneficial.
Extracting finalize_module_binding and single_import_can_define_name are certainly good though.

b-naber · 2025-06-23T13:20:39Z

Don't understand the CI failure. It seems to checkout old code, the line that fails isn't on this branch.

edit: Should have rebased and run a check.

b-naber · 2025-06-23T13:22:39Z

I changed NameResolution to contain non_glob_binding and glob_binding. This causes some changes to diagnostics when an existing non-glob resolution fails. I think this is expected though.

…ding and glob_binding

petrochenkov · 2025-06-24T17:40:19Z

Could you move the (non_)glob_binding change, and the finalize_module_binding/single_import_can_define_name extraction to separate PRs?
To avoid mixing changes that can be merged soon and other changes that need unclear amount of work.

b-naber · 2025-06-25T13:21:32Z

I'd prefer if you would review the changes to NameResolution here. There's really no urgency to merge those two method extraction changes imo, and I can always pull them out later if the NameResolution change isn't feasible.

The introduction of ModuleScope also doesn't change any semantics, but if you believe that negatively affects the review of the NameResolution change, then I'd split those changes out. I would assume you wouldn't want to merge the change to ModuelScope when splitting the PRs though, right?

petrochenkov · 2025-07-08T18:29:48Z

I don't have anything new to add now besides repeating #142547 (comment) and #142547 (comment).
@rustbot author

rustbot · 2025-07-08T18:29:53Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

b-naber · 2025-07-08T20:42:36Z

Since that causes me extra work, I would like to at least get a justification for why that is necessary. The (non_)glob-part of this re-factoring is simplified by introducing ModuleScope. If I were to create a separate PR for the (non_)glob_binding change, then that PR would also contain ModuleScope (and the methods that would be extracted to another PR). I also don't see the urgency for extracting those two methods to a separate PR.

As I previously said, I would be open to remove ModuleScope and include it in Scope if that were necessary. I would like to get an argument for that before making that change, however. I understand that you would need to play around with the code in order to give definitive answers for that, but that's also possible with the PR in its current form, and as mentioned, a separate PR for the (non_)glob_binding chang wouldn't look any different from this PR.

petrochenkov · 2025-07-08T20:55:06Z

@LorrensP-2158466 will do #142547 (comment).
We'll need to somehow partition fn resolve_ident_in_module_unadjusted into mutable/immutable speculative/non-speculative parts anyway as a part of import/macro resolution parallelization project.

bors · 2025-07-10T13:40:12Z

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

…extraction, r=petrochenkov Resolve refactor: extraction of `finalize_module_binding` and `single_import_can_define_name` This pr the work Vadim asked for in rust-lang#142547 (comment). This part: > finalize_module_binding/single_import_can_define_name extraction Cherry-picked commits of b-naber. Extraction of 2 processes in `resolve_ident_in_module_unadjusted`: - `finalize_module_binding` - `single_import_can_define_name` r? `@petrochenkov`

…extraction, r=petrochenkov Resolve refactor: extraction of `finalize_module_binding` and `single_import_can_define_name` This pr the work Vadim asked for in rust-lang#142547 (comment). This part: > finalize_module_binding/single_import_can_define_name extraction Cherry-picked commits of b-naber. Extraction of 2 processes in `resolve_ident_in_module_unadjusted`: - `finalize_module_binding` - `single_import_can_define_name` r? ``@petrochenkov``

…extraction, r=petrochenkov Resolve refactor: extraction of `finalize_module_binding` and `single_import_can_define_name` This pr the work Vadim asked for in rust-lang#142547 (comment). This part: > finalize_module_binding/single_import_can_define_name extraction Cherry-picked commits of b-naber. Extraction of 2 processes in `resolve_ident_in_module_unadjusted`: - `finalize_module_binding` - `single_import_can_define_name` r? ```@petrochenkov```

Rollup merge of #143728 - LorrensP-2158466:refactor-resolve-extraction, r=petrochenkov Resolve refactor: extraction of `finalize_module_binding` and `single_import_can_define_name` This pr the work Vadim asked for in #142547 (comment). This part: > finalize_module_binding/single_import_can_define_name extraction Cherry-picked commits of b-naber. Extraction of 2 processes in `resolve_ident_in_module_unadjusted`: - `finalize_module_binding` - `single_import_can_define_name` r? ```@petrochenkov```

…resolution-bindings, r=petrochenkov Refactor resolve resolution bindings This pr does the work asked in rust-lang#142547 (comment). This part: > move the `(non)_glob_binding` change r? `@petrochenkov`

rustbot assigned petrochenkov Jun 15, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 15, 2025

eholk reviewed Jun 17, 2025

View reviewed changes

compiler/rustc_resolve/src/ident.rs Show resolved Hide resolved

eholk reviewed Jun 17, 2025

View reviewed changes

compiler/rustc_resolve/src/ident.rs Outdated Show resolved Hide resolved

rust-cloud-vms bot force-pushed the refactor-resolve-ident-module branch from f6807eb to f9a6e87 Compare June 18, 2025 11:07

petrochenkov reviewed Jun 19, 2025

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 Jun 19, 2025

This comment has been minimized.

Sign in to view

refactor resolve_ident_in_module to use scopes

a1d845b

rust-cloud-vms bot force-pushed the refactor-resolve-ident-module branch from 81fff03 to 7148d37 Compare June 23, 2025 13:10

This comment has been minimized.

Sign in to view

b-naber added 3 commits June 23, 2025 17:48

replace binding and shadowed_glob on NameResolution with non_glob_bin…

e9ed7ed

…ding and glob_binding

address review

f836ab1

bless tests

c0f41d4

rust-cloud-vms bot force-pushed the refactor-resolve-ident-module branch from 3beb2f8 to c0f41d4 Compare June 23, 2025 17:50

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

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 Jun 24, 2025

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

rustbot 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 Jul 8, 2025

LorrensP-2158466 mentioned this pull request Jul 10, 2025

Resolve refactor: extraction of finalize_module_binding and single_import_can_define_name #143728

Merged

LorrensP-2158466 mentioned this pull request Jul 10, 2025

Refactor resolve resolution bindings #143734

Open

Refactor resolve_ident_in_module to use scopes #142547

Are you sure you want to change the base?

Refactor resolve_ident_in_module to use scopes #142547

Conversation

b-naber commented Jun 15, 2025

Uh oh!

Uh oh!

Uh oh!

eholk commented Jun 17, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

petrochenkov commented Jun 19, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

b-naber commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

b-naber commented Jun 23, 2025

Uh oh!

This comment has been minimized.

petrochenkov commented Jun 24, 2025

Uh oh!

b-naber commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Jul 8, 2025

Uh oh!

rustbot commented Jul 8, 2025

Uh oh!

b-naber commented Jul 8, 2025

Uh oh!

petrochenkov commented Jul 8, 2025

Uh oh!

bors commented Jul 10, 2025

Uh oh!

Uh oh!

petrochenkov Jun 19, 2025 •

edited

Loading

b-naber commented Jun 23, 2025 •

edited

Loading

b-naber commented Jun 25, 2025 •

edited

Loading