Remove last uses of gensyms #64623

matthewjasper · 2019-09-19T20:54:25Z

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

src/librustc_resolve/resolve_imports.rs

src/librustc_resolve/lib.rs

src/test/ui/underscore-imports/hygiene-2.rs

src/test/ui/underscore-imports/hygiene.rs

petrochenkov · 2019-09-21T20:59:30Z

src/librustc_resolve/build_reduced_graph.rs

@@ -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();


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.

Glob imports operate on the actual hygiene data: https://github.com/rust-lang/rust/blob/da01d31ff97feb199036359c364aedc809711123/src/librustc_resolve/resolve_imports.rs#L563

I'll see if there's a way to do this without increasing the complexity of reverse_glob_adjust

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.)

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.

nnethercote

I'd be interested to see a perf run on this before it lands, see if it improves things.

src/libsyntax_pos/symbol.rs

matthewjasper · 2019-09-28T15:11:51Z

@bors try @rust-timer queue

rust-timer · 2019-09-28T15:11:53Z

Awaiting bors try build completion

bors · 2019-09-28T15:12:03Z

⌛ Trying commit 50660a1817e3c403c532dfcdd15210917a653921 with merge a9bcdfe8870fc7bed62b6bb47e31b50629067f15...

bors · 2019-09-28T18:08:05Z

☀️ Try build successful - checks-azure
Build commit: a9bcdfe8870fc7bed62b6bb47e31b50629067f15 (a9bcdfe8870fc7bed62b6bb47e31b50629067f15)

rust-timer · 2019-09-28T18:08:06Z

Queued a9bcdfe8870fc7bed62b6bb47e31b50629067f15 with parent f3c8eba, future comparison URL.

rust-timer · 2019-09-28T21:50:49Z

Finished benchmarking try commit a9bcdfe8870fc7bed62b6bb47e31b50629067f15, comparison URL.

matthewjasper · 2019-09-28T22:27:10Z

Having a quick look at unused-warnings, I think this is caused by the sorting of reexports. I'll investigate properly tomorrow.

nnethercote · 2019-09-29T06:53:18Z

unused-warnings is one of the least important benchmarks. Still, 3-5% regressions are fairly large so if it's easy to avoid them that would be good.

matthewjasper · 2019-09-29T14:25:13Z

@bors try @rust-timer queue

rust-timer · 2019-09-29T14:25:14Z

Awaiting bors try build completion

bors · 2019-09-29T14:25:26Z

⌛ Trying commit 0e1c0b5 with merge 3498eec...

@petrochenkov

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

bors · 2019-09-29T17:12:06Z

☀️ Try build successful - checks-azure
Build commit: 3498eec (3498eec87d0c5f25b7df91c424ad1bc6130f8677)

rust-timer · 2019-09-29T17:12:07Z

Queued 3498eec with parent d046ffd, future comparison URL.

rust-timer · 2019-09-30T00:10:34Z

Finished benchmarking try commit 3498eec, comparison URL.

JohnCSimon · 2019-10-12T04:55:58Z

Ping from triage
@petrochenkov This appears to be waiting on review.
CC: @Mark-Simulacrum
Thanks.

petrochenkov · 2019-10-12T09:19:15Z

I didn't forget about this, just need some continuous period of time to focus on this.
Perhaps this weekend.

petrochenkov · 2019-10-13T00:50:23Z

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.
I've opened #65351 to describe the issues and possibly address them, marking this PR as blocked on it for now.

petrochenkov · 2019-10-13T09:22:45Z

@matthewjasper
Do you still have the branch with underscores implemented as per-module indices?
It may be a better short term way to unblock the removal of gensyms.

src/librustc_resolve/lib.rs

petrochenkov · 2019-10-14T15:39:23Z

src/librustc_resolve/build_reduced_graph.rs

@@ -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, .. }, .. } => {}


What happens if this special case is removed? Some diagnostics regress?

I don't think so, we just won't be able to remove it in resolve_imports.rs:831.

Ah, I see, the disambiguator is not available in that context.

petrochenkov · 2019-10-15T19:10:50Z

r=me with the typo #64623 (comment) fixed or not

Instead add a disambiguator to the keys used for distinguishing resolutions.

matthewjasper · 2019-10-15T21:22:40Z

@bors r=petrochenkov

bors · 2019-10-15T21:22:41Z

📌 Commit 4198df1 has been approved by petrochenkov

@petrochenkov

…=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

@ghost

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

rust-highfive assigned petrochenkov Sep 19, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2019

matthewjasper mentioned this pull request Sep 19, 2019

Add a raw "address of" operator #64588

Merged

fbstj reviewed Sep 19, 2019

View reviewed changes

src/librustc_resolve/resolve_imports.rs Outdated Show resolved Hide resolved

Centril reviewed Sep 19, 2019

View reviewed changes

src/librustc_resolve/lib.rs Outdated Show resolved Hide resolved

matthewjasper force-pushed the underscore-imports branch from 90eef3e to da01d31 Compare September 20, 2019 19:05

petrochenkov reviewed Sep 21, 2019

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 Sep 21, 2019

nnethercote reviewed Sep 26, 2019

View reviewed changes

src/libsyntax_pos/symbol.rs Outdated Show resolved Hide resolved

matthewjasper force-pushed the underscore-imports branch 2 times, most recently from 06e5858 to 50660a1 Compare September 28, 2019 13:15

matthewjasper force-pushed the underscore-imports branch from 50660a1 to 0e1c0b5 Compare September 29, 2019 13:20

matthewjasper removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 2, 2019

petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2019

petrochenkov mentioned this pull request Oct 13, 2019

[WIP] Traits in scope and def-site hygiene #65351

Closed

Add more tests for underscore imports

a975259

matthewjasper force-pushed the underscore-imports branch from bc38b56 to bdb37e5 Compare October 13, 2019 20:54

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Oct 14, 2019

petrochenkov reviewed Oct 14, 2019

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 Oct 14, 2019

matthewjasper added 3 commits October 15, 2019 21:12

Don't use gensym_if_underscore to resolve _ bindings

1a2597b

Instead add a disambiguator to the keys used for distinguishing resolutions.

Remove gensyms

94967f2

Remove some mentions of gensyms

4198df1

matthewjasper force-pushed the underscore-imports branch from bdb37e5 to 4198df1 Compare October 15, 2019 20:12

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 Oct 15, 2019

tmandry mentioned this pull request Oct 15, 2019

Rollup of 14 pull requests #65454

Merged

bors merged commit 4198df1 into rust-lang:master Oct 16, 2019

matthewjasper deleted the underscore-imports branch October 16, 2019 08:09

tshepang mentioned this pull request May 10, 2020

Glossary rust-lang/rustc-dev-guide#48

Closed

27 tasks

jruderman mentioned this pull request Dec 6, 2022

Infinite recursion crash with use _ #105353

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove last uses of gensyms #64623

Remove last uses of gensyms #64623

matthewjasper commented Sep 19, 2019 •

edited

Loading

petrochenkov Sep 21, 2019

matthewjasper Sep 21, 2019

petrochenkov Sep 22, 2019

matthewjasper Sep 22, 2019

nnethercote left a comment

matthewjasper commented Sep 28, 2019

rust-timer commented Sep 28, 2019

bors commented Sep 28, 2019

bors commented Sep 28, 2019

rust-timer commented Sep 28, 2019

rust-timer commented Sep 28, 2019

matthewjasper commented Sep 28, 2019 •

edited

Loading

nnethercote commented Sep 29, 2019

matthewjasper commented Sep 29, 2019

rust-timer commented Sep 29, 2019

bors commented Sep 29, 2019

bors commented Sep 29, 2019

rust-timer commented Sep 29, 2019

rust-timer commented Sep 30, 2019

JohnCSimon commented Oct 12, 2019

petrochenkov commented Oct 12, 2019

petrochenkov commented Oct 13, 2019

petrochenkov commented Oct 13, 2019

petrochenkov Oct 14, 2019

matthewjasper Oct 15, 2019

petrochenkov Oct 15, 2019

petrochenkov commented Oct 15, 2019

matthewjasper commented Oct 15, 2019

bors commented Oct 15, 2019

Remove last uses of gensyms #64623

Remove last uses of gensyms #64623

Conversation

matthewjasper commented Sep 19, 2019 • edited Loading

petrochenkov Sep 21, 2019

Choose a reason for hiding this comment

matthewjasper Sep 21, 2019

Choose a reason for hiding this comment

petrochenkov Sep 22, 2019

Choose a reason for hiding this comment

matthewjasper Sep 22, 2019

Choose a reason for hiding this comment

nnethercote left a comment

Choose a reason for hiding this comment

matthewjasper commented Sep 28, 2019

rust-timer commented Sep 28, 2019

bors commented Sep 28, 2019

bors commented Sep 28, 2019

rust-timer commented Sep 28, 2019

rust-timer commented Sep 28, 2019

matthewjasper commented Sep 28, 2019 • edited Loading

nnethercote commented Sep 29, 2019

matthewjasper commented Sep 29, 2019

rust-timer commented Sep 29, 2019

bors commented Sep 29, 2019

bors commented Sep 29, 2019

rust-timer commented Sep 29, 2019

rust-timer commented Sep 30, 2019

JohnCSimon commented Oct 12, 2019

petrochenkov commented Oct 12, 2019

petrochenkov commented Oct 13, 2019

petrochenkov commented Oct 13, 2019

petrochenkov Oct 14, 2019

Choose a reason for hiding this comment

matthewjasper Oct 15, 2019

Choose a reason for hiding this comment

petrochenkov Oct 15, 2019

Choose a reason for hiding this comment

petrochenkov commented Oct 15, 2019

matthewjasper commented Oct 15, 2019

bors commented Oct 15, 2019

matthewjasper commented Sep 19, 2019 •

edited

Loading

matthewjasper commented Sep 28, 2019 •

edited

Loading