-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
resolve: Split Scope::Module into two scopes for non-glob and glob bindings
#149681
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.
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
Conversation
|
r? @davidtwco rustbot has assigned @davidtwco. Use |
|
There may be minor breakage in corner cases due to ambiguity checking being unified between in-scope resolution and in-module resolution, so this needs a crater run. |
This comment has been minimized.
This comment has been minimized.
resolve: Split `Scope::Module` into two scopes for non-glob and glob bindings
| && glob_binding.res() != non_glob_binding.res() | ||
| { | ||
| resolution.non_glob_binding = Some(this.new_ambiguity_binding( | ||
| AmbiguityKind::GlobVsExpanded, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yaahc you may be interested, this PR removes one of the "gray areas".
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
fdaf022 to
1b2b0da
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors try |
resolve: Split `Scope::Module` into two scopes for non-glob and glob bindings
This comment has been minimized.
This comment has been minimized.
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'm not incredibly familiar with name resolution, so you might want another review, but if this is mostly mechanical then it's up to you
This comment was marked as resolved.
This comment was marked as resolved.
…ings in the same module to the usual ambiguity infra in `resolve_ident_in_scope_set`
|
Updated with a fix for crater regressions and one more cleanup. |
It can only be `GlobVsGlob` now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me, up to you if you want another review first or another crater run
|
A second crater run is certainly not needed. |
Rollup of 3 pull requests Successful merges: - #149681 (resolve: Split `Scope::Module` into two scopes for non-glob and glob bindings ) - #150426 (Update offload test and verify that tgt_(un)register_lib have the right type) - #150678 (relate.rs: tiny cleanup: eliminate temp vars) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #149681 - petrochenkov:openapi1, r=davidtwco resolve: Split `Scope::Module` into two scopes for non-glob and glob bindings This is a re-implementation of #144131 with all the issues mentioned there fixed. As it turned out, the non-trivial part of the split was already done in c91b6ca, so the remaining part implemented in this PR is *mostly* mechanical. After addressing the issue of already found bindings being lost due to indeterminacies in outer scopes (7e890bf) and adding one missing `Stage::Late` in `Finalize` the scope splitting refactoring just worked. (One more ICE was revealed by the refactoring, but not caused by it, fixed up in the last commit.) This is a part of implementation for the [Open API](https://rust-lang.github.io/rust-project-goals/2025h1/open-namespaces.html) proposal.
|
@rust-timer build 1782736 Testing perf for #150682. |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (1782736): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.5%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 2.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 475.648s -> 477.159s (0.32%) |
|
The regressions are mostly only in incremental check builds, but they are still not completely trivial. Any ideas about what could be done to get rid of them? |
|
There are no fundamental reasons for this to be a perf regression, all the necessary regression parts were supposed to already be merged in #149454. |
|
Awesome, thank you! @rustbot label: +perf-regression-triaged |
This is a re-implementation of #144131 with all the issues mentioned there fixed.
As it turned out, the non-trivial part of the split was already done in c91b6ca, so the remaining part implemented in this PR is mostly mechanical.
After addressing the issue of already found bindings being lost due to indeterminacies in outer scopes (7e890bf) and adding one missing
Stage::LateinFinalizethe scope splitting refactoring just worked.(One more ICE was revealed by the refactoring, but not caused by it, fixed up in the last commit.)
This is a part of implementation for the Open API proposal.