Skip to content

Refactor the two-phase check for impls and impl items #141407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 30, 2025

Conversation

mu001999
Copy link
Contributor

@mu001999 mu001999 commented May 22, 2025

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 22, 2025
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2025
@mu001999 mu001999 force-pushed the dead-code/refactor branch from b19a615 to faab5c6 Compare May 26, 2025 08:18
@mu001999
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 26, 2025
@mu001999
Copy link
Contributor Author

CI looks broken

@petrochenkov petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 26, 2025
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 26, 2025
@mu001999
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 26, 2025
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

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

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 27, 2025

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

@mu001999 mu001999 force-pushed the dead-code/refactor branch from 1cf061b to e50396e Compare May 27, 2025 13:48
@rust-log-analyzer

This comment has been minimized.

@mu001999
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 27, 2025
@mu001999 mu001999 force-pushed the dead-code/refactor branch from e50396e to f83ecd8 Compare May 27, 2025 14:03
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented May 28, 2025

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

@tgross35
Copy link
Contributor

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented May 28, 2025

📌 Commit 419897c has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 28, 2025
@mu001999
Copy link
Contributor Author

@petrochenkov update with tests

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented May 29, 2025

📌 Commit 6eb6010 has been approved by petrochenkov

It is now in the queue for this repository.

bors added a commit that referenced this pull request May 30, 2025
Rollup of 8 pull requests

Successful merges:

 - #133823 (Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion)
 - #141004 (Report text_direction_codepoint_in_literal when parsing)
 - #141407 (Refactor the two-phase check for impls and impl items)
 - #141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`)
 - #141507 (atomic_load intrinsic: use const generic parameter for ordering)
 - #141538 (implement `va_arg` for x86_64 systemv)
 - #141669 (float: Replace some approximate assertions with exact)
 - #141747 (rustdoc: display doc(cfg(false)) properly)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 896be66 into rust-lang:master May 30, 2025
9 checks passed
@rustbot rustbot added this to the 1.89.0 milestone May 30, 2025
rust-timer added a commit that referenced this pull request May 30, 2025
Rollup merge of #141407 - mu001999-contrib:dead-code/refactor, r=petrochenkov

Refactor the two-phase check for impls and impl items

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
@Kobzol
Copy link
Contributor

Kobzol commented May 30, 2025

@rust-timer build 1d94b88

(trying perf. due to #141753)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

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%)

@rustbot rustbot added the perf-regression Performance regression. label May 30, 2025
@Kobzol
Copy link
Contributor

Kobzol commented May 30, 2025

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

@mu001999
Copy link
Contributor Author

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.

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request May 31, 2025
Rollup of 8 pull requests

Successful merges:

 - rust-lang/rust#133823 (Use `cfg_attr_trace` in AST with a placeholder attribute for accurate suggestion)
 - rust-lang/rust#141004 (Report text_direction_codepoint_in_literal when parsing)
 - rust-lang/rust#141407 (Refactor the two-phase check for impls and impl items)
 - rust-lang/rust#141430 (remove `visit_clobber` and move `DummyAstNode` to `rustc_expand`)
 - rust-lang/rust#141507 (atomic_load intrinsic: use const generic parameter for ordering)
 - rust-lang/rust#141538 (implement `va_arg` for x86_64 systemv)
 - rust-lang/rust#141669 (float: Replace some approximate assertions with exact)
 - rust-lang/rust#141747 (rustdoc: display doc(cfg(false)) properly)

r? `@ghost`
`@rustbot` modify labels: rollup
@Kobzol
Copy link
Contributor

Kobzol commented May 31, 2025

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. :) )

dtolnay added a commit to serde-rs/serde that referenced this pull request May 31, 2025
Bisects to rust-lang/rust#141407.

    warning: struct `Struct` is never constructed
       --> test_suite/tests/test_gen.rs:803:16
        |
    803 |     pub struct Struct {
        |                ^^^^^^
        |
        = note: `#[warn(dead_code)]` on by default

    warning: function `vec_first_element` is never used
       --> test_suite/tests/test_gen.rs:885:4
        |
    885 | fn vec_first_element<T, S>(vec: &[T], serializer: S) -> StdResult<S::Ok, S::Error>
        |    ^^^^^^^^^^^^^^^^^

    warning: struct `S` is never constructed
     --> test_suite/tests/regression/issue2415.rs:5:12
      |
    5 | pub struct S;
      |            ^
      |
      = note: `#[warn(dead_code)]` on by default
dtolnay added a commit to dtolnay/indoc that referenced this pull request May 31, 2025
Bisects to rust-lang/rust#141407.

    warning: function `unindent` is never used
     --> src/unindent.rs:3:8
      |
    3 | pub fn unindent(s: &str) -> String {
      |        ^^^^^^^^
      |
      = note: `#[warn(dead_code)]` on by default

    warning: function `unindent_bytes` is never used
      --> src/unindent.rs:10:8
       |
    10 | pub fn unindent_bytes(s: &[u8]) -> Vec<u8> {
       |        ^^^^^^^^^^^^^^

    warning: trait `Unindent` is never used
      --> src/unindent.rs:53:11
       |
    53 | pub trait Unindent {
       |           ^^^^^^^^
@Mark-Simulacrum Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Jun 2, 2025
@alexcrichton
Copy link
Member

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
Copy link
Contributor Author

mu001999 commented Jun 3, 2025

@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
Copy link
Member

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
Copy link
Contributor Author

mu001999 commented Jun 4, 2025

@alexcrichton I will have a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dead_code warning when private trait is only used as type bound in another trait type alias Plus2 is never used
10 participants