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

Stabilize `underscore_imports` #56303

Merged
merged 1 commit into from Dec 18, 2018

Conversation

Projects
None yet
9 participants
@petrochenkov
Contributor

petrochenkov commented Nov 28, 2018

Closes #48216

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 28, 2018

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 28, 2018

Stabilization report

Feature name: #![feature(underscore_imports)]
Version target: 1.32 (2019-01-18)

Implemented in #48922 and landed 8.5 months ago.
Tracking issue is #48216.
Documentation issue is rust-lang-nursery/reference#470.
Tests for the feature can be found in src/test/ui/rfc-2166-underscore-imports.

What is being stabilized

use path as _; imports the path path and renames it into some unique identifier that can't be written explicitly.
For traits this means Trait's methods are now in scope in module/block into which use Trait as _; is placed.
For non-traits the import has no effect and is reported as unused by the unused_imports lint.

Additional effect on 2018 edition is that use my_crate::a::b as _; can refer to an extern crate my_crate and link it.
However, the import is still reported as unused in this case, unless it's of the form use my_crate as _; (add test tested in #56392).

extern crate name as _; imports the crate name and renames it into some unique identifier that can't be written explicitly.
The crate imported this way is linked, so the feature can be used for link-only crates, and the crate is not reported as unused in this case (add test tested in #56392).
If #[macro_use] is attached to such crate it works as usual and imports macros from name into prelude (add test? tested in #56392).

Some details and corner cases

Despite the name introduced with _ being unnameable explicitly, it still can be fetched by glob imports.
So, something like use super::*; reimports traits brought in scope with use Trait as _; in the parent module, and their methods become available in the child module.

Underscore imports can be public - pub use Trait as _;, this also can make sense in combination with glob imports.

Open questions

If procedural macro clones the _ token from its input, will it refer to the same unique identifier?
Right now the answer is yes, the "unique identifier" is created during parsing, so procedural macros (and declarative macros as well, if you try hard enough (add test tested in #56392)) can clone it while keeping its identity.
It's possible to implement it differently, so unique identifiers are only created during name resolution, after macro expansion.
UPDATE: Creation of gensyms is delayed until name resolution in #56392, so macros cannot clone them anymore.

Known issues

"Unique identifiers" (gensyms) are lost in crate metadata, so all _s become same after being imported from another crate.
This is a hard issue to solve, but it doesn't prevent the common use case of _ imports for importing traits, which doesn't normally assume reexporting _ imports to other crates.
UPDATE: Mostly fixed in #56392.

_ imports and other imports in general, are known to be buggy when used together with macros 2.0 (macro items or Span::def_site), but this only affects unstable code.

@petrochenkov petrochenkov referenced this pull request Nov 28, 2018

Closed

Tracking issue for RFC #2166: impl-only-use #48216

3 of 3 tasks complete
@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 28, 2018

I'm especially interested in opinions regarding interactions with proc macros ("Open questions" in #56303 (comment)).
cc @dtolnay

@Centril

Centril approved these changes Nov 28, 2018 edited

Looks good pending tests passing and pending some of the additional tests discussed above.

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 28, 2018

Thanks for the thoroughly detailed report <3.

I propose that we stabilize #![feature(underscore_imports)] with a version target of 1.32 (or later if necessary...) for all editions (2015 and 2018).

As for the open questions re. macros, I propose that we create unique identifiers during name resolution and after macro expansion such that (#56303 (comment)):

#[my_macro]
use path::to::Trait as _;

// expands to

use path::to::Trait1 as _;
use path::to::Trait2 as _; // the same underscore token

is not an error (same for the declarative macro case in #56303 (comment)).

@rfcbot merge

@rfcbot

This comment has been minimized.

rfcbot commented Nov 28, 2018

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@dtolnay

This comment has been minimized.

Member

dtolnay commented Nov 28, 2018

To make sure I understand the procedural macro consideration: would the following be an example of the edge case you have in mind?

#[my_macro]
use path::to::Trait as _;

// expands to

use path::to::Trait1 as _;
use path::to::Trait2 as _; // the same underscore token

and these would be conflicting imports because they reuse the same gensym?

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 28, 2018

@dtolnay
Yes, exactly.

The declarative macro equivalent is

#![feature(underscore_imports)]
#![allow(unused)]

macro_rules! m {
    ($item: item) => { $item $item }
}

m!(use std as _;); // ERROR the name `_` is defined multiple times

fn main() {}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2015&gist=a56fe430e06fcb300afcb01a64bcfe8b

@dtolnay

This comment has been minimized.

Member

dtolnay commented Nov 28, 2018

Thanks @petrochenkov. If you were to ask me which of the two behaviors I would want for the code in my previous comment, I would mildly prefer for those not to be conflicting imports -- like how if a macro expands to let x: Result<_, _> = ... with the two underscores being the same token, they still operate independently and wouldn't somehow end up being constrained to be the same inferred type.

But I would be comfortable stabilizing this as currently implemented. I don't foresee the behavior being at all problematic for procedural macros in actual usage.

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 28, 2018

@dtolnay

like how if a macro expands to let x: Result<_, _> = ... with the two underscores being the same token, they still operate independently and wouldn't somehow end up being constrained to be the same inferred type.

I agree with this rationale and have amended the proposal accordingly.

@Nemo157

This comment has been minimized.

Contributor

Nemo157 commented Nov 28, 2018

"Unique identifiers" (gensyms) are lost in crate metadata, so all _s become same after being imported from another crate.
This is a hard issue to solve, but it doesn't prevent the common use case of _ imports for importing tratis, which doesn't normally assume reexporting _ imports to other crates.

Re-exporting _ traits from a prelude is one of the usecases I was looking forward to. embedded_hal::prelude uses manually renamed traits to try and avoid conflicts, trying to replace this with underscore_imports compiles the crate fine, but gives an error related to this when glob imported:

error[E0428]: the name `_` is defined multiple times
  |
  = note: `_` must be defined only once in the type namespace of this module

It would be nice if there were a specific diagnostic for this usecase (and an issue to track implementing a fix?).

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 28, 2018

One more example showing that gensym creation at parse-time is at least suspicious:

duplicate! {
    // The macro argument is parsed as an unstructured token stream, `_` is parsed as a plain `_` token, gensym is not created.
    // Duplication results in two plain `_` tokens.
    // Two plain `_`s are parsed after the macro is expanded, two different gensyms are created, no name conflict happens during resolution.
    use foo as _;
}

// The macro argument is parsed as an item, gensym is created when parsing `_`.
// Duplication results in two identical gensyms.
// Two gensyms are parsed after the macro is expanded, name conflict happens during resolution.
#[duplicate]
use foo as _;
@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 28, 2018

@Nemo157
FWIW, we can easily invert the bug, especially if gensym creation is delayed until name resolution.

Now we always consider gensyms from _ same cross-crate, while they can actually be different.
But instead we could always consider them different cross-crate, while they can actually be same.

Actually, I have hard time imagining situation in which the reverted bug would be observable, if we delay the gensym creation until resolve.
We can create a "name conflict":

// Crate 1
pub mod m {
    pub use foo as _;
}

pub use m::*;

// Crate 2
use crate1::m::*;
use crate1::*;

but it won't result in an error, because glob vs glob conflict is only an error if definitions of conflicting names are different, but in this case it's the same foo.
And apparently we can't create non-glob conflicts because gensyms from _ are unnameable.

@bors

This comment has been minimized.

Contributor

bors commented Nov 29, 2018

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

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 30, 2018

All the suggestions and additional tests are implemented in #56392.

@scottmcm

This comment has been minimized.

Member

scottmcm commented Nov 30, 2018

@rfcbot concern waiting-on-56392-fix

It looks like there's currently one test in https://github.com/rust-lang/rust/tree/master/src/test/ui/rfc-2166-underscore-imports. Is that sufficient? Are there success stories of people trying this out and it working, so people wanting it stabilized to be able to check in? (I didn't see any in #48216, and only a failure story here.)

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Dec 1, 2018

It looks like there's currently one test in https://github.com/rust-lang/rust/tree/master/src/test/ui/rfc-2166-underscore-imports. Is that sufficient?

Well, this is a small feature. Most of intra-crate cases are basically covered in that test. (And #56392 adds some more.)

@Nemo157

This comment has been minimized.

Contributor

Nemo157 commented Dec 1, 2018

Oh, I can give a success story where I've been using this for a while as well 😄 here I've been using it for bringing in link-only crates (and by the sounds of #56392 that will fix the allow(unused_imports) I had to use).

bors added a commit that referenced this pull request Dec 6, 2018

Auto merge of #56392 - petrochenkov:regensym, r=oli-obk
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.
@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Dec 6, 2018

#56392 is merged now.

@scottmcm

This comment has been minimized.

Member

scottmcm commented Dec 7, 2018

@rfcbot resolved waiting-on-56392-fix

@rfcbot

This comment has been minimized.

rfcbot commented Dec 7, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

Show resolved Hide resolved src/libsyntax/feature_gate.rs Outdated
@torkleyy

This comment has been minimized.

torkleyy commented Dec 13, 2018

Awesome! When exporting traits from a prelude like this, what is rustdoc going to generate?

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Dec 13, 2018

@torkleyy
Some quick testing shows this:

underscore_imports

@rfcbot

This comment has been minimized.

rfcbot commented Dec 17, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@cramertj

This comment has been minimized.

Member

cramertj commented Dec 17, 2018

r=me with @Centril's above fix to the feature gate stabilization version.

@petrochenkov petrochenkov force-pushed the petrochenkov:stabuseas branch from 9b8ec84 to 7c901ba Dec 17, 2018

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Dec 17, 2018

@bors r=cramertj

@bors

This comment has been minimized.

Contributor

bors commented Dec 17, 2018

📌 Commit 7c901ba has been approved by cramertj

@bors

This comment has been minimized.

Contributor

bors commented Dec 17, 2018

⌛️ Testing commit 7c901ba with merge c9bb68f...

bors added a commit that referenced this pull request Dec 17, 2018

Auto merge of #56303 - petrochenkov:stabuseas, r=cramertj
Stabilize `underscore_imports`

Closes #48216
@bors

This comment has been minimized.

Contributor

bors commented Dec 18, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: cramertj
Pushing c9bb68f to master...

@bors bors merged commit 7c901ba into rust-lang:master Dec 18, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment