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

Respect #[expect] the same way #[allow] is with the dead_code lint #114710

Merged
merged 1 commit into from
Aug 12, 2023
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
126 changes: 93 additions & 33 deletions compiler/rustc_passes/src/dead.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use hir::def_id::{LocalDefIdMap, LocalDefIdSet};
use itertools::Itertools;
use rustc_data_structures::unord::UnordSet;
use rustc_errors::MultiSpan;
use rustc_hir as hir;
use rustc_hir::def::{CtorOf, DefKind, Res};
Expand Down Expand Up @@ -42,8 +43,16 @@ fn should_explore(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
)
}

/// 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)]
enum ComesFromAllowExpect {
Yes,
No,
}

struct MarkSymbolVisitor<'tcx> {
worklist: Vec<LocalDefId>,
worklist: Vec<(LocalDefId, ComesFromAllowExpect)>,
tcx: TyCtxt<'tcx>,
maybe_typeck_results: Option<&'tcx ty::TypeckResults<'tcx>>,
live_symbols: LocalDefIdSet,
Expand Down Expand Up @@ -72,7 +81,7 @@ impl<'tcx> MarkSymbolVisitor<'tcx> {
fn check_def_id(&mut self, def_id: DefId) {
if let Some(def_id) = def_id.as_local() {
if should_explore(self.tcx, def_id) || self.struct_constructors.contains_key(&def_id) {
self.worklist.push(def_id);
self.worklist.push((def_id, ComesFromAllowExpect::No));
}
self.live_symbols.insert(def_id);
}
Expand Down Expand Up @@ -269,12 +278,14 @@ impl<'tcx> MarkSymbolVisitor<'tcx> {
}

fn mark_live_symbols(&mut self) {
let mut scanned = LocalDefIdSet::default();
while let Some(id) = self.worklist.pop() {
if !scanned.insert(id) {
let mut scanned = UnordSet::default();
while let Some(work) = self.worklist.pop() {
if !scanned.insert(work) {
continue;
}

let (id, comes_from_allow_expect) = work;

// Avoid accessing the HIR for the synthesized associated type generated for RPITITs.
if self.tcx.is_impl_trait_in_trait(id.to_def_id()) {
self.live_symbols.insert(id);
Expand All @@ -286,7 +297,30 @@ impl<'tcx> MarkSymbolVisitor<'tcx> {
let id = self.struct_constructors.get(&id).copied().unwrap_or(id);

if let Some(node) = self.tcx.hir().find_by_def_id(id) {
self.live_symbols.insert(id);
// When using `#[allow]` or `#[expect]` of `dead_code`, we do a QOL improvement
// by declaring fn calls, statics, ... within said items as live, as well as
// the item itself, although technically this is not the case.
//
// This means that the lint for said items will never be fired.
//
// This doesn't make any difference for the item declared with `#[allow]`, as
// the lint firing will be a nop, as it will be silenced by the `#[allow]` of
// the item.
//
// However, for `#[expect]`, the presence or absence of the lint is relevant,
// so we don't add it to the list of live symbols when it comes from a
// `#[expect]`. This means that we will correctly report an item as live or not
// for the `#[expect]` case.
//
// Note that an item can and will be duplicated on the worklist with different
// `ComesFromAllowExpect`, particulary if it was added from the
// `effective_visibilities` query or from the `#[allow]`/`#[expect]` checks,
// this "duplication" is essential as otherwise a function with `#[expect]`
// called from a `pub fn` may be falsely reported as not live, falsely
// triggering the `unfulfilled_lint_expectations` lint.
if comes_from_allow_expect != ComesFromAllowExpect::Yes {
self.live_symbols.insert(id);
}
self.visit_node(node);
}
}
Expand Down Expand Up @@ -513,16 +547,20 @@ impl<'tcx> Visitor<'tcx> for MarkSymbolVisitor<'tcx> {
}
}

fn has_allow_dead_code_or_lang_attr(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
fn has_allow_dead_code_or_lang_attr(
tcx: TyCtxt<'_>,
def_id: LocalDefId,
) -> Option<ComesFromAllowExpect> {
fn has_lang_attr(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
tcx.has_attr(def_id, sym::lang)
// Stable attribute for #[lang = "panic_impl"]
|| tcx.has_attr(def_id, sym::panic_handler)
}

fn has_allow_dead_code(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
fn has_allow_expect_dead_code(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
let hir_id = tcx.hir().local_def_id_to_hir_id(def_id);
tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir_id).0 == lint::Allow
let lint_level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir_id).0;
matches!(lint_level, lint::Allow | lint::Expect(_))
}

fn has_used_like_attr(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool {
Expand All @@ -537,9 +575,13 @@ fn has_allow_dead_code_or_lang_attr(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool
}
}

has_allow_dead_code(tcx, def_id)
|| has_used_like_attr(tcx, def_id)
|| has_lang_attr(tcx, def_id)
if has_allow_expect_dead_code(tcx, def_id) {
Some(ComesFromAllowExpect::Yes)
} else if has_used_like_attr(tcx, def_id) || has_lang_attr(tcx, def_id) {
Some(ComesFromAllowExpect::No)
} else {
None
}
}

// These check_* functions seeds items that
Expand All @@ -557,21 +599,23 @@ fn has_allow_dead_code_or_lang_attr(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool
// * Implementations of traits and trait methods
fn check_item<'tcx>(
tcx: TyCtxt<'tcx>,
worklist: &mut Vec<LocalDefId>,
worklist: &mut Vec<(LocalDefId, ComesFromAllowExpect)>,
struct_constructors: &mut LocalDefIdMap<LocalDefId>,
id: hir::ItemId,
) {
let allow_dead_code = has_allow_dead_code_or_lang_attr(tcx, id.owner_id.def_id);
if allow_dead_code {
worklist.push(id.owner_id.def_id);
if let Some(comes_from_allow) = allow_dead_code {
worklist.push((id.owner_id.def_id, comes_from_allow));
}

match tcx.def_kind(id.owner_id) {
DefKind::Enum => {
let item = tcx.hir().item(id);
if let hir::ItemKind::Enum(ref enum_def, _) = item.kind {
if allow_dead_code {
worklist.extend(enum_def.variants.iter().map(|variant| variant.def_id));
if let Some(comes_from_allow) = allow_dead_code {
worklist.extend(
enum_def.variants.iter().map(|variant| (variant.def_id, comes_from_allow)),
);
}

for variant in enum_def.variants {
Expand All @@ -583,7 +627,7 @@ fn check_item<'tcx>(
}
DefKind::Impl { of_trait } => {
if of_trait {
worklist.push(id.owner_id.def_id);
worklist.push((id.owner_id.def_id, ComesFromAllowExpect::No));
}

// get DefIds from another query
Expand All @@ -594,8 +638,10 @@ fn check_item<'tcx>(

// And we access the Map here to get HirId from LocalDefId
for id in local_def_ids {
if of_trait || has_allow_dead_code_or_lang_attr(tcx, id) {
worklist.push(id);
if of_trait {
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));
}
}
}
Expand All @@ -609,43 +655,59 @@ fn check_item<'tcx>(
}
DefKind::GlobalAsm => {
// global_asm! is always live.
worklist.push(id.owner_id.def_id);
worklist.push((id.owner_id.def_id, ComesFromAllowExpect::No));
}
_ => {}
}
}

fn check_trait_item(tcx: TyCtxt<'_>, worklist: &mut Vec<LocalDefId>, id: hir::TraitItemId) {
fn check_trait_item(
tcx: TyCtxt<'_>,
worklist: &mut Vec<(LocalDefId, ComesFromAllowExpect)>,
id: hir::TraitItemId,
) {
use hir::TraitItemKind::{Const, Fn};
if matches!(tcx.def_kind(id.owner_id), DefKind::AssocConst | DefKind::AssocFn) {
let trait_item = tcx.hir().trait_item(id);
if matches!(trait_item.kind, Const(_, Some(_)) | Fn(_, hir::TraitFn::Provided(_)))
&& has_allow_dead_code_or_lang_attr(tcx, trait_item.owner_id.def_id)
&& let Some(comes_from_allow) = has_allow_dead_code_or_lang_attr(tcx, trait_item.owner_id.def_id)
{
worklist.push(trait_item.owner_id.def_id);
worklist.push((trait_item.owner_id.def_id, comes_from_allow));
}
}
}

fn check_foreign_item(tcx: TyCtxt<'_>, worklist: &mut Vec<LocalDefId>, id: hir::ForeignItemId) {
fn check_foreign_item(
tcx: TyCtxt<'_>,
worklist: &mut Vec<(LocalDefId, ComesFromAllowExpect)>,
id: hir::ForeignItemId,
) {
if matches!(tcx.def_kind(id.owner_id), DefKind::Static(_) | DefKind::Fn)
&& has_allow_dead_code_or_lang_attr(tcx, id.owner_id.def_id)
&& let Some(comes_from_allow) = has_allow_dead_code_or_lang_attr(tcx, id.owner_id.def_id)
{
worklist.push(id.owner_id.def_id);
worklist.push((id.owner_id.def_id, comes_from_allow));
}
}

fn create_and_seed_worklist(tcx: TyCtxt<'_>) -> (Vec<LocalDefId>, LocalDefIdMap<LocalDefId>) {
fn create_and_seed_worklist(
tcx: TyCtxt<'_>,
) -> (Vec<(LocalDefId, ComesFromAllowExpect)>, LocalDefIdMap<LocalDefId>) {
let effective_visibilities = &tcx.effective_visibilities(());
// see `MarkSymbolVisitor::struct_constructors`
let mut struct_constructors = Default::default();
let mut worklist = effective_visibilities
.iter()
.filter_map(|(&id, effective_vis)| {
effective_vis.is_public_at_level(Level::Reachable).then_some(id)
effective_vis
.is_public_at_level(Level::Reachable)
.then_some(id)
.map(|id| (id, ComesFromAllowExpect::No))
})
// Seed entry point
.chain(tcx.entry_fn(()).and_then(|(def_id, _)| def_id.as_local()))
.chain(
tcx.entry_fn(())
.and_then(|(def_id, _)| def_id.as_local().map(|id| (id, ComesFromAllowExpect::No))),
)
.collect::<Vec<_>>();

let crate_items = tcx.hir_crate_items(());
Expand Down Expand Up @@ -878,9 +940,7 @@ impl<'tcx> DeadVisitor<'tcx> {
return true;
};

self.live_symbols.contains(&def_id)
|| has_allow_dead_code_or_lang_attr(self.tcx, def_id)
|| name.as_str().starts_with('_')
self.live_symbols.contains(&def_id) || name.as_str().starts_with('_')
}
}

Expand Down
19 changes: 19 additions & 0 deletions tests/ui/lint/dead-code/allow-or-expect-dead_code-114557-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// check-pass

// this test checks that the `dead_code` lint is *NOT* being emited
// for `foo` as `foo` is being used by `main`, and so the `#[expect]`
// is unfulfilled
//
// it also checks that the `dead_code` lint is also *NOT* emited
// for `bar` as it's suppresed by the `#[expect]` on `bar`

#![feature(lint_reasons)]
#![warn(dead_code)] // to override compiletest

fn bar() {}

#[expect(dead_code)]
//~^ WARN this lint expectation is unfulfilled
fn foo() { bar() }

fn main() { foo() }
10 changes: 10 additions & 0 deletions tests/ui/lint/dead-code/allow-or-expect-dead_code-114557-2.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
warning: this lint expectation is unfulfilled
--> $DIR/allow-or-expect-dead_code-114557-2.rs:15:10
|
LL | #[expect(dead_code)]
| ^^^^^^^^^
|
= note: `#[warn(unfulfilled_lint_expectations)]` on by default

warning: 1 warning emitted

13 changes: 13 additions & 0 deletions tests/ui/lint/dead-code/allow-or-expect-dead_code-114557-3.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// check-pass

// this test makes sure that the `unfulfilled_lint_expectations` lint
// is being emited for `foo` as foo is not dead code, it's pub

#![feature(lint_reasons)]
#![warn(dead_code)] // to override compiletest

#[expect(dead_code)]
//~^ WARN this lint expectation is unfulfilled
pub fn foo() {}

fn main() {}
10 changes: 10 additions & 0 deletions tests/ui/lint/dead-code/allow-or-expect-dead_code-114557-3.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
warning: this lint expectation is unfulfilled
--> $DIR/allow-or-expect-dead_code-114557-3.rs:9:10
|
LL | #[expect(dead_code)]
| ^^^^^^^^^
|
= note: `#[warn(unfulfilled_lint_expectations)]` on by default

warning: 1 warning emitted

18 changes: 18 additions & 0 deletions tests/ui/lint/dead-code/allow-or-expect-dead_code-114557.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// check-pass
// revisions: allow expect

// this test checks that no matter if we put #[allow(dead_code)]
// or #[expect(dead_code)], no warning is being emited

#![feature(lint_reasons)]
#![warn(dead_code)] // to override compiletest

fn f() {}

#[cfg_attr(allow, allow(dead_code))]
#[cfg_attr(expect, expect(dead_code))]
fn g() {
f();
}

fn main() {}