Prepare to make `rustc_trivial_field_reads` usable in user code by tehforsch · Pull Request #164217 · rust-lang/rust

tehforsch · 2026-10-12T16:05:13Z

Zulip discussion:
#t-lang > diagnostics attribute to ignore code for unused_code lints
#t-compiler/help > #160621 (Allow ignoring fns for dead code analysis)

Open questions I have:

Some of the Debug impls are not pointed out as usage sites. For example this test:

#[derive(Debug)]
enum TupleVariant {
    Variant1(i32), //~ ERROR: variant `Variant1` is never constructed
    Variant2,
}

fn main() {
	let _ = TupleVariant::Variant2;
}

now loses its the note in the error message:

note: `TupleVariant` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis

but in my mind this also makes sense - the Debug impl never constructs that Variant1 and even if rustc_trivial_field_reads is removed from the debug impl, the "variant never constructed" warning would still occur. Note that this doesn't apply to Clone impls, which do technically construct that variant although that brings me to point 2

If I am reading the code right, the whole point of the ignored_variant_stack is to make sure we don't mark a variant of an enum live from within a match arm that matches on that exact enum variant. But isn't that exactly what the Clone impl does? The inner match arm of it would look like Variant1(x) => Variant1(x). I looked into necessary_variants and found that it directly ignores the kind of variant we encounter, namely PatKind::TupleStruct(hir::QPath::TypeRelative(...)) but I don't really understand why and whether that's a bug or whether I am missing something

panstromek · 2026-10-12T16:57:28Z

@bors try @rust-timer queue

Prepare to make `rustc_trivial_field_reads` usable in user code

rust-bors · 2026-10-12T17:56:23Z

☀️ Try build successful (CI)
Build commit: e790d10 (e790d107e7868a817cc9ec9592a9b2d5fc634365)
Base parent: 0093837 (0093837afdf14ec7069c81da19cd51ed7d3476d9)

rust-timer · 2026-10-12T18:42:04Z

Finished benchmarking commit (e790d10): comparison URL.

Overall result: ❌ regressions - 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 @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.1%, 0.7%]	47
Regressions ❌ (secondary)	0.4%	[0.2%, 0.8%]	19
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.1%, 0.7%]	47

Max RSS (memory usage)

Results (primary 5.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	5.1%	[5.1%, 5.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	5.1%	[5.1%, 5.1%]	1

Cycles

Results (primary 2.6%, secondary 2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.2%, 3.0%]	3
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[2.2%, 3.0%]	3

Binary size

Results (secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 488.816s -> 489.518s (0.14%)
Artifact size: 406.86 MiB -> 406.85 MiB (-0.00%)

mejrs · 2026-10-12T21:39:29Z

@bors try @rust-timer queue

Prepare to make `rustc_trivial_field_reads` usable in user code

rust-bors · 2026-10-12T22:39:24Z

☀️ Try build successful (CI)
Build commit: a64a0d1 (a64a0d13d3c0d5e6be02384b77719f7a3d8a651f)
Base parent: d64de46 (d64de46d0aab619b69efe68a99d43d11434b6924)

rust-timer · 2026-10-13T00:10:04Z

Finished benchmarking commit (a64a0d1): 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 @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.1%, 0.6%]	48
Regressions ❌ (secondary)	0.4%	[0.2%, 0.9%]	16
Improvements ✅ (primary)	-0.4%	[-0.6%, -0.2%]	3
Improvements ✅ (secondary)	-0.5%	[-0.7%, -0.2%]	35
All ❌✅ (primary)	0.2%	[-0.6%, 0.6%]	51

Max RSS (memory usage)

Results (secondary 2.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -1.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.3%	[-1.3%, -1.3%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.1%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.2%]	74
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	53
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.2%]	74

Bootstrap: 490.869s -> 490.424s (-0.09%)
Artifact size: 406.63 MiB -> 406.91 MiB (0.07%)

mejrs · 2026-10-13T10:59:11Z

Let's run it again, i think the improvements are spurious.

@bors try @rust-timer queue

tehforsch · 2026-11-02T14:24:59Z

I was delaying this PR because I thought #164913 would make it easier to implement. I still think so but some of the semantics for #[allow(dead_code)] are currently being discussed (#157885) and so I am not sure it really makes sense to do a large refactoring like this before that discussion is settled. So now since the perf regressions on #164217 seem a lot better with your attribute parsing changes, I am thinking of going back to this naive approach and fixing the remaining issues with it and to maybe do the refactoring at some other time. I also found a lot of edge cases (and probably bugs) while working on the refactoring, so I'll also have to check how those are handled in this PR. I currently worry that some of them might be really tricky to solve in the old structure, but let's see

mejrs · 2026-11-02T17:34:50Z

I am thinking of going back to this naive approach and fixing the remaining issues with it and to maybe do the refactoring at some other time

Sounds good, if it's not required for this feature let's skip the refactoring.

I also found a lot of edge cases (and probably bugs) while working on the refactoring, so I'll also have to check how those are handled in this PR. I currently worry that some of them might be really tricky to solve in the old structure, but let's see

Ideally the behavior wouldn't change here, but if it easier to fix the bugs while you're at it (rather than emulate the old buggy behavior), I'd rather see the bugs fixed.

Before, when a trivial fn was used from multiple seeds, the "used here but ignored" note would appear twice.

tehforsch · 2026-11-06T15:45:19Z

I addressed some changes and added some tests and am making this ready for review. This still adds a good amount of complexity to some already complex code, so I'm still interested in the approach in #164913 .

Also we definitely have to check perf again - since the last perf checks, I added more complex diagnostics and changed the logic that decides whether items should be scanned or not, so this can definitely have an impact.

Open questions

The two questions in the PR description still apply, the first is a change of behavior in this PR that makes sense to me. The second I think is just a bug?

There is also one more decision: An item can be unused but be used from multiple trivial usage sites. In this case, I emit one note per usage site. However, diagnostics are also grouped together (for the fields in a struct, for example, if many of them are unused) and in this case I am now emitting one note per usage site and then point out all the fields that are unused from that particular site. An example diagnostic, taken from the grouped-diagnostic.rs test:

error: fields `first` and `second` are never read
  --> $DIR/grouped-diagnostic.rs:6:5
   |
LL | struct Struct {
   |        ------ fields in this struct
LL |     first: bool,
   |     ^^^^^
LL |     second: bool,
   |     ^^^^^^
   |
note: `first` is used here, but this is intentionally ignored during dead code analysis
  --> $DIR/grouped-diagnostic.rs:11:1
   |
LL | fn read_first(data: &Struct) {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: `second` is used here, but this is intentionally ignored during dead code analysis
  --> $DIR/grouped-diagnostic.rs:16:1
   |
LL | fn read_second(data: &Struct) {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: `first` and `second` are used here, but this is intentionally ignored during dead code analysis
  --> $DIR/grouped-diagnostic.rs:21:1
   |
LL | fn read_first_and_second(data: &Struct) {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I think this is quite clear, but we could also do this in other ways.

Finally, it's not entirely clear what to do about propagation of trivial usage sites from traits to their impls. I don't know whether we have to get this completely right in this PR though

rustbot · 2026-11-06T15:48:30Z

Some changes occurred in compiler/rustc_attr_ir

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Changes to the code generated for builtin derived traits.

cc @nnethercote

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

rustbot · 2026-11-06T15:48:34Z

r? @mati865

rustbot has assigned @mati865.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler
compiler expanded to 79 candidates
Random selection from 22 candidates

tehforsch · 2026-11-06T15:50:06Z

+        // We immediately sort these by span afterwards, so
+        // the instability does not matter
+        #[allow(rustc::potential_query_instability)]
+        let mut notes: Vec<_> = sites
+            .into_iter()
+            .map(|(usage_site, def_ids)| {
+                let usage_site = self.live_symbols.trivial_usage_site_by_id(usage_site);
+                let names: Vec<_> =
+                    def_ids.iter().map(|def_id| self.tcx.item_name(*def_id)).collect();
+                let multiple = names.len() > 1;
+                let names = names.into();
+                let (span, usage_kind) = if usage_site.attr_span.in_derive_expansion() {
+                    (usage_site.attr_span, "in this derive macro")
+                } else if usage_site.attr_span.in_external_macro(self.tcx.sess.source_map()) {
+                    (usage_site.attr_span, "in this macro invocation")
+                } else {
+                    (self.tcx.hir_span(usage_site.hir_id), "here")
+                };
+                crate::diagnostics::TrivialUsageSite { names, span, usage_kind, multiple }
+            })
+            .collect();
+        notes.sort_unstable_by_key(|note| note.span.lo_hi());
+        notes


I am not super sure about this - is this a reasonable approach? They need to be sorted somehow, but I don't know if doing it by span is reasonable.

View changes since the review

tehforsch · 2026-11-06T20:22:38Z

r? @mejrs

tehforsch force-pushed the fix-160621 branch from c06e80c to cf722b6 Compare October 12, 2026 16:12