Skip to content

Add global_variables lint #14657

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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 @@ -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
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 @@ -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,
Expand Down
87 changes: 75 additions & 12 deletions clippy_lints/src/non_copy_const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
Expand Down Expand Up @@ -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 },
}
Expand All @@ -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",
Expand All @@ -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;
};
Expand All @@ -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");
Expand All @@ -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 {
Expand Down Expand Up @@ -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 });
}
},
_ => {},
}
}

Expand Down
51 changes: 51 additions & 0 deletions tests/ui/global_variables.rs
Original file line number Diff line number Diff line change
@@ -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<u32> = 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<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;

// 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
};
}
}
58 changes: 58 additions & 0 deletions tests/ui/global_variables.stderr
Original file line number Diff line number Diff line change
@@ -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<u32> = 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<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 6 previous errors