New lint: `redundant_test_prefix` #13710

farazdagi · 2024-11-19T21:27:09Z

This PR has started as an effort to proceed from the feedback in #12861.

Checks test functions 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 collision (either because function foo() is defined within the current scope, or because the foo() call exists within function thus creating an unwanted recursion), lint suggests replacing prefix with suffix, i.e. test_foo() becomes foo_works(). The _works suffix is configurable.
Integration tests do not have #[cfg(test)] defined for them, so by default they are not checked. There exists an option to make sure that any functions with #[test] annotation (including those outside of nodes marked with cfg(test)) are linted.

changelog: new lint: [redundant_test_prefix]

rustbot · 2024-11-19T21:27:15Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dswij (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

farazdagi · 2024-12-08T19:56:29Z

@dswij any ETA on when you'll be able to review (it has been more than 2 weeks -- so, I am not sure whether there needs to be some action on my part to get this rolling).

farazdagi · 2024-12-12T18:32:09Z

@J-ZhengLi @xFrednet sorry to bother you, but maybe you'll have time to review this PR (since you've provided good feedback on its previous incarnation -- and I've incorporated your suggestions).

Really eager to receive my first feedback on this one (still learning the clippy's tooling), before diving into any other open tasks/issues.

xFrednet · 2024-12-13T13:22:40Z

Hej, thank you for the ping. Sure, I'll add it to my reviewing list. You should get some feedback this weekend!

@J-ZhengLi You're welcome to also help with the review if you want. But also don't feel any pressure :)

r? @xFrednet

xFrednet

Very solid start. Thank you for picking up the other PR, it's nice to see it being worked on! :D

Please let me know if you have any questions.

xFrednet · 2024-12-15T19:49:51Z

clippy_config/src/conf.rs

+    /// 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).


Suggested change

/// 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).

/// Indicates if `redundant_test_prefix` should check functions outside of

/// items marked with `#[cfg(test)]`.

///

/// This option can be used for integration tests which also use the `#[test]`

/// attribute without the `#[cfg(test)]`.

xFrednet · 2024-12-15T20:00:14Z

clippy_config/src/conf.rs

+    /// 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"),


I don't think we should create machine applicable suggestions for these cases. Mostly because I don't see a good universal way. The default will look weird for tests that actually try to panic:

#[test] #[should_panic(expected = "explicit panic")] fn test_invalid_indexing() { panic!("explicit panic"); }

The suggested name invalid_indexing_works() doesn't make much sense. It feels like _test would be the most universal prefix, but removing the prefix to then have it as a suffix would be weird.

I'm guessing that collisions after the renaming are rare. And if they happen it should be fine to just emit a warning and expect the user to come up with a new name.

Yep, I've had the same feeling that it is not the most elegant approach. So, ignoring in auto-fix (but still warning about it) makes total sense.

xFrednet · 2024-12-15T20:03:25Z

clippy_lints/src/redundant_test_prefix.rs

+    /// ```
+    #[clippy::version = "1.84.0"]
+    pub REDUNDANT_TEST_PREFIX,
+    pedantic,


Copy of the discussion from the previous PR: https://github.com/rust-lang/rust-clippy/pull/12861/files#r1625940772

This should be decided on during the FCP

@xFrednet
I feel like this shouldn't be in pedantic but in restriction instead. The usage of the test_ prefix doesn't seem like an antipattern to me. There are good arguments against it, but the number of changes required in existing changes shows that the usage is still quite common.

I'll include this decision in the group discussion before we merge this PR :)

@Caylub
There are arguments to be made for several categories: style, pedantic, restriction, etc.

The usage of test_ seems to be pedantic to me, because it's clearly unnecessary in almost every case, including some of the ones mentioned in the threads in this PR. For example, test names that have test_ + method_name, should just be super::method_name IMO, following the same like of reasoning for as everywhere else.

@xFrednet
One argument I always consider is the number of lint triggers that happen in current code. Clippy can be a very pedantic, but the number of lint emissions has to be reasonable. If users get 100+ warnings of this lint with the next Clippy release it's way more likely that users get annoyed and simply allow the lint and that would help nobody.

This is not meant as a hard no, but just an explanation of my suggestion. I'm not sure where to place it and would leave this up to a group decision.

Have no strong opinion on this one (used pedantic just as a starting point). Can wait for the group discussion, or can proactively turn it to restriction, since potentially this can indeed result in 100s of warnings on big projects, and that will annoy ppl.

xFrednet · 2024-12-15T20:05:23Z

clippy_lints/src/redundant_test_prefix.rs

+        // Ignore methods and closures.
+        let FnKind::ItemFn(ref ident, ..) = kind else {
+            return;
+        };


We should also have a check if the name was written by a macro or user. It should be:

if ident.span.from_expansion() { return; }

xFrednet · 2024-12-15T20:09:15Z

clippy_lints/src/redundant_test_prefix.rs

+            span_lint_and_sugg(
+                cx,
+                REDUNDANT_TEST_PREFIX,
+                ident.span,
+                "redundant `test_` prefix in test function name",
+                help_msg,
+                non_prefixed,
+                Applicability::MachineApplicable,
+            );


For long help message I think verbose suggestions look better. Can you try this:

Suggested change

span_lint_and_sugg(

cx,

REDUNDANT_TEST_PREFIX,

ident.span,

"redundant `test_` prefix in test function name",

help_msg,

non_prefixed,

Applicability::MachineApplicable,

);

span_lint_and_then(

cx,

REDUNDANT_TEST_PREFIX,

ident.span,

"redundant `test_` prefix in test function name",

|diag| {

diag.span_suggestion_verbose(

ident.span,

help_msg,

non_prefixed,

Applicability::MachineApplicable

);

}

);

xFrednet · 2024-12-15T20:11:35Z

clippy_lints/src/redundant_test_prefix.rs

+                non_prefixed.push_str(&self.custom_sufix);
+                help_msg = "consider removing the `test_` prefix (suffix avoids name conflict)";


As mentioned at the place with the configuration, I think it would be better to just change the Applicability instead of adding a predefined suffix. We could still suggest renaming it to non_prefixed.push_str("_works") but have it as Applicability::HasPlaceholders

xFrednet · 2024-12-15T20:12:40Z

clippy_lints/src/redundant_test_prefix.rs

+/// 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 {


Nice check! :D

xFrednet · 2024-12-15T20:15:03Z

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

+//@revisions: default outside_cfg_test custom_suffix
+//@[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


Awesome that you figured out how to do this all with one test file 💪

xFrednet · 2024-12-15T20:22:55Z

clippy_lints/src/redundant_test_prefix.rs

+        }
+
+        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();


In theory, it can happen that the new string can no longer be an ident. For example, if it starts with an integer or is empty. IDK, if there is a simple way to check this 🤔

Examples would be test_1, test_const, test_.

It looks like the lexer is responsible for checking this in rustc, but it doesn't have an easily accessible util function. We should be able to reuse the lexer. (rn I just don't know how from the top of my head)

Nice catch! I will look into it, starting from sample case and investigate how we can resolve this.

xFrednet · 2024-12-15T20:24:23Z

tests/ui/redundant_test_prefix.rs

@@ -0,0 +1,211 @@
+#![allow(dead_code)]
+#![warn(clippy::redundant_test_prefix)]


Very nice comments, this makes the tests easy to read :D

farazdagi added 7 commits November 20, 2024 00:02

new lint: redundant_test_prefix basic help, no auto-fix suggestion

a37bdca

refactor: redundant_test_prefix configuration tests

de98412

refactor: redundant_test_prefix rustfix

c45551c

refactor: redundant_test_prefix make suffix configurable

c9cecf2

fix: redundant_test_prefix allow nested prefixed functions

e48de8a

refactor: redundant_test_prefix rename option

24aa783

fix: redundant_test_prefix dogfooding

eb06e26

rustbot assigned dswij Nov 19, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 19, 2024

farazdagi mentioned this pull request Nov 19, 2024

Lint to prevent redundant test_ prefix in test names. #12861

Closed

farazdagi marked this pull request as ready for review November 19, 2024 21:37

rustbot assigned xFrednet and unassigned dswij Dec 13, 2024

rust-lang deleted a comment from rustbot Dec 13, 2024

xFrednet reviewed Dec 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lint: `redundant_test_prefix` #13710

New lint: `redundant_test_prefix` #13710

farazdagi commented Nov 19, 2024 •

edited

Loading

rustbot commented Nov 19, 2024

farazdagi commented Dec 8, 2024

farazdagi commented Dec 12, 2024

xFrednet commented Dec 13, 2024 •

edited

Loading

xFrednet left a comment

xFrednet Dec 15, 2024

xFrednet Dec 15, 2024

farazdagi Dec 17, 2024

xFrednet Dec 15, 2024

farazdagi Dec 17, 2024

xFrednet Dec 15, 2024

xFrednet Dec 15, 2024

xFrednet Dec 15, 2024

xFrednet Dec 15, 2024

xFrednet Dec 15, 2024

xFrednet Dec 15, 2024

farazdagi Dec 17, 2024

xFrednet Dec 15, 2024

		non_prefixed.push_str(&self.custom_sufix);
		help_msg = "consider removing the `test_` prefix (suffix avoids name conflict)";

		@@ -0,0 +1,211 @@
		#![allow(dead_code)]
		#![warn(clippy::redundant_test_prefix)]

New lint: redundant_test_prefix #13710

Are you sure you want to change the base?

New lint: redundant_test_prefix #13710

Conversation

farazdagi commented Nov 19, 2024 • edited Loading

rustbot commented Nov 19, 2024

farazdagi commented Dec 8, 2024

farazdagi commented Dec 12, 2024

xFrednet commented Dec 13, 2024 • edited Loading

xFrednet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

New lint: `redundant_test_prefix` #13710

New lint: `redundant_test_prefix` #13710

farazdagi commented Nov 19, 2024 •

edited

Loading

xFrednet commented Dec 13, 2024 •

edited

Loading