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.

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

Don't warn on fields in the unreachable_pub lint #126040

Merged
merged 1 commit into from
Jun 7, 2024
Merged

Don't warn on fields in the unreachable_pub lint #126040

merged 1 commit into from
Jun 7, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Jun 5, 2024 •
edited
Loading

This PR restrict the unreachable_pub lint by not linting on pub fields of pub(restricted) structs and unions. This is done because that can quickly clutter the code for an uncertain value, in particular since the "real" visibility is defined by the parent (the struct it-self).

This is meant to address one of the last concern of the unreachable_pub lint.

r? @petrochenkov

@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 Jun 5, 2024
@Urgau Urgau mentioned this pull request Jun 5, 2024
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Urgau Urgau force-pushed the unreachable_pub-fields-less branch from 2e2b3b3 to 0156e08 Compare June 5, 2024 17:42
@Urgau
Copy link
Member Author

Urgau commented Jun 6, 2024

One issue with not linting on fields of pub(restricted) structs is that when the struct and it's fields are pub, we would still lint with a machine-applicable lint on the struct and the fields, while linting on the fields would not be required anymore.

I wonder if in those cases we should not lint on the fields? or maybe downgrade from MachineApplicable to MaybeIncorrect for struct fields?

petrochenkov
petrochenkov reviewed Jun 6, 2024
View reviewed changes
compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/builtin.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

One issue with not linting on fields of pub(restricted) structs is that when the struct and it's fields are pub, we would still lint with a machine-applicable lint on the struct and the fields, while linting on the fields would not be required anymore.

I wonder if in those cases we should not lint on the fields? or maybe downgrade from MachineApplicable to MaybeIncorrect for struct fields?

When checking fields we already know whether the lint was reported for the struct or not.
I agree that if it was reported, then the fields should be skipped.

@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 Jun 6, 2024
@petrochenkov
Copy link
Contributor

Hmm, should we ever check fields at all?

  • If a structure is reported then we don't need to check fields.
  • If a structure is not reported because it's not pub - we don't need to check fields (that's the original point of this PR).
  • If a structure is not reported because it's reachable - we also don't need to check fields because then they are reachable by construction if they are pub.

It seems like the whole check_field_def method can be just removed.

@Urgau Urgau force-pushed the unreachable_pub-fields-less branch from 0156e08 to 89d86ae Compare June 6, 2024 17:07
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 6, 2024

📌 Commit 89d86ae 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 Jun 6, 2024
@Urgau Urgau changed the title Don't warn on pub fields of pub(restricted) structs Don't warn on fields in the unreachable_pub lint Jun 6, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 7, 2024
@workingjubilee
Rollup merge of rust-lang#126040 - Urgau:unreachable_pub-fields-less,…
cfb9479 
… r=petrochenkov

Don't warn on fields in the `unreachable_pub` lint

This PR restrict the `unreachable_pub` lint by not linting on `pub` fields of `pub(restricted)` structs and unions. This is done because that can quickly clutter the code for an uncertain value, in particular since the "real" visibility is defined by the parent (the struct it-self).

This is meant to address one of the last concern of the `unreachable_pub` lint.

r? `@petrochenkov`
@workingjubilee workingjubilee mentioned this pull request Jun 7, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 7, 2024
@bors
Auto merge of rust-lang#126107 - workingjubilee:rollup-0h6p5sq, r=wor…
a8f09d4 
…kingjubilee

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125606 (Size optimize int formatting)
 - rust-lang#125724 (Uplift `Relate`/`TypeRelation` into `rustc_next_trait_solver`)
 - rust-lang#126040 (Don't warn on fields in the `unreachable_pub` lint )
 - rust-lang#126065 (mark binding undetermined if target name exist and not obtained)
 - rust-lang#126098 (Remove `same-lib-two-locations-no-panic` run-make test)
 - rust-lang#126099 (Crate loader cleanups)
 - rust-lang#126101 (Revert "Disallow ambiguous attributes on expressions" on nightly)
 - rust-lang#126103 (Improve Docs for `hir::Impl` and `hir::ImplItem`)

r? `@ghost`
`@rustbot` modify labels: rollup
@workingjubilee workingjubilee mentioned this pull request Jun 7, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 7, 2024
@bors
Auto merge of rust-lang#126108 - workingjubilee:rollup-g7m92b6, r=wor…
468310e 
…kingjubilee

Rollup of 7 pull requests

Successful merges:

 - rust-lang#125606 (Size optimize int formatting)
 - rust-lang#125724 (Uplift `Relate`/`TypeRelation` into `rustc_next_trait_solver`)
 - rust-lang#126040 (Don't warn on fields in the `unreachable_pub` lint )
 - rust-lang#126098 (Remove `same-lib-two-locations-no-panic` run-make test)
 - rust-lang#126099 (Crate loader cleanups)
 - rust-lang#126101 (Revert "Disallow ambiguous attributes on expressions" on nightly)
 - rust-lang#126103 (Improve Docs for `hir::Impl` and `hir::ImplItem`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6a42df7 into rust-lang:master Jun 7, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 7, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 7, 2024
@rust-timer
Unrolled build for rust-lang#126040
56a59ea 
Rollup merge of rust-lang#126040 - Urgau:unreachable_pub-fields-less, r=petrochenkov

Don't warn on fields in the `unreachable_pub` lint

This PR restrict the `unreachable_pub` lint by not linting on `pub` fields of `pub(restricted)` structs and unions. This is done because that can quickly clutter the code for an uncertain value, in particular since the "real" visibility is defined by the parent (the struct it-self).

This is meant to address one of the last concern of the `unreachable_pub` lint.

r? ``@petrochenkov``
@nnethercote nnethercote mentioned this pull request Oct 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

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.
Projects
None yet
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Urgau @rust-log-analyzer @petrochenkov @bors @rustbot