Skip to content
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

Improve dead code analysis for structs and traits defined locally #128637

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mu001999
Copy link
Contributor

@mu001999 mu001999 commented Aug 4, 2024

This PR does some refactor and improvement on the dead code analysis, and doesn't lint pub structs.

  1. refactors the two-phase check for impls and impl items of trait
    1. checks them all later because we must use the trait/trait item and the adt defined locally firstly
    2. makes the logic cleaner and doesn't require special checks about whether it's public or not
  2. mark the adt live if it appears in pattern, like generic argument, it also implies the use of the adt
    1. based on 1 and 2, we can detect unused private adts impl Default, without adding special logics for Default
    2. so that we can remove rustc_trivial_field_reads on Default, and the logic in should_ignore_item
  3. extends rules to impls for types which refer to adts, like &Foo/[Foo] things
  4. lints unused assoc consts like assoc fns, and unused traits with assoc tys by marking assoc tys live only if the trait is live (is same to Mark assoc tys live only if the corresponding trait is live #126618 reverted in Revert recent changes to dead code analysis #128404)

Fixes #120770
Fixes #126729
Fixes #127911
Fixes #128839

cc @pnkfelix
r? @compiler-errors

@rustbot rustbot added PG-exploit-mitigations Project group: Exploit mitigations 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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 4, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2024

Some changes occurred in tests/ui/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

@rust-log-analyzer

This comment has been minimized.

@mu001999 mu001999 force-pushed the dead-code/enhance branch 2 times, most recently from 02e0843 to edee22b Compare August 4, 2024 13:34
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

Please separate this into separate commits each implementing an individual tweak to the dead code analysis, with the tests adjusted at each commit so I can see the fallout from each change specifically. It's very difficult to map the changes to the UI tests to each code change without that. --- I want to think very hard about each of these changes to determine if there are any false positives that are caused by each change, and that is harder to do with a single commit.

Also, if you want, please open a separate PR that removes the dead code from the compiler/standard library that is now detected after these changes. That can be landed separately.

@rust-log-analyzer

This comment has been minimized.

@mu001999 mu001999 force-pushed the dead-code/enhance branch 2 times, most recently from 865ba84 to 7759822 Compare August 5, 2024 15:38
@mu001999
Copy link
Contributor Author

mu001999 commented Aug 6, 2024

@compiler-errors I have separated this to separate commits.

@bors
Copy link
Contributor

bors commented Aug 7, 2024

☔ The latest upstream changes (presumably #128761) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot self-assigned this Aug 8, 2024
@mu001999 mu001999 force-pushed the dead-code/enhance branch 2 times, most recently from eab62e8 to 6f4b522 Compare August 9, 2024 00:32
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/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 Show resolved Hide resolved
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 Show resolved Hide resolved
compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Aug 13, 2024

☔ The latest upstream changes (presumably #129046) made this pull request unmergeable. Please resolve the merge conflicts.

@cjgillot
Copy link
Contributor

Could you submit 1317d54 as a separate PR?

@cjgillot cjgillot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 25, 2024
@rust-log-analyzer

This comment has been minimized.

@mu001999
Copy link
Contributor Author

It is probably necessary to make the lint handle associated types, I'd like an explanation why it is necessary

Just detecting more dead codes? I think removing more unused items is helpful. And we have detected unused inherit assoc types, although the feature is unstable for now.

and why we can't just rely on HIR and typeck-output resolutions.

We can only get Res::Err when meeting a assoc type in hir. typeck_results.qpath_res works but it only work for QPaths in an Expr or Pat node. And we may have cases like:

trait TwoWayStrategy {
    type Output;
    fn use_early_reject() -> bool;
    fn rejecting(a: usize, b: usize) -> Self::Output;
    fn matching(a: usize, b: usize) -> Self::Output;
}

I asked such things before in zulip

@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 Aug 25, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 25, 2024
…r=compiler-errors

Removes dead code from the compiler

Detected by rust-lang#128637
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 25, 2024
…r=compiler-errors

Removes dead code from the compiler

Detected by rust-lang#128637
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 26, 2024
Rollup merge of rust-lang#129544 - mu001999-contrib:dead-code/clean, r=compiler-errors

Removes dead code from the compiler

Detected by rust-lang#128637
@bors
Copy link
Contributor

bors commented Aug 26, 2024

☔ The latest upstream changes (presumably #129595) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Sep 7, 2024

☔ The latest upstream changes (presumably #129941) made this pull request unmergeable. Please resolve the merge conflicts.

@mu001999
Copy link
Contributor Author

mu001999 commented Oct 6, 2024

@cjgillot friendly ping

@mu001999
Copy link
Contributor Author

mu001999 commented Nov 3, 2024

It is probably necessary to make the lint handle associated types, I'd like an explanation why it is necessary

@cjgillot

Updated: using simpler rules for assoc tys instead of reasoning on types, i.e., only marking assoc tys live only if the corresponding trait is live, which can help to lint more unused traits (is same to #126618 reverted in #128404).

Removes old logic for unused assoc tys.
I think the it needs further consideration and could later be made as a separate PR.

@rust-log-analyzer

This comment has been minimized.

@wesleywiser
Copy link
Member

Hi @cjgillot, it looks like this PR is ready for review again when you have time. Thanks 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PG-exploit-mitigations Project group: Exploit mitigations 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
7 participants