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

Detect pub structs never constructed and unused associated constants #125572

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

mu001999
Copy link
Contributor

@mu001999 mu001999 commented May 26, 2024

Lints never constructed public structs.

If we don't provide public methods to construct public structs with private fields, and don't construct them in the local crate. They would be never constructed. So that we can detect such public structs.


Update:

Also lints unused associated constants in traits.

@rustbot
Copy link
Collaborator

rustbot commented May 26, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 26, 2024
@mu001999
Copy link
Contributor Author

r? @pnkfelix

cc @shepmaster

@rustbot rustbot assigned pnkfelix and unassigned wesleywiser May 26, 2024
@rust-log-analyzer

This comment has been minimized.

@mu001999 mu001999 changed the title Detect pub structs never constructed [WIP] Detect pub structs never constructed May 26, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented May 26, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@mu001999 mu001999 changed the title [WIP] Detect pub structs never constructed Detect pub structs never constructed May 27, 2024
@mu001999 mu001999 force-pushed the dead/enhance branch 3 times, most recently from 2fc16d5 to 1390585 Compare May 27, 2024 12:36
@mu001999 mu001999 changed the title Detect pub structs never constructed Detect pub structs never constructed and unused associated constants May 27, 2024
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
@mu001999 mu001999 force-pushed the dead/enhance branch 2 times, most recently from 37bc658 to cc7aa4d Compare May 28, 2024 07:41
@mu001999 mu001999 requested a review from SparrowLii May 28, 2024 09:05
Copy link
Member

@SparrowLii SparrowLii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some nits

@pnkfelix

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

bors commented May 31, 2024

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

@pnkfelix
Copy link
Member

pnkfelix commented Jun 5, 2024

I found the state of this analysis interesting, especially the fact that it flags the struct as dead even when there's a impl Clone of it sitting there.

This makes sense: the presence of clone does not provide evidence that the struct is live, because you need an instance of the struct in order to be able to enter the clone code in the first place, in order to call the fn clone(&self) -> Self method.

But it led me to wonder: How far does this reasoning go? I.e., does it also filter out other variations of the same inductive reasoning, such as a variant trait that takes x: Self or x: &Self?

So I built a local copy, and soon determined: No, the reasoning does not go that far. That is, either of the following trait impls will suffice to treat a struct constructor as "live code":

pub trait U { fn f(_: Self) -> Self; }
impl U for S { fn f(x: Self) -> Self { S(10) } }


pub trait W { fn f(_: &Self) -> Self; }
impl W for S { fn f(x: &Self) -> Self { S(10) } }

Note: This is fine. I'd rather we start with a lint that doesn't attempt to catch such cases, than risk shipping an over-aggressive lint that ends up also accidentally treating things like fn f(x: Option<&Self>) -> Self as proof of life.

asomers added a commit to asomers/mockall that referenced this pull request Jun 9, 2024
These structs are never constructed, and the latest nightly no longer
considers a trait impl as enough to keep the struct "live".

rust-lang/rust#125572
@mu001999 mu001999 deleted the dead/enhance branch June 10, 2024 14:55
@SparrowLii
Copy link
Member

SparrowLii commented Jun 11, 2024

This should be because struct_all_fields_are_public additionally checks that each field of the struct is pub or not. We may have to find a way to reduce the number of extra checks for struct fields, such as changing the return value of the function to

struct Publicness {
    ty_is_public: bool,
    ty_and_all_fields_are_public: Option<bool>,
}

Obtain whether each field of the structure is pub only when necessary.

@mu001999 @Kobzol

@mu001999
Copy link
Contributor Author

mu001999 commented Jun 11, 2024

I think there is another source of extra time like #121752 (comment), more items in the worklist means that these items may be checked repeatedly.

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jul 30, 2024
…nce, r=pnkfelix"

This reverts commit 13314df, reversing
changes made to 6e534c7.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jul 30, 2024
…nce, r=pnkfelix"

This reverts commit 13314df, reversing
changes made to 6e534c7.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jul 30, 2024
…nce, r=pnkfelix"

This reverts commit 13314df, reversing
changes made to 6e534c7.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jul 30, 2024
…nce, r=pnkfelix"

This reverts commit 13314df, reversing
changes made to 6e534c7.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 31, 2024
…hanges, r=pnkfelix

Revert recent changes to dead code analysis

This is a revert to recent changes to dead code analysis, namely:
* efdf219 Rollup merge of rust-lang#128104 - mu001999-contrib:fix/128053, r=petrochenkov
* a70dc29 Rollup merge of rust-lang#127017 - mu001999-contrib:dead/enhance, r=pnkfelix
* 31fe962 Rollup merge of rust-lang#127107 - mu001999-contrib:dead/enhance-2, r=pnkfelix
* 2724aea Rollup merge of rust-lang#126618 - mu001999-contrib:dead/enhance, r=pnkfelix
* 977c5fd Rollup merge of rust-lang#126315 - mu001999-contrib:fix/126289, r=petrochenkov
* 13314df Rollup merge of rust-lang#125572 - mu001999-contrib:dead/enhance, r=pnkfelix

There is an additional change stacked on top, which suppresses false-negatives that were masked by this work. I believe the functions that are touched in that code are legitimately unused functions and the types are not reachable since this `AnonPipe` type is not publically reachable -- please correct me if I'm wrong cc `@NobodyXu` who added these in #rust-lang#127153.

Some of these reverts (rust-lang#126315 and rust-lang#126618) are only included because it makes the revert apply cleanly, and I think these changes were only done to fix follow-ups from the other PRs?

I apologize for the size of the PR and the churn that it has on the codebase (and for reverting `@mu001999's` work here), but I'm putting this PR up because I am concerned that we're making ad-hoc changes to fix bugs that are fallout of these PRs, and I'd like to see these changes reimplemented in a way that's more separable from the existing dead code pass. I am happy to review any code to reapply these changes in a more separable way.

cc `@mu001999`
r? `@pnkfelix`

Fixes rust-lang#128272
Fixes rust-lang#126169
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jul 31, 2024
…nce, r=pnkfelix"

This reverts commit 13314df, reversing
changes made to 6e534c7.
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 1, 2024
…hanges, r=pnkfelix

Revert recent changes to dead code analysis

This is a revert to recent changes to dead code analysis, namely:
* efdf219 Rollup merge of rust-lang#128104 - mu001999-contrib:fix/128053, r=petrochenkov
* a70dc29 Rollup merge of rust-lang#127017 - mu001999-contrib:dead/enhance, r=pnkfelix
* 31fe962 Rollup merge of rust-lang#127107 - mu001999-contrib:dead/enhance-2, r=pnkfelix
* 2724aea Rollup merge of rust-lang#126618 - mu001999-contrib:dead/enhance, r=pnkfelix
* 977c5fd Rollup merge of rust-lang#126315 - mu001999-contrib:fix/126289, r=petrochenkov
* 13314df Rollup merge of rust-lang#125572 - mu001999-contrib:dead/enhance, r=pnkfelix

There is an additional change stacked on top, which suppresses false-negatives that were masked by this work. I believe the functions that are touched in that code are legitimately unused functions and the types are not reachable since this `AnonPipe` type is not publically reachable -- please correct me if I'm wrong cc `@NobodyXu` who added these in #rust-lang#127153.

Some of these reverts (rust-lang#126315 and rust-lang#126618) are only included because it makes the revert apply cleanly, and I think these changes were only done to fix follow-ups from the other PRs?

I apologize for the size of the PR and the churn that it has on the codebase (and for reverting `@mu001999's` work here), but I'm putting this PR up because I am concerned that we're making ad-hoc changes to fix bugs that are fallout of these PRs, and I'd like to see these changes reimplemented in a way that's more separable from the existing dead code pass. I am happy to review any code to reapply these changes in a more separable way.

cc `@mu001999`
r? `@pnkfelix`

Fixes rust-lang#128272
Fixes rust-lang#126169
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Aug 1, 2024
…nce, r=pnkfelix"

This reverts commit 13314df, reversing
changes made to 6e534c7.
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 1, 2024
…hanges, r=pnkfelix

Revert recent changes to dead code analysis

This is a revert to recent changes to dead code analysis, namely:
* efdf219 Rollup merge of rust-lang#128104 - mu001999-contrib:fix/128053, r=petrochenkov
* a70dc29 Rollup merge of rust-lang#127017 - mu001999-contrib:dead/enhance, r=pnkfelix
* 31fe962 Rollup merge of rust-lang#127107 - mu001999-contrib:dead/enhance-2, r=pnkfelix
* 2724aea Rollup merge of rust-lang#126618 - mu001999-contrib:dead/enhance, r=pnkfelix
* 977c5fd Rollup merge of rust-lang#126315 - mu001999-contrib:fix/126289, r=petrochenkov
* 13314df Rollup merge of rust-lang#125572 - mu001999-contrib:dead/enhance, r=pnkfelix

There is an additional change stacked on top, which suppresses false-negatives that were masked by this work. I believe the functions that are touched in that code are legitimately unused functions and the types are not reachable since this `AnonPipe` type is not publically reachable -- please correct me if I'm wrong cc `@NobodyXu` who added these in #rust-lang#127153.

Some of these reverts (rust-lang#126315 and rust-lang#126618) are only included because it makes the revert apply cleanly, and I think these changes were only done to fix follow-ups from the other PRs?

I apologize for the size of the PR and the churn that it has on the codebase (and for reverting `@mu001999's` work here), but I'm putting this PR up because I am concerned that we're making ad-hoc changes to fix bugs that are fallout of these PRs, and I'd like to see these changes reimplemented in a way that's more separable from the existing dead code pass. I am happy to review any code to reapply these changes in a more separable way.

cc `@mu001999`
r? `@pnkfelix`

Fixes rust-lang#128272
Fixes rust-lang#126169
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 2, 2024
…hanges, r=pnkfelix

Revert recent changes to dead code analysis

This is a revert to recent changes to dead code analysis, namely:
* efdf219 Rollup merge of rust-lang#128104 - mu001999-contrib:fix/128053, r=petrochenkov
* a70dc29 Rollup merge of rust-lang#127017 - mu001999-contrib:dead/enhance, r=pnkfelix
* 31fe962 Rollup merge of rust-lang#127107 - mu001999-contrib:dead/enhance-2, r=pnkfelix
* 2724aea Rollup merge of rust-lang#126618 - mu001999-contrib:dead/enhance, r=pnkfelix
* 977c5fd Rollup merge of rust-lang#126315 - mu001999-contrib:fix/126289, r=petrochenkov
* 13314df Rollup merge of rust-lang#125572 - mu001999-contrib:dead/enhance, r=pnkfelix

There is an additional change stacked on top, which suppresses false-negatives that were masked by this work. I believe the functions that are touched in that code are legitimately unused functions and the types are not reachable since this `AnonPipe` type is not publically reachable -- please correct me if I'm wrong cc ``@NobodyXu`` who added these in #rust-lang#127153.

Some of these reverts (rust-lang#126315 and rust-lang#126618) are only included because it makes the revert apply cleanly, and I think these changes were only done to fix follow-ups from the other PRs?

I apologize for the size of the PR and the churn that it has on the codebase (and for reverting ``@mu001999's`` work here), but I'm putting this PR up because I am concerned that we're making ad-hoc changes to fix bugs that are fallout of these PRs, and I'd like to see these changes reimplemented in a way that's more separable from the existing dead code pass. I am happy to review any code to reapply these changes in a more separable way.

cc ``@mu001999``
r? ``@pnkfelix``

Fixes rust-lang#128272
Fixes rust-lang#126169
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 2, 2024
…hanges, r=pnkfelix

Revert recent changes to dead code analysis

This is a revert to recent changes to dead code analysis, namely:
* efdf219 Rollup merge of rust-lang#128104 - mu001999-contrib:fix/128053, r=petrochenkov
* a70dc29 Rollup merge of rust-lang#127017 - mu001999-contrib:dead/enhance, r=pnkfelix
* 31fe962 Rollup merge of rust-lang#127107 - mu001999-contrib:dead/enhance-2, r=pnkfelix
* 2724aea Rollup merge of rust-lang#126618 - mu001999-contrib:dead/enhance, r=pnkfelix
* 977c5fd Rollup merge of rust-lang#126315 - mu001999-contrib:fix/126289, r=petrochenkov
* 13314df Rollup merge of rust-lang#125572 - mu001999-contrib:dead/enhance, r=pnkfelix

There is an additional change stacked on top, which suppresses false-negatives that were masked by this work. I believe the functions that are touched in that code are legitimately unused functions and the types are not reachable since this `AnonPipe` type is not publically reachable -- please correct me if I'm wrong cc ```@NobodyXu``` who added these in #rust-lang#127153.

Some of these reverts (rust-lang#126315 and rust-lang#126618) are only included because it makes the revert apply cleanly, and I think these changes were only done to fix follow-ups from the other PRs?

I apologize for the size of the PR and the churn that it has on the codebase (and for reverting ```@mu001999's``` work here), but I'm putting this PR up because I am concerned that we're making ad-hoc changes to fix bugs that are fallout of these PRs, and I'd like to see these changes reimplemented in a way that's more separable from the existing dead code pass. I am happy to review any code to reapply these changes in a more separable way.

cc ```@mu001999```
r? ```@pnkfelix```

Fixes rust-lang#128272
Fixes rust-lang#126169
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Aug 3, 2024
…hanges, r=pnkfelix

Revert recent changes to dead code analysis

This is a revert to recent changes to dead code analysis, namely:
* efdf219 Rollup merge of rust-lang#128104 - mu001999-contrib:fix/128053, r=petrochenkov
* a70dc29 Rollup merge of rust-lang#127017 - mu001999-contrib:dead/enhance, r=pnkfelix
* 31fe962 Rollup merge of rust-lang#127107 - mu001999-contrib:dead/enhance-2, r=pnkfelix
* 2724aea Rollup merge of rust-lang#126618 - mu001999-contrib:dead/enhance, r=pnkfelix
* 977c5fd Rollup merge of rust-lang#126315 - mu001999-contrib:fix/126289, r=petrochenkov
* 13314df Rollup merge of rust-lang#125572 - mu001999-contrib:dead/enhance, r=pnkfelix

There is an additional change stacked on top, which suppresses false-negatives that were masked by this work. I believe the functions that are touched in that code are legitimately unused functions and the types are not reachable since this `AnonPipe` type is not publically reachable -- please correct me if I'm wrong cc ````@NobodyXu```` who added these in #rust-lang#127153.

Some of these reverts (rust-lang#126315 and rust-lang#126618) are only included because it makes the revert apply cleanly, and I think these changes were only done to fix follow-ups from the other PRs?

I apologize for the size of the PR and the churn that it has on the codebase (and for reverting ````@mu001999's```` work here), but I'm putting this PR up because I am concerned that we're making ad-hoc changes to fix bugs that are fallout of these PRs, and I'd like to see these changes reimplemented in a way that's more separable from the existing dead code pass. I am happy to review any code to reapply these changes in a more separable way.

cc ````@mu001999````
r? ````@pnkfelix````

Fixes rust-lang#128272
Fixes rust-lang#126169
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Aug 3, 2024
…nce, r=pnkfelix"

This reverts commit 13314df, reversing
changes made to 6e534c7.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
…nges, r=pnkfelix

Revert recent changes to dead code analysis

This is a revert to recent changes to dead code analysis, namely:
* efdf219 Rollup merge of rust-lang#128104 - mu001999-contrib:fix/128053, r=petrochenkov
* a70dc29 Rollup merge of rust-lang#127017 - mu001999-contrib:dead/enhance, r=pnkfelix
* 31fe962 Rollup merge of rust-lang#127107 - mu001999-contrib:dead/enhance-2, r=pnkfelix
* 2724aea Rollup merge of rust-lang#126618 - mu001999-contrib:dead/enhance, r=pnkfelix
* 977c5fd Rollup merge of rust-lang#126315 - mu001999-contrib:fix/126289, r=petrochenkov
* 13314df Rollup merge of rust-lang#125572 - mu001999-contrib:dead/enhance, r=pnkfelix

There is an additional change stacked on top, which suppresses false-negatives that were masked by this work. I believe the functions that are touched in that code are legitimately unused functions and the types are not reachable since this `AnonPipe` type is not publically reachable -- please correct me if I'm wrong cc `@NobodyXu` who added these in #rust-lang#127153.

Some of these reverts (rust-lang#126315 and rust-lang#126618) are only included because it makes the revert apply cleanly, and I think these changes were only done to fix follow-ups from the other PRs?

I apologize for the size of the PR and the churn that it has on the codebase (and for reverting `@mu001999's` work here), but I'm putting this PR up because I am concerned that we're making ad-hoc changes to fix bugs that are fallout of these PRs, and I'd like to see these changes reimplemented in a way that's more separable from the existing dead code pass. I am happy to review any code to reapply these changes in a more separable way.

cc `@mu001999`
r? `@pnkfelix`

Fixes rust-lang#128272
Fixes rust-lang#126169
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Aug 3, 2024
…nce, r=pnkfelix"

This reverts commit 13314df, reversing
changes made to 6e534c7.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Aug 3, 2024
…nce, r=pnkfelix"

This reverts commit 13314df, reversing
changes made to 6e534c7.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2024
…nges-beta, r=cuviper

Revert recent changes to dead code analysis (beta flavor)

This PR cherry-picks the subset of rust-lang#128404 which actually applies to beta, namely:

* 31fe962 Rollup merge of rust-lang#127107 - mu001999-contrib:dead/enhance-2, r=pnkfelix
* 2724aea Rollup merge of rust-lang#126618 - mu001999-contrib:dead/enhance, r=pnkfelix
* 977c5fd Rollup merge of rust-lang#126315 - mu001999-contrib:fix/126289, r=petrochenkov
* 13314df Rollup merge of rust-lang#125572 - mu001999-contrib:dead/enhance, r=pnkfelix

And then re-blesses tests.

I opted to do a manual backport of that PR since preparing the revert correctly was quite a hassle.

r? `@cuviper` (or anyone on release)
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Aug 6, 2024
…nce, r=pnkfelix"

This reverts commit 13314df, reversing
changes made to 6e534c7.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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
Development

Successfully merging this pull request may close these issues.

8 participants