fix(useless_format): improve suggestion

[nyurik]

View all comments

Fix #3361

Handled test case:

fn literal(lit: &str) {}
let lit = "hello";

let _: &str = &format!("hello");  /* => */  let _: &str = "hello";
literal(&format!("hello"));       /* => */  literal("hello");
literal(&format!("{}", lit));     /* => */  literal(lit);
literal(&format!("{lit}"));       /* => */  literal(lit);

This pull request improves the useless_format lint in Clippy by making its suggestions when format! is immediately borrowed as a string slice. The changes also refactor how lint messages and suggestions are generated for better clarity and maintainability.

Improvements to lint suggestions:

• The lint now detects when format! is immediately borrowed as &format!("literal") and suggests using the string literal directly, instead of calling .to_string(). This provides a more idiomatic and efficient suggestion.
• The suggestion logic is updated so that only immutable borrows (&format!) are handled with the new message; mutable borrows (&mut format!) still suggest .to_string().

changelog: [useless_format]: for &format!("literal") suggest "literal" as a replacement

[rustbot]

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 7 candidates
• 7 candidates expanded to 7 candidates
• Random selection from Jarcho, dswij, llogiq, samueltardieu

[llogiq]

This looks like a solid improvement. I just have a few small nits, otherwise fine to merge.

View changes since this review

[nyurik]

thanks @llogiq, fixed

[llogiq]

Thank you!

[nyurik]

@llogiq seems like merge queue is being ... resistive

[nyurik]

@llogiq could you try to add it to the merge queue again?

[ada4a]

Hm, but clippy lints are supposed to compose, no? If a user runs cargo clippy --fix, then useless_format and needless_conversion will trigger in sequence, achieving the same result while allowing each lint to only think about its thing

[nyurik]

@ada4a yes, but clippy is frequently used via rust analyzer, so people would see a suggestion in the IDE, click "fix", and the fix would be flagged again as another fix - i.e. annoying :)

@llogiq friendly ping, or let me know if someone else should review this
cc: @samueltardieu

[llogiq]

I think @ada4a is right about the composability issue. Can we fix it within this PR or should we wait for a followup?

[nyurik]

@llogiq I am not certain how this PR should be modified... If clippy makes a suggestion, I would think the suggestion should be "as good as possible", rather than relying on a chain of lints, esp if some lints in that chain might have been disabled for various reasons.

[nyurik]

@llogiq friendly ping - what should be done here?

[ada4a]

Actually, I've changed my mind: in general, lints should be composable, and shouldn't be overcomplicated by trying to predict and handle subsequent fixes -- but in simple cases like this, doing a little extra work to clearly improve the suggestion and avoid another round of cargo clippy is obviously worth it.

So I'd say this PR would be good to merge as-is, after a rebase.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[nyurik]

@ada4a thx, rebased

r? @ada4a

[rustbot]

ada4a is not on the review rotation at the moment.
They may take a while to respond.

[CommanderStorm]

r? clippy
Think this is ready with no objections..

[ada4a]

Sorry, seems like I got distracted while approving this :p

Thanks for the reminder, @CommanderStorm ^^

View changes since this review