format_push_string could use some more information

[yoav-lavi]

Description

clippy::format_push_string currently outputs the following:

(the following is part of a fork of human_panic)

error: `format!(..)` appended to existing `String`
   --> my/project/file/mod.rs:119:27
    |
119 |           Some(location) => explanation.push_str(&format!(
    |  ___________________________^
120 | |             "Panic occurred in file '{}' at line {}\n",
121 | |             location.file(),
122 | |             location.line()
123 | |         )),
    | |__________^
    |
    = note: `-D clippy::format-push-string` implied by `-D clippy::all`
    = help: consider using `write!` to avoid the extra allocation
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#format_push_string

error: could not compile `project_name` due to previous error

I think this description could use a bit more information regarding the difference between format! and write!, as write! returns a Result, which the linked example instructs you to ignore:

let _ = write!(s, "0x{:X}", 1024);

The issue with this is that if you're not familiar with write!, you don't know which errors you're ignoring by making this change, and if you use -D clippy::all (as e.g. we do in my project) you have to either make this change or use #[allow(clippy::format_push_string)] for your code to compile. This isn't ideal as you may end up making a decision without all the needed information.

If you look into write!, you'll find that the write_xyz methods have the following description:

Errors
This function will return an instance of Error on error.

Which doesn't really give you any more information. (edit: apparently this is meant to reflect any implementation rather than the default implementation, which is why it's a bit vague)

write_fmt, used by write! (on $dst which may mean it's using something other than the linked documentation as I'm not familiar with what $dst is in this context), doesn't have an # Errors section.

If you continue going down the rabbit hole, write_fmt calls write, which also does not have an # Errors section. It itself calls write_str, and has an unsafe block that returns a Result as well.

My point isn't to complain or criticize, but rather to point out that if you're not familiar with the intricacies of write! the lint info currently doesn't give you a good idea of what potential errors you're ignoring by making this switch if using it as directed. Even if you do add handling code and change the function you used format! in to return a Result, the result (no pun intended) is that you now have a new failure scenario you don't know enough about.

My assumption is that format! calls write! somewhere and ignores the error and that's the reason why they were deemed equivalent, but I'd add that to the description if that's the case.

Thanks!

Version

rustc 1.62.0 (a8314ef7d 2022-06-27)
binary: rustc
commit-hash: a8314ef7d0ec7b75c336af2c9857bfaf43002bfc
commit-date: 2022-06-27
host: aarch64-apple-darwin
release: 1.62.0
LLVM version: 14.0.5

Additional Labels

No response

[Jasperav]

I am curious why this change landed on Rust stable and has preference over the &format way of appending a &str to a String. Now I can either ignore an error with let _ = which is ugly, or call unwrap, which isn't much better. &format didn't had both.

[yoav-lavi]

@Jasperav seems like a choice of performance over style:

Introduces an extra, avoidable heap allocation

I'm not necessarily opposed to the idea (not sure how I feel about it yet since I haven't run into it much), but if write! to a String is truly infallible and equivalent to format! + push without an allocation, maybe an additional macro or method could be created to not return a Result, which would be the best of both worlds. Wouldn't be an issue for Clippy though.

my_string.push_fmt("{} world", "hello");
my_string.push_format("{} world", "hello");

// or format_push / fmt_push etc.

write_string!(my_string, "{} world", "hello");

Would all be nice (although the method seems more convenient)

[obeis]

@rustbot claim

[Victor-N-Suadicani]

I'm not a huge fan of this lint. format! being infallible and write! returning a result is quite a difference in API and I don't think switching between them should be suggested so lightly.

Also, I haven't checked but I'd be curious if the extra allocation by format! is optimized away by the compiler anyway in release mode.

[intgr]

Recommending to switch clear infallible code to code that ignores errors certainly seems like a step backwards to me, even if it is faster. If people get into the habit of ignoring errors, it will eventually bite them, likely in a hard to debug way.

Surely unwrap() is better than totally ignoring the error?

[ghost]

As I understand it, this is the source for the the Write impl used by String: https://doc.rust-lang.org/src/std/io/impls.rs.html#380-412. No Errs are ever returned.

[Victor-N-Suadicani]

As I understand it, this is the source for the the Write impl used by String: https://doc.rust-lang.org/src/std/io/impls.rs.html#380-412. No Errs are ever returned.

I don't think it matters if Err is ever returned or not, the API is fallible and it's not obvious that it never fails. I wouldn't mind the lint if it suggested a hypothetical infallible write_format! macro or something but unfortunately that doesn't exist.

[yoav-lavi]

@Victor-N-Suadicani that's what I meant here: #9077 (comment), I agree that it would make more sense (and have less of a "surprise" element) as a separate macro that doesn't return an Err