Support user format-like macros

[nyurik]

Add support for #[clippy::format_args] attribute that can be attached to any macro to indicate that it functions the same as the built-in format macros like format!, println! and write!

---

changelog: Enhancement: [format_in_format_args], [recursive_format_impl], [to_string_in_format_args], [uninlined_format_args], [unused_format_specs]: Recognizes #[clippy::format_args] to support custom 3rs party format macros.

[rust-highfive]

r? @Alexendoo

(rust-highfive has picked a reviewer for you, use r? to override)

[Alexendoo]

There are some reasonable situations where inline args can't be used, such as

macro_rules! error {
    ($fmt:literal $(, $e:expr)* $(,)?) => { println!(concat!("ERROR: ", $fmt), $($e,)*) }
}

fn main() {
    let x = 1;
    error!("{}", x);
}

Another thing is that it could break macro invocations themselves, initially I wrote that as ($fmt:literal, $($e:expr),*) where replacing it with error!("{x}") would result in error: unexpected end of macro invocation

[nyurik]

@Alexendoo I just added your example to my unit tests - it passes just fine. Do you have a test that fails?

[Alexendoo]

concat! does seem to be safe since it gets the correct span, for the second one an example would be

macro_rules! error {
    ($fmt:literal, $($e:expr),*) => { println!($fmt, $($e,)*) }
}

fn main() {
    let x = 1;
    error!("{}", x);
}

[nyurik]

@Alexendoo that's an excellent catch, thank you! The issue here is with the bug in macro definition (granted that macros are a special beast and a "bug" could be someone's feature). TBH, I am surprised there is no lint that tries to catch this mistake. The bigger question though is if this is something we should or should not process? I.e. something that suggests a cleaner repeated macro arg definition. I would even make this as a suspicious category.

That said, on one hand we have badly created user macros that probably should be fixed. On the other, there is a very common pattern with all sorts of log and other macros that use format strings and could really use this lint. Which of these takes priority?

[nyurik]

@Alexendoo I created a lint suggestion #9959 that would solve this types of issues - having both lints on at the same time would fix the issue i think

[Alexendoo]

It would be neat to be able to lint macro definitions, however as they are almost entirely represented as a big TokenStream that may be expanded into an arbitrary place linting them isn't easy - https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Get.20paths.20in.20a.20macro/near/292040802

Another one to bring up is that a common way of defining things like this is two arms, so that lint wouldn't be generally applicable

($fmt:literal) => ...
($fmt:literal, $($e:expr),*) => ...

e.g. the format_args macro_rules stub is defined like that. There are macros out there that assume the single arg version is a plain string much like the old panic! macro, I found this one in a search https://github.com/scottlamb/moonfire-nvr/blob/d509d3ff40160a58e4fbdd01be240f2be474b693/server/base/error.rs#L159-L166

That would make the suggestion change the behaviour of the code, silently even if the printed variable is otherwise used

My biggest concern is probably all the proc macros out there, the current implementation is not super robust against them. If a proc macro did an internal format!() but happened to use certain input spans we could be matching up a completely different format string than the one actually being expanded

To that end it may be worth keeping an eye on rust-lang/rust#99012 / rust-lang/compiler-team#541

[Alexendoo]

Another fun one:

macro_rules! used_twice {
    (
        large = $large:literal,
        small = $small:literal,
        $val:expr,
    ) => {{
        if $val < 5 {
            println!($small, $val);
        } else {
            println!($large, $val);
        }
    }}
}

fn main() {
    let x = 1;
    used_twice! {
        large = "large value: {}",
        small = "small value: {}",
        x,
    }
}

[nyurik]

I have a better idea - let's disable Rust macros, as they are clearly a work of Cthulhu, designed to steer us away from the light... as this is clearly a case of "I miss Perl, so I will make things as interesting as possible".

I don't think Clippy should care about such cases TBH as they probably represent 1% - when on the other hand we have 99% of the simple log::warn! type. Moreover, if our lint is used often enough, the 1% edge cases should disappear out right as abominations against humanity. That said, we may want to detect such cases (e.g. have a list of well known crates), and if the first macro in a call chain is not in the well known list, reduce the certainty of replacement from MachineApplicable to MaybeIncorrect.

[llogiq]

Please keep in mind that we deem false positives a worse outcome than false negatives. I'd be OK with having either a config that makes the lint apply everywhere instead of just format while making the suggestion MaybeIncorrect or having a second lint that is allowed by default that catches those cases aside format though, with a slight preference towards the latter.

[nyurik]

@llogiq I think we should also take into account how often we get false positive vs false negative. I agree that if they are about the same, false positive is worse, but if we get 100x more false negatives, I would prefer get a few more false positives.

I like the idea of an extra config param, something like include_custom_format_macros (naming?) - this way all lints that currently use is_format_arg would handle 3rd-party format macros.

[llogiq]

I would fully support that argument for a suspicious lint, but not for a style lint. Please note that a lint rarely works in isolation – clippy usually runs many of them. If one of them gives even a modest amount of false positives, this reduces the usefulness of clippy as a whole. While that may be acceptable for code that is likely problematic and warrants a closer look, I don't think it is needed for style lints which IMHO are a teaching tool.

With that said, as long as the user explicitly asks for it, I'm ok with changing this balance.

[bors]

☔ The latest upstream changes (presumably #9860) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #9865) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #9102) made this pull request unmergeable. Please resolve the merge conflicts.

[nyurik]

Took a bit of a year+ pause on this one.. Still, seems there is plenty of interest in this, so now this lint is rebased and cleaned up a bit. The question still remains: how can we progress. Perhaps someone has a better idea after Clippy has improved so much over the last year? @Alexendoo had some comments back then - do we still want to go with a one-per-project allow-list of valid crates? Feels really messy.

Maybe we should duplicate all format lints, e.g. create uninlined_format_args_ext in addition to uninlined_format_args -- the _ext would indicate this lint works on 3rd party crates. And this way that lint can be suspicious.

My original thoughts were posted in the log crate.

automatic

Fully automatic - decide at the linting time if my_format!("{}", foo} is the same as my_format!("{foo}") - i.e. the "what-if" macro expansion - this way the lints will try the proposed substitutions, and if the resulting code is the same as original, make the suggestion.

PROs: easiest to use everyone, and this approach can be used by many other types of lints to validate replacements instead of declaring them as MachineApplicable by the lint authors.
CONs: hardest to implement, only works for the lints which offer a MachineApplicable replacement

attribute

Create an attribute that macro developers can use to indicate that their macro is format! compatible:

#[format]
some_macro!( [...,] string_literal [, args]* )

PROs: allows macro authors to declare a macro as OK for Clippy to process as a format-like macro for many lints
CONs: existing libs don't have it, possibly will raise some warnings if used with older versions, a bit chatty

configuration-based

add a Clippy config param for end users to list all macros they will want to inline, plus provide a built-in list of all well-known format-like macros in popular crates. If the user wants to extend the built-in ones, they can just use the format-like-macros: ["..", "some-other-mods", "and-macros"] syntax. This approach was implemented as a bool flag in #9948.

This may require more "magic" options: "*" = include all, "-mod.macro" = exclude a specific mod or macro, etc.

PROs: easy to configure by end user, can be applied to older crates
CONs: Clippy team would have to maintain it, each user would need to manually add the ones not on the built-in list, macro writers have no way to control the built-in list for a specific version

[bors]

☔ The latest upstream changes (presumably #12269) made this pull request unmergeable. Please resolve the merge conflicts.

[Alexendoo]

My preferred solution would be the tool attribute applied to macro definitions, e.g.

#[clippy::format_args_compatible]
macro_rules! log {
// ...

Config based relies on assuming third party crate layouts to be widely useful which we're trying not to expand, I think putting it in the macro authors hands is preferable there

If there's a good heuristic that works for automatic detection I would be fine with that, but I'm not sure if there is one

[nyurik]

@Alexendoo do you know of any example of an attribute set on a macro rule, and Clippy using that attribute while parsing the expansion of the macro?

We could introduce a new pedantic (?) lint that would tag all cases as suspicious if they resolve to format_args! internally, but there are guaranteed to be rare false positives. We could even add a link to a guide on how to write proper format macros so they won't break if they use a single string without args.

[Alexendoo]

I don't think we have any cases where it's used, but MacroCall contains the macro's DefId which can be used to retrieve its attributes

Linting macro definitions is considered too difficult because the only thing you get from unexpanded macros is a token stream

[bors]

☔ The latest upstream changes (presumably #12635) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

Hey, this is a ping from triage. @Alexendoo can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using r? clippy.

@rustbot ready

[bors]

☔ The latest upstream changes (presumably #13088) made this pull request unmergeable. Please resolve the merge conflicts.

[Alexendoo]

Ah sorry missed that notification @xFrednet, my position is still if we're doing this it should be through an attribute rather than configuration

[bors]

☔ The latest upstream changes (presumably #13173) made this pull request unmergeable. Please resolve the merge conflicts.

[nyurik]

@Alexendoo i updated this PR to rely on #[clippy::format_args] attribute