Auto merge of #11611 - Alexendoo:items-after-test-module-check-crate, r=blyxyas

Fix `items_after_test_module` for non root modules, add applicable suggestion

Fixes #11050
Fixes #11153

changelog: [`items_after_test_module`]: Now suggests a machine-applicable suggestion.
changelog: [`items:after_test_module`]: Also lints for non root modules

clippy_lints/src/items_after_test_module.rs

@@ -1,10 +1,12 @@
-use clippy_utils::diagnostics::span_lint_and_help;
-use clippy_utils::{is_from_proc_macro, 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 clippy_utils::diagnostics::span_lint_hir_and_then;
+use clippy_utils::source::snippet_opt;
+use clippy_utils::{fulfill_or_allowed, is_cfg_test, is_from_proc_macro};
+use rustc_errors::{Applicability, SuggestionStyle};
+use rustc_hir::{HirId, Item, ItemKind, Mod};
+use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::{sym, Span};
+use rustc_span::hygiene::AstPass;
+use rustc_span::{sym, ExpnKind};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -41,46 +43,72 @@ declare_clippy_lint! {
 
 declare_lint_pass!(ItemsAfterTestModule => [ITEMS_AFTER_TEST_MODULE]);
 
+fn cfg_test_module<'tcx>(cx: &LateContext<'tcx>, item: &Item<'tcx>) -> bool {
+    if let ItemKind::Mod(test_mod) = item.kind
+        && item.span.hi() == test_mod.spans.inner_span.hi()
+        && is_cfg_test(cx.tcx, item.hir_id())
+        && !item.span.from_expansion()
+        && !is_from_proc_macro(cx, item)
+    {
+        true
+    } else {
+        false
+    }
+}
+
 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;
+    fn check_mod(&mut self, cx: &LateContext<'_>, module: &Mod<'_>, _: HirId) {
+        let mut items = module.item_ids.iter().map(|&id| cx.tcx.hir().item(id));
 
-        let hir = cx.tcx.hir();
-        let items = hir.items().collect::<Vec<ItemId>>();
+        let Some((mod_pos, test_mod)) = items.by_ref().enumerate().find(|(_, item)| cfg_test_module(cx, item)) else {
+            return;
+        };
 
-        for (i, itid) in items.iter().enumerate() {
-            let item = hir.item(*itid);
+        let after: Vec<_> = items
+            .filter(|item| {
+                // Ignore the generated test main function
+                !(item.ident.name == sym::main
+                    && item.span.ctxt().outer_expn_data().kind == ExpnKind::AstPass(AstPass::TestHarness))
+            })
+            .collect();
 
-            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);
-            if !is_from_proc_macro(cx, item);
+        if let Some(last) = after.last()
+            && after.iter().all(|&item| {
+                !matches!(item.kind, ItemKind::Mod(_))
+                    && !item.span.from_expansion()
+                    && !is_from_proc_macro(cx, item)
+            })
+            && !fulfill_or_allowed(cx, ITEMS_AFTER_TEST_MODULE, after.iter().map(|item| item.hir_id()))
+        {
+            let def_spans: Vec<_> = std::iter::once(test_mod.owner_id)
+                .chain(after.iter().map(|item| item.owner_id))
+                .map(|id| cx.tcx.def_span(id))
+                .collect();
 
-            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 let ItemKind::Mod(module) = item.kind && item.span.hi() == module.spans.inner_span.hi() {
-			// Check that it works the same way, the only I way I've found for #10713
-				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);
-                        }
-                    }
-                }
-			}
+            span_lint_hir_and_then(
+                cx,
+                ITEMS_AFTER_TEST_MODULE,
+                test_mod.hir_id(),
+                def_spans,
+                "items after a test module",
+                |diag| {
+                    if let Some(prev) = mod_pos.checked_sub(1)
+                        && let prev = cx.tcx.hir().item(module.item_ids[prev])
+                        && let items_span = last.span.with_lo(test_mod.span.hi())
+                        && let Some(items) = snippet_opt(cx, items_span)
+                    {
+                        diag.multipart_suggestion_with_style(
+                            "move the items to before the test module was defined",
+                            vec![
+                                (prev.span.shrink_to_hi(), items),
+                                (items_span, String::new())
+                            ],
+                            Applicability::MachineApplicable,
+                            SuggestionStyle::HideCodeAlways,
+                        );
+                    }
+                },
+            );
         }
     }
 }

clippy_utils/src/lib.rs

@@ -80,7 +80,6 @@ use std::sync::{Mutex, MutexGuard, OnceLock};
 use if_chain::if_chain;
 use itertools::Itertools;
 use rustc_ast::ast::{self, LitKind, RangeLimits};
-use rustc_ast::Attribute;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::unhash::UnhashMap;
 use rustc_hir::def::{DefKind, Res};
@@ -2452,11 +2451,12 @@ pub fn is_in_test_function(tcx: TyCtxt<'_>, id: hir::HirId) -> bool {
     })
 }
 
-/// Checks if the item containing the given `HirId` has `#[cfg(test)]` attribute applied
+/// Checks if `id` has a `#[cfg(test)]` attribute applied
 ///
-/// Note: Add `//@compile-flags: --test` to UI tests with a `#[cfg(test)]` function
-pub fn is_in_cfg_test(tcx: TyCtxt<'_>, id: hir::HirId) -> bool {
-    fn is_cfg_test(attr: &Attribute) -> bool {
+/// This only checks directly applied attributes, to see if a node is inside a `#[cfg(test)]` parent
+/// use [`is_in_cfg_test`]
+pub fn is_cfg_test(tcx: TyCtxt<'_>, id: hir::HirId) -> bool {
+    tcx.hir().attrs(id).iter().any(|attr| {
         if attr.has_name(sym::cfg)
             && let Some(items) = attr.meta_item_list()
             && let [item] = &*items
@@ -2466,11 +2466,14 @@ pub fn is_in_cfg_test(tcx: TyCtxt<'_>, id: hir::HirId) -> bool {
         } else {
             false
         }
+    })
 }
+
+/// Checks if any parent node of `HirId` has `#[cfg(test)]` attribute applied
+pub fn is_in_cfg_test(tcx: TyCtxt<'_>, id: hir::HirId) -> bool {
     tcx.hir()
-        .parent_iter(id)
-        .flat_map(|(parent_id, _)| tcx.hir().attrs(parent_id))
-        .any(is_cfg_test)
+        .parent_id_iter(id)
+        .any(|parent_id| is_cfg_test(tcx, parent_id))
 }
 
 /// Checks if the item of any of its parents has `#[cfg(...)]` attribute applied.

tests/ui/items_after_test_module/after_proc_macros.rs

@@ -0,0 +1,11 @@
+//@aux-build:../auxiliary/proc_macros.rs
+extern crate proc_macros;
+
+proc_macros::with_span! {
+    span
+    #[cfg(test)]
+    mod tests {}
+}
+
+#[test]
+fn f() {}

tests/ui/items_after_test_module/auxiliary/submodule.rs

@@ -0,0 +1,4 @@
+#[cfg(test)]
+mod tests {}
+
+fn in_submodule() {}

tests/ui/items_after_test_module/block_module.stderr

@@ -1,2 +0,0 @@
-error: Option 'test' given more than once
-

tests/ui/items_after_test_module/in_submodule.rs

@@ -0,0 +1,8 @@
+#[path = "auxiliary/submodule.rs"]
+mod submodule;
+
+#[cfg(test)]
+mod tests {
+    #[test]
+    fn t() {}
+}

tests/ui/items_after_test_module/in_submodule.stderr

@@ -0,0 +1,14 @@
+error: items after a test module
+  --> $DIR/auxiliary/submodule.rs:2:1
+   |
+LL | mod tests {}
+   | ^^^^^^^^^
+LL |
+LL | fn in_submodule() {}
+   | ^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::items-after-test-module` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::items_after_test_module)]`
+
+error: aborting due to previous error
+

tests/ui/items_after_test_module/multiple_modules.rs

@@ -0,0 +1,11 @@
+#[cfg(test)]
+mod tests {
+    #[test]
+    fn f() {}
+}
+
+#[cfg(test)]
+mod more_tests {
+    #[test]
+    fn g() {}
+}

tests/ui/items_after_test_module/root_module.fixed

@@ -1,4 +1,3 @@
-//@compile-flags: --test
 #![allow(unused)]
 #![warn(clippy::items_after_test_module)]
 
@@ -6,6 +5,13 @@ fn main() {}
 
 fn should_not_lint() {}
 
+fn should_lint() {}
+
+const SHOULD_ALSO_LINT: usize = 1;
+macro_rules! should_lint {
+    () => {};
+}
+
 #[allow(dead_code)]
 #[allow(unused)] // Some attributes to check that span replacement is good enough
 #[allow(clippy::allow_attributes)]
@@ -14,10 +20,3 @@ 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/root_module.rs

@@ -0,0 +1,22 @@
+#![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_lint {
+    () => {};
+}

tests/ui/items_after_test_module/root_module.stderr

@@ -0,0 +1,20 @@
+error: items after a test module
+  --> $DIR/root_module.rs:12:1
+   |
+LL | mod tests {
+   | ^^^^^^^^^
+...
+LL | fn should_lint() {}
+   | ^^^^^^^^^^^^^^^^
+LL |
+LL | const SHOULD_ALSO_LINT: usize = 1;
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL | macro_rules! should_lint {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::items-after-test-module` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::items_after_test_module)]`
+   = help: move the items to before the test module was defined
+
+error: aborting due to previous error
+