Auto merge of rust-lang#114414 - cjgillot:early-unnameable-test, r=petrochenkov

Make test harness lint about unnnameable tests.

Implementation of rust-lang#113734 (comment)

About the options suggested in rust-lang#36629 (comment): adding this case to unused_attribute was just more complicated. I'll try to understand a bit more what you had in mind in rust-lang/rfcs#2471 (comment)

This was just simpler to do in a standalone PR. I'll remove the corresponding changes from rust-lang#113734 later.

r? `@petrochenkov`

Author: bors

compiler/rustc_builtin_macros/messages.ftl

@@ -227,3 +227,5 @@ builtin_macros_unexpected_lit = expected path to a trait, found literal
     .label = not a trait
     .str_lit = try using `#[derive({$sym})]`
     .other = for example, write `#[derive(Debug)]` for `Debug`
+
+builtin_macros_unnameable_test_items = cannot test inner items

compiler/rustc_builtin_macros/src/test_harness.rs

@@ -4,10 +4,12 @@ use rustc_ast as ast;
 use rustc_ast::entry::EntryPointType;
 use rustc_ast::mut_visit::{ExpectOne, *};
 use rustc_ast::ptr::P;
+use rustc_ast::visit::{walk_item, Visitor};
 use rustc_ast::{attr, ModKind};
 use rustc_expand::base::{ExtCtxt, ResolverExpand};
 use rustc_expand::expand::{AstFragment, ExpansionConfig};
 use rustc_feature::Features;
+use rustc_session::lint::builtin::UNNAMEABLE_TEST_ITEMS;
 use rustc_session::Session;
 use rustc_span::hygiene::{AstPass, SyntaxContext, Transparency};
 use rustc_span::symbol::{sym, Ident, Symbol};
@@ -137,11 +139,31 @@ impl<'a> MutVisitor for TestHarnessGenerator<'a> {
             let prev_tests = mem::take(&mut self.tests);
             noop_visit_item_kind(&mut item.kind, self);
             self.add_test_cases(item.id, span, prev_tests);
+        } else {
+            // But in those cases, we emit a lint to warn the user of these missing tests.
+            walk_item(&mut InnerItemLinter { sess: self.cx.ext_cx.sess }, &item);
         }
         smallvec![P(item)]
     }
 }
 
+struct InnerItemLinter<'a> {
+    sess: &'a Session,
+}
+
+impl<'a> Visitor<'a> for InnerItemLinter<'_> {
+    fn visit_item(&mut self, i: &'a ast::Item) {
+        if let Some(attr) = attr::find_by_name(&i.attrs, sym::rustc_test_marker) {
+            self.sess.parse_sess.buffer_lint(
+                UNNAMEABLE_TEST_ITEMS,
+                attr.span,
+                i.id,
+                crate::fluent_generated::builtin_macros_unnameable_test_items,
+            );
+        }
+    }
+}
+
 // Beware, this is duplicated in librustc_passes/entry.rs (with
 // `rustc_hir::Item`), so make sure to keep them in sync.
 fn entry_point_type(item: &ast::Item, depth: usize) -> EntryPointType {

compiler/rustc_lint/messages.ftl

@@ -130,8 +130,6 @@ lint_builtin_unexpected_cli_config_name = unexpected `{$name}` as condition name
 lint_builtin_unexpected_cli_config_value = unexpected condition value `{$value}` for condition name `{$name}`
     .help = was set with `--cfg` but isn't in the `--check-cfg` expected values
 
-lint_builtin_unnameable_test_items = cannot test inner items
-
 lint_builtin_unpermitted_type_init_label = this code causes undefined behavior when executed
 lint_builtin_unpermitted_type_init_label_suggestion = help: use `MaybeUninit<T>` instead, and only call `assume_init` after initialization is done
 

compiler/rustc_lint/src/builtin.rs

@@ -35,7 +35,7 @@ use crate::{
         BuiltinSpecialModuleNameUsed, BuiltinTrivialBounds, BuiltinTypeAliasGenericBounds,
         BuiltinTypeAliasGenericBoundsSuggestion, BuiltinTypeAliasWhereClause,
         BuiltinUnexpectedCliConfigName, BuiltinUnexpectedCliConfigValue,
-        BuiltinUngatedAsyncFnTrackCaller, BuiltinUnnameableTestItems, BuiltinUnpermittedTypeInit,
+        BuiltinUngatedAsyncFnTrackCaller, BuiltinUnpermittedTypeInit,
         BuiltinUnpermittedTypeInitSub, BuiltinUnreachablePub, BuiltinUnsafe,
         BuiltinUnstableFeatures, BuiltinUnusedDocComment, BuiltinUnusedDocCommentSub,
         BuiltinWhileTrue, SuggestChangingAssocTypes,
@@ -1770,82 +1770,6 @@ impl EarlyLintPass for EllipsisInclusiveRangePatterns {
     }
 }
 
-declare_lint! {
-    /// The `unnameable_test_items` lint detects [`#[test]`][test] functions
-    /// that are not able to be run by the test harness because they are in a
-    /// position where they are not nameable.
-    ///
-    /// [test]: https://doc.rust-lang.org/reference/attributes/testing.html#the-test-attribute
-    ///
-    /// ### Example
-    ///
-    /// ```rust,test
-    /// fn main() {
-    ///     #[test]
-    ///     fn foo() {
-    ///         // This test will not fail because it does not run.
-    ///         assert_eq!(1, 2);
-    ///     }
-    /// }
-    /// ```
-    ///
-    /// {{produces}}
-    ///
-    /// ### Explanation
-    ///
-    /// In order for the test harness to run a test, the test function must be
-    /// located in a position where it can be accessed from the crate root.
-    /// This generally means it must be defined in a module, and not anywhere
-    /// else such as inside another function. The compiler previously allowed
-    /// this without an error, so a lint was added as an alert that a test is
-    /// not being used. Whether or not this should be allowed has not yet been
-    /// decided, see [RFC 2471] and [issue #36629].
-    ///
-    /// [RFC 2471]: https://github.com/rust-lang/rfcs/pull/2471#issuecomment-397414443
-    /// [issue #36629]: https://github.com/rust-lang/rust/issues/36629
-    UNNAMEABLE_TEST_ITEMS,
-    Warn,
-    "detects an item that cannot be named being marked as `#[test_case]`",
-    report_in_external_macro
-}
-
-pub struct UnnameableTestItems {
-    boundary: Option<hir::OwnerId>, // Id of the item under which things are not nameable
-    items_nameable: bool,
-}
-
-impl_lint_pass!(UnnameableTestItems => [UNNAMEABLE_TEST_ITEMS]);
-
-impl UnnameableTestItems {
-    pub fn new() -> Self {
-        Self { boundary: None, items_nameable: true }
-    }
-}
-
-impl<'tcx> LateLintPass<'tcx> for UnnameableTestItems {
-    fn check_item(&mut self, cx: &LateContext<'_>, it: &hir::Item<'_>) {
-        if self.items_nameable {
-            if let hir::ItemKind::Mod(..) = it.kind {
-            } else {
-                self.items_nameable = false;
-                self.boundary = Some(it.owner_id);
-            }
-            return;
-        }
-
-        let attrs = cx.tcx.hir().attrs(it.hir_id());
-        if let Some(attr) = attr::find_by_name(attrs, sym::rustc_test_marker) {
-            cx.emit_spanned_lint(UNNAMEABLE_TEST_ITEMS, attr.span, BuiltinUnnameableTestItems);
-        }
-    }
-
-    fn check_item_post(&mut self, _cx: &LateContext<'_>, it: &hir::Item<'_>) {
-        if !self.items_nameable && self.boundary == Some(it.owner_id) {
-            self.items_nameable = true;
-        }
-    }
-}
-
 declare_lint! {
     /// The `keyword_idents` lint detects edition keywords being used as an
     /// identifier.

compiler/rustc_lint/src/lib.rs

@@ -189,8 +189,6 @@ late_lint_methods!(
     [
         pub BuiltinCombinedLateLintPass,
         [
-            // Tracks state across modules
-            UnnameableTestItems: UnnameableTestItems::new(),
             // Tracks attributes of parents
             MissingDoc: MissingDoc::new(),
             // Builds a global list of all impls of `Debug`.

compiler/rustc_lint/src/lints.rs

@@ -370,10 +370,6 @@ pub enum BuiltinEllipsisInclusiveRangePatternsLint {
     },
 }
 
-#[derive(LintDiagnostic)]
-#[diag(lint_builtin_unnameable_test_items)]
-pub struct BuiltinUnnameableTestItems;
-
 #[derive(LintDiagnostic)]
 #[diag(lint_builtin_keyword_idents)]
 pub struct BuiltinKeywordIdents {

compiler/rustc_lint_defs/src/builtin.rs

@@ -2846,6 +2846,45 @@ declare_lint! {
     };
 }
 
+declare_lint! {
+    /// The `unnameable_test_items` lint detects [`#[test]`][test] functions
+    /// that are not able to be run by the test harness because they are in a
+    /// position where they are not nameable.
+    ///
+    /// [test]: https://doc.rust-lang.org/reference/attributes/testing.html#the-test-attribute
+    ///
+    /// ### Example
+    ///
+    /// ```rust,test
+    /// fn main() {
+    ///     #[test]
+    ///     fn foo() {
+    ///         // This test will not fail because it does not run.
+    ///         assert_eq!(1, 2);
+    ///     }
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// In order for the test harness to run a test, the test function must be
+    /// located in a position where it can be accessed from the crate root.
+    /// This generally means it must be defined in a module, and not anywhere
+    /// else such as inside another function. The compiler previously allowed
+    /// this without an error, so a lint was added as an alert that a test is
+    /// not being used. Whether or not this should be allowed has not yet been
+    /// decided, see [RFC 2471] and [issue #36629].
+    ///
+    /// [RFC 2471]: https://github.com/rust-lang/rfcs/pull/2471#issuecomment-397414443
+    /// [issue #36629]: https://github.com/rust-lang/rust/issues/36629
+    pub UNNAMEABLE_TEST_ITEMS,
+    Warn,
+    "detects an item that cannot be named being marked as `#[test_case]`",
+    report_in_external_macro
+}
+
 declare_lint! {
     /// The `useless_deprecated` lint detects deprecation attributes with no effect.
     ///
@@ -3403,6 +3442,7 @@ declare_lint_pass! {
         UNKNOWN_CRATE_TYPES,
         UNKNOWN_DIAGNOSTIC_ATTRIBUTES,
         UNKNOWN_LINTS,
+        UNNAMEABLE_TEST_ITEMS,
         UNNAMEABLE_TYPES,
         UNREACHABLE_CODE,
         UNREACHABLE_PATTERNS,