Skip to content

Introduce Scope::NonGlobModule and Scope::GlobModule #144131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Introduce Scope::NonGlobModule and Scope::GlobModule #144131

wants to merge 1 commit into from
+356 −118

Conversation

b-naber
Copy link
Contributor

@b-naber b-naber commented Jul 18, 2025

Splits Scope::Module into Scope::NonGlobModule and Scope::GlobModule. This makes it easier to implement the open namespaces project, where we have to introduce a third scope. Introducing two separate scopes for a module was also a change requested by @petrochenkov, and should make it easier to output better diagnostics.

r? @petrochenkov

@rustbot 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 Jul 18, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-cloud-vms rust-cloud-vms bot force-pushed the scope-refactoring branch from f1097bc to c3185be Compare July 18, 2025 14:48
@petrochenkov petrochenkov mentioned this pull request Jul 23, 2025
Merged
petrochenkov
petrochenkov reviewed Jul 23, 2025
View reviewed changes
fmease added a commit to fmease/rust that referenced this pull request Jul 24, 2025
@fmease
Rollup merge of rust-lang#144368 - petrochenkov:rmrootscope, r=b-naber
0d4b813 
resolve: Remove `Scope::CrateRoot`

Use `Scope::Module` with the crate root module inside instead, which should be identical.
This is a simplification by itself, but it will be even larger simplification if something like rust-lang#144131 is implemented, because `Scope::CrateRoot` is also a module with two actual scopes in it (for globs and non-globs).

I also did some renamings for consistency:
- `ScopeSet::AbsolutePath` -> `ScopeSet::ModuleAndExternPrelude`
- `ModuleOrUniformRoot::CrateRootAndExternPrelude` -> `ModuleOrUniformRoot::ModuleAndExternPrelude`
- `is_absolute_path` -> `module_and_extern_prelude`
fmease added a commit to fmease/rust that referenced this pull request Jul 25, 2025
@fmease
Rollup merge of rust-lang#144368 - petrochenkov:rmrootscope, r=b-naber
8e35f2e 
resolve: Remove `Scope::CrateRoot`

Use `Scope::Module` with the crate root module inside instead, which should be identical.
This is a simplification by itself, but it will be even larger simplification if something like rust-lang#144131 is implemented, because `Scope::CrateRoot` is also a module with two actual scopes in it (for globs and non-globs).

I also did some renamings for consistency:
- `ScopeSet::AbsolutePath` -> `ScopeSet::ModuleAndExternPrelude`
- `ModuleOrUniformRoot::CrateRootAndExternPrelude` -> `ModuleOrUniformRoot::ModuleAndExternPrelude`
- `is_absolute_path` -> `module_and_extern_prelude`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 25, 2025
@matthiaskrgr
Rollup merge of rust-lang#144368 - petrochenkov:rmrootscope, r=b-naber
f713180 
resolve: Remove `Scope::CrateRoot`

Use `Scope::Module` with the crate root module inside instead, which should be identical.
This is a simplification by itself, but it will be even larger simplification if something like rust-lang#144131 is implemented, because `Scope::CrateRoot` is also a module with two actual scopes in it (for globs and non-globs).

I also did some renamings for consistency:
- `ScopeSet::AbsolutePath` -> `ScopeSet::ModuleAndExternPrelude`
- `ModuleOrUniformRoot::CrateRootAndExternPrelude` -> `ModuleOrUniformRoot::ModuleAndExternPrelude`
- `is_absolute_path` -> `module_and_extern_prelude`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 25, 2025
@matthiaskrgr
Rollup merge of rust-lang#144368 - petrochenkov:rmrootscope, r=b-naber
9c257e5 
resolve: Remove `Scope::CrateRoot`

Use `Scope::Module` with the crate root module inside instead, which should be identical.
This is a simplification by itself, but it will be even larger simplification if something like rust-lang#144131 is implemented, because `Scope::CrateRoot` is also a module with two actual scopes in it (for globs and non-globs).

I also did some renamings for consistency:
- `ScopeSet::AbsolutePath` -> `ScopeSet::ModuleAndExternPrelude`
- `ModuleOrUniformRoot::CrateRootAndExternPrelude` -> `ModuleOrUniformRoot::ModuleAndExternPrelude`
- `is_absolute_path` -> `module_and_extern_prelude`
rust-timer added a commit that referenced this pull request Jul 25, 2025
@rust-timer
Unrolled build for #144368
1a65d89 
Rollup merge of #144368 - petrochenkov:rmrootscope, r=b-naber

resolve: Remove `Scope::CrateRoot`

Use `Scope::Module` with the crate root module inside instead, which should be identical.
This is a simplification by itself, but it will be even larger simplification if something like #144131 is implemented, because `Scope::CrateRoot` is also a module with two actual scopes in it (for globs and non-globs).

I also did some renamings for consistency:
- `ScopeSet::AbsolutePath` -> `ScopeSet::ModuleAndExternPrelude`
- `ModuleOrUniformRoot::CrateRootAndExternPrelude` -> `ModuleOrUniformRoot::ModuleAndExternPrelude`
- `is_absolute_path` -> `module_and_extern_prelude`
@bors
Copy link
Collaborator

bors commented Jul 25, 2025

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

github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Jul 28, 2025
@matthiaskrgr
Rollup merge of #144368 - petrochenkov:rmrootscope, r=b-naber
e16518f 
resolve: Remove `Scope::CrateRoot`

Use `Scope::Module` with the crate root module inside instead, which should be identical.
This is a simplification by itself, but it will be even larger simplification if something like rust-lang/rust#144131 is implemented, because `Scope::CrateRoot` is also a module with two actual scopes in it (for globs and non-globs).

I also did some renamings for consistency:
- `ScopeSet::AbsolutePath` -> `ScopeSet::ModuleAndExternPrelude`
- `ModuleOrUniformRoot::CrateRootAndExternPrelude` -> `ModuleOrUniformRoot::ModuleAndExternPrelude`
- `is_absolute_path` -> `module_and_extern_prelude`
@rust-cloud-vms rust-cloud-vms bot force-pushed the scope-refactoring branch from c3185be to 9c0ee44 Compare July 28, 2025 21:02
@b-naber b-naber mentioned this pull request Jul 28, 2025
Open
3 tasks
@b-naber
Copy link
Contributor Author

b-naber commented Aug 1, 2025

@petrochenkov do you have time to review this in the next couple of days?

@petrochenkov
Copy link
Contributor

Probably after #144579, this is low priority for me.

@b-naber
Copy link
Contributor Author

b-naber commented Aug 1, 2025

I figured, I'll just re-roll then.

r? @rust-lang/compiler

@rustbot rustbot assigned SparrowLii and unassigned petrochenkov Aug 1, 2025
@petrochenkov petrochenkov self-assigned this Aug 1, 2025
@petrochenkov
Copy link
Contributor

I still need to look, this is an important part of the resolve infrastructure.

@b-naber
Copy link
Contributor Author

b-naber commented Aug 1, 2025

Other maintainers are also qualified to review this code. PR writers are allowed to re-roll reviewers after 2 weeks of inactivity; the fact that you just ignore this is kind of wild.

@Kobzol
Copy link
Member

Kobzol commented Aug 1, 2025

Vadim is the foremost expert on name resolution in the compiler, and if he wants to take a look at the PR, that is completely fine. No one is taking away your option of rerolling, but that doesn't mean that you get to choose the only reviewer. If there are other people with concerns or comments, their input also has to be taken into account, regardless of whether they are the currently assigned reviewer or not.

@RalfJung
Copy link
Member

RalfJung commented Aug 1, 2025 •
edited
Loading

(Btw you just pinged the entire compiler team, more than 50 people. The way to re-roll is r? compiler, then it only pings the chosen reviewer. But even that is unlikely to work well since it doesn't take into account expertise, unlike the initial assignment.)

@b-naber
Copy link
Contributor Author

b-naber commented Aug 1, 2025

Vadim is the foremost expert on name resolution in the compiler, and if he wants to take a look at the PR, that is completely fine. No one is taking away your option of rerolling, but that doesn't mean that you get to choose the only reviewer. If there are other people with concerns or comments, their input also has to be taken into account, regardless of whether they are the currently assigned reviewer or not.

I understand, but there must be some limit for how long someone can request to take a look at a PR; if the PR has low priority for the maintainer requesting a review, at some point there should still be a way to get this merged in a reasonable amount of time. Now obviously 2 weeks isn't anywhere close to such a limit, so I apologize for overreacting here.

@petrochenkov
Copy link
Contributor

I'm certainly going to review this in some time.
I've been doing some experiments in #144368 and https://github.com/petrochenkov/rust/tree/extprel3 to understand how to do this better.
The second branch in particular, once finished, will give a simplified example of what this change should look like eventually.

@petrochenkov
Copy link
Contributor

#144793 splits extern prelude into two scopes and demonstrates some things that need to be done for the module scope split as well.

  • Introduction of ScopeSet::ExternPrelude for doing searches just in the two extern prelude scopes, and its use in resolve_ident_in_module_unadjusted. ScopeSet::Module should also be introduced and behave similarly.
  • Replacement of the ad hoc MacroExpandedExternCrateCannotShadowExternArguments error with a regular restricted shadowing error (AmbiguityKind::MoreExpandedVsOuter), slightly specialized to produce better diagnostics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

@SparrowLii SparrowLii

Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@b-naber @rust-log-analyzer @bors @petrochenkov @Kobzol @RalfJung @rustbot @SparrowLii