Lint against String.repeat(1) #3060

einashaddad · 2018-08-17T18:49:53Z

No description provided.

mati865 · 2018-08-17T19:06:25Z

clippy_lints/src/strings.rs

+
+/// **What it does:** Checks for usage of `.repeat(1)` since its equivalent to clone()
+///
+/// **Why is this bad?** It's more readable.


I'd say it's the opposite.

mati865 · 2018-08-17T19:10:45Z

clippy_lints/src/strings.rs

+declare_clippy_lint! {
+    pub STRING_REPEAT_ONCE,
+    pedantic,
+    " using `String.repeat(1)` instead of `String.clone()`"


Remove white space preceding using.

flip1995

Thanks for your contribution and welcome to Clippy! I commented on mostly small things that you could improve.

flip1995 · 2018-08-17T23:37:00Z

tests/ui/strings.rs

+    let a = "this shouldn't work".repeat(1);
+    let b = String::from("neither should this");
+    let c = b.repeat(1);
+    let d = "this works".repeat(2);


What happens if you use a constant as the argument of repeat:

const N: u32 = 1; b.repeat(N);

flip1995 · 2018-08-17T23:39:30Z

clippy_lints/src/strings.rs

+        use syntax::ast::LitKind;
+        use crate::utils::{in_macro, snippet};
+
+        if let ExprKind::MethodCall(ref path, _, ref args) = e.node {


NIT: you can use the if_chain!() macro here to avoid the nested if syntax

flip1995 · 2018-08-17T23:40:47Z

clippy_lints/src/strings.rs

+/// **What it does:** Checks for usage of `.repeat(1)` since its equivalent to clone()
+///
+/// **Why is this bad?** `.repeat(1)` is not very readable.
+///


**Known problems:** None.

flip1995 · 2018-08-17T23:44:03Z

clippy_lints/src/strings.rs

@@ -169,3 +169,61 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringLitAsBytes {
        }
    }
 }
+
+/// **What it does:** Checks for usage of `.repeat(1)` since its equivalent to clone()


Missing backticks around .clone()

Maybe add something like "while clone represents the intention better."

Please also include #3060 (comment) in the lint documentation.

flip1995 · 2018-08-17T23:47:29Z

clippy_lints/src/strings.rs

+
+/// **What it does:** Checks for usage of `.repeat(1)` since its equivalent to clone()
+///
+/// **Why is this bad?** `.repeat(1)` is not very readable.


Here it might also be better to talk about the hidden intention behind the repeat(1) call.

"Isn't very readable" is a very vague argument for a lint.

flip1995 · 2018-08-17T23:50:22Z

clippy_lints/src/lib.rs

@@ -303,6 +303,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
    reg.register_late_lint_pass(box block_in_if_condition::BlockInIfCondition);
    reg.register_late_lint_pass(box unicode::Unicode);
    reg.register_late_lint_pass(box strings::StringAdd);
+    reg.register_late_lint_pass(box strings::StringRepeatOnce);


You also have to add the Lint to the right point group. Running the util/update_lints.py script should do this for you!

flip1995 · 2018-08-17T23:55:22Z

clippy_lints/src/strings.rs

+/// ```
+declare_clippy_lint! {
+    pub STRING_REPEAT_ONCE,
+    pedantic,


I would classify this as a complexity lint. If too many people complain about this lint (which I don't think) we can move this to pedantic later.

matthiaskrgr · 2018-08-18T21:14:29Z

Should we perhaps limit this to a few known std types that implement repeat()?

When I make my own type and impl repeat(), we can't really know that repeat(1) isn't totally legit in this case and maybe don't want to warn. Thoughts?

struct Hello {
	x: i32,
}

impl Hello {
	fn repeat(&mut self, number: i32) {
		self.x +=number * 3;
	}
}

fn main() {
    let mut a = Hello { x: 7 }; 
    a.repeat(1);
    println!("a: {}", a.x);
}

If I tested this correctly I also got a warning for this case.

flip1995 · 2018-08-20T08:19:12Z

@matthiaskrgr Yes good point! Searching in the doc of the std library there are 3 types where it makes sense:

str: here repeat(1) creates a String. The suggestion should be to use .to_string() instead. This would be a case of new lint for &str to String conversion #2824
String: we can suggest .clone() here
slice: we can suggest to_vec() here. (This is feature gated, but easy to implement, if the other two cases are covered)

This should cover all the cases: Playground

Did I miss something?

@einashaddad You practically just have to add a match over the type of args[0] and adapt the suggestion. For the type String there is already a method is_string you can use:
https://github.com/rust-lang-nursery/rust-clippy/blob/f05a1038b59cd4217e58b3aef7a0751a0efd01e4/clippy_lints/src/strings.rs#L120-L122

einashaddad · 2018-08-22T16:03:32Z

Thanks for the review @flip1995, @matthiaskrgr & @mati865. Sounds good, I'll address.

flip1995 · 2018-09-06T10:21:57Z

ping @einashaddad
Are you still working on this? Do you have any questions we can help you with?

flip1995 · 2018-12-13T13:49:44Z

Ping from triage @einashaddad. Do you want to continue working on this?

flip1995 · 2018-12-27T16:17:06Z

Ping from triage @einashaddad: It looks like this PR hasn't received any updates in a while, so I'm closing it per our new guidelines. Thank you for your contributions and please feel free to re-open in the future.

HKalbasi · 2021-07-29T20:47:06Z

finished in #5773
@rustbot label -S-inactive-closed

HKalbasi · 2021-07-29T21:04:48Z

@rustbot label -S-inactive-closed

einashaddad mentioned this pull request Aug 17, 2018

lint against String.repeat(1) #3028

Closed

mati865 reviewed Aug 17, 2018

View reviewed changes

einashaddad force-pushed the eh-fix branch from 7c4ebde to 450e15e Compare August 17, 2018 19:16

Lint against String.repeat(1)

24e2426

einashaddad force-pushed the eh-fix branch from 450e15e to 24e2426 Compare August 17, 2018 19:34

flip1995 requested changes Aug 17, 2018

View reviewed changes

flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Sep 3, 2018

flip1995 closed this Dec 27, 2018

flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 27, 2018

flip1995 removed the S-inactive-closed Status: Closed due to inactivity label Jul 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint against String.repeat(1) #3060

Lint against String.repeat(1) #3060

einashaddad commented Aug 17, 2018

mati865 Aug 17, 2018

mati865 Aug 17, 2018

flip1995 left a comment

flip1995 Aug 17, 2018

flip1995 Aug 17, 2018

flip1995 Aug 17, 2018

flip1995 Aug 17, 2018

flip1995 Aug 20, 2018

flip1995 Aug 17, 2018

flip1995 Aug 17, 2018

flip1995 Aug 17, 2018

matthiaskrgr commented Aug 18, 2018

flip1995 commented Aug 20, 2018

einashaddad commented Aug 22, 2018

flip1995 commented Sep 6, 2018

flip1995 commented Dec 13, 2018

flip1995 commented Dec 27, 2018

HKalbasi commented Jul 29, 2021

HKalbasi commented Jul 29, 2021

Lint against String.repeat(1) #3060

Lint against String.repeat(1) #3060

Conversation

einashaddad commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 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

matthiaskrgr commented Aug 18, 2018

flip1995 commented Aug 20, 2018

einashaddad commented Aug 22, 2018

flip1995 commented Sep 6, 2018

flip1995 commented Dec 13, 2018

flip1995 commented Dec 27, 2018

HKalbasi commented Jul 29, 2021

HKalbasi commented Jul 29, 2021