Skip to content
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

Delay gensym creation for "underscore items" (use foo as _/const _) until name resolution #56392

Merged
merged 3 commits into from
Dec 6, 2018

Conversation

petrochenkov
Copy link
Contributor

So they cannot be cloned by macros. See #56303 for the discussion.

Mostly fix cross-crate use of underscore items by inverting the "gensyms are lost in metadata" bug as described in #56303 (comment).
Fix unused import warnings for single-segment imports (first commit) and use crate_name as _ imports (as specified in #56303 (comment)).
Prohibit accidentally implemented static _: TYPE = EXPR; (cc #55983).
Add more tests for use foo as _ imports.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 30, 2018
src/librustc_resolve/lib.rs Show resolved Hide resolved
src/librustc_resolve/resolve_imports.rs Show resolved Hide resolved
let Export { ident, def, vis, span } = child;
// FIXME: We shouldn't create the gensym here, it should come from metadata,
// but metadata cannot encode gensyms currently, so we create it here.
// This is only a guess, two equivalent idents may incorrectly get different gensyms here.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this even a hypothetical problem for _?

Copy link
Contributor Author

@petrochenkov petrochenkov Dec 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm... not entirely sure.
I haven't found an example where it would caused observable problems so far (assuming gensym creation is delayed until name resolution).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#56303 (comment) has an example of gensyms being created incorrectly internally, but it's not observable from the outside.

@@ -9,6 +9,5 @@
// except according to those terms.

const _: () = (); //~ ERROR is unstable
static _: () = (); //~ ERROR is unstable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this wasn't in the RFC, but I see no reason not to support it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const _: T = expr; was permitted because it has a reasonable generalization to irrefutable patterns (e.g. const (A, B) = (1, 2);) but does static also have that with the same-address notion?

Copy link
Contributor Author

@petrochenkov petrochenkov Dec 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least the motivation in rust-lang/rfcs#2526 does not apply to statics.
Constants don't even exist in MIR (AFAIK), so they can be used for compile-time asserts, while statics end up in the resulting executable/library, or need to be optimized away.

@petrochenkov
Copy link
Contributor Author

Updated with comments addressed.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 5, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Dec 5, 2018

📌 Commit eb1d2e6 has been approved by oli-obk

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2018
@bors
Copy link
Contributor

bors commented Dec 6, 2018

⌛ Testing commit eb1d2e6 with merge 4bb5d35...

bors added a commit that referenced this pull request Dec 6, 2018
Delay gensym creation for "underscore items" (`use foo as _`/`const _`) until name resolution

So they cannot be cloned by macros. See #56303 for the discussion.

Mostly fix cross-crate use of underscore items by inverting the "gensyms are lost in metadata" bug as described in #56303 (comment).
Fix unused import warnings for single-segment imports (first commit) and `use crate_name as _` imports (as specified in #56303 (comment)).
Prohibit accidentally implemented `static _: TYPE = EXPR;` (cc #55983).
Add more tests for `use foo as _` imports.
@bors
Copy link
Contributor

bors commented Dec 6, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 4bb5d35 to master...

@bors bors merged commit eb1d2e6 into rust-lang:master Dec 6, 2018
@pietroalbini
Copy link
Member

cc @rust-lang/compiler, this is needed to backport #57160.

@pietroalbini pietroalbini added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 3, 2019
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jan 3, 2019

cc @rust-lang/compiler, this is needed to backport #57160.

Clarification: not whole PR, but only the first commit (c658d73).
(It's also a prerequisite for #56759.)

@petrochenkov
Copy link
Contributor Author

c658d73 is included into #57483

@nikomatsakis nikomatsakis added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jan 10, 2019
@nikomatsakis
Copy link
Contributor

I'm removing the beta-nominated tag here because this is rolled into #57483

Centril added a commit to Centril/rust that referenced this pull request Mar 19, 2019
Do not encode gensymed imports in metadata

(Unless they are underscore `_` imports which are re-gensymed on crate loading, see rust-lang#56392.)

We cannot encode gensymed imports properly in metadata and if we encode them improperly, we can get erroneous name conflicts downstream.
Gensymed imports are produced by the compiler, so we control their set, and can be sure that none of them needs being encoded for use from other crates.

A workaround that fixes rust-lang#59243.
kennytm added a commit to kennytm/rust that referenced this pull request Mar 24, 2019
Do not encode gensymed imports in metadata

(Unless they are underscore `_` imports which are re-gensymed on crate loading, see rust-lang#56392.)

We cannot encode gensymed imports properly in metadata and if we encode them improperly, we can get erroneous name conflicts downstream.
Gensymed imports are produced by the compiler, so we control their set, and can be sure that none of them needs being encoded for use from other crates.

A workaround that fixes rust-lang#59243.
@petrochenkov petrochenkov deleted the regensym branch June 5, 2019 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants