Move global variables check to non_copy_const.rs

Author: EFanZh

clippy_lints/src/declared_lints.rs

@@ -192,7 +192,6 @@ pub static LINTS: &[&crate::LintInfo] = &[
  crate::functions::TOO_MANY_ARGUMENTS_INFO,
  crate::functions::TOO_MANY_LINES_INFO,
  crate::future_not_send::FUTURE_NOT_SEND_INFO,
- crate::global_variables::GLOBAL_VARIABLES_INFO,
  crate::if_let_mutex::IF_LET_MUTEX_INFO,
  crate::if_not_else::IF_NOT_ELSE_INFO,
  crate::if_then_some_else_none::IF_THEN_SOME_ELSE_NONE_INFO,
@@ -557,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,

clippy_lints/src/global_variables.rs

@@ -155,7 +155,6 @@ mod from_raw_with_void_ptr;
 mod from_str_radix_10;
 mod functions;
 mod future_not_send;
-mod global_variables;
 mod if_let_mutex;
 mod if_not_else;
 mod if_then_some_else_none;
@@ -944,6 +943,5 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
  store.register_late_pass(|_| Box::new(manual_option_as_slice::ManualOptionAsSlice::new(conf)));
  store.register_late_pass(|_| Box::new(single_option_map::SingleOptionMap));
  store.register_late_pass(move |_| Box::new(redundant_test_prefix::RedundantTestPrefix));
- store.register_late_pass(|_| Box::new(global_variables::GlobalVariables));
  // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/lib.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<State> = 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 });
+ }
+ },
+ _ => {},
  }
  }
 

clippy_lints/src/non_copy_const.rs

@@ -13,6 +13,7 @@ macro_rules! define_global_variable_with_macro {
  };
 }
 
+#[allow(clippy::missing_const_for_thread_local)]
 fn main() {
  define_global_variable_with_macro!();
 
@@ -22,7 +23,29 @@ fn main() {
  static GLOBAL_VARIABLE_3: Mutex<u32> = Mutex::new(3);
  //~^ global_variables
 
+ static GLOBAL_VARIABLE_4: Option<AtomicU32> = Some(AtomicU32::new(0));
+ //~^ global_variables
+
  static NOT_GLOBAL_VARIABLE_0: u32 = 1;
- static NOT_GLOBAL_VARIABLE_1: Vec<AtomicU32> = Vec::new();
- static NOT_GLOBAL_VARIABLE_2: Vec<Mutex<u32>> = Vec::new();
+
+ // Does not work yet: ZST fields should be ignored.
+ // static NOT_GLOBAL_VARIABLE_1: Vec<AtomicU32> = 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<AtomicU32> = 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
+ };
+ }
 }

tests/ui/global_variables.rs

@@ -1,4 +1,4 @@
-error: found global variable
+error: global variable should not be used
  --> tests/ui/global_variables.rs:8:9
  |
 LL | static GLOBAL_VARIABLE_0: AtomicU32 = AtomicU32::new(2);
@@ -11,7 +11,7 @@ LL | define_global_variable_with_macro!();
  = 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: found global variable
+error: global variable should not be used
  --> tests/ui/global_variables.rs:11:9
  |
 LL | static GLOBAL_VARIABLE_1: Mutex<u32> = Mutex::new(3);
@@ -22,17 +22,37 @@ LL | define_global_variable_with_macro!();
  |
  = note: this error originates in the macro `define_global_variable_with_macro` (in Nightly builds, run with -Z macro-backtrace for more info)
 
-error: found global variable
- --> tests/ui/global_variables.rs:19:5
+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: found global variable
- --> tests/ui/global_variables.rs:22:5
+error: global variable should not be used
+ --> tests/ui/global_variables.rs:23:5
  |
 LL | static GLOBAL_VARIABLE_3: Mutex<u32> = 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<AtomicU32> = 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 4 previous errors
+error: aborting due to 6 previous errors