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..df932f905788 --- /dev/null +++ b/clippy_lints/src/set_env_in_tests.rs @@ -0,0 +1,55 @@ +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 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 + /// #[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, + "use of set_env in tests" +} +declare_lint_pass!(SetEnvInTests => [SET_ENV_IN_TESTS]); + +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 cause flaky tests", + ); + } + } +} diff --git a/tests/ui/set_env_in_tests.rs b/tests/ui/set_env_in_tests.rs new file mode 100644 index 000000000000..fb509d087b63 --- /dev/null +++ b/tests/ui/set_env_in_tests.rs @@ -0,0 +1,25 @@ +#![warn(clippy::set_env_in_tests)] + +use std::env; + +fn main() { + unsafe { env::set_var("CLIPPY_TESTS_THIS_IS_OK", "1") } +} + +#[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 new file mode 100644 index 000000000000..05fa4dfa25e6 --- /dev/null +++ b/tests/ui/set_env_in_tests.stderr @@ -0,0 +1,28 @@ +error: env::set_var called from a test + --> tests/ui/set_env_in_tests.rs:16:18 + | +LL | unsafe { set_var("CLIPPY_TESTS_THIS_IS_NOT_OK", "1") } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = 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: 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 +