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

Add field_scoped_visibility_modifiers lint #12893

Merged
merged 1 commit into from
Jun 16, 2024

Conversation

kyleoneill
Copy link
Contributor

changelog: [field_scoped_visibility_modifiers]: Add a lint which checks for struct fields using Restricted (not inherited, not public) visibility modifiers.

@rustbot
Copy link
Collaborator

rustbot commented Jun 6, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @blyxyas (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 6, 2024
@kyleoneill kyleoneill force-pushed the field_scoped_visibility_modifiers branch from e12bae5 to 2ea4242 Compare June 6, 2024 05:06
@blyxyas
Copy link
Member

blyxyas commented Jun 6, 2024

Hmmmm... I'm not really understanding the appeal of this lint, could you explain it a little bit more?

@kyleoneill
Copy link
Contributor Author

The primary goal is to make unsafe code more audit-able, the lint makes it easier to verify that invariants for a struct are upheld. If you opt-in to it then you only have to do local analysis on a struct when auditing it. If a struct has a field with pub(crate) visibility, then that fields usage must be checked across an entire crate. Global analysis like this is a pain once crates get large enough.

As an example, this is a pub(crate) modifier being used on an id field in tokio. Presumably you would have the invariant that an id should never be changed, but that must be verified by checking the entire tokio crate where this struct is used. A better way to access this would be to add a pub(crate) method to the struct which returns the id, effectively making it read only.

@djkoloski
Copy link

+1, I'd really like to have this lint available

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I've been kinda busy these few days. After some thinking, it seems like this lint could be useful in the strictest of codebases, and it doesn't represent much overhead to have a restriction lint.

LGTM, thanks! ❤️

@kyleoneill
Copy link
Contributor Author

No problem at all, thank you for reviewing 😊

@bors
Copy link
Collaborator

bors commented Jun 11, 2024

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

@blyxyas
Copy link
Member

blyxyas commented Jun 12, 2024

https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/FCP.20field_scoped_visibility_modifiers

I've opened the Final Comment Period stream here, and we have some feedback:

I think the Use instead in the example could be improved. It suggests to make those fields public, which IMO goes against the original intention of the lint. Maybe it should suggest to remove them and add a method, as the author wrote in a comment.

@kyleoneill kyleoneill force-pushed the field_scoped_visibility_modifiers branch 2 times, most recently from b28bbde to 3c5dcf3 Compare June 13, 2024 01:07
@bors
Copy link
Collaborator

bors commented Jun 15, 2024

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

@kyleoneill kyleoneill force-pushed the field_scoped_visibility_modifiers branch from 2ce2725 to e2e57e6 Compare June 16, 2024 19:19
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

Okis, now that some time has passed and the FCP seems to be alright, we can now merge it! Could you squash these 3 commits into one?

@kyleoneill kyleoneill force-pushed the field_scoped_visibility_modifiers branch from e2e57e6 to 3405ce3 Compare June 16, 2024 19:56
@kyleoneill
Copy link
Contributor Author

Sounds good, just squashed them. Thanks again for the review process!

@blyxyas
Copy link
Member

blyxyas commented Jun 16, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 16, 2024

📌 Commit 3405ce3 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 16, 2024

⌛ Testing commit 3405ce3 with merge 9f5d60f...

@bors
Copy link
Collaborator

bors commented Jun 16, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 9f5d60f to master...

@bors bors merged commit 9f5d60f into rust-lang:master Jun 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants