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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5820,6 +5820,7 @@ Released 2018-09-13
[`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next
[`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
[`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive
[`static_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#static_mut
[`std_instead_of_alloc`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_alloc
[`std_instead_of_core`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_core
[`str_split_at_newline`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_split_at_newline
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT_INFO,
crate::size_of_ref::SIZE_OF_REF_INFO,
crate::slow_vector_initialization::SLOW_VECTOR_INITIALIZATION_INFO,
crate::static_mut::STATIC_MUT_INFO,
crate::std_instead_of_core::ALLOC_INSTEAD_OF_CORE_INFO,
crate::std_instead_of_core::STD_INSTEAD_OF_ALLOC_INFO,
crate::std_instead_of_core::STD_INSTEAD_OF_CORE_INFO,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ mod single_range_in_vec_init;
mod size_of_in_element_count;
mod size_of_ref;
mod slow_vector_initialization;
mod static_mut;
mod std_instead_of_core;
mod string_patterns;
mod strings;
Expand Down Expand Up @@ -1169,6 +1170,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
})
});
store.register_late_pass(|_| Box::new(string_patterns::StringPatterns));
store.register_early_pass(|| Box::new(static_mut::StaticMut));
// add lints here, do not remove this comment, it's used in `new_lint`
}

Expand Down
60 changes: 60 additions & 0 deletions clippy_lints/src/static_mut.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
use clippy_utils::diagnostics::span_lint_and_help;
use rustc_ast::ast::{Item, ItemKind, Mutability, StaticItem};
use rustc_lint::{EarlyContext, EarlyLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_session::declare_lint_pass;

declare_clippy_lint! {
/// ### What it does
/// Produces warnings when a `static mut` is declared.
///
/// ### Why is this bad?
/// `static mut` can [easily produce undefined behavior][1] and
/// [may be removed in the future][2].
///
/// ### Example
/// ```no_run
/// static mut GLOBAL_INT: u8 = 0;
/// ```
/// Use instead:
/// ```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.

/// ```
///
/// [1]: https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-reference.html
/// [2]: https://github.com/rust-lang/rfcs/pull/3560
#[clippy::version = "1.80.0"]
pub STATIC_MUT,
nursery,
"detect mutable static definitions"
}

declare_lint_pass!(StaticMut => [STATIC_MUT]);

impl EarlyLintPass for StaticMut {
fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) {
Comment on lines +36 to +37
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.

if in_external_macro(cx.sess(), item.span) {
return;
};
let ItemKind::Static(ref static_item_box) = item.kind else {
return;
};
let StaticItem {
mutability: Mutability::Mut,
..
} = static_item_box.as_ref()
else {
return;
};
span_lint_and_help(
cx,
STATIC_MUT,
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.

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`",

);
}
}
3 changes: 3 additions & 0 deletions tests/ui/static_mut.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#![warn(clippy::static_mut)]

static mut NUMBER: u8 = 3;
12 changes: 12 additions & 0 deletions tests/ui/static_mut.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error: declaration of static mut
--> tests/ui/static_mut.rs:3:1
|
LL | static mut NUMBER: u8 = 3;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: remove the `mut` and use a type with interior mutibability that implements `Sync`, such as `std::sync::Mutex`
= note: `-D clippy::static-mut` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::static_mut)]`

error: aborting due to 1 previous error

Loading