expanding #[rustc_trivial_field_reads] to more targets, place on Derive and Clone

[josetorrs]

View all comments

part of #160621.

also minor discussion in:

• #t-lang > diagnostics attribute to ignore code for unused_code lints

• #t-compiler > Irrelevant unused code link issue #160621

• I did not use an LLM to create a change in this PR.

• I used an LLM to create a change in this PR, and I have explained below how it was used.

r? @mejrs

[josetorrs]

TBH not really sure about this but my computer has been really slow so wanted to see what CI would do

View changes since the review

[mejrs]

The changes look correct so far, you just need to delete the #[rustc_trivial_field_reads] in the minicore.rs file.

[mejrs]

Let's see whether this affects perf so far.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: af61126 (af61126db7562afad7eb31247e4ba2d8968c4766)
Base parent: 8b798d4 (8b798d41cc5d215ef8d5424f61b6dc2575ac738b)

[rust-timer]

Finished benchmarking commit (af61126): 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
Regressions ❌ (secondary)	0.3%	[0.2%, 0.4%]	9
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.1%]	2
Improvements ✅ (secondary)	-0.6%	[-0.6%, -0.6%]	1
All ❌✅ (primary)	-0.3%	[-0.4%, -0.1%]	2

Max RSS (memory usage)

Results (primary 2.1%, secondary 0.7%)

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

	mean	range	count
Regressions ❌ (primary)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	1.6%	[0.4%, 8.3%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-1.4%, -0.4%]	5
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

Cycles

Results (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
Regressions ❌ (secondary)	0.7%	[0.5%, 1.1%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.9%	[-2.3%, -0.5%]	7
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.1%]	40
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	23
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.1%]	40

Bootstrap: 459.298s -> 462.975s (0.80%)
Artifact size: 398.58 MiB -> 399.35 MiB (0.19%)

[mejrs]

Hmm that's not going to get better if we extend this to more places than implitems. Also, I've been thinking and I don't think this is the right approach.

I've been looking at the prior history of this (much discussion in #85200) and the derive expansion of tracing macros, and I think it makes most sense to implement it as suggested in #85200 (comment). Then you can also put it on manually written Debug impls for example and macros can more easily use it in a more granular way.

Meaning, this attribute shouldn't go on the macro (or the trait); the macro should put the attribute on the items it emits. To do that you'd have to

• adjust what the attribute is allowed on
• change the implementations (located in rustc_expand) of Debug/Clone to emit the attribute
• change the macro shims in the standard library to allow the use of this attribute inside them, with #[allow_internal_unstable]
• adjust the dead code pass to check for the presence of the attribute on more items

What do you think?

[josetorrs]

Yeah I was looking at that PR Friday night while exploring the changes for this. Also while looking the linked issues, do you think that would alleviate some of the concerns in #88900?

What do you think?

I'm using this issue as a learning experience for something a little more involved and very much appreciate your pointers but I may be the wrong person to bounce ideas off of 😅

[mejrs]

Also while looking the linked issues, do you think that would alleviate some of the concerns in #88900?

The changes in this PR shouldn't be user visible, so it shouldn't change anything in that respect.

That said we get issues like that with every noticeable addition of lints, I wouldn't worry about it.

[josetorrs]

I think I made some progress after a lot of debugging but now my tests not passing after recent commit yikes but looking for a gut check on this so far

[mejrs]

Remember to set the waiting-on-review label if you need help/review, otherwise it's likely to slip through the cracks :)

now my tests not passing after recent commit

It looks like you put the attribute on the methods but check for its presence on the trait ref itself.

View changes since this review

[mejrs]

BTW, rather than trying to fix this case, I'd suggest just proceeding with this point I mentioned above:

• adjust the dead code pass to check for the presence of the attribute on more items

So that would entail allowing more targets for the attribute, and, instead of this:

rust/compiler/rustc_passes/src/dead.rs

Lines 461 to 463 in f7d782a

	ifletNode::ImplItem(impl_item) = node
	&& self.should_ignore_impl_item(impl_item)
	{

it's probably easiest to get the HirId of the node, and check, in visit_node, whether the attribute is there or not, and bail out if it does.

[rustbot]

Changes to the code generated for builtin derived traits.

cc @nnethercote

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[josetorrs]

okay I think it's ready now.

for the targets:

AllowedTargets::AllowList(&[Allow(Target::Fn),Allow(Target::Method(MethodKind::Inherent)),Allow(Target::Method(MethodKind::Trait{body:true})),Allow(Target::Method(MethodKind::TraitImpl)),]);

I kind of just chose the common ones I saw for the other attributes but that I think would make sense for this one

@rustbot ready