Rollup merge of #123344 - pietroalbini:pa-unused-imports, r=Nilstrieb

Remove braces when fixing a nested use tree into a single item

[Back in 2019](https://github.com/rust-lang/rust/pull/56645) I added rustfix support for the `unused_imports` lint, to automatically remove them when running `cargo fix`. For the most part this worked great, but when removing all but one childs of a nested use tree it turned `use foo::{Unused, Used}` into `use foo::{Used}`. This is slightly annoying, because it then requires you to run `rustfmt` to get `use foo::Used`.

This PR automatically removes braces and the surrouding whitespace when all but one child of a nested use tree are unused. To get it done I had to add the span of the nested use tree to the AST, and refactor a bit the code I wrote back then.

A thing I noticed is, there doesn't seem to be any `//@ run-rustfix` test for fixing the `unused_imports` lint. I created a test in `tests/suggestions` (is that the right directory?) that for now tests just what I added in the PR. I can followup in a separate PR to add more tests for fixing `unused_lints`.

This PR is best reviewed commit-by-commit.

compiler/rustc_ast/src/ast.rs

@@ -2729,7 +2729,7 @@ pub enum UseTreeKind {
     /// `use prefix` or `use prefix as rename`
     Simple(Option<Ident>),
     /// `use prefix::{...}`
-    Nested(ThinVec<(UseTree, NodeId)>),
+    Nested { items: ThinVec<(UseTree, NodeId)>, span: Span },
     /// `use prefix::*`
     Glob,
 }

compiler/rustc_ast/src/mut_visit.rs

@@ -441,7 +441,7 @@ fn noop_visit_use_tree<T: MutVisitor>(use_tree: &mut UseTree, vis: &mut T) {
     vis.visit_path(prefix);
     match kind {
         UseTreeKind::Simple(rename) => visit_opt(rename, |rename| vis.visit_ident(rename)),
-        UseTreeKind::Nested(items) => {
+        UseTreeKind::Nested { items, .. } => {
             for (tree, id) in items {
                 vis.visit_use_tree(tree);
                 vis.visit_id(id);

compiler/rustc_ast/src/visit.rs

@@ -517,8 +517,8 @@ pub fn walk_use_tree<'a, V: Visitor<'a>>(
             visit_opt!(visitor, visit_ident, rename);
         }
         UseTreeKind::Glob => {}
-        UseTreeKind::Nested(ref use_trees) => {
-            for &(ref nested_tree, nested_id) in use_trees {
+        UseTreeKind::Nested { ref items, .. } => {
+            for &(ref nested_tree, nested_id) in items {
                 try_visit!(visitor.visit_use_tree(nested_tree, nested_id, true));
             }
         }

compiler/rustc_ast_lowering/src/item.rs

@@ -135,8 +135,8 @@ impl<'hir> LoweringContext<'_, 'hir> {
 
     fn lower_item_id_use_tree(&mut self, tree: &UseTree, vec: &mut SmallVec<[hir::ItemId; 1]>) {
         match &tree.kind {
-            UseTreeKind::Nested(nested_vec) => {
-                for &(ref nested, id) in nested_vec {
+            UseTreeKind::Nested { items, .. } => {
+                for &(ref nested, id) in items {
                     vec.push(hir::ItemId {
                         owner_id: hir::OwnerId { def_id: self.local_def_id(id) },
                     });
@@ -518,7 +518,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
                 let path = self.lower_use_path(res, &path, ParamMode::Explicit);
                 hir::ItemKind::Use(path, hir::UseKind::Glob)
             }
-            UseTreeKind::Nested(ref trees) => {
+            UseTreeKind::Nested { items: ref trees, .. } => {
                 // Nested imports are desugared into simple imports.
                 // So, if we start with
                 //

compiler/rustc_ast_pretty/src/pprust/state/item.rs

@@ -715,7 +715,7 @@ impl<'a> State<'a> {
                 }
                 self.word("*");
             }
-            ast::UseTreeKind::Nested(items) => {
+            ast::UseTreeKind::Nested { items, .. } => {
                 if !tree.prefix.segments.is_empty() {
                     self.print_path(&tree.prefix, false, 0);
                     self.word("::");
@@ -734,7 +734,7 @@ impl<'a> State<'a> {
                         self.print_use_tree(&use_tree.0);
                         if !is_last {
                             self.word(",");
-                            if let ast::UseTreeKind::Nested(_) = use_tree.0.kind {
+                            if let ast::UseTreeKind::Nested { .. } = use_tree.0.kind {
                                 self.hardbreak();
                             } else {
                                 self.space();

compiler/rustc_builtin_macros/src/assert/context.rs

@@ -120,10 +120,13 @@ impl<'cx, 'a> Context<'cx, 'a> {
                 thin_vec![self.cx.attr_nested_word(sym::allow, sym::unused_imports, self.span)],
                 ItemKind::Use(UseTree {
                     prefix: self.cx.path(self.span, self.cx.std_path(&[sym::asserting])),
-                    kind: UseTreeKind::Nested(thin_vec![
-                        nested_tree(self, sym::TryCaptureGeneric),
-                        nested_tree(self, sym::TryCapturePrintable),
-                    ]),
+                    kind: UseTreeKind::Nested {
+                        items: thin_vec![
+                            nested_tree(self, sym::TryCaptureGeneric),
+                            nested_tree(self, sym::TryCapturePrintable),
+                        ],
+                        span: self.span,
+                    },
                     span: self.span,
                 }),
             ),

compiler/rustc_expand/src/expand.rs

@@ -1194,8 +1194,8 @@ impl InvocationCollectorNode for P<ast::Item> {
                 match &ut.kind {
                     ast::UseTreeKind::Glob => {}
                     ast::UseTreeKind::Simple(_) => idents.push(ut.ident()),
-                    ast::UseTreeKind::Nested(nested) => {
-                        for (ut, _) in nested {
+                    ast::UseTreeKind::Nested { items, .. } => {
+                        for (ut, _) in items {
                             collect_use_tree_leaves(ut, idents);
                         }
                     }

compiler/rustc_lint/src/unused.rs

@@ -1501,7 +1501,7 @@ declare_lint_pass!(UnusedImportBraces => [UNUSED_IMPORT_BRACES]);
 
 impl UnusedImportBraces {
     fn check_use_tree(&self, cx: &EarlyContext<'_>, use_tree: &ast::UseTree, item: &ast::Item) {
-        if let ast::UseTreeKind::Nested(ref items) = use_tree.kind {
+        if let ast::UseTreeKind::Nested { ref items, .. } = use_tree.kind {
             // Recursively check nested UseTrees
             for (tree, _) in items {
                 self.check_use_tree(cx, tree, item);
@@ -1522,7 +1522,7 @@ impl UnusedImportBraces {
                     rename.unwrap_or(orig_ident).name
                 }
                 ast::UseTreeKind::Glob => Symbol::intern("*"),
-                ast::UseTreeKind::Nested(_) => return,
+                ast::UseTreeKind::Nested { .. } => return,
             };
 
             cx.emit_span_lint(

compiler/rustc_parse/src/parser/item.rs

@@ -336,7 +336,7 @@ impl<'a> Parser<'a> {
                 UseTreeKind::Glob => {
                     e.note("the wildcard token must be last on the path");
                 }
-                UseTreeKind::Nested(..) => {
+                UseTreeKind::Nested { .. } => {
                     e.note("glob-like brace syntax must be last on the path");
                 }
                 _ => (),
@@ -1056,7 +1056,11 @@ impl<'a> Parser<'a> {
         Ok(if self.eat(&token::BinOp(token::Star)) {
             UseTreeKind::Glob
         } else {
-            UseTreeKind::Nested(self.parse_use_tree_list()?)
+            let lo = self.token.span;
+            UseTreeKind::Nested {
+                items: self.parse_use_tree_list()?,
+                span: lo.to(self.prev_token.span),
+            }
         })
     }
 

compiler/rustc_resolve/src/build_reduced_graph.rs

@@ -560,7 +560,7 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
 
                 self.add_import(prefix, kind, use_tree.span, item, root_span, item.id, vis);
             }
-            ast::UseTreeKind::Nested(ref items) => {
+            ast::UseTreeKind::Nested { ref items, .. } => {
                 // Ensure there is at most one `self` in the list
                 let self_spans = items
                     .iter()

compiler/rustc_resolve/src/check_unused.rs

@@ -128,7 +128,7 @@ impl<'a, 'b, 'tcx> UnusedImportCheckVisitor<'a, 'b, 'tcx> {
                     self.unused_import(self.base_id).add(id);
                 }
             }
-            ast::UseTreeKind::Nested(ref items) => self.check_imports_as_underscore(items),
+            ast::UseTreeKind::Nested { ref items, .. } => self.check_imports_as_underscore(items),
             _ => {}
         }
     }
@@ -254,7 +254,7 @@ impl<'a, 'b, 'tcx> Visitor<'a> for UnusedImportCheckVisitor<'a, 'b, 'tcx> {
             return;
         }
 
-        if let ast::UseTreeKind::Nested(ref items) = use_tree.kind {
+        if let ast::UseTreeKind::Nested { ref items, .. } = use_tree.kind {
             if items.is_empty() {
                 self.unused_import(self.base_id).add(id);
             }
@@ -268,9 +268,8 @@ impl<'a, 'b, 'tcx> Visitor<'a> for UnusedImportCheckVisitor<'a, 'b, 'tcx> {
 
 enum UnusedSpanResult {
     Used,
-    FlatUnused(Span, Span),
-    NestedFullUnused(Vec<Span>, Span),
-    NestedPartialUnused(Vec<Span>, Vec<Span>),
+    Unused { spans: Vec<Span>, remove: Span },
+    PartialUnused { spans: Vec<Span>, remove: Vec<Span> },
 }
 
 fn calc_unused_spans(
@@ -288,36 +287,33 @@ fn calc_unused_spans(
     match use_tree.kind {
         ast::UseTreeKind::Simple(..) | ast::UseTreeKind::Glob => {
             if unused_import.unused.contains(&use_tree_id) {
-                UnusedSpanResult::FlatUnused(use_tree.span, full_span)
+                UnusedSpanResult::Unused { spans: vec![use_tree.span], remove: full_span }
             } else {
                 UnusedSpanResult::Used
             }
         }
-        ast::UseTreeKind::Nested(ref nested) => {
+        ast::UseTreeKind::Nested { items: ref nested, span: tree_span } => {
             if nested.is_empty() {
-                return UnusedSpanResult::FlatUnused(use_tree.span, full_span);
+                return UnusedSpanResult::Unused { spans: vec![use_tree.span], remove: full_span };
             }
 
             let mut unused_spans = Vec::new();
             let mut to_remove = Vec::new();
-            let mut all_nested_unused = true;
+            let mut used_childs = 0;
+            let mut contains_self = false;
             let mut previous_unused = false;
             for (pos, (use_tree, use_tree_id)) in nested.iter().enumerate() {
                 let remove = match calc_unused_spans(unused_import, use_tree, *use_tree_id) {
                     UnusedSpanResult::Used => {
-                        all_nested_unused = false;
+                        used_childs += 1;
                         None
                     }
-                    UnusedSpanResult::FlatUnused(span, remove) => {
-                        unused_spans.push(span);
-                        Some(remove)
-                    }
-                    UnusedSpanResult::NestedFullUnused(mut spans, remove) => {
+                    UnusedSpanResult::Unused { mut spans, remove } => {
                         unused_spans.append(&mut spans);
                         Some(remove)
                     }
-                    UnusedSpanResult::NestedPartialUnused(mut spans, mut to_remove_extra) => {
-                        all_nested_unused = false;
+                    UnusedSpanResult::PartialUnused { mut spans, remove: mut to_remove_extra } => {
+                        used_childs += 1;
                         unused_spans.append(&mut spans);
                         to_remove.append(&mut to_remove_extra);
                         None
@@ -326,7 +322,7 @@ fn calc_unused_spans(
                 if let Some(remove) = remove {
                     let remove_span = if nested.len() == 1 {
                         remove
-                    } else if pos == nested.len() - 1 || !all_nested_unused {
+                    } else if pos == nested.len() - 1 || used_childs > 0 {
                         // Delete everything from the end of the last import, to delete the
                         // previous comma
                         nested[pos - 1].0.span.shrink_to_hi().to(use_tree.span)
@@ -344,14 +340,38 @@ fn calc_unused_spans(
                         to_remove.push(remove_span);
                     }
                 }
+                contains_self |= use_tree.prefix == kw::SelfLower
+                    && matches!(use_tree.kind, ast::UseTreeKind::Simple(None));
                 previous_unused = remove.is_some();
             }
             if unused_spans.is_empty() {
                 UnusedSpanResult::Used
-            } else if all_nested_unused {
-                UnusedSpanResult::NestedFullUnused(unused_spans, full_span)
+            } else if used_childs == 0 {
+                UnusedSpanResult::Unused { spans: unused_spans, remove: full_span }
             } else {
-                UnusedSpanResult::NestedPartialUnused(unused_spans, to_remove)
+                // If there is only one remaining child that is used, the braces around the use
+                // tree are not needed anymore. In that case, we determine the span of the left
+                // brace and the right brace, and tell rustfix to remove them as well.
+                //
+                // This means that `use a::{B, C};` will be turned into `use a::B;` rather than
+                // `use a::{B};`, removing a rustfmt roundtrip.
+                //
+                // Note that we cannot remove the braces if the only item inside the use tree is
+                // `self`: `use foo::{self};` is valid Rust syntax, while `use foo::self;` errors
+                // out. We also cannot turn `use foo::{self}` into `use foo`, as the former doesn't
+                // import types with the same name as the module.
+                if used_childs == 1 && !contains_self {
+                    // Left brace, from the start of the nested group to the first item.
+                    to_remove.push(
+                        tree_span.shrink_to_lo().to(nested.first().unwrap().0.span.shrink_to_lo()),
+                    );
+                    // Right brace, from the end of the last item to the end of the nested group.
+                    to_remove.push(
+                        nested.last().unwrap().0.span.shrink_to_hi().to(tree_span.shrink_to_hi()),
+                    );
+                }
+
+                UnusedSpanResult::PartialUnused { spans: unused_spans, remove: to_remove }
             }
         }
     }
@@ -417,15 +437,11 @@ impl Resolver<'_, '_> {
             let mut fixes = Vec::new();
             let spans = match calc_unused_spans(unused, &unused.use_tree, unused.use_tree_id) {
                 UnusedSpanResult::Used => continue,
-                UnusedSpanResult::FlatUnused(span, remove) => {
-                    fixes.push((remove, String::new()));
-                    vec![span]
-                }
-                UnusedSpanResult::NestedFullUnused(spans, remove) => {
+                UnusedSpanResult::Unused { spans, remove } => {
                     fixes.push((remove, String::new()));
                     spans
                 }
-                UnusedSpanResult::NestedPartialUnused(spans, remove) => {
+                UnusedSpanResult::PartialUnused { spans, remove } => {
                     for fix in &remove {
                         fixes.push((*fix, String::new()));
                     }

compiler/rustc_resolve/src/late.rs

@@ -2347,8 +2347,8 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
                     None => {}
                 }
             }
-        } else if let UseTreeKind::Nested(use_trees) = &use_tree.kind {
-            for (use_tree, _) in use_trees {
+        } else if let UseTreeKind::Nested { items, .. } = &use_tree.kind {
+            for (use_tree, _) in items {
                 self.future_proof_import(use_tree);
             }
         }
@@ -2525,7 +2525,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
             ItemKind::Use(ref use_tree) => {
                 let maybe_exported = match use_tree.kind {
                     UseTreeKind::Simple(_) | UseTreeKind::Glob => MaybeExported::Ok(item.id),
-                    UseTreeKind::Nested(_) => MaybeExported::NestedUse(&item.vis),
+                    UseTreeKind::Nested { .. } => MaybeExported::NestedUse(&item.vis),
                 };
                 self.resolve_doc_links(&item.attrs, maybe_exported);
 

src/tools/clippy/clippy_lints/src/single_component_path_imports.rs

@@ -201,8 +201,8 @@ impl SingleComponentPathImports {
 
                 if segments.is_empty() {
                     // keep track of `use {some_module, some_other_module};` usages
-                    if let UseTreeKind::Nested(trees) = &use_tree.kind {
-                        for tree in trees {
+                    if let UseTreeKind::Nested { items, .. } = &use_tree.kind {
+                        for tree in items {
                             let segments = &tree.0.prefix.segments;
                             if segments.len() == 1 {
                                 if let UseTreeKind::Simple(None) = tree.0.kind {
@@ -229,8 +229,8 @@ impl SingleComponentPathImports {
                         }
 
                         // nested case such as `use self::{module1::Struct1, module2::Struct2}`
-                        if let UseTreeKind::Nested(trees) = &use_tree.kind {
-                            for tree in trees {
+                        if let UseTreeKind::Nested { items, .. } = &use_tree.kind {
+                            for tree in items {
                                 let segments = &tree.0.prefix.segments;
                                 if !segments.is_empty() {
                                     imports_reused_with_self.push(segments[0].ident.name);

src/tools/clippy/clippy_lints/src/unnecessary_self_imports.rs

@@ -36,8 +36,8 @@ declare_lint_pass!(UnnecessarySelfImports => [UNNECESSARY_SELF_IMPORTS]);
 impl EarlyLintPass for UnnecessarySelfImports {
     fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) {
         if let ItemKind::Use(use_tree) = &item.kind
-            && let UseTreeKind::Nested(nodes) = &use_tree.kind
-            && let [(self_tree, _)] = &**nodes
+            && let UseTreeKind::Nested { items, .. } = &use_tree.kind
+            && let [(self_tree, _)] = &**items
             && let [self_seg] = &*self_tree.prefix.segments
             && self_seg.ident.name == kw::SelfLower
             && let Some(last_segment) = use_tree.prefix.segments.last()

src/tools/clippy/clippy_lints/src/unsafe_removed_from_name.rs

@@ -49,8 +49,8 @@ fn check_use_tree(use_tree: &UseTree, cx: &EarlyContext<'_>, span: Span) {
             unsafe_to_safe_check(old_name, new_name, cx, span);
         },
         UseTreeKind::Simple(None) | UseTreeKind::Glob => {},
-        UseTreeKind::Nested(ref nested_use_tree) => {
-            for (use_tree, _) in nested_use_tree {
+        UseTreeKind::Nested { ref items, .. } => {
+            for (use_tree, _) in items {
                 check_use_tree(use_tree, cx, span);
             }
         },

src/tools/clippy/clippy_utils/src/ast_utils.rs

@@ -648,7 +648,7 @@ pub fn eq_use_tree_kind(l: &UseTreeKind, r: &UseTreeKind) -> bool {
     match (l, r) {
         (Glob, Glob) => true,
         (Simple(l), Simple(r)) => both(l, r, |l, r| eq_id(*l, *r)),
-        (Nested(l), Nested(r)) => over(l, r, |(l, _), (r, _)| eq_use_tree(l, r)),
+        (Nested { items: l, .. }, Nested { items: r, .. }) => over(l, r, |(l, _), (r, _)| eq_use_tree(l, r)),
         _ => false,
     }
 }

src/tools/rustfmt/src/imports.rs

@@ -458,7 +458,9 @@ impl UseTree {
                     version,
                 });
             }
-            UseTreeKind::Nested(ref list) => {
+            UseTreeKind::Nested {
+                items: ref list, ..
+            } => {
                 // Extract comments between nested use items.
                 // This needs to be done before sorting use items.
                 let items = itemize_list(

tests/ui/suggestions/unused-imports.fixed

@@ -0,0 +1,35 @@
+//@ run-rustfix
+//@ check-pass
+
+#![warn(unused_imports)]
+
+pub mod nested {
+    pub struct A;
+    pub struct B;
+    pub struct C;
+    pub struct D;
+    pub mod even_more {
+        pub struct E;
+        pub struct F;
+        pub struct G;
+    }
+    pub mod another {
+        pub struct H;
+        pub struct I;
+    }
+}
+
+use nested::B;
+//~^ WARN unused import
+
+use nested::even_more::F;
+//~^^^^^^^ WARN unused import
+
+// Note that the following fix should result in `::{self}`, not `::self`. The latter is invalid
+// Rust syntax, so the braces should not be removed.
+use nested::another::{self};
+//~^ WARN unused import
+
+fn main() {
+    let _ = (B, F, another::I);
+}

tests/ui/suggestions/unused-imports.rs

@@ -0,0 +1,42 @@
+//@ run-rustfix
+//@ check-pass
+
+#![warn(unused_imports)]
+
+pub mod nested {
+    pub struct A;
+    pub struct B;
+    pub struct C;
+    pub struct D;
+    pub mod even_more {
+        pub struct E;
+        pub struct F;
+        pub struct G;
+    }
+    pub mod another {
+        pub struct H;
+        pub struct I;
+    }
+}
+
+use nested::{A, B, C};
+//~^ WARN unused import
+
+use nested::{
+    D,
+    even_more::{
+        E,
+        F,
+        G,
+                         },
+            };
+//~^^^^^^^ WARN unused import
+
+// Note that the following fix should result in `::{self}`, not `::self`. The latter is invalid
+// Rust syntax, so the braces should not be removed.
+use nested::another::{self, I};
+//~^ WARN unused import
+
+fn main() {
+    let _ = (B, F, another::I);
+}

tests/ui/suggestions/unused-imports.stderr

@@ -0,0 +1,32 @@
+warning: unused imports: `A`, `C`
+  --> $DIR/unused-imports.rs:22:14
+   |
+LL | use nested::{A, B, C};
+   |              ^     ^
+   |
+note: the lint level is defined here
+  --> $DIR/unused-imports.rs:4:9
+   |
+LL | #![warn(unused_imports)]
+   |         ^^^^^^^^^^^^^^
+
+warning: unused imports: `D`, `E`, `G`
+  --> $DIR/unused-imports.rs:26:5
+   |
+LL |     D,
+   |     ^
+LL |     even_more::{
+LL |         E,
+   |         ^
+LL |         F,
+LL |         G,
+   |         ^
+
+warning: unused import: `I`
+  --> $DIR/unused-imports.rs:37:29
+   |
+LL | use nested::another::{self, I};
+   |                             ^
+
+warning: 3 warnings emitted
+