new lint: redundant_test_prefix

Author: farazdagi

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

@@ -668,6 +668,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,168 @@
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
+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::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 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();
+
+            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::MaybeIncorrect,
+                );
+            } 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::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: &str) -> 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));
+    for item in module.item_ids {
+        let item = tcx.hir_item(*item);
+        if let hir::ItemKind::Fn { ident, .. } = item.kind
+            && ident.name.as_str() == 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.as_str() == fn_name
+        {
+            // Function call with the same name found
+            return ControlFlow::Break(());
+        }
+
+        ControlFlow::Continue(())
+    })
+    .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

@@ -2695,6 +2695,26 @@ pub fn is_in_test_function(tcx: TyCtxt<'_>, id: HirId) -> bool {
     })
 }
 
+/// Checks if `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<'_>, id: HirId) -> bool {
+    with_test_item_names(tcx, tcx.parent_module(id), |names| {
+        if let Node::Item(item) = tcx.hir_node(id)
+            && 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.
+            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);
+}