rust-clippy icon indicating copy to clipboard operation
rust-clippy copied to clipboard

[literal_string_with_formatting_args] support for `lazy_format`

Open madonuko opened this issue 8 months ago • 3 comments

Summary

literal_string_with_formatting_args does not support lazy_format::lazy_format!.

Lint Name

literal_string_with_formatting_args

Reproducer

warning: this looks like a formatting argument but it is not part of a formatting macro
   --> src/util.rs:387:54
    |
387 |             .note(lazy_format::lazy_format!("Status: {status}")),
    |                                                      ^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#literal_string_with_formatting_args

Version

rustc 1.86.0 (05f9846f8 2025-03-31)
binary: rustc
commit-hash: 05f9846f893b09a1be1fc8560e33fc3c815cfecb
commit-date: 2025-03-31
host: x86_64-unknown-linux-gnu
release: 1.86.0
LLVM version: 19.1.7

Additional Labels

No response

madonuko avatar May 25 '25 10:05 madonuko

Could you provide an example where this false positive is active?

blyxyas avatar Jun 19 '25 17:06 blyxyas

#[warn(clippy::literal_string_with_formatting_args)]
fn main() {
  let x = 1;
  println!("{}", lazy_format::lazy_format!("{x}"));
}

madonuko avatar Jun 19 '25 19:06 madonuko

Bisection result: This bug is present from the lint's addition.

lazy_format expands to:

#![feature(prelude_import)]
#![feature(print_internals)]
#[prelude_import]
use std::prelude::rust_2024::*;
#[macro_use]
extern crate std;
#[deny(clippy::literal_string_with_formatting_args)]
fn main() {
    let x = 1;
    {
        ::std::io::_print(
            format_args!(
                "{0}\n",
                {
                    struct LazyFormat<
                        F: Fn(&mut ::core::fmt::Formatter) -> ::core::fmt::Result,
                    >(
                        F,
                    );
                    #[automatically_derived]
                    impl<
                        F: ::core::clone::Clone
                            + Fn(&mut ::core::fmt::Formatter) -> ::core::fmt::Result,
                    > ::core::clone::Clone for LazyFormat<F> {
                        #[inline]
                        fn clone(&self) -> LazyFormat<F> {
                            LazyFormat(::core::clone::Clone::clone(&self.0))
                        }
                    }
                    #[automatically_derived]
                    impl<
                        F: ::core::marker::Copy
                            + Fn(&mut ::core::fmt::Formatter) -> ::core::fmt::Result,
                    > ::core::marker::Copy for LazyFormat<F> {}
                    impl<
                        F: Fn(&mut ::core::fmt::Formatter) -> ::core::fmt::Result,
                    > ::core::fmt::Debug for LazyFormat<F> {
                        #[inline]
                        fn fmt(
                            &self,
                            f: &mut ::core::fmt::Formatter,
                        ) -> ::core::fmt::Result {
                            f.write_str(
                                "make_lazy_format!(| f | $crate :: write! (f, \"{x}\"))",
                            )
                        }
                    }
                    impl<
                        F: Fn(&mut ::core::fmt::Formatter) -> ::core::fmt::Result,
                    > ::core::fmt::Display for LazyFormat<F> {
                        #[inline]
                        fn fmt(
                            &self,
                            f: &mut ::core::fmt::Formatter,
                        ) -> ::core::fmt::Result {
                            (self.0)(f)
                        }
                    }
                    LazyFormat(move |
                        f: &mut ::core::fmt::Formatter,
                    | -> ::core::fmt::Result {
                        {
                            enum Style {
                                Empty,
                                Plain,
                                Format,
                            }
                            match {
                                const STYLE: Style = match "{x}".as_bytes().split_first() {
                                    ::core::option::Option::None => Style::Empty,
                                    ::core::option::Option::Some((&(b'}' | b'{'), _)) => {
                                        Style::Format
                                    }
                                    ::core::option::Option::Some((_, mut s)) => {
                                        loop {
                                            s = match s.split_first() {
                                                None => break Style::Plain,
                                                Some((&(b'}' | b'{'), _)) => break Style::Format,
                                                Some((_, s)) => s,
                                            };
                                        }
                                    }
                                };
                                STYLE
                            } {
                                Style::Empty => ::core::fmt::Result::Ok(()),
                                Style::Plain => ::core::fmt::Write::write_str(f, "{x}"),
                                Style::Format => {
                                    ::core::fmt::Write::write_fmt(f, format_args!("{0}", x))
                                }
                            }
                        }
                    })
                },
            ),
        );
    };
}

Note that Style::Plain => ::core::fmt::Write::write_str(f, "{x}"),.

blyxyas avatar Jun 19 '25 19:06 blyxyas

I believe this false positive is occurring because of the way the lint is designed. It simply checks for formatting args in every single string literal expression that is not generated by a macro or is a dummy. If it has a variable, it checks if it's a valid identifier and local as well. The reason that it does not lint on string literals in formatting macros is because of how they expand. Notice how the expanded println! macro replaces the variable with a 0, which is not a valid identifier. It becomes macro generated anyways. Similar behavior can be seen with other formatting args, like {:?}, and other formatting macros, like print! and format!. Unexpanded:

println!("{x}");

Expanded:

{ ::std::io::_print(format_args!("{0}\n", x)); };

However, the flaws with this method show up when using lazy_format!. If we look at the expansion that @blyxyas provided, we can see that one line contains Style::Plain => ::core::fmt::Write::write_str(f, "{x}"),. Although the rest of the code was written by the macro, the string literal "{x}" is exactly what was passed into the macro, so I believe that means it is not considered macro generated. Since it meets all the other criteria, the lint triggers. I think the only was this could be fixed was if the lint was rewritten in a different manner. I don't have the time right now, so I'm just leaving my observations for whoever wants to do it. I might be wrong, there might be a way to fix this without rewriting the lint. If you do end up rewriting the lint, you might want to design it in a way that works with #13994.

mrchilliballs avatar Aug 06 '25 07:08 mrchilliballs

Looks interesting, good beginner issue to try out :)

@rustbot claim

J0s3c4rl0s avatar Sep 06 '25 10:09 J0s3c4rl0s

I think @mrchilliballs is right on the money. The lint has checks to check if a given expression comes from a macro (from_expansion, is_from_proc_macro) which doesnt trip up because the string is considered code from a user rather than a macro.

I tried using this as a test case for a macro that contains the exact string value as the argument since the lint does not seem to actually check if the macro is specifically a formatting macro (which I believe is what #13994 is for).

macro_rules! passthrough_str {
    ($s:expr) => {
        $s
    };
}

I feel like there are three possible solutions, which all feel a bit inadequate.

  1. Find some context that contains information that this string was "contained in a macro" in the source -> I searched for and couldnt find this, but generally feels weird that there would be two data points for tracking macro origin in different ways.
  2. Add this information to some context/change behaviour of macro -> Seems potentially disruptive and more like a feature than a bugfix and theres an ongoing bugfix
  3. Let the lint do its own traversal of the AST. If the lint traverses the AST itself then it may be able to do an "early stopping" when it reaches a node that came from a macro -> Would not make use of the builtin machinery for traversing the AST, not entirely clear either that the AST will still have this "hierarchical" nature of the macro expansion collected under one root node.

I might explore a bit more what information can be gleaned from the sourcemap thats used for error messages since it may more naturally contain this information and could allow me to do step 3. Might also be worth exploring what the other options are for traversing the AST besides check_expression

J0s3c4rl0s avatar Sep 27 '25 11:09 J0s3c4rl0s