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 static_mut lint #12914

Closed
wants to merge 1 commit into from
Closed

Conversation

lolbinarycat
Copy link

Fixes #12896

changelog: new [static_mut] lint added

@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (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 9, 2024
clippy_lints/src/static_mut.rs Outdated Show resolved Hide resolved
item.span,
"declaration of static mut",
None,
"remove the `mut` and use a type with interior mutibability that implements `Sync`, such as `std::sync::Mutex`",
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a pretty long text. Perhaps we might move some of it to the docs?

Copy link
Author

Choose a reason for hiding this comment

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

my concern is that a lot of beginners won't know what "interior mutability" is without an example, and removing the qualification about Sync will make it technically inaccurate, as trying to use something like UnsafeCell will result in a compile error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then perhaps we can move that into a help text, to keep the lint message short.

Copy link
Author

Choose a reason for hiding this comment

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

that is a help message, the actual lint message is just declaration of static mut.

@lolbinarycat lolbinarycat requested a review from llogiq June 9, 2024 20:49
/// ```no_run
/// use std::sync::RwLock;
///
/// static GLOBAL_INT: RwLock<u8> = RwLock::new(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should extend the example to also show at least one of the Atomic* types, as well as possibly Lazy or perhaps even UnsafeCell.

Copy link
Author

Choose a reason for hiding this comment

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

correctly using Atomic* is tricky, and i don't think we should be reccomending them to beginners. UnsafeCell won't work, we need SyncUnsafeCell, which isn't stable yet.

LazyLock and LazyCell are both still unstable.

OnceCell is a decent option, however, it makes it pretty easy to introduce TOC-TOU bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that all of these types have their pros and cons, yet just showing RwLock makes the docs woefully incomplete.

How about at least listing the options with a short description of their niche and shortcomings? Such as:

  • If you just have an integer type and want to avoid blocking, use the appropriate sized std::sync::atomic::Atomic* type
  • If you just need the mutation for setting up the data because the code isn't const and don't actually need to update later on, you can use std::sync::Lazy
  • Or if you are in an application and can call the setup in main before starting any threads, you can use std::cell::Once
  • If you can manage the unsafety, one of the std::cell::*Cell types might work for you, but be aware that the other options avoid the unsafety, often without measurably affecting performance.

Copy link
Author

Choose a reason for hiding this comment

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

unless things have changed since i last checked, the type of a static must always implement Sync, which means most of these will never work. i suppose i could mention the atomic primatives, and maybe LazyLock and UnsafeSyncCell under a "if you're already using nightly" section.

Copy link
Contributor

Choose a reason for hiding this comment

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

std::sync::Once and the std::sync::atomics do implement Sync last I checked. I'm on mobile now, so I won't check all of them, but that certainly gives us some options.

@flip1995
Copy link
Member

Relevant links:

This might conflict with those. Not saying, that we shouldn't merge this lint. But should the RFC get merged, this lint will be deprecated again (or needs an edition check).

@lolbinarycat
Copy link
Author

But should the RFC get merged, this lint will be deprecated again (or needs an edition check).

is there no established procedure for promoting clippy lints into compiler lints? surely this will have come up before?

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

@flip1995
Copy link
Member

is there no established procedure for promoting clippy lints into compiler lints? surely this will have come up before?

If someone thinks a Clippy lint should go into the compiler, they can propose this with a PR to the Rust repo. This PR then moves the lint implementation to rustc and removes it from Clippy (cargo dev deprecate LINT_NAME --uplift or something like that can do this).

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

Comment on lines +36 to +37
impl EarlyLintPass for StaticMut {
fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) {
Copy link
Member

Choose a reason for hiding this comment

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

The issue mentioned some suggestions to replace static mut including using Mutex or Atomics, but the problem is non of those are available in no_std context, so it might be a good idea to check that with is_no_std_crate. But you might need to change the pass to Late

Copy link
Member

Choose a reason for hiding this comment

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

The issue does mention SyncUnsafeCell (or the stable alternative: a newtype around UnsafeCell with an unsafe impl Sync). I think that should work fine even in no_std since they're defined in core. The description should probably mention this though.

item.span,
"declaration of static mut",
None,
"remove the `mut` and use a type with interior mutibability that implements `Sync`, such as `std::sync::Mutex`",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"remove the `mut` and use a type with interior mutibability that implements `Sync`, such as `std::sync::Mutex`",
"remove the `mut` and use a type with interior mutability that implements `Sync`, such as `std::sync::Mutex`",

@lolbinarycat
Copy link
Author

after some reflection, i think it's too early to implement this. the chance of it overlapping with builtin rustc lints is too high, and the usefulness of its output is limited while SyncUnsafeCell is unstable.

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.

Deny static mut declarations entirely
8 participants