Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect unused struct impls pub trait #121752

Merged
merged 2 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading