Fix #[expect(dead_code)] liveness propagation - #154377
Conversation
|
r? @TaKO8Ki rustbot has assigned @TaKO8Ki. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rustbot author I realized that this would fail during secondary propagation. This still needs to be revised a bit. |
|
Reminder, once the PR becomes ready for a review, use |
|
oops, forgot to update the label @rustbot ready |
|
See #152370 (comment) . |
|
Test cases look good here. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@rustbot reroll |
|
r? TaKO8Ki @bors r+ rollup |
…TaKO8Ki Fix `#[expect(dead_code)]` liveness propagation Fixes rust-lang#154324 Fixes rust-lang#152370 (cc @eggyal) Previously, when traversing from a `ComesFromAllowExpect::Yes` item (i.e., with `#[allow(dead_code)]` or `#[expect(dead_code)]`), other `ComesFromAllowExpect::Yes` items reached during propagation would be updated to `ComesFromAllowExpect::No` and inserted into `live_symbols`. That caused `dead_code` lint couldn't be emitted correctly. After this PR, `ComesFromAllowExpect::Yes` items no longer incorrectly update other `ComesFromAllowExpect::Yes` items during propagation or mark them live by mistake, then `dead_code` lint could behave as expected.
…TaKO8Ki Fix `#[expect(dead_code)]` liveness propagation Fixes rust-lang#154324 Fixes rust-lang#152370 (cc @eggyal) Previously, when traversing from a `ComesFromAllowExpect::Yes` item (i.e., with `#[allow(dead_code)]` or `#[expect(dead_code)]`), other `ComesFromAllowExpect::Yes` items reached during propagation would be updated to `ComesFromAllowExpect::No` and inserted into `live_symbols`. That caused `dead_code` lint couldn't be emitted correctly. After this PR, `ComesFromAllowExpect::Yes` items no longer incorrectly update other `ComesFromAllowExpect::Yes` items during propagation or mark them live by mistake, then `dead_code` lint could behave as expected.
…TaKO8Ki Fix `#[expect(dead_code)]` liveness propagation Fixes rust-lang#154324 Fixes rust-lang#152370 (cc @eggyal) Previously, when traversing from a `ComesFromAllowExpect::Yes` item (i.e., with `#[allow(dead_code)]` or `#[expect(dead_code)]`), other `ComesFromAllowExpect::Yes` items reached during propagation would be updated to `ComesFromAllowExpect::No` and inserted into `live_symbols`. That caused `dead_code` lint couldn't be emitted correctly. After this PR, `ComesFromAllowExpect::Yes` items no longer incorrectly update other `ComesFromAllowExpect::Yes` items during propagation or mark them live by mistake, then `dead_code` lint could behave as expected.
Rollup of 6 pull requests Successful merges: - #155028 (tests: add whitespace tests for vertical tab behavior) - #155582 (Rewrite `FlatMapInPlace`.) - #151194 (Fix wrong suggestion for returning async closure) - #154377 (Fix `#[expect(dead_code)]` liveness propagation) - #155572 (Move diagnostic attribute target checks from check_attr) - #155586 (Ensure we don't feed owners from ast lowering if we ever make that query tracked)
Rollup merge of #154377 - mu001999-contrib:fix/dead-code, r=TaKO8Ki Fix `#[expect(dead_code)]` liveness propagation Fixes #154324 Fixes #152370 (cc @eggyal) Previously, when traversing from a `ComesFromAllowExpect::Yes` item (i.e., with `#[allow(dead_code)]` or `#[expect(dead_code)]`), other `ComesFromAllowExpect::Yes` items reached during propagation would be updated to `ComesFromAllowExpect::No` and inserted into `live_symbols`. That caused `dead_code` lint couldn't be emitted correctly. After this PR, `ComesFromAllowExpect::Yes` items no longer incorrectly update other `ComesFromAllowExpect::Yes` items during propagation or mark them live by mistake, then `dead_code` lint could behave as expected.
|
@rust-timer build 40d0e95 |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (40d0e95): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.1%, secondary 3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.8%, secondary 5.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 492.256s -> 491.986s (-0.05%) |
|
Looks like this caused some regressions, including outside incremental scenarios. I think since it's a correctness fix it's probably acceptable, marking as triaged. I suspect most of the regressions are predominantly just in instruction count based on the changes -- since we're moving slightly larger structs around that'll take a few extra instructions -- but that shouldn't take meaningfully more wall time, which aligns with minimal cycle count changes. |
…yukang Add regression tests for fixed dead-code issues Fixes rust-lang#56750 (fixed by rust-lang#142485) Fixes rust-lang#102190 (fixed by rust-lang#102538) Fixes rust-lang#102217 (fixed by rust-lang#102538) Fixes rust-lang#148861 (fixed by rust-lang#154377)
…yukang Add regression tests for fixed dead-code issues Fixes rust-lang#56750 (fixed by rust-lang#142485) Fixes rust-lang#102190 (fixed by rust-lang#102538) Fixes rust-lang#102217 (fixed by rust-lang#102538) Fixes rust-lang#148861 (fixed by rust-lang#154377)
…yukang Add regression tests for fixed dead-code issues Fixes rust-lang#56750 (fixed by rust-lang#142485) Fixes rust-lang#102190 (fixed by rust-lang#102538) Fixes rust-lang#102217 (fixed by rust-lang#102538) Fixes rust-lang#148861 (fixed by rust-lang#154377)
…yukang Add regression tests for fixed dead-code issues Fixes rust-lang#56750 (fixed by rust-lang#142485) Fixes rust-lang#102190 (fixed by rust-lang#102538) Fixes rust-lang#102217 (fixed by rust-lang#102538) Fixes rust-lang#148861 (fixed by rust-lang#154377)
Add regression tests for fixed dead-code issues Fixes rust-lang/rust#56750 (fixed by rust-lang/rust#142485) Fixes rust-lang/rust#102190 (fixed by rust-lang/rust#102538) Fixes rust-lang/rust#102217 (fixed by rust-lang/rust#102538) Fixes rust-lang/rust#148861 (fixed by rust-lang/rust#154377)
…ct and the `#[automatically_derived]` placements (#723) Three fixes to `#[ohno::error]` / `#[derive(ohno::Error)]`. The third changes what consumers see — please read that part first. ## 1. A unit struct is emitted in the macro's syntax context An error type declared as a **unit struct** is never reported by `dead_code`, even when nothing in the crate constructs or names it. A caller who writes `#[expect(dead_code)]` on such a type gets `unfulfilled_lint_expectations` instead, which fails a `-D warnings` gate with nothing pointing at ohno as the cause. ```rust #[expect(dead_code, reason = "kept for a later change")] #[ohno::error] pub(crate) struct XsoError; // warning: this lint expectation is unfulfilled ``` ### Cause `#[ohno::error]` gives a unit struct room for the `OhnoCore` field by turning it into a tuple struct. The parentheses and the semicolon it synthesises for that came from `Paren::default()` and `<Token![;]>::default()`, both of which carry `Span::call_site()`, so the rewritten item landed in the macro's syntax context rather than the caller's. `rustc_passes::dead` reports a struct at `ident_span.with_ctxt(def_span.ctxt())` — the identifier's span re-tagged with the *item's* context — and `lint_level` cancels any lint whose primary span sits in an external macro expansion. The `dead_code` diagnostic was therefore dropped before it could fulfil the expectation. Only the unit shape is affected. The named and tuple shapes push a field onto delimiters the author wrote, and `#[derive(ohno::Error)]` never rebuilds the item at all. ### Fix Take the synthesised spans from the declaration's own identifier. Measured against a consumer crate declaring all four forms with `#[expect(dead_code)]`, on rustc 1.97 (`ok` = the expectation is fulfilled): | form | before | after | |---|---|---| | `#[ohno::error] struct E;` | unfulfilled | ok | | `#[ohno::error] struct E(u32);` | ok | ok | | `#[ohno::error] struct E { n: u32 }` | ok | ok | | `#[derive(ohno::Error)] struct E(#[error] OhnoCore);` | ok | ok | ### Why there is no regression test A test would have to assert that a `#[expect(dead_code)]` is fulfilled, which means denying `unfulfilled_lint_expectations`. That cannot pass on the repo's MSRV: rustc kept an error type alive through the `#[allow(dead_code)]` on its generated constructors until [rust-lang/rust#154377](rust-lang/rust#154377) landed in **1.97**, and `RUST_MSRV` is `1.95`. Such a test becomes possible once the MSRV reaches 1.97. That rustc bug is a **separate, second** cause of the same symptom: on 1.93–1.96 every shape is affected, including the derive form, and no change to ohno avoids it. This PR fixes the part that is ohno's, and is the part still reproducible on 1.97 and later. ## 2. `#[automatically_derived]` on the inherent constructors impl That attribute is accepted only on a trait `impl`. The derive put it on the inherent `impl` holding `new` and `caused_by`, where `rustc` answers: ``` warning: `#[automatically_derived]` attribute cannot be used on inherent impl blocks = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! ``` No consumer sees that warning, because lints raised inside an external macro expansion are discarded. The unit-struct fix above does **not** change that — those tokens come from the derive rather than from the caller's declaration, which was verified by reverting the attribute independently and re-running a consumer crate. The defect is latent: it costs nothing until the release that promotes it to an error, at which point it breaks every `#[ohno::error]` and `#[derive(ohno::Error)]` user at once. Removing it changes no behaviour. The constructors carry their own `#[allow(dead_code)]` and never relied on it. ## 3. The generated `Debug` now carries `#[automatically_derived]` **This one changes what consumers see.** `Debug` carries `#[rustc_trivial_field_reads]`, so an `#[automatically_derived]` `Debug` impl has its field reads discarded by dead-code analysis. The generated impl went without the attribute deliberately, to avoid making a field that only `Debug` reads look unused. It does make it look unused — and that is the correct answer. Such a field *is* unused, `#[derive(Debug)]` reports it, and the remedy belongs to the author: `#[allow(dead_code)]`, an accessor, or a place in the `#[display]` message. Withholding the attribute suppressed that report for every error type in every consumer crate, so a field left behind by a refactor was never flagged. The suppression was also wider than the original reasoning assumed. `Display`, `Error`, `Enrichable` and `ErrorExt` are **not** `#[rustc_trivial_field_reads]`, so their reads still count. Measured on one crate carrying all four cases, rustc 1.95: | field is read by | reported? | |---|---| | `#[display("cannot open {path}")]` | no | | a user accessor | no | | the generated `Debug` only | **yes** | | nothing — it is the core | no | So the core stays live through the other four impls, and a field named by the `#[display]` template stays live through `Display`. Only a genuinely unused field is reported. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #154324
Fixes #152370 (cc @eggyal)
Previously, when traversing from a
ComesFromAllowExpect::Yesitem (i.e., with#[allow(dead_code)]or#[expect(dead_code)]), otherComesFromAllowExpect::Yesitems reached during propagation would be updated toComesFromAllowExpect::Noand inserted intolive_symbols. That causeddead_codelint couldn't be emitted correctly.After this PR,
ComesFromAllowExpect::Yesitems no longer incorrectly update otherComesFromAllowExpect::Yesitems during propagation or mark them live by mistake, thendead_codelint could behave as expected.