From 881cee0c9ae89d97167917baeba6a6eb6c131230 Mon Sep 17 00:00:00 2001 From: Jack Amadeo Date: Thu, 30 Oct 2025 12:00:46 -0400 Subject: [PATCH 1/4] cargo dev new_lint --name=set_env_in_tests --category=restriction --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 ++ clippy_lints/src/set_env_in_tests.rs | 25 +++++++++++++++++++++++++ tests/ui/set_env_in_tests.rs | 5 +++++ 5 files changed, 34 insertions(+) create mode 100644 clippy_lints/src/set_env_in_tests.rs create mode 100644 tests/ui/set_env_in_tests.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index c04735a495a7..0ea997daaba9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6766,6 +6766,7 @@ Released 2018-09-13 [`separated_literal_suffix`]: https://rust-lang.github.io/rust-clippy/master/index.html#separated_literal_suffix [`serde_api_misuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#serde_api_misuse [`set_contains_or_insert`]: https://rust-lang.github.io/rust-clippy/master/index.html#set_contains_or_insert +[`set_env_in_tests`]: https://rust-lang.github.io/rust-clippy/master/index.html#set_env_in_tests [`shadow_reuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_reuse [`shadow_same`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_same [`shadow_unrelated`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_unrelated diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index a754eea31165..76e157e49a35 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -669,6 +669,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::semicolon_if_nothing_returned::SEMICOLON_IF_NOTHING_RETURNED_INFO, crate::serde_api::SERDE_API_MISUSE_INFO, crate::set_contains_or_insert::SET_CONTAINS_OR_INSERT_INFO, + crate::set_env_in_tests::SET_ENV_IN_TESTS_INFO, crate::shadow::SHADOW_REUSE_INFO, crate::shadow::SHADOW_SAME_INFO, crate::shadow::SHADOW_UNRELATED_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8e35883ae9d4..b58adce91280 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -324,6 +324,7 @@ mod semicolon_block; mod semicolon_if_nothing_returned; mod serde_api; mod set_contains_or_insert; +mod set_env_in_tests; mod shadow; mod significant_drop_tightening; mod single_call_fn; @@ -821,5 +822,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg)); store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites)); store.register_late_pass(|_| Box::new(replace_box::ReplaceBox)); + store.register_late_pass(|_| Box::new(set_env_in_tests::SetEnvInTests)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/set_env_in_tests.rs b/clippy_lints/src/set_env_in_tests.rs new file mode 100644 index 000000000000..5be389985aea --- /dev/null +++ b/clippy_lints/src/set_env_in_tests.rs @@ -0,0 +1,25 @@ +use rustc_hir::*; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// + /// ### Why restrict this? + /// + /// ### Example + /// ```no_run + /// // example code where clippy issues a warning + /// ``` + /// Use instead: + /// ```no_run + /// // example code which does not raise clippy warning + /// ``` + #[clippy::version = "1.92.0"] + pub SET_ENV_IN_TESTS, + restriction, + "default lint description" +} +declare_lint_pass!(SetEnvInTests => [SET_ENV_IN_TESTS]); + +impl LateLintPass<'_> for SetEnvInTests {} diff --git a/tests/ui/set_env_in_tests.rs b/tests/ui/set_env_in_tests.rs new file mode 100644 index 000000000000..b634281422ce --- /dev/null +++ b/tests/ui/set_env_in_tests.rs @@ -0,0 +1,5 @@ +#![warn(clippy::set_env_in_tests)] + +fn main() { + // test code goes here +} From 3dbc2742a967ae5443ab5a825fd0c3b5f50761fe Mon Sep 17 00:00:00 2001 From: Jack Amadeo Date: Thu, 30 Oct 2025 14:30:38 -0400 Subject: [PATCH 2/4] check for env::set_var in tests --- clippy_lints/src/set_env_in_tests.rs | 42 ++++++++++++++++++++++------ tests/ui/set_env_in_tests.rs | 15 +++++++++- tests/ui/set_env_in_tests.stderr | 12 ++++++++ 3 files changed, 60 insertions(+), 9 deletions(-) create mode 100644 tests/ui/set_env_in_tests.stderr diff --git a/clippy_lints/src/set_env_in_tests.rs b/clippy_lints/src/set_env_in_tests.rs index 5be389985aea..1a085eef211a 100644 --- a/clippy_lints/src/set_env_in_tests.rs +++ b/clippy_lints/src/set_env_in_tests.rs @@ -1,25 +1,51 @@ -use rustc_hir::*; +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::paths::{PathNS, lookup_path_str}; +use clippy_utils::{fn_def_id, is_in_test_function}; +use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; declare_clippy_lint! { /// ### What it does + /// Checks for use of `std::env::set_env` in tests /// /// ### Why restrict this? + /// Setting environment varibales in tests could lead + /// to leaking state between tests. /// /// ### Example /// ```no_run - /// // example code where clippy issues a warning - /// ``` - /// Use instead: - /// ```no_run - /// // example code which does not raise clippy warning + /// #[cfg(test)] + /// mod tests { + /// fn my_test() { + /// unsafe std::env::set_var("MY_VAR", "1"); + /// } + /// } /// ``` #[clippy::version = "1.92.0"] pub SET_ENV_IN_TESTS, restriction, - "default lint description" + "use of set_env in tests" } declare_lint_pass!(SetEnvInTests => [SET_ENV_IN_TESTS]); -impl LateLintPass<'_> for SetEnvInTests {} +impl<'tcx> LateLintPass<'tcx> for SetEnvInTests { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + let set_var_ids = lookup_path_str(cx.tcx, PathNS::Value, "std::env::set_var"); + + if matches!(expr.kind, ExprKind::Call(..)) + && let Some(call_def_id) = fn_def_id(cx, expr) + && is_in_test_function(cx.tcx, expr.hir_id) + && set_var_ids.contains(&call_def_id) + { + span_lint_and_help( + cx, + SET_ENV_IN_TESTS, + expr.span, + "env::set_var called from a test", + None, + "this might indicate state leakage and flaky tests", + ); + } + } +} diff --git a/tests/ui/set_env_in_tests.rs b/tests/ui/set_env_in_tests.rs index b634281422ce..32de925a8704 100644 --- a/tests/ui/set_env_in_tests.rs +++ b/tests/ui/set_env_in_tests.rs @@ -1,5 +1,18 @@ #![warn(clippy::set_env_in_tests)] +use std::env; + fn main() { - // test code goes here + unsafe { env::set_var("CLIPPY_TESTS_THIS_IS_OK", "1") } +} + +#[cfg(test)] +mod tests { + use std::env; + + #[test] + fn my_test() { + unsafe { env::set_var("CLIPPY_TESTS_THIS_IS_NOT_OK", "1") } + //~^ set_env_in_tests + } } diff --git a/tests/ui/set_env_in_tests.stderr b/tests/ui/set_env_in_tests.stderr new file mode 100644 index 000000000000..6ca71d2d3663 --- /dev/null +++ b/tests/ui/set_env_in_tests.stderr @@ -0,0 +1,12 @@ +error: env::set_var called from a test + --> tests/ui/set_env_in_tests.rs:15:18 + | +LL | unsafe { env::set_var("CLIPPY_TESTS_THIS_IS_NOT_OK", "1") } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: this might indicate state leakage and flaky tests + = note: `-D clippy::set-env-in-tests` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::set_env_in_tests)]` + +error: aborting due to 1 previous error + From 919092d3eef398ce5ecabf1ee73d109000f74f6c Mon Sep 17 00:00:00 2001 From: Jack Amadeo Date: Thu, 30 Oct 2025 14:55:26 -0400 Subject: [PATCH 3/4] other paths --- clippy_lints/src/set_env_in_tests.rs | 12 ++++++++---- tests/ui/set_env_in_tests.rs | 7 +++++++ tests/ui/set_env_in_tests.stderr | 26 +++++++++++++++++++++----- 3 files changed, 36 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/set_env_in_tests.rs b/clippy_lints/src/set_env_in_tests.rs index 1a085eef211a..f086f69fe98a 100644 --- a/clippy_lints/src/set_env_in_tests.rs +++ b/clippy_lints/src/set_env_in_tests.rs @@ -7,11 +7,15 @@ use rustc_session::declare_lint_pass; declare_clippy_lint! { /// ### What it does - /// Checks for use of `std::env::set_env` in tests + /// Checks for use of `std::env::set_env` in tests. /// /// ### Why restrict this? - /// Setting environment varibales in tests could lead - /// to leaking state between tests. + /// Setting environment varibales in tests often means the subject code + /// is reading and acting on the environment. By default, rust tests + /// are run concurrently, and setting environment variables cannot be + /// done in a way that is scoped only to the test. Even if care is taken + /// to clean up any mutations, concurrent test runs will affect each + /// other's environment. /// /// ### Example /// ```no_run @@ -44,7 +48,7 @@ impl<'tcx> LateLintPass<'tcx> for SetEnvInTests { expr.span, "env::set_var called from a test", None, - "this might indicate state leakage and flaky tests", + "this might indicate state leakage and cause flaky tests", ); } } diff --git a/tests/ui/set_env_in_tests.rs b/tests/ui/set_env_in_tests.rs index 32de925a8704..fb509d087b63 100644 --- a/tests/ui/set_env_in_tests.rs +++ b/tests/ui/set_env_in_tests.rs @@ -9,10 +9,17 @@ fn main() { #[cfg(test)] mod tests { use std::env; + use std::env::set_var; #[test] fn my_test() { + unsafe { set_var("CLIPPY_TESTS_THIS_IS_NOT_OK", "1") } + //~^ set_env_in_tests + unsafe { env::set_var("CLIPPY_TESTS_THIS_IS_NOT_OK", "1") } //~^ set_env_in_tests + + unsafe { std::env::set_var("CLIPPY_TESTS_THIS_IS_NOT_OK", "1") } + //~^ set_env_in_tests } } diff --git a/tests/ui/set_env_in_tests.stderr b/tests/ui/set_env_in_tests.stderr index 6ca71d2d3663..05fa4dfa25e6 100644 --- a/tests/ui/set_env_in_tests.stderr +++ b/tests/ui/set_env_in_tests.stderr @@ -1,12 +1,28 @@ error: env::set_var called from a test - --> tests/ui/set_env_in_tests.rs:15:18 + --> tests/ui/set_env_in_tests.rs:16:18 | -LL | unsafe { env::set_var("CLIPPY_TESTS_THIS_IS_NOT_OK", "1") } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | unsafe { set_var("CLIPPY_TESTS_THIS_IS_NOT_OK", "1") } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: this might indicate state leakage and flaky tests + = help: this might indicate state leakage and cause flaky tests = note: `-D clippy::set-env-in-tests` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::set_env_in_tests)]` -error: aborting due to 1 previous error +error: env::set_var called from a test + --> tests/ui/set_env_in_tests.rs:19:18 + | +LL | unsafe { env::set_var("CLIPPY_TESTS_THIS_IS_NOT_OK", "1") } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: this might indicate state leakage and cause flaky tests + +error: env::set_var called from a test + --> tests/ui/set_env_in_tests.rs:22:18 + | +LL | unsafe { std::env::set_var("CLIPPY_TESTS_THIS_IS_NOT_OK", "1") } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: this might indicate state leakage and cause flaky tests + +error: aborting due to 3 previous errors From 33efb1970de6bea06811dbca9d9ddf1674c93725 Mon Sep 17 00:00:00 2001 From: Jack Amadeo Date: Thu, 30 Oct 2025 16:37:10 -0400 Subject: [PATCH 4/4] fix a typo --- clippy_lints/src/set_env_in_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/set_env_in_tests.rs b/clippy_lints/src/set_env_in_tests.rs index f086f69fe98a..df932f905788 100644 --- a/clippy_lints/src/set_env_in_tests.rs +++ b/clippy_lints/src/set_env_in_tests.rs @@ -22,7 +22,7 @@ declare_clippy_lint! { /// #[cfg(test)] /// mod tests { /// fn my_test() { - /// unsafe std::env::set_var("MY_VAR", "1"); + /// unsafe { std::env::set_var("MY_VAR", "1"); } /// } /// } /// ```