From 0354cee1372ac80028588f9a2f2772c868b0a21c Mon Sep 17 00:00:00 2001 From: blyxyas Date: Fri, 31 Mar 2023 09:06:39 +0200 Subject: [PATCH 1/2] Add lint `items_after_test_module` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/items_after_test_module.rs | 82 +++++++++++++++++++++ clippy_lints/src/lib.rs | 2 + tests/ui/items_after_test_module.rs | 24 ++++++ tests/ui/items_after_test_module.stderr | 19 +++++ 6 files changed, 129 insertions(+) create mode 100644 clippy_lints/src/items_after_test_module.rs create mode 100644 tests/ui/items_after_test_module.rs create mode 100644 tests/ui/items_after_test_module.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 10dd28a8f6af2..d7102d2474ef4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4730,6 +4730,7 @@ Released 2018-09-13 [`invisible_characters`]: https://rust-lang.github.io/rust-clippy/master/index.html#invisible_characters [`is_digit_ascii_radix`]: https://rust-lang.github.io/rust-clippy/master/index.html#is_digit_ascii_radix [`items_after_statements`]: https://rust-lang.github.io/rust-clippy/master/index.html#items_after_statements +[`items_after_test_module`]: https://rust-lang.github.io/rust-clippy/master/index.html#items_after_test_module [`iter_cloned_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_cloned_collect [`iter_count`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_count [`iter_kv_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_kv_map diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index f24dab6278095..0c66d36a1d630 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -215,6 +215,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::invalid_upcast_comparisons::INVALID_UPCAST_COMPARISONS_INFO, crate::invalid_utf8_in_unchecked::INVALID_UTF8_IN_UNCHECKED_INFO, crate::items_after_statements::ITEMS_AFTER_STATEMENTS_INFO, + crate::items_after_test_module::ITEMS_AFTER_TEST_MODULE_INFO, crate::iter_not_returning_iterator::ITER_NOT_RETURNING_ITERATOR_INFO, crate::large_const_arrays::LARGE_CONST_ARRAYS_INFO, crate::large_enum_variant::LARGE_ENUM_VARIANT_INFO, diff --git a/clippy_lints/src/items_after_test_module.rs b/clippy_lints/src/items_after_test_module.rs new file mode 100644 index 0000000000000..9d949a44c0206 --- /dev/null +++ b/clippy_lints/src/items_after_test_module.rs @@ -0,0 +1,82 @@ +use clippy_utils::{diagnostics::span_lint_and_help, is_in_cfg_test}; +use rustc_hir::{HirId, ItemId, ItemKind, Mod}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::sym; + +declare_clippy_lint! { + /// ### What it does + /// Triggers if an item is declared after the testing module marked with `#[cfg(test)]`. + /// ### Why is this bad? + /// Having items declared after the testing module is confusing and may lead to bad test coverage. + /// ### Example + /// ```rust + /// #[cfg(test)] + /// mod tests { + /// // [...] + /// } + /// + /// fn my_function() { + /// // [...] + /// } + /// ``` + /// Use instead: + /// ```rust + /// fn my_function() { + /// // [...] + /// } + /// + /// #[cfg(test)] + /// mod tests { + /// // [...] + /// } + /// ``` + #[clippy::version = "1.70.0"] + pub ITEMS_AFTER_TEST_MODULE, + style, + "An item was found after the testing module `tests`" +} + +declare_lint_pass!(ItemsAfterTestModule => [ITEMS_AFTER_TEST_MODULE]); + +impl LateLintPass<'_> for ItemsAfterTestModule { + fn check_mod(&mut self, cx: &LateContext<'_>, _: &Mod<'_>, _: HirId) { + let mut was_test_mod_visited = false; + let mut test_mod_hash: Option = None; + + let hir = cx.tcx.hir(); + let items = hir.items().collect::>(); + + for itid in &items { + let item = hir.item(*itid); + + if_chain! { + if was_test_mod_visited; + if cx.sess().source_map().lookup_char_pos(item.span.lo()).file.name_hash + == test_mod_hash.unwrap(); // Will never fail + if !matches!(item.kind, ItemKind::Mod(_) | ItemKind::Macro(_, _)); + if !is_in_cfg_test(cx.tcx, itid.hir_id()); // The item isn't in the testing module itself + + if !in_external_macro(cx.sess(), item.span); + then { + span_lint_and_help(cx, ITEMS_AFTER_TEST_MODULE, item.span, "an item was found after the testing module", None, "move the item to before the testing module was defined"); + }}; + + if matches!(item.kind, ItemKind::Mod(_)) { + for attr in cx.tcx.get_attrs(item.owner_id.to_def_id(), sym::cfg) { + if_chain! { + if attr.has_name(sym::cfg); + if let Some(mitems) = attr.meta_item_list(); + if let [mitem] = &*mitems; + if mitem.has_name(sym::test); + then { + was_test_mod_visited = true; + test_mod_hash = Some(cx.sess().source_map().lookup_char_pos(item.span.lo()).file.name_hash); + } + } + } + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index af2c780360162..573ffe349ec23 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -158,6 +158,7 @@ mod int_plus_one; mod invalid_upcast_comparisons; mod invalid_utf8_in_unchecked; mod items_after_statements; +mod items_after_test_module; mod iter_not_returning_iterator; mod large_const_arrays; mod large_enum_variant; @@ -961,6 +962,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(tests_outside_test_module::TestsOutsideTestModule)); store.register_late_pass(|_| Box::new(manual_slice_size_calculation::ManualSliceSizeCalculation)); store.register_early_pass(|| Box::new(suspicious_doc_comments::SuspiciousDocComments)); + store.register_late_pass(|_| Box::new(items_after_test_module::ItemsAfterTestModule)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/items_after_test_module.rs b/tests/ui/items_after_test_module.rs new file mode 100644 index 0000000000000..735c06e51ffcd --- /dev/null +++ b/tests/ui/items_after_test_module.rs @@ -0,0 +1,24 @@ +// compile-flags: --test +#![allow(unused)] +#![warn(clippy::items_after_test_module)] + +fn main() {} + +fn should_not_lint() {} + +#[allow(dead_code)] +#[allow(unused)] // Some attributes to check that span replacement is good enough +#[allow(clippy::allow_attributes)] +#[cfg(test)] +mod tests { + #[test] + fn hi() {} +} + +fn should_lint() {} + +const SHOULD_ALSO_LINT: usize = 1; + +macro_rules! should_not_lint { + () => {}; +} diff --git a/tests/ui/items_after_test_module.stderr b/tests/ui/items_after_test_module.stderr new file mode 100644 index 0000000000000..53ca4746476f4 --- /dev/null +++ b/tests/ui/items_after_test_module.stderr @@ -0,0 +1,19 @@ +error: an item was found after the testing module + --> $DIR/items_after_test_module.rs:18:1 + | +LL | fn should_lint() {} + | ^^^^^^^^^^^^^^^^^^^ + | + = help: move the item to before the testing module was defined + = note: `-D clippy::items-after-test-module` implied by `-D warnings` + +error: an item was found after the testing module + --> $DIR/items_after_test_module.rs:20:1 + | +LL | const SHOULD_ALSO_LINT: usize = 1; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: move the item to before the testing module was defined + +error: aborting due to 2 previous errors + From 1ac8dc51bc379136cbcba737e63ba619f9524269 Mon Sep 17 00:00:00 2001 From: blyxyas Date: Wed, 12 Apr 2023 20:17:26 +0200 Subject: [PATCH 2/2] Compact emmited lint --- clippy_lints/src/items_after_test_module.rs | 33 +++++++++++---------- tests/ui/items_after_test_module.rs | 3 +- tests/ui/items_after_test_module.stderr | 26 ++++++++-------- 3 files changed, 30 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/items_after_test_module.rs b/clippy_lints/src/items_after_test_module.rs index 9d949a44c0206..52d716feea02f 100644 --- a/clippy_lints/src/items_after_test_module.rs +++ b/clippy_lints/src/items_after_test_module.rs @@ -3,7 +3,7 @@ use rustc_hir::{HirId, ItemId, ItemKind, Mod}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::sym; +use rustc_span::{sym, Span}; declare_clippy_lint! { /// ### What it does @@ -43,38 +43,39 @@ declare_lint_pass!(ItemsAfterTestModule => [ITEMS_AFTER_TEST_MODULE]); impl LateLintPass<'_> for ItemsAfterTestModule { fn check_mod(&mut self, cx: &LateContext<'_>, _: &Mod<'_>, _: HirId) { let mut was_test_mod_visited = false; - let mut test_mod_hash: Option = None; + let mut test_mod_span: Option = None; let hir = cx.tcx.hir(); let items = hir.items().collect::>(); - for itid in &items { + for (i, itid) in items.iter().enumerate() { let item = hir.item(*itid); if_chain! { if was_test_mod_visited; + if i == (items.len() - 3 /* Weird magic number (HIR-translation behaviour) */); if cx.sess().source_map().lookup_char_pos(item.span.lo()).file.name_hash - == test_mod_hash.unwrap(); // Will never fail - if !matches!(item.kind, ItemKind::Mod(_) | ItemKind::Macro(_, _)); + == cx.sess().source_map().lookup_char_pos(test_mod_span.unwrap().lo()).file.name_hash; // Will never fail + if !matches!(item.kind, ItemKind::Mod(_)); if !is_in_cfg_test(cx.tcx, itid.hir_id()); // The item isn't in the testing module itself - if !in_external_macro(cx.sess(), item.span); + then { - span_lint_and_help(cx, ITEMS_AFTER_TEST_MODULE, item.span, "an item was found after the testing module", None, "move the item to before the testing module was defined"); + span_lint_and_help(cx, ITEMS_AFTER_TEST_MODULE, test_mod_span.unwrap().with_hi(item.span.hi()), "items were found after the testing module", None, "move the items to before the testing module was defined"); }}; if matches!(item.kind, ItemKind::Mod(_)) { for attr in cx.tcx.get_attrs(item.owner_id.to_def_id(), sym::cfg) { if_chain! { - if attr.has_name(sym::cfg); - if let Some(mitems) = attr.meta_item_list(); - if let [mitem] = &*mitems; - if mitem.has_name(sym::test); - then { - was_test_mod_visited = true; - test_mod_hash = Some(cx.sess().source_map().lookup_char_pos(item.span.lo()).file.name_hash); - } - } + if attr.has_name(sym::cfg); + if let Some(mitems) = attr.meta_item_list(); + if let [mitem] = &*mitems; + if mitem.has_name(sym::test); + then { + was_test_mod_visited = true; + test_mod_span = Some(item.span); + } + } } } } diff --git a/tests/ui/items_after_test_module.rs b/tests/ui/items_after_test_module.rs index 735c06e51ffcd..5136b2557ec1a 100644 --- a/tests/ui/items_after_test_module.rs +++ b/tests/ui/items_after_test_module.rs @@ -1,4 +1,4 @@ -// compile-flags: --test +//@compile-flags: --test #![allow(unused)] #![warn(clippy::items_after_test_module)] @@ -18,7 +18,6 @@ mod tests { fn should_lint() {} const SHOULD_ALSO_LINT: usize = 1; - macro_rules! should_not_lint { () => {}; } diff --git a/tests/ui/items_after_test_module.stderr b/tests/ui/items_after_test_module.stderr index 53ca4746476f4..8f1616dabc1f0 100644 --- a/tests/ui/items_after_test_module.stderr +++ b/tests/ui/items_after_test_module.stderr @@ -1,19 +1,17 @@ -error: an item was found after the testing module - --> $DIR/items_after_test_module.rs:18:1 +error: items were found after the testing module + --> $DIR/items_after_test_module.rs:13:1 | -LL | fn should_lint() {} - | ^^^^^^^^^^^^^^^^^^^ +LL | / mod tests { +LL | | #[test] +LL | | fn hi() {} +LL | | } +... | +LL | | () => {}; +LL | | } + | |_^ | - = help: move the item to before the testing module was defined + = help: move the items to before the testing module was defined = note: `-D clippy::items-after-test-module` implied by `-D warnings` -error: an item was found after the testing module - --> $DIR/items_after_test_module.rs:20:1 - | -LL | const SHOULD_ALSO_LINT: usize = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: move the item to before the testing module was defined - -error: aborting due to 2 previous errors +error: aborting due to previous error