lint against repeated repr attributes

[ettolrach]

View all comments

fixes #156029 by adding a lint against repeated repr attributes.

r? scottmcm

[scottmcm]

Wearing my lang hat but not speaking as team consensus (since we've not talked about it):

I think you should start with the "obviously good" version, and we can do future PRs later to expand it or to update severity. Concretely, thus, I'd say it should be a warning (because if it's inert it doesn't need to be deny) when you have a clearly-unnecessary one that wasn't emitted from a macro, or something like that.

(Our bar in the compiler for deny-by-default is pretty high. My personal heuristic is whether it's worth running your unit tests despite the warning, and an inert extra attribute doesn't need to block that.)

We can then look at impacts and ratchet it up over time or over an edition, but those decisions can be made later and don't need to block you making a useful lint in the meantime.

---

I'm a bad reviewer for lint stuff code-wise, though, so let's try
r? JonathanBrouwer

[traviscross]

+1 to what @scottmcm said on how best to approach this.

[rustbot]

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

cc @jdonszelmann, @JonathanBrouwer

[jdonszelmann]

r? jdonszelmann
I don't mind giving this one a look :)

[jdonszelmann]

@ettolrach if you rebase and address what lang asked, let's go merge! :)

[rustbot]

Some changes occurred in compiler/rustc_attr_ir

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[ettolrach]

that's the merge conflicts fixed. would appreciate a quick look at the latest two commits to make sure that i added the lint to the unused group correctly :)

[jdonszelmann]

@ettolrach could you squash the typo commits? Not all of them, I generally like many commits in a PR to show the way its built up, but those tiny ones should probably either be part of the other commits, or all typos together in one commit if you can.

[jdonszelmann]

formatting changes you can keep in its own commit, though it's always nicer if you keep every commit formatted. Sometimes the rebase is bad though...

[jdonszelmann]

r=me otherwise :) ping me once you reordered some commits and I'll merge it

[ettolrach]

tests pass, that's ready to be merged now @jdonszelmann

[jdonszelmann]

@bors r+

[rust-bors]

📌 Commit e99f23f has been approved by jdonszelmann

It is now in the queue for this repository.

[rust-timer]

Note

This PR was benchmarked as part of triage of its containing rollup: triage URL.

Finished benchmarking commit (39d59b8): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (secondary 2.2%)

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.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 0.6%, 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)	3.4%	[3.4%, 3.4%]	1
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[-2.1%, 3.4%]	2

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 475.812s -> 480.439s (0.97%)
Artifact size: 403.37 MiB -> 402.66 MiB (-0.18%)