Skip to content

Commit 318f6fa

Browse files
EFanZhEFanZh
committed
Add global_variables lint
1 parent 189b4c3 commit 318f6fa

File tree

5 files changed

+232
-36
lines changed

5 files changed

+232
-36
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6167,6 +6167,7 @@ Released 2018-09-13
61676167
[`get_first`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_first
61686168
[`get_last_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_last_with_len
61696169
[`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap
6170+
[`global_variables`]: https://rust-lang.github.io/rust-clippy/master/index.html#global_variables
61706171
[`host_endian_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#host_endian_bytes
61716172
[`identity_conversion`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_conversion
61726173
[`identity_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_op

‎clippy_lints/src/declared_lints.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
563563
crate::non_canonical_impls::NON_CANONICAL_PARTIAL_ORD_IMPL_INFO,
564564
crate::non_copy_const::BORROW_INTERIOR_MUTABLE_CONST_INFO,
565565
crate::non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST_INFO,
566+
crate::non_copy_const::GLOBAL_VARIABLES_INFO,
566567
crate::non_expressive_names::JUST_UNDERSCORES_AND_DIGITS_INFO,
567568
crate::non_expressive_names::MANY_SINGLE_CHAR_NAMES_INFO,
568569
crate::non_expressive_names::SIMILAR_NAMES_INFO,

‎clippy_lints/src/non_copy_const.rs‎

Lines changed: 120 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,51 @@ declare_clippy_lint! {
159159
"referencing `const` with interior mutability"
160160
}
161161

162+
declare_clippy_lint! {
163+
/// ### What it does
164+
///
165+
/// Checks whether a global variable defined.
166+
///
167+
/// ### Why restrict this?
168+
///
169+
/// - Global variables can be modified from any part of the program, making it difficult to
170+
/// track and control their state.
171+
/// - Global variables introduce implicit dependencies that are not visible in function
172+
/// signatures, making the code harder to understand and maintain.
173+
/// - Global variables introduce persistent state, complicating unit tests and making them
174+
/// prone to side effects.
175+
/// - Global variables create tight coupling between different parts of the program, making it
176+
/// harder to modify one part without affecting others.
177+
///
178+
/// ### Example
179+
///
180+
/// ```no_run
181+
/// use std::sync::Mutex;
182+
///
183+
/// struct State {}
184+
///
185+
/// static STATE: Mutex<State> = Mutex::new(State {});
186+
///
187+
/// fn foo() {
188+
/// // Access global variable `STATE`.
189+
/// }
190+
/// ```
191+
///
192+
/// Use instead:
193+
///
194+
/// ```no_run
195+
/// struct State {}
196+
///
197+
/// fn foo(state: &mut State) {
198+
/// // Access `state` argument instead of a global variable.
199+
/// }
200+
/// ```
201+
#[clippy::version = "1.88.0"]
202+
pub GLOBAL_VARIABLES,
203+
nursery,
204+
"declaring global variables"
205+
}
206+
162207
#[derive(Clone, Copy)]
163208
enum IsFreeze {
164209
/// The type and all possible values are `Freeze`
@@ -256,7 +301,7 @@ pub struct NonCopyConst<'tcx> {
256301
freeze_tys: FxHashMap<Ty<'tcx>, IsFreeze>,
257302
}
258303

259-
impl_lint_pass!(NonCopyConst<'_> => [DECLARE_INTERIOR_MUTABLE_CONST, BORROW_INTERIOR_MUTABLE_CONST]);
304+
impl_lint_pass!(NonCopyConst<'_> => [DECLARE_INTERIOR_MUTABLE_CONST, BORROW_INTERIOR_MUTABLE_CONST, GLOBAL_VARIABLES]);
260305

261306
impl<'tcx> NonCopyConst<'tcx> {
262307
pub fn new(tcx: TyCtxt<'tcx>, conf: &'static Conf) -> Self {
@@ -695,43 +740,82 @@ impl<'tcx> NonCopyConst<'tcx> {
695740

696741
impl<'tcx> LateLintPass<'tcx> for NonCopyConst<'tcx> {
697742
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
698-
if let ItemKind::Const(ident, .., body_id) = item.kind
699-
&& !ident.is_special()
700-
&& let ty = cx.tcx.type_of(item.owner_id).instantiate_identity()
701-
&& match self.is_ty_freeze(cx.tcx, cx.typing_env(), ty) {
702-
IsFreeze::No => true,
703-
IsFreeze::Yes => false,
704-
IsFreeze::Maybe => match cx.tcx.const_eval_poly(item.owner_id.to_def_id()) {
705-
Ok(val) if let Ok(is_freeze) = self.is_value_freeze(cx.tcx, cx.typing_env(), ty, val) => !is_freeze,
706-
_ => !self.is_init_expr_freeze(
707-
cx.tcx,
708-
cx.typing_env(),
709-
cx.tcx.typeck(item.owner_id),
710-
GenericArgs::identity_for_item(cx.tcx, item.owner_id),
711-
cx.tcx.hir_body(body_id).value,
712-
),
713-
},
714-
}
715-
&& !item.span.in_external_macro(cx.sess().source_map())
743+
let ty = || cx.tcx.type_of(item.owner_id).instantiate_identity();
744+
745+
let is_init_expr_freeze = |this: &mut Self, body_id| {
746+
this.is_init_expr_freeze(
747+
cx.tcx,
748+
cx.typing_env(),
749+
cx.tcx.typeck(item.owner_id),
750+
GenericArgs::identity_for_item(cx.tcx, item.owner_id),
751+
cx.tcx.hir_body(body_id).value,
752+
)
753+
};
754+
755+
let is_thread_local_or_external_macro = || {
756+
item.span.in_external_macro(cx.sess().source_map())
716757
// Only needed when compiling `std`
717-
&& !is_thread_local(cx, item)
718-
{
719-
span_lint_and_then(
720-
cx,
721-
DECLARE_INTERIOR_MUTABLE_CONST,
722-
ident.span,
723-
"named constant with interior mutability",
724-
|diag| {
725-
let Some(sync_trait) = cx.tcx.lang_items().sync_trait() else {
726-
return;
727-
};
728-
if implements_trait(cx, ty, sync_trait, &[]) {
729-
diag.help("did you mean to make this a `static` item");
730-
} else {
731-
diag.help("did you mean to make this a `thread_local!` item");
758+
|| is_thread_local(cx, item)
759+
};
760+
761+
match item.kind {
762+
ItemKind::Const(ident, .., body_id) => {
763+
if !clippy_utils::is_lint_allowed(cx, DECLARE_INTERIOR_MUTABLE_CONST, item.hir_id())
764+
&& !ident.is_special()
765+
&& let ty = ty()
766+
&& match self.is_ty_freeze(cx.tcx, cx.typing_env(), ty) {
767+
IsFreeze::No => true,
768+
IsFreeze::Yes => false,
769+
IsFreeze::Maybe => match cx.tcx.const_eval_poly(item.owner_id.to_def_id()) {
770+
Ok(val) if let Ok(is_freeze) = self.is_value_freeze(cx.tcx, cx.typing_env(), ty, val) => {
771+
!is_freeze
772+
},
773+
_ => !is_init_expr_freeze(self, body_id),
774+
},
732775
}
733-
},
734-
);
776+
&& !is_thread_local_or_external_macro()
777+
{
778+
span_lint_and_then(
779+
cx,
780+
DECLARE_INTERIOR_MUTABLE_CONST,
781+
ident.span,
782+
"named constant with interior mutability",
783+
|diag| {
784+
let Some(sync_trait) = cx.tcx.lang_items().sync_trait() else {
785+
return;
786+
};
787+
788+
diag.help(if implements_trait(cx, ty, sync_trait, &[]) {
789+
"did you mean to make this a `static` item"
790+
} else {
791+
"did you mean to make this a `thread_local!` item"
792+
});
793+
},
794+
);
795+
}
796+
},
797+
ItemKind::Static(_, ident, _, body_id) => {
798+
if !clippy_utils::is_lint_allowed(cx, GLOBAL_VARIABLES, item.hir_id())
799+
&& !ident.is_special()
800+
&& match self.is_ty_freeze(cx.tcx, cx.typing_env(), ty()) {
801+
IsFreeze::No => true,
802+
IsFreeze::Yes => false,
803+
IsFreeze::Maybe => !is_init_expr_freeze(self, body_id),
804+
}
805+
&& !is_thread_local_or_external_macro()
806+
{
807+
span_lint_and_then(
808+
cx,
809+
GLOBAL_VARIABLES,
810+
ident.span,
811+
"global variable should not be used",
812+
|diag| {
813+
diag.help("consider passing this as a function argument or using a `thread_local`");
814+
},
815+
);
816+
}
817+
},
818+
_ => {},
735819
}
736820
}
737821

‎tests/ui/global_variables.rs‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
#![warn(clippy::global_variables)]
2+
3+
use std::sync::Mutex;
4+
use std::sync::atomic::AtomicU32;
5+
6+
macro_rules! define_global_variables_with_macro {
7+
() => {
8+
static GLOBAL_VARIABLE_0: AtomicU32 = AtomicU32::new(2);
9+
//~^ global_variables
10+
11+
static GLOBAL_VARIABLE_1: Mutex<u32> = Mutex::new(3);
12+
//~^ global_variables
13+
};
14+
}
15+
16+
#[allow(clippy::missing_const_for_thread_local)]
17+
fn main() {
18+
define_global_variables_with_macro!();
19+
20+
static GLOBAL_VARIABLE_2: AtomicU32 = AtomicU32::new(2);
21+
//~^ global_variables
22+
23+
static GLOBAL_VARIABLE_3: Mutex<u32> = Mutex::new(3);
24+
//~^ global_variables
25+
26+
static GLOBAL_VARIABLE_4: Option<AtomicU32> = Some(AtomicU32::new(0));
27+
//~^ global_variables
28+
29+
static NOT_GLOBAL_VARIABLE_0: u32 = 1;
30+
31+
// ZST fields (like `PhantomData`) should be ignored.
32+
static NOT_GLOBAL_VARIABLE_1: Vec<AtomicU32> = Vec::new();
33+
34+
// Having a initializer with a freeze value should be ignored.
35+
static NOT_GLOBAL_VARIABLE_2: Option<AtomicU32> = None;
36+
37+
// Thread-local variables are ignored.
38+
thread_local! {
39+
static THREAD_LOCAL_VARIABLE_0: u32 = 0;
40+
static THREAD_LOCAL_VARIABLE_1: AtomicU32 = AtomicU32::new(0);
41+
static THREAD_LOCAL_VARIABLE_2: u32 = const { 0 };
42+
static THREAD_LOCAL_VARIABLE_3: AtomicU32 = const { AtomicU32::new(0) };
43+
44+
static THREAD_LOCAL_VARIABLE_4: () = {
45+
// Global variables inside a thread-local initializer are also considered.
46+
static GLOBAL_VARIABLE_IN_THREAD_LOCAL: AtomicU32 = AtomicU32::new(0);
47+
//~^ global_variables
48+
};
49+
}
50+
}

‎tests/ui/global_variables.stderr‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
error: global variable should not be used
2+
--> tests/ui/global_variables.rs:8:16
3+
|
4+
LL | static GLOBAL_VARIABLE_0: AtomicU32 = AtomicU32::new(2);
5+
| ^^^^^^^^^^^^^^^^^
6+
...
7+
LL | define_global_variables_with_macro!();
8+
| ------------------------------------- in this macro invocation
9+
|
10+
= help: consider passing this as a function argument or using a `thread_local`
11+
= note: `-D clippy::global-variables` implied by `-D warnings`
12+
= help: to override `-D warnings` add `#[allow(clippy::global_variables)]`
13+
= note: this error originates in the macro `define_global_variables_with_macro` (in Nightly builds, run with -Z macro-backtrace for more info)
14+
15+
error: global variable should not be used
16+
--> tests/ui/global_variables.rs:11:16
17+
|
18+
LL | static GLOBAL_VARIABLE_1: Mutex<u32> = Mutex::new(3);
19+
| ^^^^^^^^^^^^^^^^^
20+
...
21+
LL | define_global_variables_with_macro!();
22+
| ------------------------------------- in this macro invocation
23+
|
24+
= help: consider passing this as a function argument or using a `thread_local`
25+
= note: this error originates in the macro `define_global_variables_with_macro` (in Nightly builds, run with -Z macro-backtrace for more info)
26+
27+
error: global variable should not be used
28+
--> tests/ui/global_variables.rs:20:12
29+
|
30+
LL | static GLOBAL_VARIABLE_2: AtomicU32 = AtomicU32::new(2);
31+
| ^^^^^^^^^^^^^^^^^
32+
|
33+
= help: consider passing this as a function argument or using a `thread_local`
34+
35+
error: global variable should not be used
36+
--> tests/ui/global_variables.rs:23:12
37+
|
38+
LL | static GLOBAL_VARIABLE_3: Mutex<u32> = Mutex::new(3);
39+
| ^^^^^^^^^^^^^^^^^
40+
|
41+
= help: consider passing this as a function argument or using a `thread_local`
42+
43+
error: global variable should not be used
44+
--> tests/ui/global_variables.rs:26:12
45+
|
46+
LL | static GLOBAL_VARIABLE_4: Option<AtomicU32> = Some(AtomicU32::new(0));
47+
| ^^^^^^^^^^^^^^^^^
48+
|
49+
= help: consider passing this as a function argument or using a `thread_local`
50+
51+
error: global variable should not be used
52+
--> tests/ui/global_variables.rs:46:20
53+
|
54+
LL | static GLOBAL_VARIABLE_IN_THREAD_LOCAL: AtomicU32 = AtomicU32::new(0);
55+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
56+
|
57+
= help: consider passing this as a function argument or using a `thread_local`
58+
59+
error: aborting due to 6 previous errors
60+

0 commit comments

Comments
 (0)