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

RFC: Nested groups in imports #2128

Merged
merged 3 commits into from Sep 11, 2017

Conversation

Projects
None yet
@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 25, 2017

This is an incremental improvement to syntax of imports.

With this RFC groups inside of braces {} in imports can be nested

use syntax::{
    tokenstream::TokenTree, // >1 segments
    ext::base::{ExtCtxt, MacResult, DummyResult, MacEager}, // nested braces
};

and globs * can be placed into groups as well

use syntax::ast::{self, *}; // * in braces

Closes #1400
Closes #1290

Rendered

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Aug 25, 2017

Thanks, @petrochenkov, for hopping on this! Somehow it hadn't been targeted as part of the ergonomics initiative previously. I'm very much in favor; I've wanted this syntax many, many times.

There's not really a lot to critique here, so I'm going to go ahead and propose FCP, which will ping the other members of the lang team to review.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Aug 25, 2017

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

No concerns currently listed.

Once these reviewers reach consensus, 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.

@Ixrec

This comment has been minimized.

Copy link
Contributor

Ixrec commented Aug 25, 2017

I had no idea insta-FCP insta-PFCP was even allowed, but this is a perfect RFC to do it on.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Aug 25, 2017

@Ixrec yeah my thoughts exactly. Note though that the lang team still has to sign off to go into FCP, which is part of why I wanted to get the ball rolling right away. With luck, we'll be merging this in two weeks time...

@aturon aturon self-assigned this Aug 25, 2017

@lukaramu

This comment has been minimized.

Copy link
Contributor

lukaramu commented Aug 25, 2017

Would the following be legal under this RFC? (Without asking about if it would be desirable to use it this way although I think I like this 😅)

// adapted from rand's lib.rs
use ::{
    chacha::ChaChaRng,
    isaac::{IsaacRng, Isaac64Rng},
    distributions::{
        range::SampleRange,
        Range,
        IndependentSample,
    },
    std::{
        cell::RefCell,
        io, marker, mem,
        num::Wrapping as w,
    },
};

And, if that was legal, would it also legal without the leading :: (e.g. use { /* *snip* */ })? Should it be?

Overall, I'm a big 👍 !

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Aug 25, 2017

@lukaramu
Yes, this should be legal, both with and without :: (unless #2126 changes something in this area, I haven't read it yet).
This is a logical extension of use ::{a, b, c}; and use {a, b, c}; which are legal currently.
(Also, see the grammar in the RFC text, it accepts the example.)

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Aug 26, 2017

Minor thing: It looks like the grammar allows

use std::mem::{*, copy};

At the grammar level that's reasonable, but should there be a lint about it?

@retep998

This comment has been minimized.

Copy link
Member

retep998 commented Aug 26, 2017

@scottmcm Do we currently lint on use std::mem::*; use std::mem::copy;? If yes, that lint should apply to that example too. If no, then that lint would have to be proposed separately.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Aug 26, 2017

@scottmcm
It's assumed that code like this is reported by unused_imports lint.

https://play.rust-lang.org/?gist=eaeded63b68e2b63d84f173fce79c832&version=nightly

// use std::mem::{*, swap};
use std::mem::*; // WARN unused import: `std::mem::*;`
use std::mem::swap;

fn main() {
    let mut x = 0;
    let mut y = 0;
    swap(&mut x, &mut y);
}

https://play.rust-lang.org/?gist=5d9069fe10d07f8a3046d5a9bc82ecce&version=nightly

// use std::mem::{*, *};
use std::mem::*;
use std::mem::*; // WARN unused import: `std::mem::*;`

fn main() {
    let mut x = 0;
    let mut y = 0;
    swap(&mut x, &mut y);
}
@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Aug 26, 2017

IIRC how name resolution works, that's not even a useless thing to do, because if you have multiple glob imports, the explicit import of copy will shadow any other items called copy from the other globs, instead of causing an ambiguity error upon use?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Aug 26, 2017

@glaebhoerl

if you have multiple glob imports, the explicit import of copy will shadow any other items called copy from the other globs, instead of causing an ambiguity error upon use

Yep, that's a valid use case too.

In general, the "don't think, desugar" seems like a reasonable approach because If some imports look suspicious in "merged" form, then they should be reported in "unmerged" form as well.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Aug 26, 2017

I second @petrochenkov that simple desugarings help users form the intuition that "it's just a shorthand", and for refactoring the transformation is simpler for both humans and tools.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Aug 28, 2017

I'm pretty strongly against importing from multiple crates with a single use statements and I'd like us to consider it unidiomatic. But it makes sense for it to be allowed technically.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Aug 30, 2017

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

1 similar comment
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Aug 30, 2017

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

use syntax::ast::*;
use rustc::mir::*;
use rustc::transform::{MirPass, MirSource};

This comment has been minimized.

@tapeinosyne

tapeinosyne Sep 2, 2017

Small correction: here the path should be rustc::mir::transform::{MirPass, MirSource}.

@tapeinosyne

This comment has been minimized.

Copy link

tapeinosyne commented Sep 2, 2017

The RFC is jolly good as it stands, but I am tempted to ask: are there obvious reasons not allow this in re-exports? As in

pub use module::this_here_fn;
pub use module::yonder::{Struct, Trait};
pub use module::yonder::window::*;

becoming

pub use module::{
    this_here_fn,
    yonder::{Struct, Trait, window::*},
};

Such "re-export lists" would work nicely with the façade pattern. Moreover, once nested imports land on stable, it would feel rather inconsistent to have use declarations nest in one direction, but not the other.

@retep998

This comment has been minimized.

Copy link
Member

retep998 commented Sep 2, 2017

@tapeinosyne I don't see anything in this RFC that explicitly restricts this from working with visibility modifiers, so I don't see why your example wouldn't work when this RFC is eventually implemented. Might be worth clarifying in the RFC that this feature can be combined with visibility modifiers just fine.

@tapeinosyne

This comment has been minimized.

Copy link

tapeinosyne commented Sep 2, 2017

@retep998 Explicitly restricts, no. Carefully avoids mentioning, yes. Imports and imports only are explicitly mentioned in the title, the examples, and the syntax section of the Reference-level explanation. (As I understand, pub use is simply an import made public, but the RFC may be approaching things from a broader perspective. Either way, as you say, it's worth clarifying.)

Besides, re-export lists look a bit like a little zombie hand sticking out of the grave of export lists long gone, which may unduly faze some…

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Sep 2, 2017

@tapeinosyne It may be worth calling out explicitly, but I guarantee given the author that this was assumed to cover pub use.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Sep 2, 2017

@tapeinosyne

Carefully avoids mentioning, yes.

The grammar explicitly contains visibilities, but I should make it more prominent, yes.
This change is independent from pub, so it didn't occur to me to mention it specifically.

@tapeinosyne

This comment has been minimized.

Copy link

tapeinosyne commented Sep 2, 2017

@petrochenkov The grammar explicitly contains visibilities

Oh, you are right. Apologies, I must have missed that.

@crumblingstatue

This comment has been minimized.

Copy link

crumblingstatue commented Sep 6, 2017

👍 from me.

It's not something that's used often, but there are some common patterns that it makes prettier / less painful to write, and (probably) doesn't add too much complexity to the import system.

One example that always annoys me is

use std::io;
use std::io::prelude::*;

which can now be simply

use std::io::{self, prelude::*};
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Sep 9, 2017

The final comment period is now complete.

@aturon aturon referenced this pull request Sep 11, 2017

Closed

Tracking issue for RFC 2128: Nested groups in imports #44494

3 of 3 tasks complete
@aturon

This comment has been minimized.

Copy link
Member

aturon commented Sep 11, 2017

This RFC has been merged! Tracking issue.

Thanks @petrochenkov!

@aturon aturon merged commit e7737b3 into rust-lang:master Sep 11, 2017

bors added a commit to rust-lang/rust that referenced this pull request Nov 30, 2017

Auto merge of #45846 - pietroalbini:use-nested-groups, r=petrochenkov
Add nested groups in imports

This PR adds support for nested groups in imports (rust-lang/rfcs#2128, tracking issue #44494).

r? @petrochenkov

### Todo

- [x] Fix existing tests failing
- [x] Add new tests to cover the feature
- [x] Put the new behavior behind a feature flag
- [x] Write something for the unstable book

bors added a commit to rust-lang/rust that referenced this pull request Dec 1, 2017

Auto merge of #45846 - pietroalbini:use-nested-groups, r=petrochenkov
Add nested groups in imports

This PR adds support for nested groups in imports (rust-lang/rfcs#2128, tracking issue #44494).

r? @petrochenkov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment