diff --git a/CHANGELOG.md b/CHANGELOG.md index 30781d3d33fb..bf9c0b4e97b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6167,6 +6167,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 7645ac2dd3f7..55fc8127c767 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -563,6 +563,7 @@ pub static LINTS: &[&::declare_clippy_lint::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 2fffc4244a73..be9f8f3ef664 100644 --- a/clippy_lints/src/non_copy_const.rs +++ b/clippy_lints/src/non_copy_const.rs @@ -19,7 +19,7 @@ use clippy_config::Conf; use clippy_utils::consts::{ConstEvalCtxt, Constant}; -use clippy_utils::diagnostics::{span_lint, span_lint_and_then}; +use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_and_then}; use clippy_utils::is_in_const_context; use clippy_utils::macros::macro_backtrace; use clippy_utils::paths::{PathNS, lookup_path_str}; @@ -159,6 +159,51 @@ declare_clippy_lint! { "referencing `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" +} + #[derive(Clone, Copy)] enum IsFreeze { /// The type and all possible values are `Freeze` @@ -256,7 +301,7 @@ pub struct NonCopyConst<'tcx> { freeze_tys: FxHashMap, IsFreeze>, } -impl_lint_pass!(NonCopyConst<'_> => [DECLARE_INTERIOR_MUTABLE_CONST, BORROW_INTERIOR_MUTABLE_CONST]); +impl_lint_pass!(NonCopyConst<'_> => [DECLARE_INTERIOR_MUTABLE_CONST, BORROW_INTERIOR_MUTABLE_CONST, GLOBAL_VARIABLES]); impl<'tcx> NonCopyConst<'tcx> { pub fn new(tcx: TyCtxt<'tcx>, conf: &'static Conf) -> Self { @@ -695,43 +740,83 @@ impl<'tcx> NonCopyConst<'tcx> { impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { - if let ItemKind::Const(ident, .., body_id) = item.kind - && !ident.is_special() - && let ty = cx.tcx.type_of(item.owner_id).instantiate_identity() - && match self.is_ty_freeze(cx.tcx, cx.typing_env(), ty) { - IsFreeze::No => true, - IsFreeze::Yes => false, - IsFreeze::Maybe => match cx.tcx.const_eval_poly(item.owner_id.to_def_id()) { - Ok(val) if let Ok(is_freeze) = self.is_value_freeze(cx.tcx, cx.typing_env(), ty, val) => !is_freeze, - _ => !self.is_init_expr_freeze( - cx.tcx, - cx.typing_env(), - cx.tcx.typeck(item.owner_id), - GenericArgs::identity_for_item(cx.tcx, item.owner_id), - cx.tcx.hir_body(body_id).value, - ), - }, - } - && !item.span.in_external_macro(cx.sess().source_map()) + // This can only be called after verifying that `item` is of kinds so that its type can be + // queried, like `const` or `static` items. + let ty = || cx.tcx.type_of(item.owner_id).instantiate_identity(); + + let is_init_expr_freeze = |this: &mut Self, body_id| { + this.is_init_expr_freeze( + cx.tcx, + cx.typing_env(), + cx.tcx.typeck(item.owner_id), + GenericArgs::identity_for_item(cx.tcx, item.owner_id), + cx.tcx.hir_body(body_id).value, + ) + }; + + let is_thread_local_or_external_macro = || { + item.span.in_external_macro(cx.sess().source_map()) // Only needed when compiling `std` - && !is_thread_local(cx, item) - { - span_lint_and_then( - cx, - DECLARE_INTERIOR_MUTABLE_CONST, - ident.span, - "named constant with interior mutability", - |diag| { - let Some(sync_trait) = cx.tcx.lang_items().sync_trait() else { - return; - }; - if implements_trait(cx, ty, sync_trait, &[]) { - diag.help("did you mean to make this a `static` item"); - } else { - diag.help("did you mean to make this a `thread_local!` item"); + || is_thread_local(cx, item) + }; + + match item.kind { + ItemKind::Const(ident, .., body_id) => { + if !clippy_utils::is_lint_allowed(cx, DECLARE_INTERIOR_MUTABLE_CONST, item.hir_id()) + && !ident.is_special() + && let ty = ty() + && match self.is_ty_freeze(cx.tcx, cx.typing_env(), ty) { + IsFreeze::No => true, + IsFreeze::Yes => false, + IsFreeze::Maybe => match cx.tcx.const_eval_poly(item.owner_id.to_def_id()) { + Ok(val) if let Ok(is_freeze) = self.is_value_freeze(cx.tcx, cx.typing_env(), ty, val) => { + !is_freeze + }, + _ => !is_init_expr_freeze(self, body_id), + }, } - }, - ); + && !is_thread_local_or_external_macro() + { + span_lint_and_then( + cx, + DECLARE_INTERIOR_MUTABLE_CONST, + ident.span, + "named constant with interior mutability", + |diag| { + let Some(sync_trait) = cx.tcx.lang_items().sync_trait() else { + return; + }; + + diag.help(if implements_trait(cx, ty, sync_trait, &[]) { + "did you mean to make this a `static` item" + } else { + "did you mean to make this a `thread_local!` item" + }); + }, + ); + } + }, + ItemKind::Static(_, ident, _, body_id) => { + if !clippy_utils::is_lint_allowed(cx, GLOBAL_VARIABLES, item.hir_id()) + && !ident.is_special() + && match self.is_ty_freeze(cx.tcx, cx.typing_env(), ty()) { + IsFreeze::No => true, + IsFreeze::Yes => false, + IsFreeze::Maybe => !is_init_expr_freeze(self, body_id), + } + && !is_thread_local_or_external_macro() + { + span_lint_and_help( + cx, + GLOBAL_VARIABLES, + ident.span, + "global variable should not be used", + None, + "consider passing this as a function argument or using a `thread_local`", + ); + } + }, + _ => {}, } } diff --git a/tests/ui/global_variables.rs b/tests/ui/global_variables.rs new file mode 100644 index 000000000000..aa82a38ab5bc --- /dev/null +++ b/tests/ui/global_variables.rs @@ -0,0 +1,50 @@ +#![warn(clippy::global_variables)] + +use std::sync::Mutex; +use std::sync::atomic::AtomicU32; + +macro_rules! define_global_variables_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_variables_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; + + // ZST fields (like `PhantomData`) should be ignored. + static NOT_GLOBAL_VARIABLE_1: Vec = Vec::new(); + + // Having a initializer with a freeze value should be ignored. + 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..11aa83806f48 --- /dev/null +++ b/tests/ui/global_variables.stderr @@ -0,0 +1,60 @@ +error: global variable should not be used + --> tests/ui/global_variables.rs:8:16 + | +LL | static GLOBAL_VARIABLE_0: AtomicU32 = AtomicU32::new(2); + | ^^^^^^^^^^^^^^^^^ +... +LL | define_global_variables_with_macro!(); + | ------------------------------------- in this macro invocation + | + = help: consider passing this as a function argument or using a `thread_local` + = 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_variables_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:16 + | +LL | static GLOBAL_VARIABLE_1: Mutex = Mutex::new(3); + | ^^^^^^^^^^^^^^^^^ +... +LL | define_global_variables_with_macro!(); + | ------------------------------------- in this macro invocation + | + = help: consider passing this as a function argument or using a `thread_local` + = note: this error originates in the macro `define_global_variables_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:12 + | +LL | static GLOBAL_VARIABLE_2: AtomicU32 = AtomicU32::new(2); + | ^^^^^^^^^^^^^^^^^ + | + = help: consider passing this as a function argument or using a `thread_local` + +error: global variable should not be used + --> tests/ui/global_variables.rs:23:12 + | +LL | static GLOBAL_VARIABLE_3: Mutex = Mutex::new(3); + | ^^^^^^^^^^^^^^^^^ + | + = help: consider passing this as a function argument or using a `thread_local` + +error: global variable should not be used + --> tests/ui/global_variables.rs:26:12 + | +LL | static GLOBAL_VARIABLE_4: Option = Some(AtomicU32::new(0)); + | ^^^^^^^^^^^^^^^^^ + | + = help: consider passing this as a function argument or using a `thread_local` + +error: global variable should not be used + --> tests/ui/global_variables.rs:46:20 + | +LL | static GLOBAL_VARIABLE_IN_THREAD_LOCAL: AtomicU32 = AtomicU32::new(0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider passing this as a function argument or using a `thread_local` + +error: aborting due to 6 previous errors +