New lint: redundant_test_prefix (rust-lang#13710)

This PR has started as an effort to proceed from the feedback in
rust-lang/rust-clippy#12861.

- Checks test functions (functions marked with `#[test]` annotation) for
redundant "test_" prefix.
- Auto-fix is supported (and handles collisions gracefully, see below).
- If removing "test_" prefix from, say, `test_foo()` results in a name
collision (either because function `foo()` is already defined within the
current scope, or because the `foo()` call exists within function --
thus creating an unwanted recursion), lint suggests function rename,
warning the user that a simple trimming of `test_` prefix will result in
a name collision.
- If removing "test_" prefix results in invalid identifier (consider
`test_const`, `test_`, `test_42`), then again no auto-fix is suggested,
user is asked to rename function, with a note that a simple prefix
trimming will result in an invalid function name.
(`Applicability::HasPlaceholders` is used and user is suggested to: drop
`test_` prefix + add `_works` suffix, i.e. `test_foo` becomes
`foo_works` -- but again, user has to apply those changes manually).
- If trimmed version of the function name is a valid identifier, doesn't
result in name collision or unwanted recursion, then user is able to run
auto-fix.

fixes rust-lang/rust-clippy#8931

changelog: new lint: [`redundant_test_prefix`]

Author: samueltardieu

CHANGELOG.md

@@ -6116,6 +6116,7 @@ Released 2018-09-13
 [`redundant_pub_crate`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pub_crate
 [`redundant_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_slicing
 [`redundant_static_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes
+[`redundant_test_prefix`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_test_prefix
 [`redundant_type_annotations`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_type_annotations
 [`ref_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_as_ptr
 [`ref_binding_to_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_binding_to_reference

clippy_lints/src/declared_lints.rs

@@ -667,6 +667,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::redundant_slicing::DEREF_BY_SLICING_INFO,
     crate::redundant_slicing::REDUNDANT_SLICING_INFO,
     crate::redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES_INFO,
+    crate::redundant_test_prefix::REDUNDANT_TEST_PREFIX_INFO,
     crate::redundant_type_annotations::REDUNDANT_TYPE_ANNOTATIONS_INFO,
     crate::ref_option_ref::REF_OPTION_REF_INFO,
     crate::ref_patterns::REF_PATTERNS_INFO,

clippy_lints/src/lib.rs

@@ -320,6 +320,7 @@ mod redundant_locals;
 mod redundant_pub_crate;
 mod redundant_slicing;
 mod redundant_static_lifetimes;
+mod redundant_test_prefix;
 mod redundant_type_annotations;
 mod ref_option_ref;
 mod ref_patterns;
@@ -984,5 +985,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(move |_| Box::new(non_std_lazy_statics::NonStdLazyStatic::new(conf)));
     store.register_late_pass(|_| Box::new(manual_option_as_slice::ManualOptionAsSlice::new(conf)));
     store.register_late_pass(|_| Box::new(single_option_map::SingleOptionMap));
+    store.register_late_pass(move |_| Box::new(redundant_test_prefix::RedundantTestPrefix));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/redundant_test_prefix.rs

@@ -0,0 +1,161 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::is_test_function;
+use clippy_utils::visitors::for_each_expr;
+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::declare_lint_pass;
+use rustc_span::def_id::LocalDefId;
+use rustc_span::{Span, Symbol, edition};
+use std::borrow::Cow;
+use std::ops::ControlFlow;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for test functions (functions annotated with `#[test]`) that are prefixed
+    /// with `test_` which is redundant.
+    ///
+    /// ### Why is this bad?
+    /// This is redundant because test functions are already annotated with `#[test]`.
+    /// Moreover, it clutters the output of `cargo test` since test functions are expanded as
+    /// `module::tests::test_use_case` in the output. Without the redundant prefix, the output
+    /// becomes `module::tests::use_case`, which is more readable.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// #[cfg(test)]
+    /// mod tests {
+    ///   use super::*;
+    ///
+    ///   #[test]
+    ///   fn test_use_case() {
+    ///       // test code
+    ///   }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// #[cfg(test)]
+    /// mod tests {
+    ///   use super::*;
+    ///
+    ///   #[test]
+    ///   fn use_case() {
+    ///       // test code
+    ///   }
+    /// }
+    /// ```
+    #[clippy::version = "1.88.0"]
+    pub REDUNDANT_TEST_PREFIX,
+    restriction,
+    "redundant `test_` prefix in test function name"
+}
+
+declare_lint_pass!(RedundantTestPrefix => [REDUNDANT_TEST_PREFIX]);
+
+impl<'tcx> LateLintPass<'tcx> for RedundantTestPrefix {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        kind: FnKind<'_>,
+        _decl: &FnDecl<'_>,
+        body: &'tcx Body<'_>,
+        _span: Span,
+        fn_def_id: LocalDefId,
+    ) {
+        // Ignore methods and closures.
+        let FnKind::ItemFn(ref ident, ..) = kind else {
+            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;
+        }
+
+        // If the function is not a test function, skip the lint.
+        if !is_test_function(cx.tcx, fn_def_id) {
+            return;
+        }
+
+        span_lint_and_then(
+            cx,
+            REDUNDANT_TEST_PREFIX,
+            ident.span,
+            "redundant `test_` prefix in test function name",
+            |diag| {
+                let non_prefixed = Symbol::intern(ident.as_str().trim_start_matches("test_"));
+                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.
+                    diag.help(
+                        "consider function renaming (just removing `test_` prefix will produce invalid function name)",
+                    );
+                } else {
+                    let (sugg, msg): (Cow<'_, str>, _) = 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.
+                        (
+                            format!("{non_prefixed}_works").into(),
+                            "consider function renaming (just removing `test_` prefix will cause a name conflict)",
+                        )
+                    } else {
+                        // If `non_prefixed` is a valid identifier and does not conflict with another function,
+                        // so we can suggest an auto-fix.
+                        (non_prefixed.as_str().into(), "consider removing the `test_` prefix")
+                    };
+                    diag.span_suggestion(ident.span, msg, sugg, Applicability::MaybeIncorrect);
+                }
+            },
+        );
+    }
+}
+
+/// Checks whether removal of the `_test` prefix from the function name will cause a name conflict.
+///
+/// There should be no other function with the same name in the same module/scope. Also, there
+/// should not be any function call with the same name within the body of the function, to avoid
+/// recursion.
+fn name_conflicts<'tcx>(cx: &LateContext<'tcx>, body: &'tcx Body<'_>, fn_name: Symbol) -> bool {
+    let tcx = cx.tcx;
+    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));
+    if module
+        .item_ids
+        .iter()
+        .any(|item| matches!(tcx.hir_item(*item).kind, hir::ItemKind::Fn { ident, .. } if ident.name == fn_name))
+    {
+        // Name conflict found
+        return true;
+    }
+
+    // Also check that within the body of the function there is also no function call
+    // with the same name (since it will result in recursion)
+    for_each_expr(cx, body, |expr| {
+        if let ExprKind::Path(qpath) = &expr.kind
+            && let Some(def_id) = cx.qpath_res(qpath, expr.hir_id).opt_def_id()
+            && let Some(name) = tcx.opt_item_name(def_id)
+            && name == fn_name
+        {
+            // Function call with the same name found
+            ControlFlow::Break(())
+        } else {
+            ControlFlow::Continue(())
+        }
+    })
+    .is_some()
+}
+
+fn is_invalid_ident(ident: Symbol) -> bool {
+    // The identifier is either a reserved keyword, or starts with an invalid sequence.
+    ident.is_reserved(|| edition::LATEST_STABLE_EDITION) || !is_ident(ident.as_str())
+}

clippy_utils/src/lib.rs

@@ -2641,7 +2641,9 @@ pub fn is_hir_ty_cfg_dependant(cx: &LateContext<'_>, ty: &hir::Ty<'_>) -> bool {
 
 static TEST_ITEM_NAMES_CACHE: OnceLock<Mutex<FxHashMap<LocalModDefId, Vec<Symbol>>>> = OnceLock::new();
 
-fn with_test_item_names(tcx: TyCtxt<'_>, module: LocalModDefId, f: impl Fn(&[Symbol]) -> bool) -> bool {
+/// Apply `f()` to the set of test item names.
+/// The names are sorted using the default `Symbol` ordering.
+fn with_test_item_names(tcx: TyCtxt<'_>, module: LocalModDefId, f: impl FnOnce(&[Symbol]) -> bool) -> bool {
     let cache = TEST_ITEM_NAMES_CACHE.get_or_init(|| Mutex::new(FxHashMap::default()));
     let mut map: MutexGuard<'_, FxHashMap<LocalModDefId, Vec<Symbol>>> = cache.lock().unwrap();
     let value = map.entry(module);
@@ -2695,6 +2697,25 @@ pub fn is_in_test_function(tcx: TyCtxt<'_>, id: HirId) -> bool {
     })
 }
 
+/// Checks if `fn_def_id` has a `#[test]` attribute applied
+///
+/// This only checks directly applied attributes. To see if a node has a parent function marked with
+/// `#[test]` use [`is_in_test_function`].
+///
+/// Note: Add `//@compile-flags: --test` to UI tests with a `#[test]` function
+pub fn is_test_function(tcx: TyCtxt<'_>, fn_def_id: LocalDefId) -> bool {
+    let id = tcx.local_def_id_to_hir_id(fn_def_id);
+    if let Node::Item(item) = tcx.hir_node(id)
+        && let ItemKind::Fn { ident, .. } = item.kind
+    {
+        with_test_item_names(tcx, tcx.parent_module(id), |names| {
+            names.binary_search(&ident.name).is_ok()
+        })
+    } else {
+        false
+    }
+}
+
 /// Checks if `id` has a `#[cfg(test)]` attribute applied
 ///
 /// This only checks directly applied attributes, to see if a node is inside a `#[cfg(test)]` parent

tests/ui/redundant_test_prefix.fixed

@@ -0,0 +1,158 @@
+#![allow(dead_code)]
+#![warn(clippy::redundant_test_prefix)]
+
+fn main() {
+    // Normal function, no redundant prefix.
+}
+
+fn f1() {
+    // Normal function, no redundant prefix.
+}
+
+fn test_f2() {
+    // Has prefix, but no `#[test]` attribute, ignore.
+}
+
+#[test]
+fn f3() {
+    //~^ redundant_test_prefix
+
+    // Has prefix, has `#[test]` attribute. Not within a `#[cfg(test)]`.
+    // No collision with other functions, should emit warning.
+}
+
+#[cfg(test)]
+#[test]
+fn f4() {
+    //~^ redundant_test_prefix
+
+    // Has prefix, has `#[test]` attribute, within a `#[cfg(test)]`.
+    // No collision with other functions, should emit warning.
+}
+
+mod m1 {
+    pub fn f5() {}
+}
+
+#[cfg(test)]
+#[test]
+fn f6() {
+    //~^ redundant_test_prefix
+
+    use m1::f5;
+
+    f5();
+    // Has prefix, has `#[test]` attribute, within a `#[cfg(test)]`.
+    // No collision, has function call, but it will not result in recursion.
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn foo() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn foo_with_call() {
+        //~^ redundant_test_prefix
+
+        main();
+    }
+
+    #[test]
+    fn f1() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn f2() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn f3() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn f4() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn f5() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn f6() {
+        //~^ redundant_test_prefix
+    }
+}
+
+mod tests_no_annotations {
+    use super::*;
+
+    #[test]
+    fn foo() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn foo_with_call() {
+        //~^ redundant_test_prefix
+
+        main();
+    }
+
+    #[test]
+    fn f1() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn f2() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn f3() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn f4() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn f5() {
+        //~^ redundant_test_prefix
+    }
+
+    #[test]
+    fn f6() {
+        //~^ redundant_test_prefix
+    }
+}
+
+// This test is inspired by real test in `clippy_utils/src/sugg.rs`.
+// The `is_in_test_function()` checks whether any identifier within a given node's parents is
+// marked with `#[test]` attribute. Thus flagging false positives when nested functions are
+// prefixed with `test_`. Therefore `is_test_function()` has been defined in `clippy_utils`,
+// allowing to select only functions that are immediately marked with `#[test]` annotation.
+//
+// This test case ensures that for such nested functions no error is emitted.
+#[test]
+fn not_op() {
+    fn test_not(foo: bool) {
+        assert!(foo);
+    }
+
+    // Use helper function
+    test_not(true);
+    test_not(false);
+}