Add nested groups in imports #45846

pietroalbini · 2017-11-07T20:52:06Z

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

rust-highfive · 2017-11-07T20:52:14Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

pietroalbini · 2017-11-07T20:58:52Z

I know some tests are failing right now, but rustc compiles itself successfully and the actual implementation of the feature is ready.

cc @nikomatsakis @eddyb

petrochenkov · 2017-11-09T15:57:03Z

src/libsyntax/ast.rs

 }

+pub type ViewPath = Spanned<ViewPath_>;


Could you avoid Spanned here and add the span: Span field into ViewPath_ explicitly, and then rename ViewPath_ -> ViewPath and ViewPathKind_ -> ViewPathKind.

It's also a good opportunity to finally rename ViewPath (what does it even mean?) into something more reasonable, e.g. ImportTree or UseTree.

petrochenkov · 2017-11-09T16:01:32Z

src/libsyntax/ast.rs

+#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
+pub struct ViewPath_ {
+    pub kind: ViewPathKind_,
+    pub path: Path,


path -> prefix

petrochenkov · 2017-11-09T16:03:17Z

src/libsyntax/ast.rs

-    /// `foo::bar::{a,b,c}`
-    ViewPathList(Path, Vec<PathListItem>)
+pub enum ViewPathKind_ {
+    ViewPathSimple(Ident),


Could you use the "new-style" naming

enum ViewPathKind { Simple(Ident), Glob, ... }

without pub use self::ViewPathKind_::*.

petrochenkov · 2017-11-11T22:43:33Z

src/libsyntax/visit.rs

+    visitor.visit_path(&use_tree.prefix, id);
+
+    match use_tree.kind {
+        UseTreeKind::Simple(ref ident) => {


Nit: I've seen this ref ident in few places, ref is not necessary, Ident is small and Copy.

petrochenkov · 2017-11-12T00:22:35Z

src/libsyntax/parse/parser.rs

-                    self.bump();
-                    Ok(P(respan(lo.to(self.span), ViewPathGlob(prefix))))
+            // `use path::...;`
+            let mut parsed = self.parse_path(PathStyle::Mod)?;


This will accept use :: ::a::b; :)
(And also use :: $path::{a::b}.)

You really have to parse the prefix as a whole (including the starting ::) if it's a path, and treat "non-path" prefixes Ø and :: specially.

petrochenkov · 2017-11-12T00:23:25Z

src/libsyntax/parse/parser.rs

                }
            } else {
-                // `foo::bar` or `foo::bar as baz`


Argh, deleted comment!

petrochenkov · 2017-11-12T00:25:00Z

src/libsyntax/parse/parser.rs

        loop {
            segments.push(self.parse_path_segment(style, enable_warning)?);

-            if self.is_import_coupler() || !self.eat(&token::ModSep) {


Could you keep is_import_coupler alive and used here (even if it's used only here).
It will likely be useful again for fixing https://github.com/rust-lang/rust/pull/45846/files#r150397110.

bors · 2017-11-12T04:17:52Z

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

petrochenkov · 2017-11-12T11:30:40Z

src/libsyntax/parse/parser.rs

        } else {
-            let prefix = self.parse_path(PathStyle::Mod)?.default_to_global();


I'd probably like to get rid from default_to_global too, but it's better left for a separate PR.
default_to_global is also used in visibilities and is_global relies on that starting CrateRoot and now returns inconsistent results, and is_global is used in too many places that may require adjustments. I don't recommend dealing with these issues in this PR.

For now, let's keep default_to_global for the outer use tree and avoid it for nested trees.

shepmaster · 2017-11-18T17:51:10Z

Ping from triage @pietroalbini — it's been a week since we last heard from you! Will you be able to address the issues with this PR soon?

pietroalbini · 2017-11-18T22:36:29Z

@shepmaster all the review comments were already fixed, but in other files so github didn't hide them ;)
I plan to continue working on the missing parts of the PR (feature flag and tests) in the next few days.

petrochenkov · 2017-11-19T11:54:38Z

src/libsyntax/parse/parser.rs

+            if self.eat(&token::ModSep) {
+                prefix.segments.push(PathSegment::crate_root(self.prev_span));
+            } else if !nested {
+                prefix.segments.push(PathSegment::crate_root(syntax_pos::DUMMY_SP));


DUMMY_SP should not be used in anything that goes through name resolution - macro hygiene is span-based.
Actual span needs to be used even if it's zero length.

~~(This may be the reason of ICE in the proc macro test, but I haven't checked.)~~

petrochenkov · 2017-11-19T12:00:59Z

src/librustc_resolve/lib.rs

+                    span: prefix.span.to(use_tree.prefix.span),
+                };
+
+                // Prefix the path with `::` if doesn't start with a crate root or segment keyword


This should be removed, because default_to_global was restored.

petrochenkov · 2017-11-19T12:11:07Z

src/librustc_resolve/build_reduced_graph.rs

+            .map(|seg| respan(seg.span, seg.identifier))
+            .collect();
+
+        // Prefix the module path with `::` if doesn't start with a crate root or segment keyword


default_to_global was restored so this isn't necessary too.

bors · 2017-11-21T15:13:27Z

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

petrochenkov · 2017-11-21T18:47:53Z

src/librustc_resolve/build_reduced_graph.rs

@@ -243,6 +243,22 @@ impl<'a> Resolver<'a> {
                }

                for &(ref tree, id) in items {
+                    // Ensure use keywords or absolute imports aren't used in a nested list
+                    let subprefix = &tree.prefix.segments;


Looks like this commit duplicates 90f5cfd, but only for import paths.

bors · 2017-11-29T12:43:46Z

📌 Commit cb213ef has been approved by petrochenkov

bors · 2017-11-30T04:02:12Z

⌛ Testing commit cb213ef5ba6f65e846091fa59c5523a8f37fcf36 with merge b48013cebad8ec4ebb4ce75b3db17096e24cf1d0...

bors · 2017-11-30T05:26:25Z

💔 Test failed - status-travis

petrochenkov · 2017-11-30T08:29:37Z

Of course, rustfmt is broken.
This can be fixed by setting rustfmt and rls to "Broken" in https://github.com/rust-lang/rust/blob/master/src/tools/toolstate.toml

It's also desirable to send a PR to rustfmt fixing the breakage.

This commit adds support for nested groups inside `use` declarations, such as `use foo::{bar, sub::{baz::Foo, *}};`.

pietroalbini · 2017-11-30T12:27:38Z

Ok, marked rustfmt and rls as broken. I'll look into sending a PR to rustfmt.

@bors r=petrochenkov

bors · 2017-11-30T12:27:38Z

📌 Commit f7f6951 has been approved by petrochenkov

bors · 2017-12-01T03:26:00Z

⌛ Testing commit f7f6951 with merge 804b15b...

@petrochenkov

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

bors · 2017-12-01T06:06:00Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 804b15b to master...

theduke · 2017-12-01T08:28:27Z

@pietroalbini thanks for working on this, I've been waiting for the feature.

pietroalbini · 2017-12-01T08:53:16Z

🎉 🎉 🎉 🎉

Thank you @eddyb, @petrochenkov and @nikomatsakis for helping me during the implementation!

syntax: Make imports in AST closer to the source and cleanup their parsing This is a continuation of #45846 in some sense.

rust-highfive assigned petrochenkov Nov 7, 2017

pietroalbini force-pushed the use-nested-groups branch from ff1eb91 to 73011db Compare November 7, 2017 22:39

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 8, 2017

petrochenkov reviewed Nov 9, 2017

View reviewed changes

pietroalbini force-pushed the use-nested-groups branch from 0b88045 to b67f99f Compare November 10, 2017 11:56

petrochenkov reviewed Nov 11, 2017

View reviewed changes

petrochenkov reviewed Nov 12, 2017

View reviewed changes

src/libsyntax/parse/parser.rs

}

} else {

// `foo::bar` or `foo::bar as baz`

Copy link

Contributor

petrochenkov Nov 12, 2017

Choose a reason for hiding this comment

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

Argh, deleted comment!

petrochenkov reviewed Nov 12, 2017

View reviewed changes

pietroalbini force-pushed the use-nested-groups branch 2 times, most recently from 3c85490 to b074edb Compare November 12, 2017 11:29

petrochenkov reviewed Nov 12, 2017

View reviewed changes

shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2017

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 18, 2017

petrochenkov reviewed Nov 19, 2017

View reviewed changes

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 19, 2017

pietroalbini force-pushed the use-nested-groups branch 2 times, most recently from 10ea27e to e46969b Compare November 21, 2017 18:02

petrochenkov reviewed Nov 21, 2017

View reviewed changes

kennytm 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 29, 2017

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 30, 2017

pietroalbini added 2 commits November 30, 2017 13:10

Implement RFC 2128 (use_nested_groups)

91ba8b4

This commit adds support for nested groups inside `use` declarations, such as `use foo::{bar, sub::{baz::Foo, *}};`.

Mark rustfmt and rls as broken

f7f6951

pietroalbini force-pushed the use-nested-groups branch from cb213ef to f7f6951 Compare November 30, 2017 12:25

kennytm 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 30, 2017

pietroalbini mentioned this pull request Nov 30, 2017

Fix broken build after use_nested_groups lands on nightly rust-lang/rustfmt#2218

Merged

bors added a commit that referenced this pull request Dec 1, 2017

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

804b15b

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

bors merged commit f7f6951 into rust-lang:master Dec 1, 2017

bors mentioned this pull request Dec 1, 2017

Generic Associated Types Parsing & Name Resolution #45904

Merged

3 tasks

pietroalbini deleted the use-nested-groups branch December 1, 2017 08:46

DSpeckhals mentioned this pull request Dec 4, 2017

toolchain 'nightly-x86_64-unknown-linux-gnu' does not contain component 'rls-preview' for target 'x86_64-unknown-linux-gnu' rust-lang/rls#535

Closed

nikomatsakis mentioned this pull request Dec 20, 2017

Epoch.Next Tracking Issue #46889

Closed

petrochenkov mentioned this pull request Mar 10, 2018

syntax: Make imports in AST closer to the source and cleanup their parsing #48917

Merged

bors added a commit that referenced this pull request Mar 18, 2018

Auto merge of #48917 - petrochenkov:import, r=oli-obk

5e3ecdc

syntax: Make imports in AST closer to the source and cleanup their parsing This is a continuation of #45846 in some sense.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nested groups in imports #45846

Add nested groups in imports #45846

pietroalbini commented Nov 7, 2017 •

edited

Loading

rust-highfive commented Nov 7, 2017

pietroalbini commented Nov 7, 2017

petrochenkov Nov 9, 2017

petrochenkov Nov 9, 2017

petrochenkov Nov 9, 2017

petrochenkov Nov 9, 2017

petrochenkov Nov 11, 2017

petrochenkov Nov 12, 2017 •

edited

Loading

petrochenkov Nov 12, 2017

petrochenkov Nov 12, 2017

bors commented Nov 12, 2017

petrochenkov Nov 12, 2017

shepmaster commented Nov 18, 2017

pietroalbini commented Nov 18, 2017

petrochenkov Nov 19, 2017

petrochenkov Nov 19, 2017 •

edited

Loading

petrochenkov Nov 19, 2017

petrochenkov Nov 19, 2017

bors commented Nov 21, 2017

petrochenkov Nov 21, 2017

bors commented Nov 29, 2017

bors commented Nov 30, 2017

bors commented Nov 30, 2017

petrochenkov commented Nov 30, 2017

pietroalbini commented Nov 30, 2017

bors commented Nov 30, 2017

bors commented Dec 1, 2017

bors commented Dec 1, 2017

theduke commented Dec 1, 2017

pietroalbini commented Dec 1, 2017

		} else {
		let prefix = self.parse_path(PathStyle::Mod)?.default_to_global();

Add nested groups in imports #45846

Add nested groups in imports #45846

Conversation

pietroalbini commented Nov 7, 2017 • edited Loading

rust-highfive commented Nov 7, 2017

pietroalbini commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 12, 2017

Choose a reason for hiding this comment

shepmaster commented Nov 18, 2017

pietroalbini commented Nov 18, 2017

Choose a reason for hiding this comment

petrochenkov Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 21, 2017

Choose a reason for hiding this comment

bors commented Nov 29, 2017

bors commented Nov 30, 2017

bors commented Nov 30, 2017

petrochenkov commented Nov 30, 2017

pietroalbini commented Nov 30, 2017

bors commented Nov 30, 2017

bors commented Dec 1, 2017

bors commented Dec 1, 2017

theduke commented Dec 1, 2017

pietroalbini commented Dec 1, 2017

pietroalbini commented Nov 7, 2017 •

edited

Loading

petrochenkov Nov 12, 2017 •

edited

Loading

petrochenkov Nov 19, 2017 •

edited

Loading