Skip to content

Commit

Permalink
Auto merge of #121752 - mu001999:dead_code/improve, r=pnkfelix
Browse files Browse the repository at this point in the history
Detect unused struct impls pub trait

Fixes #47851
  • Loading branch information
bors committed Mar 11, 2024
2 parents cd81f5b + c73a7f0 commit c69fda7
Show file tree
Hide file tree
Showing 10 changed files with 121 additions and 82 deletions.
96 changes: 85 additions & 11 deletions compiler/rustc_passes/src/dead.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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) =
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
}
}
}
Expand Down Expand Up @@ -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()
Expand All @@ -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() {
Expand All @@ -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,
Expand All @@ -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)
}

Expand Down
60 changes: 2 additions & 58 deletions src/tools/clippy/clippy_lints/src/loops/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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 {
Expand Down
6 changes: 0 additions & 6 deletions src/tools/clippy/clippy_lints/src/macro_use.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 0 additions & 2 deletions tests/ui/lint/dead-code/issue-41883.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

Original file line number Diff line number Diff line change
Expand Up @@ -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

17 changes: 17 additions & 0 deletions tests/ui/lint/dead-code/unused-adt-impls-pub-trait.rs
Original file line number Diff line number Diff line change
@@ -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() {}
14 changes: 14 additions & 0 deletions tests/ui/lint/dead-code/unused-adt-impls-pub-trait.stderr
Original file line number Diff line number Diff line change
@@ -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

2 changes: 1 addition & 1 deletion tests/ui/rust-2018/uniform-paths/issue-55779.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//@ run-pass
//@ check-pass
//@ edition:2018
//@ aux-crate:issue_55779_extern_trait=issue-55779-extern-trait.rs

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/traits/augmented-assignments-trait.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//@ run-pass
//@ check-pass
use std::ops::AddAssign;

struct Int(#[allow(dead_code)] i32);
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/traits/issue-33187.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//@ run-pass
//@ check-pass

struct Foo<A: Repr>(<A as Repr>::Data);

Expand Down

0 comments on commit c69fda7

Please sign in to comment.