diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b62c9a59aa5..24299effaff4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5729,6 +5729,7 @@ Released 2018-09-13 [`get_first`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_first [`get_last_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_last_with_len [`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap +[`global_variables`]: https://rust-lang.github.io/rust-clippy/master/index.html#global_variables [`host_endian_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#host_endian_bytes [`identity_conversion`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_conversion [`identity_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_op diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 2cccd6ba2702..3de8e36dfe55 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -556,6 +556,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::non_canonical_impls::NON_CANONICAL_PARTIAL_ORD_IMPL_INFO, crate::non_copy_const::BORROW_INTERIOR_MUTABLE_CONST_INFO, crate::non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST_INFO, + crate::non_copy_const::GLOBAL_VARIABLES_INFO, crate::non_expressive_names::JUST_UNDERSCORES_AND_DIGITS_INFO, crate::non_expressive_names::MANY_SINGLE_CHAR_NAMES_INFO, crate::non_expressive_names::SIMILAR_NAMES_INFO, diff --git a/clippy_lints/src/non_copy_const.rs b/clippy_lints/src/non_copy_const.rs index 9b53608ae7f3..6d9675f98ee4 100644 --- a/clippy_lints/src/non_copy_const.rs +++ b/clippy_lints/src/non_copy_const.rs @@ -74,6 +74,51 @@ declare_clippy_lint! { "declaring `const` with interior mutability" } +declare_clippy_lint! { + /// ### What it does + /// + /// Checks whether a global variable defined. + /// + /// ### Why restrict this? + /// + /// - Global variables can be modified from any part of the program, making it difficult to + /// track and control their state. + /// - Global variables introduce implicit dependencies that are not visible in function + /// signatures, making the code harder to understand and maintain. + /// - Global variables introduce persistent state, complicating unit tests and making them + /// prone to side effects. + /// - Global variables create tight coupling between different parts of the program, making it + /// harder to modify one part without affecting others. + /// + /// ### Example + /// + /// ```no_run + /// use std::sync::Mutex; + /// + /// struct State {} + /// + /// static STATE: Mutex = Mutex::new(State {}); + /// + /// fn foo() { + /// // Access global variable `STATE`. + /// } + /// ``` + /// + /// Use instead: + /// + /// ```no_run + /// struct State {} + /// + /// fn foo(state: &mut State) { + /// // Access `state` argument instead of a global variable. + /// } + /// ``` + #[clippy::version = "1.88.0"] + pub GLOBAL_VARIABLES, + nursery, + "declaring global variables" +} + // FIXME: this is a correctness problem but there's no suitable // warn-by-default category. declare_clippy_lint! { @@ -115,7 +160,8 @@ declare_clippy_lint! { #[derive(Copy, Clone)] enum Source<'tcx> { - Item { item: Span, ty: Ty<'tcx> }, + ConstItem { item: Span, ty: Ty<'tcx> }, + StaticItem { item: Span }, Assoc { item: Span }, Expr { expr: Span }, } @@ -124,11 +170,12 @@ impl Source<'_> { #[must_use] fn lint(&self) -> (&'static Lint, &'static str, Span) { match self { - Self::Item { item, .. } | Self::Assoc { item, .. } => ( + Self::ConstItem { item, .. } | Self::Assoc { item, .. } => ( DECLARE_INTERIOR_MUTABLE_CONST, "a `const` item should not be interior mutable", *item, ), + Self::StaticItem { item } => (GLOBAL_VARIABLES, "global variable should not be used", *item), Self::Expr { expr } => ( BORROW_INTERIOR_MUTABLE_CONST, "a `const` item with interior mutability should not be borrowed", @@ -145,7 +192,7 @@ fn lint<'tcx>(cx: &LateContext<'tcx>, source: Source<'tcx>) { return; // Don't give suggestions into macros. } match source { - Source::Item { ty, .. } => { + Source::ConstItem { ty, .. } => { let Some(sync_trait) = cx.tcx.lang_items().sync_trait() else { return; }; @@ -157,6 +204,9 @@ fn lint<'tcx>(cx: &LateContext<'tcx>, source: Source<'tcx>) { ); } }, + Source::StaticItem { .. } => { + diag.help("consider passing this as function arguments or using a `thread_local`"); + }, Source::Assoc { .. } => (), Source::Expr { .. } => { diag.help("assign this const to a local or static variable, and use the variable here"); @@ -169,7 +219,7 @@ pub struct NonCopyConst<'tcx> { interior_mut: InteriorMut<'tcx>, } -impl_lint_pass!(NonCopyConst<'_> => [DECLARE_INTERIOR_MUTABLE_CONST, BORROW_INTERIOR_MUTABLE_CONST]); +impl_lint_pass!(NonCopyConst<'_> => [DECLARE_INTERIOR_MUTABLE_CONST, GLOBAL_VARIABLES, BORROW_INTERIOR_MUTABLE_CONST]); impl<'tcx> NonCopyConst<'tcx> { pub fn new(tcx: TyCtxt<'tcx>, conf: &'static Conf) -> Self { @@ -310,14 +360,27 @@ impl<'tcx> NonCopyConst<'tcx> { impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> { fn check_item(&mut self, cx: &LateContext<'tcx>, it: &'tcx Item<'_>) { - if let ItemKind::Const(.., body_id) = it.kind { - let ty = cx.tcx.type_of(it.owner_id).instantiate_identity(); - if !ignored_macro(cx, it) - && self.interior_mut.is_interior_mut_ty(cx, ty) - && Self::is_value_unfrozen_poly(cx, body_id, ty) - { - lint(cx, Source::Item { item: it.span, ty }); - } + let tcx = cx.tcx; + + match it.kind { + ItemKind::Const(.., body_id) => { + let ty = tcx.type_of(it.owner_id).instantiate_identity(); + if !ignored_macro(cx, it) + && self.interior_mut.is_interior_mut_ty(cx, ty) + && Self::is_value_unfrozen_poly(cx, body_id, ty) + { + lint(cx, Source::ConstItem { item: it.span, ty }); + } + }, + ItemKind::Static(..) => { + let ty = tcx.type_of(it.owner_id).instantiate_identity(); + + if !tcx.is_thread_local_static(it.owner_id.to_def_id()) && self.interior_mut.is_interior_mut_ty(cx, ty) + { + lint(cx, Source::StaticItem { item: it.span }); + } + }, + _ => {}, } } diff --git a/tests/ui/global_variables.rs b/tests/ui/global_variables.rs new file mode 100644 index 000000000000..a6988a359c26 --- /dev/null +++ b/tests/ui/global_variables.rs @@ -0,0 +1,51 @@ +#![warn(clippy::global_variables)] + +use std::sync::Mutex; +use std::sync::atomic::AtomicU32; + +macro_rules! define_global_variable_with_macro { + () => { + static GLOBAL_VARIABLE_0: AtomicU32 = AtomicU32::new(2); + //~^ global_variables + + static GLOBAL_VARIABLE_1: Mutex = Mutex::new(3); + //~^ global_variables + }; +} + +#[allow(clippy::missing_const_for_thread_local)] +fn main() { + define_global_variable_with_macro!(); + + static GLOBAL_VARIABLE_2: AtomicU32 = AtomicU32::new(2); + //~^ global_variables + + static GLOBAL_VARIABLE_3: Mutex = Mutex::new(3); + //~^ global_variables + + static GLOBAL_VARIABLE_4: Option = Some(AtomicU32::new(0)); + //~^ global_variables + + static NOT_GLOBAL_VARIABLE_0: u32 = 1; + + // Does not work yet: ZST fields should be ignored. + // static NOT_GLOBAL_VARIABLE_1: Vec = Vec::new(); + + // Does not work yet: Initializer with variant value that does not contain interior fields + // should not be considered global variable. + // static NOT_GLOBAL_VARIABLE_2: Option = None; + + // Thread-local variables are ignored. + thread_local! { + static THREAD_LOCAL_VARIABLE_0: u32 = 0; + static THREAD_LOCAL_VARIABLE_1: AtomicU32 = AtomicU32::new(0); + static THREAD_LOCAL_VARIABLE_2: u32 = const { 0 }; + static THREAD_LOCAL_VARIABLE_3: AtomicU32 = const { AtomicU32::new(0) }; + + static THREAD_LOCAL_VARIABLE_4: () = { + // Global variables inside a thread-local initializer are also considered. + static GLOBAL_VARIABLE_IN_THREAD_LOCAL: AtomicU32 = AtomicU32::new(0); + //~^ global_variables + }; + } +} diff --git a/tests/ui/global_variables.stderr b/tests/ui/global_variables.stderr new file mode 100644 index 000000000000..68b2e06c83b5 --- /dev/null +++ b/tests/ui/global_variables.stderr @@ -0,0 +1,58 @@ +error: global variable should not be used + --> tests/ui/global_variables.rs:8:9 + | +LL | static GLOBAL_VARIABLE_0: AtomicU32 = AtomicU32::new(2); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | define_global_variable_with_macro!(); + | ------------------------------------ in this macro invocation + | + = note: `-D clippy::global-variables` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::global_variables)]` + = note: this error originates in the macro `define_global_variable_with_macro` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: global variable should not be used + --> tests/ui/global_variables.rs:11:9 + | +LL | static GLOBAL_VARIABLE_1: Mutex = Mutex::new(3); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | define_global_variable_with_macro!(); + | ------------------------------------ in this macro invocation + | + = note: this error originates in the macro `define_global_variable_with_macro` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: global variable should not be used + --> tests/ui/global_variables.rs:20:5 + | +LL | static GLOBAL_VARIABLE_2: AtomicU32 = AtomicU32::new(2); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider passing this as function arguments or using a `thread_local` + +error: global variable should not be used + --> tests/ui/global_variables.rs:23:5 + | +LL | static GLOBAL_VARIABLE_3: Mutex = Mutex::new(3); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider passing this as function arguments or using a `thread_local` + +error: global variable should not be used + --> tests/ui/global_variables.rs:26:5 + | +LL | static GLOBAL_VARIABLE_4: Option = Some(AtomicU32::new(0)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider passing this as function arguments or using a `thread_local` + +error: global variable should not be used + --> tests/ui/global_variables.rs:47:13 + | +LL | static GLOBAL_VARIABLE_IN_THREAD_LOCAL: AtomicU32 = AtomicU32::new(0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider passing this as function arguments or using a `thread_local` + +error: aborting due to 6 previous errors +