Auto merge of rust-lang#121752 - mu001999:dead_code/improve, r=pnkfelix

Detect unused struct impls pub trait

Fixes rust-lang#47851

Author: bors

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<LocalDefId>,
+    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<LocalDefId>) {
+) -> (
+    Vec<(LocalDefId, ComesFromAllowExpect)>,
+    LocalDefIdMap<LocalDefId>,
+    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<Vec<(DefId, DefId)>>) {
-    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)
 }
 

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 {

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 {

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
 

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
 

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() {}

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
+

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
 

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);

tests/ui/traits/issue-33187.rs

@@ -1,4 +1,4 @@
-//@ run-pass
+//@ check-pass
 
 struct Foo<A: Repr>(<A as Repr>::Data);