add a new lint for `repeat().take()` that can be replaced with `repeat_n()` #13858

lapla-cogito · 2024-12-20T13:34:54Z

changelog: [redundant_repeat_take]: new lint

rustbot · 2024-12-20T13:35:00Z

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

lapla-cogito · 2024-12-20T13:57:21Z

clippy_utils/src/lib.rs

@@ -15,6 +15,7 @@
    clippy::missing_errors_doc,
    clippy::missing_panics_doc,
    clippy::must_use_candidate,
+    clippy::redundant_repeat_take,


I’m unsure whether I should add an allow rule like this, or if I should fix the part of the code that triggers the linting error, since currently there is only one part that is affected by this lint...

This lint looks useful to have to me so I think keeping it enabled is a good idea. The case also looks like it should be fairly easy to fix

y21 · 2024-12-20T14:14:48Z

clippy_lints/src/methods/mod.rs

+    ///
+    /// ### Why is this bad?
+    ///
+    /// Using `repeat_n()` is more concise and clearer.


May also be worth pointing out that it can be faster for types that are non-trivial to clone as it knows the repeat count and only needs to clone n-1 times

y21 · 2024-12-20T14:16:43Z

clippy_utils/src/lib.rs

@@ -15,6 +15,7 @@
    clippy::missing_errors_doc,
    clippy::missing_panics_doc,
    clippy::must_use_candidate,
+    clippy::redundant_repeat_take,


This lint looks useful to have to me so I think keeping it enabled is a good idea. The case also looks like it should be fairly easy to fix

y21 · 2024-12-20T14:17:54Z

clippy_lints/src/methods/redundant_repeat_take.rs

+        && let QPath::Resolved(_, path) = qpath
+        && path.segments.len() == 1
+        && path.segments[0].ident.name.as_str() == "repeat"
+    {


This should have an MSRV check as repeat_n was only very recently stabilized

y21 · 2024-12-20T14:19:30Z

clippy_lints/src/methods/redundant_repeat_take.rs

+        && let ExprKind::Call(inner, repeat_args) = repeat_expr.kind
+        && repeat_args.len() == 1


nit:

Suggested change

&& let ExprKind::Call(inner, repeat_args) = repeat_expr.kind

&& repeat_args.len() == 1

&& let ExprKind::Call(inner, [repeat_arg]) = repeat_expr.kind

y21 · 2024-12-20T14:23:59Z

clippy_lints/src/methods/redundant_repeat_take.rs

+        && let ExprKind::Path(ref qpath) = inner.kind
+        && let QPath::Resolved(_, path) = qpath
+        && path.segments.len() == 1
+        && path.segments[0].ident.name.as_str() == "repeat"


iter::repeat is a diagnostic item, so you should be able to match against its DefId rather than looking for exact paths. Something like

&& let Some(def_id) = fn_def_id(inner) && cx.tcx.is_diagnostic_item(sym::iter_repeat, def_id)

There should also be a check that the .take() call is the take from the Iterator trait and not from a user defined trait, with is_trait_method(cx, expr, sym::Iterator)

y21 · 2024-12-20T14:26:52Z

tests/ui/manual_str_repeat.rs

@@ -1,4 +1,4 @@
-#![allow(non_local_definitions)]
+#![allow(non_local_definitions, redundant_repeat_take)]


Should be clippy::redundant_repeat_take. The test file also has errors because of that

clippy_lints/src/methods/mod.rs

y21 · 2024-12-20T14:29:09Z

clippy_lints/src/methods/redundant_repeat_take.rs

+                snippet(cx, repeat_args[0].span, ""),
+                snippet(cx, take_arg.span, "")


Would be good to have a test case where the element is a macro call, e.g. repeat(vec![1, 2]).take(2). I suspect this currently "doesn't work" (has macro expansion in the suggestion), so this may need to use snippet_with_context (with expr.span.ctxt() for the ctxt), but I have not tested it

y21 · 2024-12-20T14:49:28Z

clippy_lints/src/methods/redundant_repeat_take.rs

+                snippet(cx, repeat_args[0].span, ""),
+                snippet(cx, take_arg.span, "")
+            ),
+            Applicability::MachineApplicable,


FWIW, this changes the expression's type so it can lead to compile errors in some cases where its type is unified with other branches, e.g.:

// From an external library the user doesn't get to control mod external_lib { pub fn iter() -> std::iter::Take<std::iter::Repeat<&'static [u8]>> { todo!() } } let iter = match 1 { 1 => external_lib::iter(), 2 => std::iter::repeat([1,2].as_slice()).take(2) // can't really change this to repeat_n };

so in those situations MachineApplicable could lead to issues. There have been a few false positive reports of the like in the past (#11761 comes to mind).

Adding !expr_use_ctxt(cx, expr).is_ty_unified to the chain could help avoid this I think. (edited, was thinking of the wrong util function before)

lapla-cogito · 2024-12-20T16:36:49Z

@y21 Thank you for your thorough review! Looks like I was able to fix the reviewed items. Could you please take a look again?

lapla-cogito

I noticed this lint should suggest to use core::iter::repeat_n in a no_std environment. Therefore, I'll make a change.

rustbot assigned dswij Dec 20, 2024

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

lapla-cogito force-pushed the repeatn branch 2 times, most recently from 55203fc to 33a5b21 Compare December 20, 2024 13:45

lapla-cogito commented Dec 20, 2024

View reviewed changes

y21 reviewed Dec 20, 2024

View reviewed changes

lapla-cogito force-pushed the repeatn branch 2 times, most recently from f3ac7b1 to 8e422ea Compare December 20, 2024 15:52

lapla-cogito changed the title ~~add a lint for redundant repeat().take()~~ add a new lint for repeat().take() that can be replaced with repeat_n() Dec 20, 2024

add a lint for repeat().take()

7290ea6

lapla-cogito force-pushed the repeatn branch from 8e422ea to 4bcd4a5 Compare December 20, 2024 16:18

use repeat_n() where available

87082a2

lapla-cogito force-pushed the repeatn branch from 4bcd4a5 to 87082a2 Compare December 20, 2024 16:19

add more tests for manual_repeat_n

672d7c0

lapla-cogito force-pushed the repeatn branch from 546bbf1 to 672d7c0 Compare December 20, 2024 16:25

lapla-cogito commented Dec 20, 2024

View reviewed changes

lapla-cogito added 2 commits December 21, 2024 01:48

suggest to use core::iter::repeat_n in a no_std environment

2208d8a

allow manual_repeat_n in other test

0667d78

lapla-cogito requested a review from y21 December 20, 2024 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a new lint for `repeat().take()` that can be replaced with `repeat_n()` #13858

add a new lint for `repeat().take()` that can be replaced with `repeat_n()` #13858

lapla-cogito commented Dec 20, 2024

rustbot commented Dec 20, 2024

lapla-cogito Dec 20, 2024

y21 Dec 20, 2024

y21 Dec 20, 2024

y21 Dec 20, 2024

y21 Dec 20, 2024

y21 Dec 20, 2024

y21 Dec 20, 2024

y21 Dec 20, 2024

y21 Dec 20, 2024

y21 Dec 20, 2024

y21 Dec 20, 2024 •

edited

Loading

lapla-cogito commented Dec 20, 2024

lapla-cogito left a comment •

edited

Loading

		&& let ExprKind::Call(inner, repeat_args) = repeat_expr.kind
		&& repeat_args.len() == 1

	&& let ExprKind::Call(inner, repeat_args) = repeat_expr.kind
	&& repeat_args.len() == 1
	&& let ExprKind::Call(inner, [repeat_arg]) = repeat_expr.kind

		@@ -1,4 +1,4 @@
		#![allow(non_local_definitions)]
		#![allow(non_local_definitions, redundant_repeat_take)]

		snippet(cx, repeat_args[0].span, ""),
		snippet(cx, take_arg.span, "")

add a new lint for repeat().take() that can be replaced with repeat_n() #13858

Are you sure you want to change the base?

add a new lint for repeat().take() that can be replaced with repeat_n() #13858

Conversation

lapla-cogito commented Dec 20, 2024

rustbot commented Dec 20, 2024

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

y21 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

lapla-cogito commented Dec 20, 2024

lapla-cogito left a comment • edited Loading

Choose a reason for hiding this comment

add a new lint for `repeat().take()` that can be replaced with `repeat_n()` #13858

add a new lint for `repeat().take()` that can be replaced with `repeat_n()` #13858

y21 Dec 20, 2024 •

edited

Loading

lapla-cogito left a comment •

edited

Loading