Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Remove last uses of gensyms #64623
Merged
Remove last uses of gensyms #64623
Conversation
matthewjasper
force-pushed the
matthewjasper:underscore-imports
branch
from
90eef3e
to
da01d31
| @@ -584,7 +588,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { | |||
| let parent_scope = &self.parent_scope; | |||
| let parent = parent_scope.module; | |||
| let expansion = parent_scope.expansion; | |||
| let ident = item.ident.gensym_if_underscore(); | |||
petrochenkov
Sep 21, 2019
Contributor
Hmm, I'd expect every _ name to get SyntaxContext from a fresh macro expansion with def-site in empty_module, like it was done for other gensyms in #63919.
Thus we'll reuse the same common mechanism, and when hygiene data becomes encodable we'll be able to properly encode _s in metadata as well, thus resolving the "FIXME: We shouldn't create the gensym here" below.
Hmm, I'd expect every _ name to get SyntaxContext from a fresh macro expansion with def-site in empty_module, like it was done for other gensyms in #63919.
Thus we'll reuse the same common mechanism, and when hygiene data becomes encodable we'll be able to properly encode _s in metadata as well, thus resolving the "FIXME: We shouldn't create the gensym here" below.
matthewjasper
Sep 21, 2019
Author
Contributor
Glob imports operate on the actual hygiene data:
rust/src/librustc_resolve/resolve_imports.rs
Line 563
in
da01d31
I'll see if there's a way to do this without increasing the complexity of reverse_glob_adjust
Glob imports operate on the actual hygiene data:
rust/src/librustc_resolve/resolve_imports.rs
Line 563 in da01d31
I'll see if there's a way to do this without increasing the complexity of reverse_glob_adjust
petrochenkov
Sep 22, 2019
Contributor
Ah, right, the difference is that _s bring traits into scope for the purpose of type-based resolution, and names from opaque macros don't.
This difference is orthogonal to globs, so perhaps it's collection of traits in scope that needs to be adjusted, not reverse_glob_adjust.
(It's somewhat unfortunate that the difference exists and the features cannot be fully unified, but in both cases the behavior looks entirely reasonable and appropriate.)
Ah, right, the difference is that _s bring traits into scope for the purpose of type-based resolution, and names from opaque macros don't.
This difference is orthogonal to globs, so perhaps it's collection of traits in scope that needs to be adjusted, not reverse_glob_adjust.
(It's somewhat unfortunate that the difference exists and the features cannot be fully unified, but in both cases the behavior looks entirely reasonable and appropriate.)
matthewjasper
Sep 22, 2019
Author
Contributor
I'm specifically concerned about preserving the behavior of the following:
mod x {
use Trait as _;
}
use x::*;
fn f() {
().method_on_trait();
// ^This resolves if the whole snippet comes from a single macro expansion
// It generally doesn't resolve otherwise
}
I think I have a working solution for this, which I'll push once I'm happy.
I'm specifically concerned about preserving the behavior of the following:
mod x {
use Trait as _;
}
use x::*;
fn f() {
().method_on_trait();
// ^This resolves if the whole snippet comes from a single macro expansion
// It generally doesn't resolve otherwise
}I think I have a working solution for this, which I'll push once I'm happy.
|
I'd be interested to see a perf run on this before it lands, see if it improves things. |
matthewjasper
force-pushed the
matthewjasper:underscore-imports
branch
2 times, most recently
from
06e5858
to
50660a1
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
bors
added a commit
that referenced
this pull request
Sep 28, 2019
Remove last uses of gensyms Bindings are now indexed in resolve with an additional disambiguator that's used for underscore bindings. This is the last use of gensyms in the compiler. I'm not completely happy with this approach, so suggestions are welcome. Moving undescore bindings into their own map didn't turn out any better: master...matthewjasper:remove-underscore-gensyms. closes #49300 cc #60869 r? @petrochenkov
|
|
|
Queued a9bcdfe with parent f3c8eba, future comparison URL. |
|
Finished benchmarking try commit a9bcdfe, comparison URL. |
|
Having a quick look at unused-warnings, I think this is caused by the sorting of reexports. I'll investigate properly tomorrow. |
|
|
matthewjasper
force-pushed the
matthewjasper:underscore-imports
branch
from
50660a1
to
0e1c0b5
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
bors
added a commit
that referenced
this pull request
Sep 29, 2019
Remove last uses of gensyms Bindings are now indexed in resolve with an additional disambiguator that's used for underscore bindings. This is the last use of gensyms in the compiler. I'm not completely happy with this approach, so suggestions are welcome. Moving undescore bindings into their own map didn't turn out any better: master...matthewjasper:remove-underscore-gensyms. closes #49300 cc #60869 r? @petrochenkov
|
|
|
Queued 3498eec with parent d046ffd, future comparison URL. |
|
Finished benchmarking try commit 3498eec, comparison URL. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Ping from triage |
|
I didn't forget about this, just need some continuous period of time to focus on this. |
|
So, I still want to pursue the approach with underscores mostly being identifiers with def-site spans produced by unique macros defined in the empty module, with all special casing done for code collecting traits in scope (the only detail in which the underscores are different). That's why I started looking at the existing code determining the set of traits in scope and I didn't like what I had seen. |
|
@matthewjasper |
matthewjasper
force-pushed the
matthewjasper:underscore-imports
branch
from
bc38b56
to
bdb37e5
| @@ -349,9 +350,12 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { | |||
|
|
|||
| self.r.indeterminate_imports.push(directive); | |||
| match directive.subclass { | |||
| // Don't add unresolved underscore imports to modules | |||
| SingleImport { target: Ident { name: kw::Underscore, .. }, .. } => {} | |||
petrochenkov
Oct 14, 2019
Contributor
What happens if this special case is removed? Some diagnostics regress?
What happens if this special case is removed? Some diagnostics regress?
matthewjasper
Oct 15, 2019
Author
Contributor
I don't think so, we just won't be able to remove it in resolve_imports.rs:831.
I don't think so, we just won't be able to remove it in resolve_imports.rs:831.
petrochenkov
Oct 15, 2019
Contributor
Ah, I see, the disambiguator is not available in that context.
Ah, I see, the disambiguator is not available in that context.
|
r=me with the typo #64623 (comment) fixed or not |
Instead add a disambiguator to the keys used for distinguishing resolutions.
matthewjasper
force-pushed the
matthewjasper:underscore-imports
branch
from
bdb37e5
to
4198df1
|
@bors r=petrochenkov |
|
|
tmandry
added a commit
to tmandry/rust
that referenced
this pull request
Oct 15, 2019
…=petrochenkov Remove last uses of gensyms Underscore bindings now use unique `SyntaxContext`s to avoid collisions. This was the last use of gensyms in the compiler, so this PR also removes them. closes rust-lang#49300 cc rust-lang#60869 r? @petrochenkov
bors
added a commit
that referenced
this pull request
Oct 15, 2019
Rollup of 14 pull requests Successful merges: - #64603 (Reducing spurious unused lifetime warnings.) - #64623 (Remove last uses of gensyms) - #65235 (don't assume we can *always* find a return type hint in async fn) - #65242 (Fix suggestion to constrain trait for method to be found) - #65265 (Cleanup librustc mir err codes) - #65293 (Optimize `try_expand_impl_trait_type`) - #65307 (Try fix incorrect "explicit lifetime name needed") - #65308 (Add long error explanation for E0574) - #65353 (save-analysis: Don't ICE when resolving qualified type paths in struct members) - #65389 (Return `false` from `needs_drop` for all zero-sized arrays.) - #65402 (Add troubleshooting section to PGO chapter in rustc book.) - #65425 (Optimize `BitIter`) - #65438 (Organize `never_type` tests) - #65444 (Implement AsRef<[T]> for List<T>) Failed merges: - #65390 (Add long error explanation for E0576) r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Underscore bindings now use unique
SyntaxContexts to avoid collisions. This was the last use of gensyms in the compiler, so this PR also removes them.closes #49300
cc #60869
r? @petrochenkov