Auto merge of rust-lang#114710 - Urgau:fix-expect-dead_code-114557, r=cjgillot

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

This PR makes the `#[expect]` attribute being respected in the same way the `#[allow]` attribute is with the `dead_code` lint.

The fix is much more involved than I would have liked (and it's not because I didn't tried!), because the implementation took advantage of the fact that firing a lint in a allow context is a nop (for the user, as the lint is suppressed) to not fire-it at all.

And will it's fine for `#[allow]`, it definitively isn't for `#[expect]`, as the presence and absence of the lint is significant. So a big part of the PR is just adding the context information of whenever an item is on the worklist because of an `[allow]`/`#[expect]` or not.

Fixes rust-lang#114557

Author: bors

Diff for: compiler/rustc_passes/src/dead.rs

@@ -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};
@@ -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,
@@ -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);
         }
@@ -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);
@@ -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);
             }
         }
@@ -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 {
@@ -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
@@ -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 {
@@ -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
@@ -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));
                 }
             }
         }
@@ -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(());
@@ -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('_')
     }
 }
 

Diff for: tests/ui/lint/dead-code/allow-or-expect-dead_code-114557-2.rs

@@ -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() }

Diff for: tests/ui/lint/dead-code/allow-or-expect-dead_code-114557-2.stderr

@@ -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
+

Diff for: tests/ui/lint/dead-code/allow-or-expect-dead_code-114557-3.rs

@@ -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() {}

Diff for: tests/ui/lint/dead-code/allow-or-expect-dead_code-114557-3.stderr

@@ -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
+

Diff for: tests/ui/lint/dead-code/allow-or-expect-dead_code-114557.rs

@@ -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() {}