Use maybe_unused_trait_imports

RuairidhWilliamson · RuairidhWilliamson · commit 20376b5baf7a · 2024-08-30T20:33:36.000+01:00
diff --git a/clippy_lints/src/anon_trait_import.rs b/clippy_lints/src/anon_trait_import.rs
@@ -1,15 +1,9 @@
-use std::ops::ControlFlow;
-
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_opt;
 use rustc_errors::Applicability;
 use rustc_hir::def::{DefKind, Res};
-use rustc_hir::def_id::DefId;
-use rustc_hir::definitions::DefPathData;
-use rustc_hir::intravisit::{walk_item, walk_mod, walk_path, Visitor};
-use rustc_hir::{HirId, Item, ItemKind, Node, Path, UseKind};
+use rustc_hir::{Item, ItemKind, UseKind};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::hir::nested_filter;
 use rustc_middle::ty::Visibility;
 use rustc_session::declare_lint_pass;
 use rustc_span::symbol::kw;
@@ -51,17 +45,15 @@ declare_lint_pass!(AnonTraitImport => [ANON_TRAIT_IMPORT]);
 
 impl<'tcx> LateLintPass<'tcx> for AnonTraitImport {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
-        let module = cx.tcx.parent_module_from_def_id(item.owner_id.def_id);
-        if cx.tcx.visibility(item.owner_id.def_id) != Visibility::Restricted(module.to_def_id()) {
-            return;
-        }
         if let ItemKind::Use(path, UseKind::Single) = item.kind
             // Ignore imports that already use Underscore
             && item.ident.name != kw::Underscore
-            && let Some(Res::Def(DefKind::Trait, def_id)) = path.res.first()
-            && let parent_id = cx.tcx.hir().get_parent_item(item.hir_id())
-            && let Node::Item(parent_item) = cx.tcx.hir_node_by_def_id(parent_id.def_id)
-            && !is_import_used_by_name_in_item(cx, *def_id, parent_item)
+            // Only check traits
+            && let Some(Res::Def(DefKind::Trait, _)) = path.res.first()
+            && cx.tcx.maybe_unused_trait_imports(()).contains(&item.owner_id.def_id)
+            // Only check this use if it is only visible to its module (no pub, pub(crate), ...)
+            && let module = cx.tcx.parent_module_from_def_id(item.owner_id.def_id)
+            && cx.tcx.visibility(item.owner_id.def_id) == Visibility::Restricted(module.to_def_id())
             && let Some(last_segment) = path.segments.last()
             && let Some(snip) = snippet_opt(cx, last_segment.ident.span)
         {
@@ -78,140 +70,3 @@ impl<'tcx> LateLintPass<'tcx> for AnonTraitImport {
         }
     }
 }
-
-fn is_import_used_by_name_in_item<'tcx>(cx: &LateContext<'tcx>, def_id: DefId, parent_item: &'tcx Item<'_>) -> bool {
-    let module = find_module(cx, parent_item);
-    let mut visitor = TraitUsedByNameVisitor {
-        cx,
-        id: def_id,
-        module,
-        module_depth: 0,
-    };
-    let result = if let ItemKind::Mod(parent_mod) = parent_item.kind {
-        visitor.visit_mod(parent_mod, parent_item.span, parent_item.hir_id())
-    } else {
-        visitor.visit_item(parent_item)
-    };
-    matches!(result, ControlFlow::Break(()))
-}
-
-fn find_module<'tcx>(cx: &LateContext<'tcx>, item: &'tcx Item<'_>) -> HirId {
-    if let ItemKind::Mod(_) = &item.kind {
-        return item.hir_id();
-    }
-    let parent = cx.tcx.hir().get_parent_item(item.hir_id());
-    let mut node = cx.tcx.hir_node_by_def_id(parent.def_id);
-    loop {
-        match node {
-            Node::Crate(r#mod) => return r#mod.item_ids[0].hir_id(),
-            Node::Item(item) => {
-                if let ItemKind::Mod(_) = &item.kind {
-                    return item.hir_id();
-                }
-                let hir_id = item.hir_id();
-                let parent = cx.tcx.hir().get_parent_item(hir_id);
-                node = cx.tcx.hir_node_by_def_id(parent.def_id);
-            },
-            _ => panic!("not an item or crate: {node:?}"),
-        };
-    }
-}
-
-struct TraitUsedByNameVisitor<'a, 'hir> {
-    cx: &'a LateContext<'hir>,
-    id: DefId,
-    module: HirId,
-    module_depth: usize,
-}
-
-impl<'a, 'hir> Visitor<'hir> for TraitUsedByNameVisitor<'a, 'hir> {
-    type NestedFilter = nested_filter::All;
-    type Result = ControlFlow<()>;
-
-    fn nested_visit_map(&mut self) -> Self::Map {
-        self.cx.tcx.hir()
-    }
-
-    fn visit_item(&mut self, item: &'hir Item<'_>) -> Self::Result {
-        match item.kind {
-            ItemKind::Mod(m) => {
-                self.module_depth += 1;
-                let result = walk_mod(self, m, item.hir_id());
-                self.module_depth -= 1;
-                result
-            },
-            _ => walk_item(self, item),
-        }
-    }
-
-    fn visit_path(&mut self, path: &Path<'hir>, _: HirId) -> Self::Result {
-        if self.module_depth > 0 {
-            if let Some(segment) = path.segments.first()
-                && segment.ident.name == kw::Crate
-            {
-                if let Some(mod_segment) = path.segments.get(path.segments.len() - 2)
-                    && Some(self.module.owner.to_def_id()) == mod_segment.res.mod_def_id()
-                    && path.res.opt_def_id() == Some(self.id)
-                {
-                    return ControlFlow::Break(());
-                }
-            } else {
-                let mut super_count = 0;
-                for segment in path.segments {
-                    if segment.ident.name == kw::Super {
-                        super_count += 1;
-                        if super_count > self.module_depth {
-                            break;
-                        }
-                    } else {
-                        if super_count == self.module_depth
-                            && (path.res.opt_def_id() == Some(self.id) || segment.res.opt_def_id() == Some(self.id))
-                        {
-                            return ControlFlow::Break(());
-                        }
-                        break;
-                    }
-                }
-            }
-        }
-        if let Some(def_id) = self.first_path_segment_def_id(path)
-            && def_id == self.id
-            && self.module_depth == 0
-        {
-            return ControlFlow::Break(());
-        }
-        walk_path(self, path)
-    }
-}
-
-impl<'hir> TraitUsedByNameVisitor<'_, 'hir> {
-    fn skip_def_id(&self, def_id: DefId) -> DefId {
-        let def_key = self.cx.tcx.def_key(def_id);
-        match def_key.disambiguated_data.data {
-            DefPathData::Ctor => {
-                if let Some(def_id) = self.cx.tcx.opt_parent(def_id) {
-                    self.skip_def_id(def_id)
-                } else {
-                    def_id
-                }
-            },
-            _ => def_id,
-        }
-    }
-
-    fn first_path_segment_def_id(&self, path: &Path<'_>) -> Option<DefId> {
-        path.res.opt_def_id().and_then(|mut def_id| {
-            def_id = self.skip_def_id(def_id);
-            for _ in path.segments.iter().skip(1) {
-                def_id = self.skip_def_id(def_id);
-                if let Some(parent_def_id) = self.cx.tcx.opt_parent(def_id) {
-                    def_id = parent_def_id;
-                } else {
-                    return None;
-                }
-            }
-
-            Some(def_id)
-        })
-    }
-}
diff --git a/tests/ui/anon_trait_import.fixed b/tests/ui/anon_trait_import.fixed
@@ -102,7 +102,7 @@ mod nested_mod_used_good3 {
     }
 }
 
-mod nested_mod_used_not_used_bad {
+mod nested_mod_used_bad {
     use std::any::Any as _;
 
     fn bar() {
@@ -118,15 +118,18 @@ mod nested_mod_used_not_used_bad {
     }
 }
 
-// Known limitation
-// This is currently a false negative because if we find any `use crate::...` for the trait then we
-// assume we can't anonymise the trait import
-/*
+// More complex example where `use std::any::Any;` should be anonymised but `use std::any::Any as
+// MyAny;` should not as it is used by a sub module. Even though if you removed `use std::any::Any;`
+// the code would still compile.
 mod nested_mod_used_bad1 {
-    use std::any::Any;
+    use std::any::Any as _;
 
     use std::any::Any as MyAny;
 
+    fn baz() {
+        println!("{:?}", "baz".type_id());
+    }
+
     mod foo {
         use crate::nested_mod_used_bad1::MyAny;
 
@@ -135,16 +138,20 @@ mod nested_mod_used_bad1 {
         }
     }
 }
-*/
 
-mod nested_mod_used_bad2 {
-    use std::any::Any as _;
+// Example of nested import with an unused import to try and trick it
+mod nested_mod_used_good5 {
+    use std::any::Any;
 
     mod foo {
         use std::any::Any;
 
+        fn baz() {
+            println!("{:?}", "baz".type_id());
+        }
+
         mod bar {
-            use crate::nested_mod_used_bad2::foo::Any;
+            use crate::nested_mod_used_good5::foo::Any;
 
             fn foo() {
                 println!("{:?}", Any::type_id("foo"));
diff --git a/tests/ui/anon_trait_import.rs b/tests/ui/anon_trait_import.rs
@@ -102,7 +102,7 @@ mod nested_mod_used_good3 {
     }
 }
 
-mod nested_mod_used_not_used_bad {
+mod nested_mod_used_bad {
     use std::any::Any;
 
     fn bar() {
@@ -118,15 +118,18 @@ mod nested_mod_used_not_used_bad {
     }
 }
 
-// Known limitation
-// This is currently a false negative because if we find any `use crate::...` for the trait then we
-// assume we can't anonymise the trait import
-/*
+// More complex example where `use std::any::Any;` should be anonymised but `use std::any::Any as
+// MyAny;` should not as it is used by a sub module. Even though if you removed `use std::any::Any;`
+// the code would still compile.
 mod nested_mod_used_bad1 {
     use std::any::Any;
 
     use std::any::Any as MyAny;
 
+    fn baz() {
+        println!("{:?}", "baz".type_id());
+    }
+
     mod foo {
         use crate::nested_mod_used_bad1::MyAny;
 
@@ -135,16 +138,20 @@ mod nested_mod_used_bad1 {
         }
     }
 }
-*/
 
-mod nested_mod_used_bad2 {
+// Example of nested import with an unused import to try and trick it
+mod nested_mod_used_good5 {
     use std::any::Any;
 
     mod foo {
         use std::any::Any;
 
+        fn baz() {
+            println!("{:?}", "baz".type_id());
+        }
+
         mod bar {
-            use crate::nested_mod_used_bad2::foo::Any;
+            use crate::nested_mod_used_good5::foo::Any;
 
             fn foo() {
                 println!("{:?}", Any::type_id("foo"));
diff --git a/tests/ui/anon_trait_import.stderr b/tests/ui/anon_trait_import.stderr
@@ -38,7 +38,7 @@ LL |     use std::any::Any;
    |                   ^^^ help: use: `Any as _`
 
 error: importing trait that is only used anonymously
-  --> tests/ui/anon_trait_import.rs:141:19
+  --> tests/ui/anon_trait_import.rs:125:19
    |
 LL |     use std::any::Any;
    |                   ^^^ help: use: `Any as _`

Original file line number	Diff line number	Diff line change
`@@ -38,7 +38,7 @@ LL \| use std::any::Any;`
`38`	`38`	\| ^^^ help: use: `Any as _`
`39`	`39`
`40`	`40`	`error: importing trait that is only used anonymously`
`41`		`- --> tests/ui/anon_trait_import.rs:141:19`
	`41`	`+ --> tests/ui/anon_trait_import.rs:125:19`
`42`	`42`	`\|`
`43`	`43`	`LL \| use std::any::Any;`
`44`	`44`	\| ^^^ help: use: `Any as _`