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

Lint suggestion: nested format! #7667

Closed
jplatte opened this issue Sep 13, 2021 · 5 comments · Fixed by #7743
Closed

Lint suggestion: nested format! #7667

jplatte opened this issue Sep 13, 2021 · 5 comments · Fixed by #7743
Labels
A-lint Area: New lints

Comments

@jplatte
Copy link
Contributor

jplatte commented Sep 13, 2021

What it does

Warn on format! within the arguments of another macro that does formatting such as format! itself, write! or println!.

Suggests replacing the inner format! call with format_args! or inlining it entirely (see example).

Categories (optional)

  • Kind: style / perf

The recommended code is both shorter and avoids a temporary allocation.

Drawbacks

There could be situations when the outer macro is format_args! where lifetimes don't work out with the format! changed to format_args! or inlined entirely. I'm not sure how hard it would be to detect these cases.

Example

println!("error: {}", format!("something failed at {}", Location::caller()));

Could be written as:

println!("error: {}", format_args!("something failed at {}", Location::caller()));

or inlined entirely (when the argument is used exactly once like here):

println!("error: something failed at {}", Location::caller());
@jplatte jplatte added the A-lint Area: New lints label Sep 13, 2021
@matthiaskrgr
Copy link
Member

Related issues: #3155 #6259

@camsteffen
Copy link
Contributor

Lint name idea: format_in_format_args. Note that panic!(.., format!(..)) is expanded to ..format_args!(.., format!(..))... The same is true for all format_args-like macros.

To implement, I would start with FormatArgsExpn::parse, then try FormatExpn::parse for each value arg, then look at the outer macro expansion to look for panic!, assert!, write!, etc.

@smoelius
Copy link
Contributor

smoelius commented Sep 28, 2021

@camsteffen If I were to implement this, would it be too invasive if I were to change FormatExpn to FormatLikeExpn so that it could handle things like println!?

Currently, it strictly handles format!:

if let ExpnKind::Macro(_, sym::format) = expn_data.kind;

@camsteffen
Copy link
Contributor

@smoelius I think I would add something like this, on top of what we already have:

enum FormatLikeExpn {
    Assert(AssertExpn),
    Format(FormatExpn),
    FormatArgs(FormatArgsExpn),
    Print(PrintExpn),
}

so we still have the option of parsing a specific macro.

@smoelius
Copy link
Contributor

OK. Thanks for your response. I'm going to play with this and see what makes sense.

smoelius added a commit to smoelius/rust-clippy that referenced this issue Sep 30, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Sep 30, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Sep 30, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Sep 30, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Sep 30, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Sep 30, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Sep 30, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Oct 2, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Oct 3, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Oct 7, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Oct 7, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Oct 7, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Oct 8, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Oct 14, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Oct 14, 2021
smoelius added a commit to smoelius/rust-clippy that referenced this issue Oct 14, 2021
bors added a commit that referenced this issue Oct 15, 2021
Add `format_in_format_args` and `to_string_in_format_args` lints

Fixes #7667 and #7729

I put these in `perf` since that was one of `@jplatte's` suggestions, and `redundant_clone` (which I consider to be similar) lives there as well.

However, I am open to changing the category or anything else.

r? `@camsteffen`

changelog: Add `format_in_format_args` and `to_string_in_format_args` lints
@bors bors closed this as completed in c9599d7 Oct 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants