Refactor the two-phase check for impls and impl items

[mu001999]

Refactor the two-phase dead code check to make the logic clearer and simpler:

1. adding assoc fn and impl into unsolved_items directly during the initial construction of the worklist
2. converge the logic of checking whether assoc fn and impl are used to item_should_be_checked, and the item is considered used only when its corresponding trait and Self adt are used

This PR only refactors as much as possible to avoid affecting the original functions. However, due to the adjustment of the order of checks, the test results are slightly different, but overall, there is no regression problem

Fixes #127911
Fixes #128839

Extracted from #128637.
r? petrochenkov

try-job: dist-aarch64-linux

[mu001999]

@rustbot ready

[mu001999]

CI looks broken

[mu001999]

@rustbot ready

[petrochenkov]

Thanks, this looks better now.
r=me after squashing commits.
@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[mu001999]

@rustbot ready

[bors]

☀️ Try build successful - checks-actions
Build commit: e460fa9 (e460fa92810ea7a96ed243a06e61d2abb6afab7e)

[tgross35]

@bors r=petrochenkov

[bors]

📌 Commit 419897c has been approved by petrochenkov

It is now in the queue for this repository.

[mu001999]

@petrochenkov update with tests

[petrochenkov]

@bors r+

[bors]

📌 Commit 6eb6010 has been approved by petrochenkov

It is now in the queue for this repository.

[Kobzol]

@rust-timer build 1d94b88

(trying perf. due to #141753)

[rust-timer]

Finished benchmarking commit (1d94b88): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.1%, 1.3%]	65
Regressions ❌ (secondary)	0.3%	[0.1%, 0.5%]	17
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.1%, 1.3%]	65

Max RSS (memory usage)

Results (primary 2.4%, secondary 0.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.4%	[1.5%, 4.6%]	7
Regressions ❌ (secondary)	1.2%	[0.5%, 2.9%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.6%	[-2.3%, -0.8%]	2
All ❌✅ (primary)	2.4%	[1.5%, 4.6%]	7

Cycles

Results (primary 1.1%, secondary 0.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	0.9%	[0.4%, 2.5%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.9%	[-1.3%, -0.5%]	4
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	1

Binary size

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

Bootstrap: missing data
Artifact size: 370.26 MiB -> 370.30 MiB (0.01%)

[Kobzol]

This seems to have caused most of the regressions in #141753.

[mu001999]

This seems to have caused most of the regressions in #141753.

Oh, this might be expected. While this PR does more, I'm not sure if the performance impact is within normal range.

[Kobzol]

It's nothing terrible, if it was expected, and this fixes some issues, I think it's fine (unless you have ideas on how to fix the perf. :) )

[alexcrichton]

Question for this: In Wasmtime I've bisected a new warning on nightly to this PR where a new dead code warning is firing for a method. The problem is that when removing the method the crate no longer compiles so I'm wondering if it might be a false positive. I've used cargo bisect-rustc to point to this PR as the regression point so far. Locally this can be reproduced with cargo +nightly check -p cranelift-codegen --features all-arch.

Unfortunately the cranelift-codegen crate is quite large and I'm having a difficult time minimizing this to something that would be worthy of opening an issue with. Which leads me to my question: would you be able to describe at a high level the changes that were made here? I'm hoping that such an explanation might trigger an idea for me of how to reduce the warning to something I can open as an issue. I basically fear that "all of cranelift-codegen" is a bit much for opening an issue with.

[mu001999]

@alexcrichton What it essentially does is simply propagate the unused status to the corresponding impl and impl items by checking whether traits and trait items, as well as ADTs, have been used.

Considering your command has features option, I guess this method might be feature-dependent? For example, it might only be unused under certain features, in other case directly deleting it would be problematic—you'd need to use cfg conditional compilation for this method instead.

You could first verify whether this dead code appears across all features.

[alexcrichton]

Alas while it may be simple for you I fear I don't understand the ramifications of such a change, so in the meantime I dug in a bit more with creduce and various other reduction tools. I believe this ended up being an interaction with a false-negative instead of a false positive since this has to do with macro-generated code which meant that the method in question wasn't the only function that was dead but was the only one that was warned about. That led me to confusion which also had to do with how many locations the macro was expanded in.

In any case, no, this is not feature dependent, but additionally I don't think there's anything that needs to change.

[mu001999]

@alexcrichton I will have a look