From 20376b5baf7aacc4562ab7b3db6316b9d655ab58 Mon Sep 17 00:00:00 2001 From: Ruairidh Williamson Date: Fri, 30 Aug 2024 20:33:36 +0100 Subject: [PATCH] Use maybe_unused_trait_imports --- clippy_lints/src/anon_trait_import.rs | 159 ++------------------------ tests/ui/anon_trait_import.fixed | 27 +++-- tests/ui/anon_trait_import.rs | 23 ++-- tests/ui/anon_trait_import.stderr | 2 +- 4 files changed, 40 insertions(+), 171 deletions(-) diff --git a/clippy_lints/src/anon_trait_import.rs b/clippy_lints/src/anon_trait_import.rs index e9b544f28571..e4c5923906b0 100644 --- 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 { - 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 index b4e128ebb3d7..2bbcd9a00b5a 100644 --- 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 index 6d29acd32c27..8f9fb8b29ad4 100644 --- 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 index 50f2167ed7ad..8e45cf0f572c 100644 --- 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 _`