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

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

Closed
3 tasks done
aturon opened this issue Sep 11, 2017 · 27 comments
Closed
3 tasks done

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

aturon opened this issue Sep 11, 2017 · 27 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Sep 11, 2017

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

Steps:

@aturon aturon added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Sep 11, 2017
@nikomatsakis nikomatsakis added this to the impl period milestone Sep 15, 2017
@aturon aturon removed this from the impl period milestone Sep 15, 2017
@TimNN TimNN added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Sep 17, 2017
@pietroalbini
Copy link
Member

I'd like to work on this.

@nikomatsakis
Copy link
Contributor

@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
Copy link
Contributor

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 nikomatsakis added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. and removed E-needs-mentor labels Sep 21, 2017
@nikomatsakis
Copy link
Contributor

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

@pietroalbini
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

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

bors added a commit that referenced this issue Dec 1, 2017
Add nested groups in imports

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

r? @petrochenkov
@pietroalbini
Copy link
Member

The feature has been merged 🎉 🎉
Can someone update the checklist?

@matklad
Copy link
Member

matklad commented Dec 4, 2017

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
Copy link
Contributor

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

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

@pietroalbini
Copy link
Member

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
Copy link
Member Author

aturon commented Jan 23, 2018

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

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jan 23, 2018

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.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 23, 2018
@nikomatsakis
Copy link
Contributor

@pietroalbini thanks for the info!

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jan 25, 2018
…s-span, r=petrochenkov

Fix spans in unused import lint for nested groups

This fixes an inconsistency for empty nested groups, and adds a test for all the possible cases of the lint.

```
warning: unused imports: `*`, `Foo`, `baz::{}`, `foobar::*`
  --> test.rs:16:11
   |
16 | use foo::{Foo, bar::{baz::{}, foobar::*}, *};
   |           ^^^        ^^^^^^^  ^^^^^^^^^   ^
   |
   = note: #[warn(unused_imports)] on by default

warning: unused import: `*`
  --> test.rs:17:24
   |
17 | use foo::bar::baz::{*, *};
   |                        ^

warning: unused import: `use foo::{};`
  --> test.rs:18:1
   |
18 | use foo::{};
   | ^^^^^^^^^^^^
```

cc rust-lang#44494
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jan 25, 2018
…s-span, r=petrochenkov

Fix spans in unused import lint for nested groups

This fixes an inconsistency for empty nested groups, and adds a test for all the possible cases of the lint.

```
warning: unused imports: `*`, `Foo`, `baz::{}`, `foobar::*`
  --> test.rs:16:11
   |
16 | use foo::{Foo, bar::{baz::{}, foobar::*}, *};
   |           ^^^        ^^^^^^^  ^^^^^^^^^   ^
   |
   = note: #[warn(unused_imports)] on by default

warning: unused import: `*`
  --> test.rs:17:24
   |
17 | use foo::bar::baz::{*, *};
   |                        ^

warning: unused import: `use foo::{};`
  --> test.rs:18:1
   |
18 | use foo::{};
   | ^^^^^^^^^^^^
```

cc rust-lang#44494
@nrc
Copy link
Member

nrc commented Jan 28, 2018

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
Copy link
Member Author

aturon commented Jan 31, 2018

@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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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
Copy link

rfcbot commented Feb 1, 2018

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

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 1, 2018
@rfcbot
Copy link

rfcbot commented Feb 1, 2018

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

@aturon
Copy link
Member Author

aturon commented Feb 1, 2018

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
Copy link
Member

Sure, I'll start sending out PRs.

kennytm added a commit to kennytm/rust that referenced this issue Feb 5, 2018
…ilize, r=petrochenkov

Stabilize use_nested_groups

As requested in rust-lang#44494. Documentation PRs already sent.
@petrochenkov
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests