Skip to content

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

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.

Sign up for GitHub

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

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

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

wants to merge 5 commits into from
+526 −197

Conversation

mu001999
Copy link
Contributor

@mu001999 mu001999 commented Aug 4, 2024 •
edited
Loading

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

@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.

Sign in to view
@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.

Sign in to view
@mu001999 mu001999 force-pushed the dead-code/enhance branch from edee22b to dda1f0d Compare August 4, 2024 14:45
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@mu001999 mu001999 force-pushed the dead-code/enhance branch from dda1f0d to 8507664 Compare August 4, 2024 16:18
@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.

Sign in to view
@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

This comment was marked as resolved.

Sign in to view
@mu001999 mu001999 force-pushed the dead-code/enhance branch from 7759822 to e400203 Compare August 7, 2024 15:08
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@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
cjgillot
cjgillot reviewed Aug 10, 2024
View reviewed changes
@bors

This comment was marked as resolved.

Sign in to view
@mu001999 mu001999 requested a review from cjgillot August 13, 2024 23:55
@cjgillot
Copy link
Contributor

Could you submit 1317d54 as a separate PR?

cjgillot
cjgillot reviewed Aug 24, 2024
View reviewed changes
@mu001999 mu001999 force-pushed the dead-code/enhance branch from 48cd073 to 994b713 Compare April 3, 2025 11:20
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot
Copy link
Contributor

cjgillot commented Apr 3, 2025

I don't have time to do review any more.
r? compiler

@rustbot rustbot assigned jieyouxu and unassigned estebank Apr 3, 2025
@mu001999 mu001999 force-pushed the dead-code/enhance branch from 994b713 to e482d0b Compare April 3, 2025 12:43
@jieyouxu
Copy link
Member

jieyouxu commented Apr 3, 2025

Busy with bootstrap things.
r? compiler

@rustbot rustbot assigned BoxyUwU and unassigned jieyouxu Apr 3, 2025
@mu001999
Copy link
Contributor Author

mu001999 commented Apr 3, 2025

r? estebank

@rustbot rustbot assigned estebank and unassigned BoxyUwU Apr 3, 2025
@apiraino apiraino removed the request for review from cjgillot April 8, 2025 10:09
@apiraino
Copy link
Contributor

r? compiler

@rustbot rustbot assigned petrochenkov and unassigned estebank May 22, 2025
@petrochenkov
Copy link
Contributor

I reviewed one of the predecessors to this PR and my experience can be summarized by this comment #122382 (comment)

And I still don't understand what you are trying to do with it in dead.rs, in general.

because neither the code itself nor its explanations were clear enough to understand what's going on, so I'm not really enthusiastic about reviewing it again.

@mu001999
Could you make a PR containing just test additions/modifications and refactorings not containing any functional changes?
After that we can reconsider the rest of the PR.
@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 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 22, 2025

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

@mu001999
Copy link
Contributor Author

@petrochenkov ok, I'll turn these commits into separate PRs

@mu001999
Copy link
Contributor Author

link to #141407, contains just test additions/modifications and refactorings

@petrochenkov petrochenkov mentioned this pull request May 26, 2025
Open
tgross35 added a commit to tgross35/rust that referenced this pull request May 28, 2025
@tgross35
Rollup merge of rust-lang#141407 - mu001999-contrib:dead-code/refacto…
784e30f 
…r, 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

Extracted from rust-lang#128637.
r? petrochenkov
jhpratt added a commit to jhpratt/rust that referenced this pull request May 28, 2025
@jhpratt
Rollup merge of rust-lang#141407 - mu001999-contrib:dead-code/refacto…
ee9bcfc 
…r, 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

Extracted from rust-lang#128637.
r? petrochenkov
bors added a commit that referenced this pull request May 28, 2025
@bors
Auto merge of #141407 - mu001999-contrib:dead-code/refactor, r=<try>
3fa0eec 
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

Extracted from #128637.
r? petrochenkov

try-job: dist-aarch64-linux
bors added a commit that referenced this pull request May 28, 2025
@bors
Auto merge of #141407 - mu001999-contrib:dead-code/refactor, r=<try>
e460fa9 
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

Extracted from #128637.
r? petrochenkov

try-job: dist-aarch64-linux
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 30, 2025
@matthiaskrgr
Rollup merge of rust-lang#141407 - mu001999-contrib:dead-code/refacto…
896be66 
…r, 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 rust-lang#127911
Fixes rust-lang#128839

Extracted from rust-lang#128637.
r? petrochenkov

try-job: dist-aarch64-linux
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@petrochenkov petrochenkov

Labels
PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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
Milestone
No milestone