Auto merge of rust-lang#122165 - chenyukang:yukang-fix-121315-suggest…

…-removing, r=<try>

Merge RedundantImport into UnusedImport for suggesting removing spans

Fixes rust-lang#121315

I also changed the label of diagnostics for RedundantImport to be compatible with UnusedImport, or any other better ways?

r? `@petrochenkov`

compiler/rustc_lint/src/context/diagnostics.rs

@@ -124,7 +124,7 @@ pub(super) fn builtin(sess: &Session, diagnostic: BuiltinLintDiag, diag: &mut Di
         BuiltinLintDiag::UnknownCrateTypes(span, note, sugg) => {
             diag.span_suggestion(span, note, sugg, Applicability::MaybeIncorrect);
         }
-        BuiltinLintDiag::UnusedImports(message, replaces, in_test_module) => {
+        BuiltinLintDiag::UnusedImports(message, replaces, in_test_module, redundant_sources) => {
             if !replaces.is_empty() {
                 diag.tool_only_multipart_suggestion(
                     message,
@@ -139,15 +139,15 @@ pub(super) fn builtin(sess: &Session, diagnostic: BuiltinLintDiag, diag: &mut Di
                     "if this is a test module, consider adding a `#[cfg(test)]` to the containing module",
                 );
             }
-        }
-        BuiltinLintDiag::RedundantImport(spans, ident) => {
-            for (span, is_imported) in spans {
-                let introduced = if is_imported { "imported" } else { "defined" };
-                let span_msg = if span.is_dummy() { "by prelude" } else { "here" };
-                diag.span_label(
-                    span,
-                    format!("the item `{ident}` is already {introduced} {span_msg}"),
-                );
+            for (ident, spans) in redundant_sources {
+                for (span, is_imported) in spans {
+                    let introduced = if is_imported { "imported" } else { "defined" };
+                    let span_msg = if span.is_dummy() { "by prelude" } else { "here" };
+                    diag.span_label(
+                        span,
+                        format!("the item `{ident}` is already {introduced} {span_msg}"),
+                    );
+                }
             }
         }
         BuiltinLintDiag::DeprecatedMacro(suggestion, span) => {

compiler/rustc_lint_defs/src/lib.rs

@@ -576,8 +576,7 @@ pub enum BuiltinLintDiag {
     MacroExpandedMacroExportsAccessedByAbsolutePaths(Span),
     ElidedLifetimesInPaths(usize, Span, bool, Span),
     UnknownCrateTypes(Span, String, String),
-    UnusedImports(String, Vec<(Span, String)>, Option<Span>),
-    RedundantImport(Vec<(Span, bool)>, Ident),
+    UnusedImports(String, Vec<(Span, String)>, Option<Span>, Vec<(Ident, Vec<(Span, bool)>)>),
     DeprecatedMacro(Option<Symbol>, Span),
     MissingAbi(Span, Abi),
     UnusedDocComment(Span),

compiler/rustc_resolve/src/check_unused.rs

@@ -31,7 +31,6 @@ use crate::NameBindingKind;
 use rustc_ast as ast;
 use rustc_ast::visit::{self, Visitor};
 use rustc_data_structures::fx::{FxHashMap, FxIndexMap, FxIndexSet};
-use rustc_data_structures::unord::UnordSet;
 use rustc_errors::{pluralize, MultiSpan};
 use rustc_hir::def::{DefKind, Res};
 use rustc_session::lint::builtin::{MACRO_USE_EXTERN_CRATE, UNUSED_EXTERN_CRATES, UNUSED_IMPORTS};
@@ -43,7 +42,7 @@ struct UnusedImport {
     use_tree: ast::UseTree,
     use_tree_id: ast::NodeId,
     item_span: Span,
-    unused: UnordSet<ast::NodeId>,
+    unused: FxIndexSet<ast::NodeId>,
 }
 
 impl UnusedImport {
@@ -96,7 +95,7 @@ impl<'a, 'b, 'tcx> UnusedImportCheckVisitor<'a, 'b, 'tcx> {
             // FIXME(#120456) - is `swap_remove` correct?
             self.r.maybe_unused_trait_imports.swap_remove(&def_id);
             if let Some(i) = self.unused_imports.get_mut(&self.base_id) {
-                i.unused.remove(&id);
+                i.unused.swap_remove(&id);
             }
         }
     }
@@ -138,6 +137,16 @@ impl<'a, 'b, 'tcx> UnusedImportCheckVisitor<'a, 'b, 'tcx> {
         }
     }
 
+    fn merge_unused_import(&mut self, unused_import: UnusedImport) {
+        if let Some(import) = self.unused_imports.get_mut(&unused_import.use_tree_id) {
+            for id in unused_import.unused {
+                import.unused.insert(id);
+            }
+        } else {
+            self.unused_imports.entry(unused_import.use_tree_id).or_insert(unused_import);
+        }
+    }
+
     fn report_unused_extern_crate_items(
         &mut self,
         maybe_unused_extern_crates: FxHashMap<ast::NodeId, Span>,
@@ -265,6 +274,40 @@ impl<'a, 'b, 'tcx> Visitor<'a> for UnusedImportCheckVisitor<'a, 'b, 'tcx> {
     }
 }
 
+struct ImportFinderVisitor {
+    unused_import: Option<UnusedImport>,
+    root_node_id: ast::NodeId,
+    node_id: ast::NodeId,
+    item_span: Span,
+}
+
+impl Visitor<'_> for ImportFinderVisitor {
+    fn visit_item(&mut self, item: &ast::Item) {
+        match item.kind {
+            ast::ItemKind::Use(..) if item.span.is_dummy() => return,
+            _ => {}
+        }
+
+        self.item_span = item.span_with_attributes();
+        visit::walk_item(self, item);
+    }
+
+    fn visit_use_tree(&mut self, use_tree: &ast::UseTree, id: ast::NodeId, _nested: bool) {
+        if id == self.root_node_id {
+            let mut unused_import = UnusedImport {
+                use_tree: use_tree.clone(),
+                use_tree_id: id,
+                item_span: self.item_span,
+                unused: Default::default(),
+            };
+            unused_import.unused.insert(self.node_id);
+            self.unused_import = Some(unused_import);
+        }
+        visit::walk_use_tree(self, use_tree, id);
+    }
+}
+
+#[derive(Debug)]
 enum UnusedSpanResult {
     Used,
     FlatUnused(Span, Span),
@@ -412,6 +455,46 @@ impl Resolver<'_, '_> {
 
         visitor.report_unused_extern_crate_items(maybe_unused_extern_crates);
 
+        let unused_imports = &visitor.unused_imports;
+        let mut check_redundant_imports = FxIndexSet::default();
+        for module in visitor.r.arenas.local_modules().iter() {
+            for (_key, resolution) in visitor.r.resolutions(*module).borrow().iter() {
+                let resolution = resolution.borrow();
+
+                if let Some(binding) = resolution.binding
+                    && let NameBindingKind::Import { import, .. } = binding.kind
+                    && let ImportKind::Single { id, .. } = import.kind
+                {
+                    if let Some(unused_import) = unused_imports.get(&import.root_id)
+                        && unused_import.unused.contains(&id)
+                    {
+                        continue;
+                    }
+
+                    check_redundant_imports.insert(import);
+                }
+            }
+        }
+
+        let mut redundant_source_spans = FxHashMap::default();
+        for import in check_redundant_imports {
+            if let Some(redundant_spans) = visitor.r.check_for_redundant_imports(import)
+                && let ImportKind::Single { source, id, .. } = import.kind
+            {
+                let mut finder = ImportFinderVisitor {
+                    node_id: id,
+                    root_node_id: import.root_id,
+                    unused_import: None,
+                    item_span: Span::default(),
+                };
+                visit::walk_crate(&mut finder, krate);
+                if let Some(unused) = finder.unused_import {
+                    visitor.merge_unused_import(unused);
+                    redundant_source_spans.insert(id, (source, redundant_spans));
+                }
+            }
+        }
+
         for unused in visitor.unused_imports.values() {
             let mut fixes = Vec::new();
             let spans = match calc_unused_spans(unused, &unused.use_tree, unused.use_tree_id) {
@@ -432,19 +515,35 @@ impl Resolver<'_, '_> {
                 }
             };
 
-            let ms = MultiSpan::from_spans(spans);
+            let redundant_sources = unused
+                .unused
+                .clone()
+                .into_iter()
+                .filter_map(|id| redundant_source_spans.get(&id))
+                .cloned()
+                .collect();
 
-            let mut span_snippets = ms
+            let multi_span = MultiSpan::from_spans(spans);
+            let mut span_snippets = multi_span
                 .primary_spans()
                 .iter()
                 .filter_map(|span| tcx.sess.source_map().span_to_snippet(*span).ok())
                 .map(|s| format!("`{s}`"))
                 .collect::<Vec<String>>();
             span_snippets.sort();
 
+            let remove_type =
+                if unused.unused.iter().all(|i| redundant_source_spans.get(i).is_none()) {
+                    "unused import"
+                } else if unused.unused.iter().all(|i| redundant_source_spans.get(i).is_some()) {
+                    "redundant import"
+                } else {
+                    "unused or redundant import"
+                };
             let msg = format!(
-                "unused import{}{}",
-                pluralize!(ms.primary_spans().len()),
+                "{}{}{}",
+                remove_type,
+                pluralize!(multi_span.primary_spans().len()),
                 if !span_snippets.is_empty() {
                     format!(": {}", span_snippets.join(", "))
                 } else {
@@ -453,11 +552,11 @@ impl Resolver<'_, '_> {
             );
 
             let fix_msg = if fixes.len() == 1 && fixes[0].0 == unused.item_span {
-                "remove the whole `use` item"
-            } else if ms.primary_spans().len() > 1 {
-                "remove the unused imports"
+                "remove the whole `use` item".to_owned()
+            } else if multi_span.primary_spans().len() > 1 {
+                format!("remove the {}s", remove_type)
             } else {
-                "remove the unused import"
+                format!("remove the {}", remove_type)
             };
 
             // If we are in the `--test` mode, suppress a help that adds the `#[cfg(test)]`
@@ -487,35 +586,15 @@ impl Resolver<'_, '_> {
             visitor.r.lint_buffer.buffer_lint_with_diagnostic(
                 UNUSED_IMPORTS,
                 unused.use_tree_id,
-                ms,
+                multi_span,
                 msg,
-                BuiltinLintDiag::UnusedImports(fix_msg.into(), fixes, test_module_span),
+                BuiltinLintDiag::UnusedImports(
+                    fix_msg.into(),
+                    fixes,
+                    test_module_span,
+                    redundant_sources,
+                ),
             );
         }
-
-        let unused_imports = visitor.unused_imports;
-        let mut check_redundant_imports = FxIndexSet::default();
-        for module in self.arenas.local_modules().iter() {
-            for (_key, resolution) in self.resolutions(*module).borrow().iter() {
-                let resolution = resolution.borrow();
-
-                if let Some(binding) = resolution.binding
-                    && let NameBindingKind::Import { import, .. } = binding.kind
-                    && let ImportKind::Single { id, .. } = import.kind
-                {
-                    if let Some(unused_import) = unused_imports.get(&import.root_id)
-                        && unused_import.unused.contains(&id)
-                    {
-                        continue;
-                    }
-
-                    check_redundant_imports.insert(import);
-                }
-            }
-        }
-
-        for import in check_redundant_imports {
-            self.check_for_redundant_imports(import);
-        }
     }
 }

compiler/rustc_resolve/src/imports.rs

@@ -1306,7 +1306,10 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         None
     }
 
-    pub(crate) fn check_for_redundant_imports(&mut self, import: Import<'a>) {
+    pub(crate) fn check_for_redundant_imports(
+        &mut self,
+        import: Import<'a>,
+    ) -> Option<Vec<(Span, bool)>> {
         // This function is only called for single imports.
         let ImportKind::Single {
             source, target, ref source_bindings, ref target_bindings, id, ..
@@ -1317,12 +1320,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
 
         // Skip if the import is of the form `use source as target` and source != target.
         if source != target {
-            return;
+            return None;
         }
 
         // Skip if the import was produced by a macro.
         if import.parent_scope.expansion != LocalExpnId::ROOT {
-            return;
+            return None;
         }
 
         // Skip if we are inside a named module (in contrast to an anonymous
@@ -1332,7 +1335,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         if import.used.get() == Some(Used::Other)
             || self.effective_visibilities.is_exported(self.local_def_id(id))
         {
-            return;
+            return None;
         }
 
         let mut is_redundant = true;
@@ -1368,14 +1371,9 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             let mut redundant_spans: Vec<_> = redundant_span.present_items().collect();
             redundant_spans.sort();
             redundant_spans.dedup();
-            self.lint_buffer.buffer_lint_with_diagnostic(
-                UNUSED_IMPORTS,
-                id,
-                import.span,
-                format!("the item `{source}` is imported redundantly"),
-                BuiltinLintDiag::RedundantImport(redundant_spans, source),
-            );
+            return Some(redundant_spans);
         }
+        None
     }
 
     fn resolve_glob_import(&mut self, import: Import<'a>) {

tests/ui/imports/redundant-import-issue-121915-2015.rs

@@ -6,6 +6,6 @@ extern crate aux_issue_121915;
 #[deny(unused_imports)]
 fn main() {
     use aux_issue_121915;
-    //~^ ERROR the item `aux_issue_121915` is imported redundantly
+    //~^ ERROR redundant import
     aux_issue_121915::item();
 }

tests/ui/imports/redundant-import-issue-121915-2015.stderr

@@ -1,4 +1,4 @@
-error: the item `aux_issue_121915` is imported redundantly
+error: redundant import: `aux_issue_121915`
   --> $DIR/redundant-import-issue-121915-2015.rs:8:9
    |
 LL | extern crate aux_issue_121915;

tests/ui/imports/redundant-import-issue-121915.rs

@@ -4,6 +4,6 @@
 #[deny(unused_imports)]
 fn main() {
     use aux_issue_121915;
-    //~^ ERROR the item `aux_issue_121915` is imported redundantly
+    //~^ ERROR redundant import
     aux_issue_121915::item();
 }

tests/ui/imports/redundant-import-issue-121915.stderr

@@ -1,4 +1,4 @@
-error: the item `aux_issue_121915` is imported redundantly
+error: redundant import: `aux_issue_121915`
   --> $DIR/redundant-import-issue-121915.rs:6:9
    |
 LL |     use aux_issue_121915;

tests/ui/imports/suggest-remove-issue-121315.fixed

@@ -0,0 +1,39 @@
+//@ run-rustfix
+//@ compile-flags: --edition 2021
+#![deny(unused_imports)]
+#![allow(dead_code)]
+
+fn test0() {
+    // Test remove FlatUnused
+
+    //~^ ERROR redundant import
+    let _ = u32::try_from(5i32);
+}
+
+fn test1() {
+    // Test remove NestedFullUnused
+
+    //~^ ERROR redundant imports
+
+    let _ = u32::try_from(5i32);
+    let _a: i32 = u32::try_into(5u32).unwrap();
+}
+
+fn test2() {
+    // Test remove both redundant and unused
+
+    //~^ ERROR unused or redundant imports: `AsMut`, `Into`
+
+    let _a: u32 = (5u8).into();
+}
+
+fn test3() {
+    // Test remove NestedPartialUnused
+    use std::convert::{Infallible};
+    //~^ ERROR unused import: `From`
+
+    trait MyTrait {}
+    impl MyTrait for fn() -> Infallible {}
+}
+
+fn main() {}