New lint: redundant_test_prefix

CHANGELOG.md

@@ -5907,6 +5907,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
@@ -6214,6 +6215,8 @@ Released 2018-09-13
 [`msrv`]: https://doc.rust-lang.org/clippy/lint_configuration.html#msrv
 [`pass-by-value-size-limit`]: https://doc.rust-lang.org/clippy/lint_configuration.html#pass-by-value-size-limit
 [`pub-underscore-fields-behavior`]: https://doc.rust-lang.org/clippy/lint_configuration.html#pub-underscore-fields-behavior
+[`redundant-test-prefix-check-outside-cfg-test`]: https://doc.rust-lang.org/clippy/lint_configuration.html#redundant-test-prefix-check-outside-cfg-test
+[`redundant-test-prefix-custom-suffix`]: https://doc.rust-lang.org/clippy/lint_configuration.html#redundant-test-prefix-custom-suffix
 [`semicolon-inside-block-ignore-singleline`]: https://doc.rust-lang.org/clippy/lint_configuration.html#semicolon-inside-block-ignore-singleline
 [`semicolon-outside-block-ignore-multiline`]: https://doc.rust-lang.org/clippy/lint_configuration.html#semicolon-outside-block-ignore-multiline
 [`single-char-binding-names-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#single-char-binding-names-threshold

book/src/lint_configuration.md

@@ -764,6 +764,34 @@ exported visibility, or whether they are marked as "pub".
 * [`pub_underscore_fields`](https://rust-lang.github.io/rust-clippy/master/index.html#pub_underscore_fields)
 
 
+## `redundant-test-prefix-check-outside-cfg-test`
+Whether to include functions outside of `#[cfg(test)]` in the linting process or not.
+
+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).
+
+**Default Value:** `false`
+
+---
+**Affected lints:**
+* [`redundant_test_prefix`](https://rust-lang.github.io/rust-clippy/master/index.html#redundant_test_prefix)
+
+
+## `redundant-test-prefix-custom-suffix`
+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).
+
+**Default Value:** `"_works"`
+
+---
+**Affected lints:**
+* [`redundant_test_prefix`](https://rust-lang.github.io/rust-clippy/master/index.html#redundant_test_prefix)
+
+
 ## `semicolon-inside-block-ignore-singleline`
 Whether to lint only if it's multiline.
 

clippy_config/src/conf.rs

@@ -633,6 +633,20 @@ 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.
+    ///
+    /// 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).
+    #[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_dev/src/update_lints.rs

@@ -963,7 +963,7 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_parse_contents() {
+    fn parse_contents_works() {
         static CONTENTS: &str = r#"
             declare_clippy_lint! {
                 #[clippy::version = "Hello Clippy!"]
@@ -1006,7 +1006,7 @@ mod tests {
     }
 
     #[test]
-    fn test_usable_lints() {
+    fn usable_lints_works() {
         let lints = vec![
             Lint::new(
                 "should_assert_eq2",
@@ -1041,7 +1041,7 @@ mod tests {
     }
 
     #[test]
-    fn test_by_lint_group() {
+    fn by_lint_group_works() {
         let lints = vec![
             Lint::new("should_assert_eq", "group1", "\"abc\"", "module_name", Range::default()),
             Lint::new(

clippy_lints/src/declared_lints.rs

@@ -642,6 +642,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/empty_with_brackets.rs

@@ -147,7 +147,7 @@ mod unit_test {
     use super::*;
 
     #[test]
-    fn test_has_no_ident_token() {
+    fn has_no_ident_token_works() {
         let input = "{ field: u8 }";
         assert!(!has_no_ident_token(input));
 

clippy_lints/src/lib.rs

@@ -313,6 +313,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;
@@ -963,5 +964,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(manual_ignore_case_cmp::ManualIgnoreCaseCmp));
     store.register_late_pass(|_| Box::new(unnecessary_literal_bound::UnnecessaryLiteralBound));
     store.register_late_pass(move |_| Box::new(arbitrary_source_item_ordering::ArbitrarySourceItemOrdering::new(conf)));
+    store.register_late_pass(move |_| Box::new(redundant_test_prefix::RedundantTestPrefix::new(conf)));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/needless_continue.rs

@@ -410,7 +410,7 @@ mod test {
 
     #[test]
     #[rustfmt::skip]
-    fn test_erode_from_back() {
+    fn erode_from_back_works() {
         let input = "\
 {
     let x = 5;
@@ -428,7 +428,7 @@ mod test {
 
     #[test]
     #[rustfmt::skip]
-    fn test_erode_from_back_no_brace() {
+    fn erode_from_back_no_brace() {
         let input = "\
 let x = 5;
 let y = something();

clippy_lints/src/redundant_test_prefix.rs

@@ -0,0 +1,143 @@
+use clippy_config::Conf;
+use clippy_utils::diagnostics::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_lint::{LateContext, LateLintPass};
+use rustc_session::impl_lint_pass;
+use rustc_span::Span;
+use rustc_span::def_id::LocalDefId;
+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
+    /// #[test]
+    /// fn test_use_case() {
+    ///     // test code
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// fn use_case() {
+    ///    // test code
+    /// }
+    /// ```
+    #[clippy::version = "1.84.0"]
+    pub REDUNDANT_TEST_PREFIX,
+    pedantic,
+    "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(),
+        }
+    }
+}
+
+impl_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 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_") {
+            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)";
+            }
+            span_lint_and_sugg(
+                cx,
+                REDUNDANT_TEST_PREFIX,
+                ident.span,
+                "redundant `test_` prefix in test function name",
+                help_msg,
+                non_prefixed,
+                Applicability::MachineApplicable,
+            );
+        }
+    }
+}
+
+/// 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(..) = item.kind {
+            if item.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::Call(path_expr, _) = expr.kind {
+            if let ExprKind::Path(qpath) = &path_expr.kind {
+                if let Some(def_id) = cx.qpath_res(qpath, path_expr.hir_id).opt_def_id() {
+                    if let Some(name) = tcx.opt_item_name(def_id) {
+                        if name.as_str() == fn_name {
+                            // Function call with the same name found
+                            return ControlFlow::Break(());
+                        }
+                    }
+                }
+            }
+        }
+        ControlFlow::Continue(())
+    })
+    .is_some()
+}

clippy_lints/src/tabs_in_doc_comments.rs

@@ -152,77 +152,77 @@ mod tests_for_get_chunks_of_tabs {
     use super::get_chunks_of_tabs;
 
     #[test]
-    fn test_unicode_han_string() {
+    fn unicode_han_string() {
         let res = get_chunks_of_tabs(" \u{4f4d}\t");
 
         assert_eq!(res, vec![(4, 5)]);
     }
 
     #[test]
-    fn test_empty_string() {
+    fn empty_string() {
         let res = get_chunks_of_tabs("");
 
         assert_eq!(res, vec![]);
     }
 
     #[test]
-    fn test_simple() {
+    fn simple() {
         let res = get_chunks_of_tabs("sd\t\t\taa");
 
         assert_eq!(res, vec![(2, 5)]);
     }
 
     #[test]
-    fn test_only_t() {
+    fn only_t() {
         let res = get_chunks_of_tabs("\t\t");
 
         assert_eq!(res, vec![(0, 2)]);
     }
 
     #[test]
-    fn test_only_one_t() {
+    fn only_one_t() {
         let res = get_chunks_of_tabs("\t");
 
         assert_eq!(res, vec![(0, 1)]);
     }
 
     #[test]
-    fn test_double() {
+    fn double() {
         let res = get_chunks_of_tabs("sd\tasd\t\taa");
 
         assert_eq!(res, vec![(2, 3), (6, 8)]);
     }
 
     #[test]
-    fn test_start() {
+    fn start() {
         let res = get_chunks_of_tabs("\t\taa");
 
         assert_eq!(res, vec![(0, 2)]);
     }
 
     #[test]
-    fn test_end() {
+    fn end() {
         let res = get_chunks_of_tabs("aa\t\t");
 
         assert_eq!(res, vec![(2, 4)]);
     }
 
     #[test]
-    fn test_start_single() {
+    fn start_single() {
         let res = get_chunks_of_tabs("\taa");
 
         assert_eq!(res, vec![(0, 1)]);
     }
 
     #[test]
-    fn test_end_single() {
+    fn end_single() {
         let res = get_chunks_of_tabs("aa\t");
 
         assert_eq!(res, vec![(2, 3)]);
     }
 
     #[test]
-    fn test_no_tabs() {
+    fn no_tabs() {
         let res = get_chunks_of_tabs("dsfs");
 
         assert_eq!(res, vec![]);