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 4 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
51 changes: 35 additions & 16 deletions compiler/rustc_passes/src/dead.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// This implements the dead-code warning pass. It follows middle::reachable
// This implements the dead-code warning pass. It follows crate::reachable
// closely. The idea is that all reachable symbols are live, codes called
// from live codes are live, and everything else is dead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still true?


Expand Down Expand Up @@ -723,6 +723,12 @@ impl<'tcx> DeadVisitor<'tcx> {
ShouldWarnAboutField::Yes(is_positional)
}

// # 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 warn_multiple_dead_codes(
&self,
Copy link
Contributor

Choose a reason for hiding this comment

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

This method needs to be renamed, so it's not used again. lint_at_single_level?

dead_codes: &[LocalDefId],
Expand All @@ -734,6 +740,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,15 +829,10 @@ 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_dead_code_grouped_by_lint_level(
&self,
Copy link
Contributor

Choose a reason for hiding this comment

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

And this one can be named warn_multiple.

def_id: LocalDefId,
participle: &str,
Expand Down Expand Up @@ -894,15 +904,19 @@ 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 = tcx.hir().local_def_id_to_hir_id(def_id);
let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir).0;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let hir = tcx.hir().local_def_id_to_hir_id(def_id);
let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir).0;
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(DeadVariant { def_id, name, level })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

DeadVariant needs to be renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what the right name would be for this. I renamed it to DeadItem.

}
visitor.warn_multiple_dead_codes(
&dead_items,
visitor.warn_dead_code_grouped_by_lint_level(
item.owner_id.def_id,
"used",
Some(item.owner_id.def_id),
dead_items,
false,
);
}
Expand Down Expand Up @@ -959,10 +973,15 @@ fn check_mod_deathness(tcx: TyCtxt<'_>, module: LocalDefId) {
}
})
.collect();
visitor.warn_dead_fields_and_variants(def_id, "read", dead_fields, is_positional)
visitor.warn_dead_code_grouped_by_lint_level(
def_id,
"read",
dead_fields,
is_positional,
)
}

visitor.warn_dead_fields_and_variants(
visitor.warn_dead_code_grouped_by_lint_level(
item.owner_id.def_id,
"constructed",
dead_variants,
Expand Down
32 changes: 32 additions & 0 deletions tests/incremental/issue-114416-expect-unused-inside-impl-block.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// revisions: rpass1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like it would be nice if it was possible to add revisions: rpass1 to a UI test so that we don't have to duplicate tests like this one across tests/incremetal and tests/ui.

First, is there already a good way to avoid this duplication?

If not, would being able to drive an incremental test from a UI test be undesirable for some reason?

If desirable, I'd be interesting in adding support for using the revisions: rpass1 annotation in a UI test. Do you have any tips on how I might best explore that (in a separate PR)?

Copy link
Contributor

Choose a reason for hiding this comment

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

See what tests/ui/dep-graph files do. They have an // incremental directive for this.

//
// The corresponding ui test can be found in
// `tests/ui/lint/rfc-2383-lint-reason/expect_unused_inside_impl_block.rs`

#![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;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// check-pass
chinedufn marked this conversation as resolved.
Show resolved Hide resolved
//
// The corresponding incremental compilation test can be found in
// `tests/incremental/issue-114416-expect-unused-inside-impl-block.rs`

#![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