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

Fix multiple expect attribs in impl block #114417

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
73 changes: 39 additions & 34 deletions compiler/rustc_passes/src/dead.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// This implements the dead-code warning pass. It follows middle::reachable
// closely. The idea is that all reachable symbols are live, codes called
// from live codes are live, and everything else is dead.
// This implements the dead-code warning pass.
// All reachable symbols are live, code called from live code is live, code with certain lint
// expectations such as `#[expect(unused)]` and `#[expect(dead_code)]` is live, and everything else
// is dead.

use hir::def_id::{LocalDefIdMap, LocalDefIdSet};
use itertools::Itertools;
Expand Down Expand Up @@ -685,7 +686,7 @@ fn live_symbols_and_ignored_derived_traits(
(symbol_visitor.live_symbols, symbol_visitor.ignored_derived_traits)
}

struct DeadVariant {
struct DeadItem {
def_id: LocalDefId,
name: Symbol,
level: lint::Level,
Expand Down Expand Up @@ -723,7 +724,13 @@ impl<'tcx> DeadVisitor<'tcx> {
ShouldWarnAboutField::Yes(is_positional)
}

fn warn_multiple_dead_codes(
// # Panics
// All `dead_codes` must have the same lint level, otherwise we will intentionally ICE.
// This is because we emit a multi-spanned lint using the lint level of the `dead_codes`'s
// first local def id.
// Prefer calling `Self.warn_dead_code` or `Self.warn_dead_code_grouped_by_lint_level`
// since those methods group by lint level before calling this method.
fn lint_at_single_level(
&self,
dead_codes: &[LocalDefId],
participle: &str,
Expand All @@ -734,6 +741,15 @@ impl<'tcx> DeadVisitor<'tcx> {
return;
};
let tcx = self.tcx;

let first_hir_id = tcx.hir().local_def_id_to_hir_id(first_id);
let first_lint_level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, first_hir_id).0;
assert!(dead_codes.iter().skip(1).all(|id| {
let hir_id = tcx.hir().local_def_id_to_hir_id(*id);
let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir_id).0;
level == first_lint_level
}));

Comment on lines +747 to +751
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warn_multiple_dead_codes method seems like it would be a fairly cold code path (as in, you'd have to have a project with a lot of dead code to hit this method a lot), so it seemed fine to add another loop here.

let names: Vec<_> =
dead_codes.iter().map(|&def_id| tcx.item_name(def_id.to_def_id())).collect();
let spans: Vec<_> = dead_codes
Expand Down Expand Up @@ -814,31 +830,26 @@ impl<'tcx> DeadVisitor<'tcx> {
}
};

self.tcx.emit_spanned_lint(
lint,
tcx.hir().local_def_id_to_hir_id(first_id),
MultiSpan::from_spans(spans),
diag,
);
self.tcx.emit_spanned_lint(lint, first_hir_id, MultiSpan::from_spans(spans), diag);
}

fn warn_dead_fields_and_variants(
fn warn_multiple(
&self,
def_id: LocalDefId,
participle: &str,
dead_codes: Vec<DeadVariant>,
dead_codes: Vec<DeadItem>,
is_positional: bool,
) {
let mut dead_codes = dead_codes
.iter()
.filter(|v| !v.name.as_str().starts_with('_'))
.collect::<Vec<&DeadVariant>>();
.collect::<Vec<&DeadItem>>();
if dead_codes.is_empty() {
return;
}
dead_codes.sort_by_key(|v| v.level);
for (_, group) in &dead_codes.into_iter().group_by(|v| v.level) {
self.warn_multiple_dead_codes(
self.lint_at_single_level(
&group.map(|v| v.def_id).collect::<Vec<_>>(),
participle,
Some(def_id),
Expand All @@ -848,7 +859,7 @@ impl<'tcx> DeadVisitor<'tcx> {
}

fn warn_dead_code(&mut self, id: LocalDefId, participle: &str) {
self.warn_multiple_dead_codes(&[id], participle, None, false);
self.lint_at_single_level(&[id], participle, None, false);
}

fn check_definition(&mut self, def_id: LocalDefId) {
Expand Down Expand Up @@ -894,17 +905,16 @@ fn check_mod_deathness(tcx: TyCtxt<'_>, module: LocalDefId) {
if let hir::ItemKind::Impl(impl_item) = tcx.hir().item(item).kind {
let mut dead_items = Vec::new();
for item in impl_item.items {
let did = item.id.owner_id.def_id;
if !visitor.is_live_code(did) {
dead_items.push(did)
let def_id = item.id.owner_id.def_id;
if !visitor.is_live_code(def_id) {
let name = tcx.item_name(def_id.to_def_id());
let hir_id = tcx.hir().local_def_id_to_hir_id(def_id);
let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir_id).0;

dead_items.push(DeadItem { def_id, name, level })
}
}
visitor.warn_multiple_dead_codes(
&dead_items,
"used",
Some(item.owner_id.def_id),
false,
);
visitor.warn_multiple(item.owner_id.def_id, "used", dead_items, false);
}

if !live_symbols.contains(&item.owner_id.def_id) {
Expand All @@ -928,7 +938,7 @@ fn check_mod_deathness(tcx: TyCtxt<'_>, module: LocalDefId) {
// Record to group diagnostics.
let hir_id = tcx.hir().local_def_id_to_hir_id(def_id);
let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir_id).0;
dead_variants.push(DeadVariant { def_id, name: variant.name, level });
dead_variants.push(DeadItem { def_id, name: variant.name, level });
continue;
}

Expand All @@ -953,21 +963,16 @@ fn check_mod_deathness(tcx: TyCtxt<'_>, module: LocalDefId) {
hir_id,
)
.0;
Some(DeadVariant { def_id, name: field.name, level })
Some(DeadItem { def_id, name: field.name, level })
} else {
None
}
})
.collect();
visitor.warn_dead_fields_and_variants(def_id, "read", dead_fields, is_positional)
visitor.warn_multiple(def_id, "read", dead_fields, is_positional);
}

visitor.warn_dead_fields_and_variants(
item.owner_id.def_id,
"constructed",
dead_variants,
false,
);
visitor.warn_multiple(item.owner_id.def_id, "constructed", dead_variants, false);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// check-pass
chinedufn marked this conversation as resolved.
Show resolved Hide resolved
// incremental

#![feature(lint_reasons)]
#![warn(unused)]

struct OneUnused;
struct TwoUnused;

impl OneUnused {
#[expect(unused)]
fn unused() {}
}

impl TwoUnused {
#[expect(unused)]
fn unused1(){}

// This unused method has `#[expect(unused)]`, so the compiler should not emit a warning.
// This ui test was added after a regression in the compiler where it did not recognize multiple
// `#[expect(unused)]` annotations inside of impl blocks.
// issue 114416
#[expect(unused)]
fn unused2(){}
}

fn main() {
let _ = OneUnused;
let _ = TwoUnused;
}
Loading