Collapse multiple dead code warnings into a single diagnostic #97853

TaKO8Ki · 2022-06-08T01:12:54Z

closes #97643

rust-highfive · 2022-06-08T01:12:57Z

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

compiler/rustc_passes/src/dead.rs

compiler-errors · 2022-06-09T00:37:30Z

Thanks for applying the changes I asked for, the code is much simpler now 🎉. But I'm not sure if I like how this renders -- I would prefer either continuing to be as verbose as we are now, or grouping all 4 of these diagnostics into one, i.e. "these fields are unused". Also this only applies to structs, not enums or unions -- if we want to land this, it should probably be generalized to all ADTs.

But I will defer to the reviewer to a call on whether this change is worth merging.

TaKO8Ki · 2022-06-09T01:05:11Z

@compiler-errors Thank you :)

grouping all 4 of these diagnostics into one, i.e. "these fields are unused".

That is a good idea. I will open an issue about that.

davidtwco · 2022-06-09T08:38:22Z

Thanks for applying the changes I asked for, the code is much simpler now 🎉. But I'm not sure if I like how this renders -- I would prefer either continuing to be as verbose as we are now, or grouping all 4 of these diagnostics into one, i.e. "these fields are unused". Also this only applies to structs, not enums or unions -- if we want to land this, it should probably be generalized to all ADTs.

I'm not sure how I feel about this change - implementation looks good, just unsure about the actual change.

On the one hand, we're giving the user less output to sift through, which is probably a good thing. On the other hand, if the user looks at one of the diagnostics without the note first, it's now less helpful, which isn't great. I think the best outcome would be to collapse these into a single diagnostic, as has been suggested already.

TaKO8Ki · 2022-06-09T10:01:47Z

@davidtwco
I understand. I will fix warnings to collapse them into a single diagnostic. Should I implement it in this PR or a new one?

davidtwco · 2022-06-09T10:51:40Z

@davidtwco I understand. I will fix warnings to collapse them into a single diagnostic. Should I implement it in this PR or a new one?

You can do either, feel free to update this or close it and open a new PR - whichever is easiest for you.

compiler/rustc_passes/src/dead.rs

cjgillot · 2022-06-10T17:37:39Z

compiler/rustc_passes/src/dead.rs

+                (
+                    Node::Item(hir::Item {
+                        kind:
+                            hir::ItemKind::Struct(hir::VariantData::Struct(..), ..)


Why not Tuple fields too?

That is because rustc does not warn about tuple fields (positional fields) like the following if I am not wrong.

rust/compiler/rustc_passes/src/dead.rs

Line 653 in 20ad820

!field.is_positional()

It'd be interesting to see how much noise would be caused by checking tuple fields as well, even if it is only on rustc 👀

I wonder what the original rationale for not doing so was.

compiler/rustc_passes/src/dead.rs

cjgillot · 2022-06-11T12:53:03Z

I really have a lot of doubts about this change. This brings a lot of additional complexity, manually re-implements some of the lint handling infrastructure, and makes testing more brittle since we can't annotate the whole list of unused fields any more.

I'd see such complexity much more keenly if they were making the lint more precise.

As we already stopped pointing to the ignored trait derives, half of the output in #97643 is gone. I'm a bit tempted to close the issue and this PR.

Other possibility: could such merging of lints be done at the level of the diagnostic infrastructure itself? (It's a naive question, I don't know that code at all.) This would allow to keep a low complexity in the analysis itself, and have the support code handle it, and maybe be much easier to extend to other verbose lints.

TaKO8Ki · 2022-06-11T13:04:40Z

@cjgillot Thank you for your review! I would like to know what the others think about. @rust-lang/wg-diagnostics

compiler-errors · 2022-06-12T04:22:00Z

As we already stopped pointing to the ignored trait derives, half of the output in #97643 is gone.

I actually was unaware that the more verbose output had already been mostly suppressed for a much shorter note somewhere between stable and nightly.

In that case, I actually probably agree with @cjgillot that this is a lot of overhead for suppressing just a few lines of diagnostics.

rust-timer · 2022-06-22T06:51:24Z

Finished benchmarking commit (3d829a0): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	3.0%	3.0%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	3.0%	3.0%	1

Cycles

Results

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	2.1%	2.1%	2
Regressions 😿 (secondary)	2.2%	2.3%	2
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.1%	2.1%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

ehuss · 2022-06-23T19:36:38Z

@TaKO8Ki and @estebank, please don't approve PRs that update cargo to an uncommitted PR. If there is a problem with cargo's tests not passing due to a diagnostic change, please work with the cargo team to change the tests to pass and to have that committed to master before updating the submodule. Thanks!

Fix tests due to change in dead_code diagnostic. rust-lang/rust#97853 changed some diagnostics which is causing some tests to fail on the latest nightly. This updates the tests to work on both stable and nightly.

TaKO8Ki · 2022-06-24T02:30:05Z

@ehuss Ok. I understand. I appreciate your help.

estebank · 2022-06-24T21:11:26Z

@ehuss apologies for the breakdown of process.

RalfJung · 2022-06-25T12:56:52Z

This also changed the rls submodule to a commit not in the rls repo (https://github.com/rust-lang/rls/tree/27f4044df03d15c7c38a483c3e4635cf4f51807d), which means currently I cannot build rustc locally since it fails to fetch that commit.

RalfJung · 2022-06-25T12:57:52Z

Can this PR be reverted to get the repo into a state again where its submodules point to existing commits?

TaKO8Ki · 2022-06-25T13:09:16Z

@RalfJung Wouldn't merging this pull request solve this provlem? rust-lang/rls#1780

RalfJung · 2022-06-25T13:30:43Z

Maybe, but when a PR has unintended side-effects the right reaction is to back it out and try again -- not to hurry and land some fix in a haste.

RalfJung · 2022-06-25T13:40:55Z

Don't worry, we do want to have your PR. :) We just want to make sure that it doesn't break anything else, and (due to no fault of yours -- the process should have caught this) this PR caused more trouble than it should. I know seeing one's PR reverted can be frustrating, but this is really just a temporary thing, and then we have all the time we need to figure out how to land this properly without causing trouble.

This merges this commit into RLS master; it was pulled into rust-lang/rust by mistake in rust-lang/rust#97853 before it was merged into rust-lang/rls master branch. The commit in its raw form is persisted on rust-lang/rls by way of a new branch (retain-for-rustc-3d829a0922d865d7a77fb284424fd8ba6afaea3b), and after being merged into master we will bump rust-lang/rust to the latest version of rust-lang/rls master.

Of primary interest, this merges rust-lang/rls@ece09b8 into rust-lang/rust, which brings in the changes that fix RLS tests broken by rust-lang#97853. rust-lang#97853 already introduced that commit's changes (under 27f4044df03d15c7c38a483c3e4635cf4f51807d) but without putting those changes on rust-lang/rls as a branch, so we ended up with an orphan commit that caused trouble when updating submodules in rust-lang/rust. This commit, once merged into rust-lang/rust, should continue to let RLS tests to pass on rust-lang/rust's side and move us back into a healthy state where tip of the submodule points to a valid master commit in the rust-lang/rls repository.

Mark-Simulacrum · 2022-06-25T14:07:52Z

OK, I've now posted branches to both rust-lang/cargo and rust-lang/rls to preserve the wrongly-introduced commits for the future; that will likely already fix the submodule updates (but I have no confirmation as of this commit from users reporting breakage).

We've already bumped Cargo since this PR in #98395, which moved us back onto Cargo master.

I've also posted #98488 to move us back onto RLS master, and nominated for beta backport (to the beta branching next week; i.e., 1.63).

I believe that should resolve the problem here and avoid a revert which would've been annoying to backport to 1.63 and such.

TaKO8Ki · 2022-06-25T14:11:06Z

@RalfJung Thanks. Is there anything I can help?

…lbini Bump RLS to latest master on rust-lang/rls Of primary interest, this merges rust-lang/rls@ece09b8 into rust-lang/rust, which brings in the changes that fix RLS tests broken by rust-lang#97853. rust-lang#97853 already introduced that commit's changes (under rust-lang/rls@27f4044) but without putting those changes on rust-lang/rls as a branch, so we ended up with an orphan commit that caused trouble when updating submodules in rust-lang/rust. This commit, once merged into rust-lang/rust, should continue to let RLS tests to pass on rust-lang/rust's side and move us back into a healthy state where tip of the submodule points to a valid master commit in the rust-lang/rls repository. cc rust-lang#98451, but not marking as fixed as I believe we need to add verification to prevent future oversights.

Of primary interest, this merges rust-lang/rls@ece09b8 into rust-lang/rust, which brings in the changes that fix RLS tests broken by rust-lang#97853. rust-lang#97853 already introduced that commit's changes (under 27f4044df03d15c7c38a483c3e4635cf4f51807d) but without putting those changes on rust-lang/rls as a branch, so we ended up with an orphan commit that caused trouble when updating submodules in rust-lang/rust. This commit, once merged into rust-lang/rust, should continue to let RLS tests to pass on rust-lang/rust's side and move us back into a healthy state where tip of the submodule points to a valid master commit in the rust-lang/rls repository.

Fix tests due to change in dead_code diagnostic. rust-lang/rust#97853 changed some diagnostics which is causing some tests to fail on the latest nightly. This updates the tests to work on both stable and nightly.

…r=lcnr dead-code-lint: de-dup multiple unused assoc functions Fixes rust-lang#109600 Prior art: rust-lang#97853

rust-highfive assigned davidtwco Jun 8, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 8, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 8, 2022

compiler-errors reviewed Jun 8, 2022

View reviewed changes

compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved

TaKO8Ki force-pushed the emit-only-one-note-per-unused-struct-field branch from 5adad17 to b7ecc07 Compare June 8, 2022 06:31

compiler-errors reviewed Jun 8, 2022

View reviewed changes

compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved

compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved

compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Jun 8, 2022

View reviewed changes

compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved

TaKO8Ki force-pushed the emit-only-one-note-per-unused-struct-field branch 2 times, most recently from fb7100b to 8a7cbf5 Compare June 8, 2022 07:36

TaKO8Ki requested a review from compiler-errors June 8, 2022 10:23

This comment has been minimized.

Sign in to view

TaKO8Ki force-pushed the emit-only-one-note-per-unused-struct-field branch from 9e9013f to 8491b73 Compare June 10, 2022 03:15

compiler-errors reviewed Jun 10, 2022

View reviewed changes

compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved

compiler/rustc_passes/src/dead.rs Show resolved Hide resolved

compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved

compiler-errors reviewed Jun 10, 2022

View reviewed changes

compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved

TaKO8Ki changed the title ~~Emit only one note per unused struct field~~ Collapse multiple dead code warnings into a single diagnostic Jun 10, 2022

TaKO8Ki requested a review from davidtwco June 10, 2022 04:12

cjgillot reviewed Jun 10, 2022

View reviewed changes

compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved

cjgillot reviewed Jun 10, 2022

View reviewed changes

compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved

TaKO8Ki requested review from cjgillot and compiler-errors June 11, 2022 06:57

bors merged commit 3d829a0 into rust-lang:master Jun 22, 2022

rustbot added this to the 1.63.0 milestone Jun 22, 2022

TaKO8Ki deleted the emit-only-one-note-per-unused-struct-field branch June 22, 2022 06:20

ehuss mentioned this pull request Jun 23, 2022

Fix tests due to change in dead_code diagnostic. rust-lang/cargo#10785

Merged

RalfJung mentioned this pull request Jun 25, 2022

src/tools/rls submodule remote commit is broken; only works when current remote is called 'origin' #98451

Closed

Mark-Simulacrum mentioned this pull request Jun 25, 2022

Bump RLS to latest master on rust-lang/rls #98488

Merged

Ezrashaw mentioned this pull request Apr 13, 2023

dead-code-lint: de-dup multiple unused assoc functions #110277

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 13, 2023

Rollup merge of rust-lang#110277 - Ezrashaw:combine-assoc-fns-dlint, …

759d4e8

…r=lcnr dead-code-lint: de-dup multiple unused assoc functions Fixes rust-lang#109600 Prior art: rust-lang#97853

matthiaskrgr mentioned this pull request Dec 7, 2023

ICE: ast lowering: no entry for node id: NodeId(26) #118697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collapse multiple dead code warnings into a single diagnostic #97853

Collapse multiple dead code warnings into a single diagnostic #97853

TaKO8Ki commented Jun 8, 2022

rust-highfive commented Jun 8, 2022

compiler-errors commented Jun 9, 2022 •

edited

Loading

TaKO8Ki commented Jun 9, 2022

davidtwco commented Jun 9, 2022

TaKO8Ki commented Jun 9, 2022

davidtwco commented Jun 9, 2022

This comment has been minimized.

cjgillot Jun 10, 2022

TaKO8Ki Jun 11, 2022

estebank Jun 15, 2022 •

edited

Loading

cjgillot commented Jun 11, 2022

TaKO8Ki commented Jun 11, 2022 •

edited

Loading

compiler-errors commented Jun 12, 2022

rust-timer commented Jun 22, 2022

ehuss commented Jun 23, 2022

TaKO8Ki commented Jun 24, 2022

estebank commented Jun 24, 2022

RalfJung commented Jun 25, 2022

RalfJung commented Jun 25, 2022

TaKO8Ki commented Jun 25, 2022

RalfJung commented Jun 25, 2022

RalfJung commented Jun 25, 2022

Mark-Simulacrum commented Jun 25, 2022

TaKO8Ki commented Jun 25, 2022

Collapse multiple dead code warnings into a single diagnostic #97853

Collapse multiple dead code warnings into a single diagnostic #97853

Conversation

TaKO8Ki commented Jun 8, 2022

rust-highfive commented Jun 8, 2022

compiler-errors commented Jun 9, 2022 • edited Loading

TaKO8Ki commented Jun 9, 2022

davidtwco commented Jun 9, 2022

TaKO8Ki commented Jun 9, 2022

davidtwco commented Jun 9, 2022

This comment has been minimized.

cjgillot Jun 10, 2022

Choose a reason for hiding this comment

TaKO8Ki Jun 11, 2022

Choose a reason for hiding this comment

estebank Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

cjgillot commented Jun 11, 2022

TaKO8Ki commented Jun 11, 2022 • edited Loading

compiler-errors commented Jun 12, 2022

rust-timer commented Jun 22, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

ehuss commented Jun 23, 2022

TaKO8Ki commented Jun 24, 2022

estebank commented Jun 24, 2022

RalfJung commented Jun 25, 2022

RalfJung commented Jun 25, 2022

TaKO8Ki commented Jun 25, 2022

RalfJung commented Jun 25, 2022

RalfJung commented Jun 25, 2022

Mark-Simulacrum commented Jun 25, 2022

TaKO8Ki commented Jun 25, 2022

compiler-errors commented Jun 9, 2022 •

edited

Loading

estebank Jun 15, 2022 •

edited

Loading

TaKO8Ki commented Jun 11, 2022 •

edited

Loading