Performance: Suggest String::new() over "".to_string()

[phansch]

From rust-lang/rust#52805 (comment). String::new() is faster than ""::to_string()

Benchmark:

#[bench]
fn bench_string_new(b: &mut Bencher) {
    b.iter(|| String::new());
}

#[bench]
fn bench_to_string(b: &mut Bencher) {
    b.iter(|| "".to_string());
}

running 2 tests
test tests::bench_string_new ... bench:           0 ns/iter (+/- 0)
test tests::bench_to_string  ... bench:           8 ns/iter (+/- 0)

[matthiaskrgr]

The lint should probably also catch String::from("")

test tests::bench_string_from ... bench:          11 ns/iter (+/- 0)
test tests::bench_string_new  ... bench:           0 ns/iter (+/- 0)
test tests::bench_to_string   ... bench:          11 ns/iter (+/- 2)

[H2CO3]

as well as "".into::<String>()

[matthiaskrgr]

Hm, interestingly, when looking in godbolt print!("{}", String::new()); seems to generate more instructions than print!("{}", "");

[flip1995]

Probably because String::new() is of type String and "" is of type &str

[ghost]

I vote for slow_empty_string_creation as the name.

The suggestions of useless_format should to be changed as well.

warning: useless use of `format!`
 --> src/main.rs:2:13
  |
2 |     let _ = format!("");
  |             ^^^^^^^^^^^ help: consider using .to_string(): `"".to_string()`
  |
  = note: #[warn(useless_format)] on by default

Should this lint be written as PreExpansionPass?

[nagisa]

We should just fix this in liballoc instead.

[phansch]

That seems like a preferable solution. If the allocation is fixed, this could still be a style lint.

[weiwei]

For people who aren't aware: It appears that the issue has been fixed. This is my test result on an Intel Mac:

test tests::bench_string_from  ... bench:           0 ns/iter (+/- 0)
test tests::bench_string_new   ... bench:           0 ns/iter (+/- 0)
test tests::bench_to_string    ... bench:           0 ns/iter (+/- 0)

[xFrednet]

Thank you for the benchmarks! Even if this no longer affects performance, I think having a lint that suggest String::new() over "".to_string() would be good for code consistency. It should probably be a lint in the pedantic category to be Allow-by-default.

[H2CO3]

String::new() over "".to_string() or String::from("") should even be a warn-by-default style lint. It's not pedantic by any measure – the alternatives are unnecessarily complicated.

[xFrednet]

I was considering if this should be a style lint, but I've also seen repos that preferred using "".to_string(). Therefore, I'm unsure if we really want this to be warn-by-default. The best way to determine this is probably to create an implementation and then check how often it triggers in normal projects with our lintcheck tool

[H2CO3]

I've also seen repos that preferred using "".to_string()

I'd be curious what their rationale is. It's less clear, up until recently it was slower, and it's one character longer (even if that's probably an insignificant difference).

I regularly have my code "broken" by new Clippy lints being introduced that I have to admit are correct, and then I have to fix them. The purpose of linters is to guide people to write better code; I think adjusting the linter to the questionable but maybe somewhat common choices of style is backwards. If one disagrees with the lint, it can still be turned off, so there's no real risk in warning by default.

[Guilherme-Vasconcelos]

@rustbot claim