Rust: Tracking issue for RFC 2128: Nested groups in imports

[aturon]

This is a tracking issue for the RFC "Nested groups in imports" (rust-lang/rfcs#2128).

Steps:

• [x] Implement the RFC (very rough mentoring instructions here and here)
• [x] Adjust documentation (see instructions on forge)
• [x] Stabilization PR (see instructions on forge)

[pietroalbini]

I'd like to work on this.

[nikomatsakis]

@pietroalbini great! I think @petrochenkov or @jseyfried would be best equipped to writing up some mentoring notes here. But I think the general idea is going to be something like this:

• Extend the AST to support the new syntax. Generally speaking, we try to keep the structure of the AST as a "close match" to the raw rust input, so I think we would want to mirror the new structure there.
• Update name resolution to handle this. This is the part where I can't help -- at least without doing some digging! Hopefully, there are some intermediate data structures that we can "desugar" these extended-form view items into, and it doesn't operate directly off of the AST.

[petrochenkov]

Update name resolution to handle this.

The desugaring is done here https://github.com/rust-lang/rust/blob/master/src/librustc_resolve/build_reduced_graph.rs#L113

[nikomatsakis]

@pietroalbini just checking in -- any progress? Any questions arisen?

[pietroalbini]

I'm still working on this. I mostly updated all of rustc to support the feature, and now I started fixing a few bugs in the code.

[nikomatsakis]

@pietroalbini how's it going? I'm just checking in -- I know you've been hard at work on this, I was just wondering if you could give a quick status update.

[pietroalbini]

@nikomatsakis sure!

I fixed a few days ago a bunch of errors related to self, and now there is an bug with the hir nodes. I plan to try fixing that tomorrow, and then _hopefully_ all the errors will be fixed. After that I'll need to update all the tests (and write new ones) and feature gate the thing, and then the feature will be ready to be merged.

[pietroalbini]

The feature has been merged :tada: :tada:
Can someone update the checklist?

[matklad]

Hm, I am quite surprised that neither the RFC nor the tracking issue had a single "no, we don't need this" comment... Given the amount of ❤️ on the RFC, I think that community overwhelmingly supports this feature, so my concerns are probably unfounded, but I still would want to register them.

I personally am neutral or slightly negative about the usefulness of this feature. The benefits seems marginal, but there's certainly a cost to chose between several ways of writing the same import now.

I already spend quite some time make imports perfect: stdlib first, then external crates, then my crate, then sort them alphabetically, then decide if I should go with use my_crate::foo::bar or use super::bar, then decide if I need the leading :: in use path. Adding one more decision point to the already confusing use statements is not a clear win.

On the other hand, duplicating path segments does not seem that unergonomic, especially given that you can easily sort them.

[SimonSapin]

I already spend quite some time make imports perfect: […]

These are all stylistic choices. You can choose not to use this new syntax.

[pietroalbini]

While someone can surely abuse this new syntax if they want, it can be really nice in a few cases. For example, I tend to group related imports together, so if I want to use an atomic across threads use std::sync::{Arc, atomic::{AtomicBool, Ordering}}; is great.

[aturon]

I think we should consider stabilizing this feature for the 1.25 release (this cycle). As such:

@rfcbot fcp merge

[rfcbot]

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

• [x] @aturon
• [x] @cramertj
• [x] @eddyb
• [x] @nikomatsakis
• [x] @nrc
• [x] @pnkfelix
• [x] @withoutboats

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.

[nikomatsakis]

I think I would be happy to stabilize, but I'd like to see a 'stabilization report' prepared. Essentially this would be a brief summary of the feature plus some links to test cases showing it works as expected, along with a list of edge cases that came up and and were considered. This seems like a sort of minimal "due diligence" that we ought to do before stabilizing.

Brief summary

Enable use blocks to have "subgroups", such as:

use a::{b1::{c, d}, b2};

which would be equivalent to:

use a::b1::{c, d};
use a::b2;

Test cases and edge cases

Looking over @pietroalbini's PR, I found these:

• basic test showing things work run-pass/use-nested-groups.rs
• also covers globs in nested imports use self::{foo::*}
• only relative paths are allowed in nested groups (e.g., use foo::bar::{super::baz} is an error)
• we warn on use foo::{*, *}

Things not currently tested

• a "doubly nested" group with multiple options, sort of like this:
  
  
  
  • use a::{b::{c1, c2}} (we have use a::b::{c::d} in the use-nested-groups.rs test)
  
  
• a test where we get an error, showing that the span is correct (e.g., if c1 is not defined for use a::{b1::{c1, c2}, b2}, but the rest are, what do we highlight?)

I think I'd like to see those tests, but overall this looks pretty good to me!

[nikomatsakis]

To be clear, I'm giving my r+ to stabizing but I'd like to see those two tests I mentioned in the stabilization PR. =)

[pietroalbini]

Other than the tests mentioned by @nikomatsakis, there are a few problems at the moment:

• use {{}, {}}; crashes the compiler -- issue #47673 and PR #47705
• Unused imports lint behave strangely when empty groups are nested -- PR #47726

I'm planning to write a fix for the unused lint later this evening, and I'll add the new tests after.

[nikomatsakis]

@pietroalbini thanks for the info!

[nrc]

Hmm, this feature has only been implented for two months - that seems a pretty short stabilisation period. I don't have a strong objection to stabilising, but I don't feel a rush to either - this has always felt like a marginally useful bit of syntax sugar, with some added complexity in exchange.

[aturon]

@nrc it will still take another 3 months to actually reach stable.

Given the pipeline of features we're hoping to stabilize for the epoch release, and the timeline thereof, we need to be steadily stabilizing features to avoid a last-minute rush -- particularly when they are as straightforward as this one.

This feature is also quite important for considerations around the new module system path syntax.

[steveklabnik]

So, I'm all in favor of stabilizing this soon. However, I see that the documentation checkbox has been checked, but I can't see any links to those PRs in this issue? Does anyone have links handy, or does @rust-lang/docs need to get on it? :)

[pietroalbini]

@steveklabnik I think it's checked because I already prepared the snippets to be added to the various docs when I sent the original PR (available on the unstable book). I can send PRs to the various repos so the docs team can review them, after the lang team accepts the stabilization.

[steveklabnik]

Ah, normally the process is, PRs should be open in order for someone to be able to call for stabilizing. This is a small enough feature that I'm happy to do it after.

[rfcbot]

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

[rfcbot]

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

[aturon]

I'd love to get this in for the current release cycle (beta branches in about two weeks) -- anyone want to take on the stabilization here?

[pietroalbini]

Sure, I'll start sending out PRs.

[petrochenkov]

The stabilization PR has actually landed (and doc PRs are sent), so this is done.