Skip to content
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

Extend useless_format to lint values other than &str and String #3753

Closed
ghost opened this issue Feb 10, 2019 · 10 comments
Closed

Extend useless_format to lint values other than &str and String #3753

ghost opened this issue Feb 10, 2019 · 10 comments
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-pedantic Lint: Belongs in the pedantic lint group

Comments

@ghost
Copy link

ghost commented Feb 10, 2019

format!("{}", x) can be replaced by x.to_string() when x implements ToString . The new code is neater and probably faster.

@sinkuu
Copy link
Contributor

sinkuu commented Feb 10, 2019

format!("{}", x) can be unconditionally rewritten to x.to_stirng() thanks to the <T: Display> ToString for T blanket impl.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 10, 2019

@RalfJung mentioned that they really dislike even changing format!("foo") to "foo".to_string() considering that they have the same perf nowadays and readability does not suffer at all. Quite the opposite: if you have 20 format! calls in a file and a few to_string sprinkled in between, that actually is worse than just having format! everywhere

@RalfJung
Copy link
Member

Addendum: I dislike this in some cases. I think this is context-dependent. I use format! when this is similar to other nearby code (e.g. formatting error messages for the user, and there are a whole bunch of cases) -- consistency in multiple match branches increases readability a lot.

But when the intention is not to format user-visible text, but to e.g. turn an integer into a string, I will use i.to_string(). I think there is a different in intent between formatting and converting to a string, and it is useful to express that in the code.

@ghost ghost changed the title Extend useless_format to lint values that implement ToString Extend useless_format to lint values other than &str and String Feb 11, 2019
@ghost
Copy link
Author

ghost commented Feb 11, 2019

Good point about <T:Display>.

So would this change be accepted? If not, should we do anything about the existing lint?

@sanmai-NL
Copy link
Contributor

So,

format!("foo")

is worse at communicating intention than

"foo".to_owned()

@RalfJung
Copy link
Member

That depends heavily on the context.

In Miri, we have some error types taking String, and then you frequently see code like

MachineError(format!("Unable to access {}", foobar))

but sometimes the message is just a string with no format variable, and then we write

MachineError(format!("Division by 0"))

I think this communicates intent much clearer than if we changed some of the error messages to use to_owned.

@sanmai-NL
Copy link
Contributor

I think you're confusing consistency and/or the principle of least surprise with communicating intent there. And I happen to value the latter more than stylistic consistency.

@RalfJung
Copy link
Member

Well, feel free to turn on that lint in your files then, but please don't make add it to a default lint.

And no, I don't think I am confusing this. The intent here is to create a string for consumption by the user -- the intent is to do formatting for UI output. The fact that the string being printed is a literal should not mean that I suddenly use entirely different syntax. The fact that the string must be owned to be put into a MachineError is a detail I couldn't care less about at this point, so to_owned conveys to useful information in this case.

@sanmai-NL
Copy link
Contributor

But you're not formatting that literal at all in the case we're talking about. You would use an &str if you could, you just need to assert ownership of it.

@camsteffen camsteffen added A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-pedantic Lint: Belongs in the pedantic lint group L-style Lint: Belongs in the style lint group and removed L-style Lint: Belongs in the style lint group labels Feb 22, 2021
@camsteffen
Copy link
Contributor

Duplicate of #3156

@camsteffen camsteffen marked this as a duplicate of #3156 Nov 15, 2021
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-pedantic Lint: Belongs in the pedantic lint group
Projects
None yet
Development

No branches or pull requests

5 participants