Auto merge of #10578 - blyxyas:items_after_test_module, r=dswij

Add `items_after_test_module` lint

Resolves task *3* of #10506, alongside *1* resolved at #10543 in an effort to help standarize a little bit more testing modules.

---

changelog:[`items_after_test_module`]: Added the lint.

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

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,

clippy_lints/src/items_after_test_module.rs

@@ -0,0 +1,83 @@
+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, Span};
+
+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_span: Option<Span> = None;
+
+        let hir = cx.tcx.hir();
+        let items = hir.items().collect::<Vec<ItemId>>();
+
+        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
+            == 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, 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_span = Some(item.span);
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

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`
 }
 

tests/ui/items_after_test_module.rs

@@ -0,0 +1,23 @@
+//@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 {
+    () => {};
+}

tests/ui/items_after_test_module.stderr

@@ -0,0 +1,17 @@
+error: items were found after the testing module
+  --> $DIR/items_after_test_module.rs:13:1
+   |
+LL | / mod tests {
+LL | |     #[test]
+LL | |     fn hi() {}
+LL | | }
+...  |
+LL | |     () => {};
+LL | | }
+   | |_^
+   |
+   = help: move the items to before the testing module was defined
+   = note: `-D clippy::items-after-test-module` implied by `-D warnings`
+
+error: aborting due to previous error
+