Skip to content

Warn on push_str(&format!()) #6261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kangalio opened this issue Oct 28, 2020 · 3 comments · Fixed by #8626
Closed

Warn on push_str(&format!()) #6261

kangalio opened this issue Oct 28, 2020 · 3 comments · Fixed by #8626
Assignees
Labels
A-lint Area: New lints good first issue These issues are a good way to get started with Clippy L-perf Lint: Belongs in the perf lint group T-middle Type: Probably requires verifiying types

Comments

@kangalio
Copy link

kangalio commented Oct 28, 2020

What it does

Emits a warning on code like ```rust
some_string.push_str(&format!(...));
// or
some_string += &format!(...);

and suggests rewriting it to ```rust
write!(&mut some_string, ...);

Categories

  • Kind: Perf

What is the advantage of the recommended code over the original code
It avoids one extra heap allocation by writing directly to the existing String instead of creating a new temporary String.

Drawbacks

The user has to use std::io::Write for the write! macro to work (which I don't get tbh, why can't write! just import the trait on its own...)

Example

let mut string = String::new();
for foo in foo_list {
    string += &format!("{:?}", foo);
}

Could be written as:

use std::io::Write;

let mut string = String::new();
for foo in foo_list {
    write!(&mut string, "{:?}", foo);
}
@kangalio kangalio added the A-lint Area: New lints label Oct 28, 2020
@Urcra
Copy link
Contributor

Urcra commented Nov 5, 2020

I had actually thought this would be optimized away but seems like the slowdown is quite significant in release mode see example here https://play.rust-lang.org/?version=nightly&mode=release&edition=2018&gist=daed734101a8d8bc0c6a7bd811154504

There are some additional drawbacks in addition to the ones you mentioned however. write! returns a result so you would need to write it as:

use std::fmt::Write;

let mut string = String::new();
for foo in foo_list {
    write!(&mut string, "{:?}", foo).expect("This can never fail");
}

Which might cause other lints to give you trouble specifically lints such as unwrap_in_result if using this piece of code in a function that returns a Result

You also need to import either/both std::fmt:Write and std::io::Write and in the case of importing both of them you need to give them an alias so they don't clash such as:

use std::fmt::Write as FmtWrite;
use std::io::Write as IoWrite;

I also feel the readability of string += &format!("{:?}", foo); is better than write!(&mut string, "{:?}", foo).expect("This can never fail"); Though that is more of a subjective opinion

All that said I still feel like the performance probably makes up for the drawbacks.

I'll try to implement a lint for this over weekend

@camsteffen
Copy link
Contributor

If anyone asks about using write! with String, just change the subject to how amazing Rust's ownership system is or something like that.

@camsteffen camsteffen added good first issue These issues are a good way to get started with Clippy L-perf Lint: Belongs in the perf lint group T-middle Type: Probably requires verifiying types labels Feb 7, 2021
@pitaj
Copy link
Contributor

pitaj commented Apr 2, 2022

@rustbot claim

bors added a commit that referenced this issue Apr 14, 2022
New lint `format_add_strings`

Closes #6261

changelog: Added [`format_add_string`]: recommend using `write!` instead of appending the result of  `format!`
@bors bors closed this as completed in aade96f Apr 14, 2022
goxberry added a commit to DataDog/lading that referenced this issue Jul 19, 2022
This commit fixes an error that Clippy flags in Rust 1.62.1 in
which appending the output of a `format!` macro to a String is an
error because "one extra heap allocation can be avoided". (See
rust-lang/rust-clippy#6261 for details.)

Signed-off-by: Geoffrey M. Oxberry <[email protected]>
goxberry added a commit to DataDog/lading that referenced this issue Jul 19, 2022
This commit fixes an error that Clippy flags in Rust 1.62.1 in
which appending the output of a `format!` macro to a String is an
error because "one extra heap allocation can be avoided". (See
rust-lang/rust-clippy#6261 for details.)

Signed-off-by: Geoffrey M. Oxberry <[email protected]>
goxberry added a commit to DataDog/lading that referenced this issue Jul 20, 2022
* rust-toolchain: bump rust to 1.62.1 (like vector)

This commit bumps the Rust version Lading uses to version 1.62.1
for parity with the Vector project
(https://github.com/vectordotdev/vector).

Signed-off-by: Geoffrey M. Oxberry <[email protected]>

* blackhole/sqs.rs: fix clippy format macro error

This commit fixes an error that Clippy flags in Rust 1.62.1 in
which appending the output of a `format!` macro to a String is an
error because "one extra heap allocation can be avoided". (See
rust-lang/rust-clippy#6261 for details.)

Signed-off-by: Geoffrey M. Oxberry <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good first issue These issues are a good way to get started with Clippy L-perf Lint: Belongs in the perf lint group T-middle Type: Probably requires verifiying types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants