From 1299aa7c18377dfbe43095b5be4ff2c0161eae8d Mon Sep 17 00:00:00 2001 From: r0cky Date: Thu, 29 Feb 2024 14:08:01 +0800 Subject: [PATCH 1/2] Detect unused struct impls pub trait --- compiler/rustc_passes/src/dead.rs | 96 ++++++++++++++++--- tests/ui/lint/dead-code/issue-41883.stderr | 2 - ...tiple-dead-codes-in-the-same-struct.stderr | 2 - .../dead-code/unused-adt-impls-pub-trait.rs | 17 ++++ .../unused-adt-impls-pub-trait.stderr | 14 +++ .../ui/rust-2018/uniform-paths/issue-55779.rs | 2 +- .../ui/traits/augmented-assignments-trait.rs | 2 +- tests/ui/traits/issue-33187.rs | 2 +- 8 files changed, 119 insertions(+), 18 deletions(-) create mode 100644 tests/ui/lint/dead-code/unused-adt-impls-pub-trait.rs create mode 100644 tests/ui/lint/dead-code/unused-adt-impls-pub-trait.stderr diff --git a/compiler/rustc_passes/src/dead.rs b/compiler/rustc_passes/src/dead.rs index a3106856a67..cdfde2b9405 100644 --- a/compiler/rustc_passes/src/dead.rs +++ b/compiler/rustc_passes/src/dead.rs @@ -15,7 +15,7 @@ use rustc_hir::{Node, PatKind, TyKind}; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::middle::privacy::Level; use rustc_middle::query::Providers; -use rustc_middle::ty::{self, TyCtxt, Visibility}; +use rustc_middle::ty::{self, TyCtxt}; use rustc_session::lint; use rustc_session::lint::builtin::DEAD_CODE; use rustc_span::symbol::{sym, Symbol}; @@ -45,6 +45,18 @@ fn should_explore(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool { ) } +fn ty_ref_to_pub_struct(tcx: TyCtxt<'_>, ty: &hir::Ty<'_>) -> bool { + if let TyKind::Path(hir::QPath::Resolved(_, path)) = ty.kind + && let Res::Def(def_kind, def_id) = path.res + && def_id.is_local() + && matches!(def_kind, DefKind::Struct | DefKind::Enum | DefKind::Union) + { + tcx.visibility(def_id).is_public() + } else { + true + } +} + /// Determine if a work from the worklist is coming from the a `#[allow]` /// or a `#[expect]` of `dead_code` #[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] @@ -415,6 +427,13 @@ impl<'tcx> MarkSymbolVisitor<'tcx> { && let ItemKind::Impl(impl_ref) = self.tcx.hir().expect_item(local_impl_id).kind { + if self.tcx.visibility(trait_id).is_public() + && matches!(trait_item.kind, hir::TraitItemKind::Fn(..)) + && !ty_ref_to_pub_struct(self.tcx, impl_ref.self_ty) + { + continue; + } + // mark self_ty live intravisit::walk_ty(self, impl_ref.self_ty); if let Some(&impl_item_id) = @@ -465,6 +484,36 @@ impl<'tcx> MarkSymbolVisitor<'tcx> { } } } + + fn solve_rest_impl_items(&mut self, mut unsolved_impl_items: Vec<(hir::ItemId, LocalDefId)>) { + let mut ready; + (ready, unsolved_impl_items) = unsolved_impl_items + .into_iter() + .partition(|&(impl_id, _)| self.impl_item_with_used_self(impl_id)); + + while !ready.is_empty() { + self.worklist = + ready.into_iter().map(|(_, id)| (id, ComesFromAllowExpect::No)).collect(); + self.mark_live_symbols(); + + (ready, unsolved_impl_items) = unsolved_impl_items + .into_iter() + .partition(|&(impl_id, _)| self.impl_item_with_used_self(impl_id)); + } + } + + fn impl_item_with_used_self(&mut self, impl_id: hir::ItemId) -> bool { + if let TyKind::Path(hir::QPath::Resolved(_, path)) = + self.tcx.hir().item(impl_id).expect_impl().self_ty.kind + && let Res::Def(def_kind, def_id) = path.res + && let Some(local_def_id) = def_id.as_local() + && matches!(def_kind, DefKind::Struct | DefKind::Enum | DefKind::Union) + { + self.live_symbols.contains(&local_def_id) + } else { + false + } + } } impl<'tcx> Visitor<'tcx> for MarkSymbolVisitor<'tcx> { @@ -652,6 +701,7 @@ fn check_item<'tcx>( tcx: TyCtxt<'tcx>, worklist: &mut Vec<(LocalDefId, ComesFromAllowExpect)>, struct_constructors: &mut LocalDefIdMap, + unsolved_impl_items: &mut Vec<(hir::ItemId, LocalDefId)>, id: hir::ItemId, ) { let allow_dead_code = has_allow_dead_code_or_lang_attr(tcx, id.owner_id.def_id); @@ -683,16 +733,33 @@ fn check_item<'tcx>( .iter() .filter_map(|def_id| def_id.as_local()); + let ty_is_pub = ty_ref_to_pub_struct(tcx, tcx.hir().item(id).expect_impl().self_ty); + // And we access the Map here to get HirId from LocalDefId - for id in local_def_ids { + for local_def_id in local_def_ids { + // check the function may construct Self + let mut may_construct_self = true; + if let Some(hir_id) = tcx.opt_local_def_id_to_hir_id(local_def_id) + && let Some(fn_sig) = tcx.hir().fn_sig_by_hir_id(hir_id) + { + may_construct_self = + matches!(fn_sig.decl.implicit_self, hir::ImplicitSelfKind::None); + } + // for impl trait blocks, mark associate functions live if the trait is public if of_trait - && (!matches!(tcx.def_kind(id), DefKind::AssocFn) - || tcx.local_visibility(id) == Visibility::Public) + && (!matches!(tcx.def_kind(local_def_id), DefKind::AssocFn) + || tcx.visibility(local_def_id).is_public() + && (ty_is_pub || may_construct_self)) { - worklist.push((id, ComesFromAllowExpect::No)); - } else if let Some(comes_from_allow) = has_allow_dead_code_or_lang_attr(tcx, id) { - worklist.push((id, comes_from_allow)); + worklist.push((local_def_id, ComesFromAllowExpect::No)); + } else if of_trait && tcx.visibility(local_def_id).is_public() { + // pub method && private ty & methods not construct self + unsolved_impl_items.push((id, local_def_id)); + } else if let Some(comes_from_allow) = + has_allow_dead_code_or_lang_attr(tcx, local_def_id) + { + worklist.push((local_def_id, comes_from_allow)); } } } @@ -743,9 +810,14 @@ fn check_foreign_item( fn create_and_seed_worklist( tcx: TyCtxt<'_>, -) -> (Vec<(LocalDefId, ComesFromAllowExpect)>, LocalDefIdMap) { +) -> ( + Vec<(LocalDefId, ComesFromAllowExpect)>, + LocalDefIdMap, + Vec<(hir::ItemId, LocalDefId)>, +) { let effective_visibilities = &tcx.effective_visibilities(()); // see `MarkSymbolVisitor::struct_constructors` + let mut unsolved_impl_item = Vec::new(); let mut struct_constructors = Default::default(); let mut worklist = effective_visibilities .iter() @@ -764,7 +836,7 @@ fn create_and_seed_worklist( let crate_items = tcx.hir_crate_items(()); for id in crate_items.items() { - check_item(tcx, &mut worklist, &mut struct_constructors, id); + check_item(tcx, &mut worklist, &mut struct_constructors, &mut unsolved_impl_item, id); } for id in crate_items.trait_items() { @@ -775,14 +847,14 @@ fn create_and_seed_worklist( check_foreign_item(tcx, &mut worklist, id); } - (worklist, struct_constructors) + (worklist, struct_constructors, unsolved_impl_item) } fn live_symbols_and_ignored_derived_traits( tcx: TyCtxt<'_>, (): (), ) -> (LocalDefIdSet, LocalDefIdMap>) { - let (worklist, struct_constructors) = create_and_seed_worklist(tcx); + let (worklist, struct_constructors, unsolved_impl_items) = create_and_seed_worklist(tcx); let mut symbol_visitor = MarkSymbolVisitor { worklist, tcx, @@ -796,6 +868,8 @@ fn live_symbols_and_ignored_derived_traits( ignored_derived_traits: Default::default(), }; symbol_visitor.mark_live_symbols(); + symbol_visitor.solve_rest_impl_items(unsolved_impl_items); + (symbol_visitor.live_symbols, symbol_visitor.ignored_derived_traits) } diff --git a/tests/ui/lint/dead-code/issue-41883.stderr b/tests/ui/lint/dead-code/issue-41883.stderr index cf079e4dda3..47ccef9a530 100644 --- a/tests/ui/lint/dead-code/issue-41883.stderr +++ b/tests/ui/lint/dead-code/issue-41883.stderr @@ -29,8 +29,6 @@ error: struct `UnusedStruct` is never constructed | LL | struct UnusedStruct; | ^^^^^^^^^^^^ - | - = note: `UnusedStruct` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis error: aborting due to 4 previous errors diff --git a/tests/ui/lint/dead-code/multiple-dead-codes-in-the-same-struct.stderr b/tests/ui/lint/dead-code/multiple-dead-codes-in-the-same-struct.stderr index b992005318f..25a7d96cb89 100644 --- a/tests/ui/lint/dead-code/multiple-dead-codes-in-the-same-struct.stderr +++ b/tests/ui/lint/dead-code/multiple-dead-codes-in-the-same-struct.stderr @@ -56,8 +56,6 @@ warning: struct `Foo` is never constructed | LL | struct Foo(usize, #[allow(unused)] usize); | ^^^ - | - = note: `Foo` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis error: aborting due to 2 previous errors; 2 warnings emitted diff --git a/tests/ui/lint/dead-code/unused-adt-impls-pub-trait.rs b/tests/ui/lint/dead-code/unused-adt-impls-pub-trait.rs new file mode 100644 index 00000000000..10d6c728c2d --- /dev/null +++ b/tests/ui/lint/dead-code/unused-adt-impls-pub-trait.rs @@ -0,0 +1,17 @@ +#![deny(dead_code)] + +enum Foo {} //~ ERROR enum `Foo` is never used + +impl Clone for Foo { + fn clone(&self) -> Foo { loop {} } +} + +pub trait PubTrait { + fn unused_method(&self) -> Self; +} + +impl PubTrait for Foo { + fn unused_method(&self) -> Foo { loop {} } +} + +fn main() {} diff --git a/tests/ui/lint/dead-code/unused-adt-impls-pub-trait.stderr b/tests/ui/lint/dead-code/unused-adt-impls-pub-trait.stderr new file mode 100644 index 00000000000..f0d0cc50f7d --- /dev/null +++ b/tests/ui/lint/dead-code/unused-adt-impls-pub-trait.stderr @@ -0,0 +1,14 @@ +error: enum `Foo` is never used + --> $DIR/unused-adt-impls-pub-trait.rs:3:6 + | +LL | enum Foo {} + | ^^^ + | +note: the lint level is defined here + --> $DIR/unused-adt-impls-pub-trait.rs:1:9 + | +LL | #![deny(dead_code)] + | ^^^^^^^^^ + +error: aborting due to 1 previous error + diff --git a/tests/ui/rust-2018/uniform-paths/issue-55779.rs b/tests/ui/rust-2018/uniform-paths/issue-55779.rs index 246b8dd82c5..f90054e6b1d 100644 --- a/tests/ui/rust-2018/uniform-paths/issue-55779.rs +++ b/tests/ui/rust-2018/uniform-paths/issue-55779.rs @@ -1,4 +1,4 @@ -//@ run-pass +//@ check-pass //@ edition:2018 //@ aux-crate:issue_55779_extern_trait=issue-55779-extern-trait.rs diff --git a/tests/ui/traits/augmented-assignments-trait.rs b/tests/ui/traits/augmented-assignments-trait.rs index 37fe42ce1c6..a0f8b39eb7a 100644 --- a/tests/ui/traits/augmented-assignments-trait.rs +++ b/tests/ui/traits/augmented-assignments-trait.rs @@ -1,4 +1,4 @@ -//@ run-pass +//@ check-pass use std::ops::AddAssign; struct Int(#[allow(dead_code)] i32); diff --git a/tests/ui/traits/issue-33187.rs b/tests/ui/traits/issue-33187.rs index 6a039527b3b..6ae7e150c51 100644 --- a/tests/ui/traits/issue-33187.rs +++ b/tests/ui/traits/issue-33187.rs @@ -1,4 +1,4 @@ -//@ run-pass +//@ check-pass struct Foo(::Data); From c73a7f0bec92297d6c28304598cee9bd6898f0fc Mon Sep 17 00:00:00 2001 From: r0cky Date: Thu, 29 Feb 2024 20:53:08 +0800 Subject: [PATCH 2/2] Remove unused structs in clippy --- .../clippy/clippy_lints/src/loops/utils.rs | 60 +------------------ .../clippy/clippy_lints/src/macro_use.rs | 6 -- 2 files changed, 2 insertions(+), 64 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/loops/utils.rs b/src/tools/clippy/clippy_lints/src/loops/utils.rs index e685274adb8..8bca33754e8 100644 --- a/src/tools/clippy/clippy_lints/src/loops/utils.rs +++ b/src/tools/clippy/clippy_lints/src/loops/utils.rs @@ -2,8 +2,8 @@ use clippy_utils::ty::{has_iter_method, implements_trait}; use clippy_utils::{get_parent_expr, is_integer_const, path_to_local, path_to_local_id, sugg}; use rustc_ast::ast::{LitIntType, LitKind}; use rustc_errors::Applicability; -use rustc_hir::intravisit::{walk_expr, walk_local, walk_pat, walk_stmt, Visitor}; -use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, HirId, HirIdMap, Local, Mutability, Pat, PatKind, Stmt}; +use rustc_hir::intravisit::{walk_expr, walk_local, Visitor}; +use rustc_hir::{BinOpKind, BorrowKind, Expr, ExprKind, HirId, HirIdMap, Local, Mutability, PatKind}; use rustc_lint::LateContext; use rustc_middle::hir::nested_filter; use rustc_middle::ty::{self, Ty}; @@ -253,62 +253,6 @@ fn is_conditional(expr: &Expr<'_>) -> bool { matches!(expr.kind, ExprKind::If(..) | ExprKind::Match(..)) } -#[derive(PartialEq, Eq)] -pub(super) enum Nesting { - Unknown, // no nesting detected yet - RuledOut, // the iterator is initialized or assigned within scope - LookFurther, // no nesting detected, no further walk required -} - -use self::Nesting::{LookFurther, RuledOut, Unknown}; - -pub(super) struct LoopNestVisitor { - pub(super) hir_id: HirId, - pub(super) iterator: HirId, - pub(super) nesting: Nesting, -} - -impl<'tcx> Visitor<'tcx> for LoopNestVisitor { - fn visit_stmt(&mut self, stmt: &'tcx Stmt<'_>) { - if stmt.hir_id == self.hir_id { - self.nesting = LookFurther; - } else if self.nesting == Unknown { - walk_stmt(self, stmt); - } - } - - fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { - if self.nesting != Unknown { - return; - } - if expr.hir_id == self.hir_id { - self.nesting = LookFurther; - return; - } - match expr.kind { - ExprKind::Assign(path, _, _) | ExprKind::AssignOp(_, path, _) => { - if path_to_local_id(path, self.iterator) { - self.nesting = RuledOut; - } - }, - _ => walk_expr(self, expr), - } - } - - fn visit_pat(&mut self, pat: &'tcx Pat<'_>) { - if self.nesting != Unknown { - return; - } - if let PatKind::Binding(_, id, ..) = pat.kind { - if id == self.iterator { - self.nesting = RuledOut; - return; - } - } - walk_pat(self, pat); - } -} - /// If `arg` was the argument to a `for` loop, return the "cleanest" way of writing the /// actual `Iterator` that the loop uses. pub(super) fn make_iterator_snippet(cx: &LateContext<'_>, arg: &Expr<'_>, applic_ref: &mut Applicability) -> String { diff --git a/src/tools/clippy/clippy_lints/src/macro_use.rs b/src/tools/clippy/clippy_lints/src/macro_use.rs index 8d3e7520a54..067384b0901 100644 --- a/src/tools/clippy/clippy_lints/src/macro_use.rs +++ b/src/tools/clippy/clippy_lints/src/macro_use.rs @@ -30,12 +30,6 @@ declare_clippy_lint! { "#[macro_use] is no longer needed" } -#[derive(Clone, Debug, PartialEq, Eq)] -struct PathAndSpan { - path: String, - span: Span, -} - /// `MacroRefData` includes the name of the macro. #[derive(Debug, Clone)] pub struct MacroRefData {