fix: resolve review issues

Author: farazdagi

clippy_config/src/conf.rs

@@ -772,20 +772,13 @@ define_Conf! {
     /// exported visibility, or whether they are marked as "pub".
     #[lints(pub_underscore_fields)]
     pub_underscore_fields_behavior: PubUnderscoreFieldsBehaviour = PubUnderscoreFieldsBehaviour::PubliclyExported,
-    /// Whether to include functions outside of `#[cfg(test)]` in the linting process or not.
+    /// Indicates if `redundant_test_prefix` should check functions outside of items marked
+    /// with `#[cfg(test)]`.
     ///
-    /// This option allows running the lint against the integration tests: test functions located
-    /// there are not inside a node marked with `#[cfg(test)]` annotation (although they are
-    /// still marked using `#[test]` annotation and thus can have redundant "test_" prefix).
+    /// This option can be used for integration tests which use the `#[test]` attribute
+    /// without the `#[cfg(test)]`.
     #[lints(redundant_test_prefix)]
     redundant_test_prefix_check_outside_cfg_test: bool = false,
-    /// What suffix to use to avoid function name collisions when `test_` prefix is removed.
-    ///
-    /// If set to `"_works"`, the lint will suggest renaming `test_foo` to `foo_works`.
-    /// Suffix is added only when there is a collision with an existing function name,
-    /// otherwise just `test_` prefix is removed (and no suffix added).
-    #[lints(redundant_test_prefix)]
-    redundant_test_prefix_custom_suffix: String = String::from("_works"),
     /// Whether to lint only if it's multiline.
     #[lints(semicolon_inside_block)]
     semicolon_inside_block_ignore_singleline: bool = false,

clippy_lints/src/redundant_test_prefix.rs

@@ -1,14 +1,15 @@
 use clippy_config::Conf;
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
 use clippy_utils::visitors::for_each_expr;
 use clippy_utils::{is_in_cfg_test, is_test_function};
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::FnKind;
 use rustc_hir::{self as hir, Body, ExprKind, FnDecl};
+use rustc_lexer::is_ident;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
-use rustc_span::Span;
 use rustc_span::def_id::LocalDefId;
+use rustc_span::{Span, Symbol, edition};
 use std::ops::ControlFlow;
 
 declare_clippy_lint! {
@@ -37,20 +38,18 @@ declare_clippy_lint! {
     /// ```
     #[clippy::version = "1.84.0"]
     pub REDUNDANT_TEST_PREFIX,
-    pedantic,
+    restriction,
     "redundant `test_` prefix in test function name"
 }
 
 pub struct RedundantTestPrefix {
     check_outside_test_cfg: bool,
-    custom_sufix: String,
 }
 
 impl RedundantTestPrefix {
     pub fn new(conf: &'static Conf) -> Self {
         Self {
             check_outside_test_cfg: conf.redundant_test_prefix_check_outside_cfg_test,
-            custom_sufix: conf.redundant_test_prefix_custom_suffix.clone(),
         }
     }
 }
@@ -72,31 +71,64 @@ impl<'tcx> LateLintPass<'tcx> for RedundantTestPrefix {
             return;
         };
 
+        // Skip the lint if the function is within a macro expansion.
+        if ident.span.from_expansion() {
+            return;
+        }
+
+        // Skip if the function name does not start with `test_`.
+        if !ident.as_str().starts_with("test_") {
+            return;
+        }
+
         // Skip the lint if the function is not within a node marked with `#[cfg(test)]` attribute.
         // Continue if the function is inside the node marked with `#[cfg(test)]` or lint is enforced
         // via configuration (most likely to include integration tests in lint's scope).
         if !(self.check_outside_test_cfg || is_in_cfg_test(cx.tcx, body.id().hir_id)) {
             return;
         }
 
-        if is_test_function(cx.tcx, cx.tcx.local_def_id_to_hir_id(fn_def_id)) && ident.as_str().starts_with("test_") {
+        if is_test_function(cx.tcx, cx.tcx.local_def_id_to_hir_id(fn_def_id)) {
+            let msg = "redundant `test_` prefix in test function name";
             let mut non_prefixed = ident.as_str().trim_start_matches("test_").to_string();
-            let mut help_msg = "consider removing the `test_` prefix";
-            // If `non_prefixed` conflicts with another function in the same module/scope,
-            // add extra suffix to avoid name conflict.
-            if name_conflicts(cx, body, &non_prefixed) {
-                non_prefixed.push_str(&self.custom_sufix);
-                help_msg = "consider removing the `test_` prefix (suffix avoids name conflict)";
+
+            if is_invalid_ident(&non_prefixed) {
+                // If the prefix-trimmed name is not a valid function name, do not provide an
+                // automatic fix, just suggest renaming the function.
+                span_lint_and_help(
+                    cx,
+                    REDUNDANT_TEST_PREFIX,
+                    ident.span,
+                    msg,
+                    None,
+                    "consider function renaming (just removing `test_` prefix will produce invalid function name)",
+                );
+            } else if name_conflicts(cx, body, &non_prefixed) {
+                // If `non_prefixed` conflicts with another function in the same module/scope,
+                // do not provide an automatic fix, but still emit a fix suggestion.
+                non_prefixed.push_str("_works");
+                span_lint_and_sugg(
+                    cx,
+                    REDUNDANT_TEST_PREFIX,
+                    ident.span,
+                    msg,
+                    "consider function renaming (just removing `test_` prefix will cause a name conflict)",
+                    non_prefixed,
+                    Applicability::HasPlaceholders,
+                )
+            } else {
+                // If `non_prefixed` is a valid identifier and does not conflict with another function,
+                // so we can suggest an auto-fix.
+                span_lint_and_sugg(
+                    cx,
+                    REDUNDANT_TEST_PREFIX,
+                    ident.span,
+                    msg,
+                    "consider removing the `test_` prefix",
+                    non_prefixed,
+                    Applicability::MachineApplicable,
+                )
             }
-            span_lint_and_sugg(
-                cx,
-                REDUNDANT_TEST_PREFIX,
-                ident.span,
-                "redundant `test_` prefix in test function name",
-                help_msg,
-                non_prefixed,
-                Applicability::MachineApplicable,
-            );
         }
     }
 }
@@ -111,11 +143,11 @@ fn name_conflicts<'tcx>(cx: &LateContext<'tcx>, body: &'tcx Body<'_>, fn_name: &
     let id = body.id().hir_id;
 
     // Iterate over items in the same module/scope
-    let (module, _module_span, _module_hir) = tcx.hir().get_module(tcx.parent_module(id));
+    let (module, _module_span, _module_hir) = tcx.hir_get_module(tcx.parent_module(id));
     for item in module.item_ids {
-        let item = tcx.hir().item(*item);
-        if let hir::ItemKind::Fn(..) = item.kind {
-            if item.ident.name.as_str() == fn_name {
+        let item = tcx.hir_item(*item);
+        if let hir::ItemKind::Fn { ident, .. } = item.kind {
+            if ident.name.as_str() == fn_name {
                 // Name conflict found
                 return true;
             }
@@ -141,3 +173,8 @@ fn name_conflicts<'tcx>(cx: &LateContext<'tcx>, body: &'tcx Body<'_>, fn_name: &
     })
     .is_some()
 }
+
+fn is_invalid_ident(ident: &str) -> bool {
+    // The identifier is either a reserved keyword, or starts with an invalid sequence.
+    Symbol::intern(ident).is_reserved(|| edition::LATEST_STABLE_EDITION) || !is_ident(ident)
+}

clippy_utils/src/lib.rs

@@ -2696,10 +2696,10 @@ pub fn is_test_function(tcx: TyCtxt<'_>, id: HirId) -> bool {
         let node = tcx.hir_node(id);
         once((id, node)).any(|(_id, node)| {
             if let Node::Item(item) = node {
-                if let ItemKind::Fn(_, _, _) = item.kind {
+                if let ItemKind::Fn { ident, .. } = item.kind {
                     // Note that we have sorted the item names in the visitor,
                     // so the binary_search gets the same as `contains`, but faster.
-                    return names.binary_search(&item.ident.name).is_ok();
+                    return names.binary_search(&ident.name).is_ok();
                 }
             }
             false

tests/ui-toml/redundant_test_prefix/custom_suffix/clippy.toml

@@ -1,55 +1,47 @@
-//@revisions: default outside_cfg_test custom_suffix
+//@revisions: default outside_cfg_test
 //@[default] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/redundant_test_prefix/default
 //@[outside_cfg_test] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/redundant_test_prefix/outside_cfg_test
-//@[custom_suffix] rustc-env:CLIPPY_CONF_DIR=tests/ui-toml/redundant_test_prefix/custom_suffix
 //@compile-flags: --test
 #![allow(dead_code)]
 #![warn(clippy::redundant_test_prefix)]
 
 fn main() {}
 
-mod tests_no_annotations {
+// Has no `#[cfg(test)]` annotation.
+mod tests_no_cfg_annotation {
     use super::*;
 
     #[test]
     fn test_has_annotation() {
         //~[outside_cfg_test]^ redundant_test_prefix
+        //~[outside_cfg_test]| HELP: consider removing the `test_` prefix
+        //~[outside_cfg_test]| HELP: to override `-D warnings`
     }
 
     fn no_annotation() {}
 }
 
-#[test]
-fn test_main_module_has_annotation() {
-    //~[outside_cfg_test]^ redundant_test_prefix
-}
-
-fn test_main_module_no_annotation() {}
-
-fn foo() {}
-
 #[cfg(test)]
-#[test]
-fn foo_works() {
-    //~^ ERROR: redundant `test_` prefix in test function name
-    //~| HELP: consider removing the `test_` prefix (suffix avoids name conflict)
+mod tests_has_cfg_annotation {
+    use super::*;
 
-    todo!()
-    // Has prefix, has `#[test]` attribute, within a `#[cfg(test)]`.
-    // Collision with existing function, so suffix is added.
-}
+    #[test]
+    fn has_annotation() {
+        //~^ redundant_test_prefix
+        //~| HELP: consider removing the `test_` prefix
+        //~[default]| HELP: to override `-D warnings`
+    }
 
-fn bar() {}
+    fn no_annotation() {}
+}
 
 #[test]
-fn test_bar() {
-    //~[custom_suffix]^ redundant_test_prefix
-
-    todo!()
-    // Has prefix, has `#[test]` attribute.
-    // NOT within a `#[cfg(test)]`, but the lint is enabled for integration tests.
-    // Collision with existing function, so suffix is added.
+fn test_main_module_has_annotation() {
+    //~[outside_cfg_test]^ redundant_test_prefix
+    //~[outside_cfg_test]| HELP: consider removing the `test_` prefix
 }
 
+fn test_main_module_no_annotation() {}
+
 #[cfg(test)]
 mod tests {}

tests/ui-toml/redundant_test_prefix/redundant_test_prefix.custom_suffix.fixed

@@ -1,8 +1,8 @@
 error: redundant `test_` prefix in test function name
-  --> tests/ui-toml/redundant_test_prefix/redundant_test_prefix.rs:33:4
+  --> tests/ui-toml/redundant_test_prefix/redundant_test_prefix.rs:29:8
    |
-LL | fn test_foo() {
-   |    ^^^^^^^^ help: consider removing the `test_` prefix (suffix avoids name conflict): `foo_works`
+LL |     fn test_has_annotation() {
+   |        ^^^^^^^^^^^^^^^^^^^ help: consider removing the `test_` prefix: `has_annotation`
    |
    = note: `-D clippy::redundant-test-prefix` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::redundant_test_prefix)]`