diff --git a/CHANGELOG.md b/CHANGELOG.md index 239631777..ba10cb53e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4936,6 +4936,7 @@ Released 2018-09-13 [`tabs_in_doc_comments`]: https://rust-lang.github.io/rust-clippy/master/index.html#tabs_in_doc_comments [`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment [`temporary_cstring_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_cstring_as_ptr +[`tests_outside_test_module`]: https://rust-lang.github.io/rust-clippy/master/index.html#tests_outside_test_module [`to_digit_is_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_digit_is_some [`to_string_in_display`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_display [`to_string_in_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#to_string_in_format_args diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 7836e5ccb..b45105570 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -577,6 +577,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::swap_ptr_to_ref::SWAP_PTR_TO_REF_INFO, crate::tabs_in_doc_comments::TABS_IN_DOC_COMMENTS_INFO, crate::temporary_assignment::TEMPORARY_ASSIGNMENT_INFO, + crate::tests_outside_test_module::TESTS_OUTSIDE_TEST_MODULE_INFO, crate::to_digit_is_some::TO_DIGIT_IS_SOME_INFO, crate::trailing_empty_array::TRAILING_EMPTY_ARRAY_INFO, crate::trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index ce055f162..c63abf097 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -291,6 +291,7 @@ mod swap; mod swap_ptr_to_ref; mod tabs_in_doc_comments; mod temporary_assignment; +mod tests_outside_test_module; mod to_digit_is_some; mod trailing_empty_array; mod trait_bounds; @@ -951,6 +952,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: )) }); store.register_late_pass(|_| Box::new(lines_filter_map_ok::LinesFilterMapOk)); + store.register_late_pass(|_| Box::new(tests_outside_test_module::TestsOutsideTestModule::new())); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/tests_outside_test_module.rs b/clippy_lints/src/tests_outside_test_module.rs new file mode 100644 index 000000000..d14a292a5 --- /dev/null +++ b/clippy_lints/src/tests_outside_test_module.rs @@ -0,0 +1,101 @@ +use clippy_utils::{diagnostics::span_lint_and_note, is_in_cfg_test, is_in_test_function, is_test_module_or_function}; +use rustc_data_structures::sync::par_for_each_in; +use rustc_hir::{intravisit::FnKind, Body, FnDecl, HirId, ItemKind, Mod}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::{def_id::LocalDefId, Span}; + +declare_clippy_lint! { + /// ### What it does + /// + /// Triggers when a testing function (marked with the `#[test]` attribute) isn't inside a testing module (marked with `#[cfg(test)]`). + /// + /// ### Why is this bad? + /// + /// The idiomatic (and more performant) way of writing tests is inside a testing module (flagged with `#[cfg(test)]`), having test functions outside of this module is confusing and may lead to them being "hidden". + /// + /// ### Example + /// ```rust + /// #[test] + /// fn my_cool_test() { + /// // [...] + /// } + /// + /// #[cfg(test)] + /// mod tests { + /// // [...] + /// } + /// + /// ``` + /// Use instead: + /// ```rust + /// #[cfg(test)] + /// mod tests { + /// #[test] + /// fn my_cool_test() { + /// // [...] + /// } + /// } + /// ``` + #[clippy::version = "1.70.0"] + pub TESTS_OUTSIDE_TEST_MODULE, + restriction, + "The test function `my_cool_test` is outside the testing module `tests`." +} + +pub(crate) struct TestsOutsideTestModule { + pub test_mod_exists: bool, +} + +impl TestsOutsideTestModule { + pub fn new() -> Self { + Self { test_mod_exists: false } + } +} + +impl_lint_pass!(TestsOutsideTestModule => [TESTS_OUTSIDE_TEST_MODULE]); + +impl LateLintPass<'_> for TestsOutsideTestModule { + fn check_mod(&mut self, cx: &LateContext<'_>, _: &Mod<'_>, _: HirId) { + self.test_mod_exists = false; + + // par_for_each_item uses Fn, while par_for_each_in uses FnMut + par_for_each_in(cx.tcx.hir_crate_items(()).items(), |itemid| { + let item = cx.tcx.hir().item(itemid); + if_chain! { + if matches!(item.kind, ItemKind::Mod(_)); + if is_test_module_or_function(cx.tcx, item); + then { + self.test_mod_exists = true; + } + } + }); + } + + fn check_fn( + &mut self, + cx: &LateContext<'_>, + kind: FnKind<'_>, + _: &FnDecl<'_>, + body: &Body<'_>, + sp: Span, + _: LocalDefId, + ) { + if_chain! { + if !matches!(kind, FnKind::Closure); + if self.test_mod_exists; + if is_in_test_function(cx.tcx, body.id().hir_id); + if !is_in_cfg_test(cx.tcx, body.id().hir_id); + then { + span_lint_and_note( + cx, + TESTS_OUTSIDE_TEST_MODULE, + sp, + "this function marked with #[test] is outside a #[cfg(test)] module", + None, + "move it to a testing module marked with #[cfg(test)]", + ); + } + } + } +} diff --git a/tests/ui/tests_outside_test_module.rs b/tests/ui/tests_outside_test_module.rs new file mode 100644 index 000000000..1982b1d01 --- /dev/null +++ b/tests/ui/tests_outside_test_module.rs @@ -0,0 +1,18 @@ +// compile-flags: --test +#![allow(unused)] +#![warn(clippy::tests_outside_test_module)] + +fn main() { + // test code goes here +} + +// Should lint +#[test] +fn my_test() {} + +#[cfg(test)] +mod tests { + // Should not lint + #[test] + fn my_test() {} +} diff --git a/tests/ui/tests_outside_test_module.stderr b/tests/ui/tests_outside_test_module.stderr new file mode 100644 index 000000000..125a79d6e --- /dev/null +++ b/tests/ui/tests_outside_test_module.stderr @@ -0,0 +1,11 @@ +error: this function marked with #[test] is outside a #[cfg(test)] module + --> $DIR/tests_outside_test_module.rs:11:1 + | +LL | fn my_test() {} + | ^^^^^^^^^^^^^^^ + | + = note: move it to a testing module marked with #[cfg(test)] + = note: `-D clippy::tests-outside-test-module` implied by `-D warnings` + +error: aborting due to previous error +