Skip to content

Fix literal_string_with_formatting_args lint emitted when it should not #13953

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Feb 13, 2025
+110 −18
Merged

Fix literal_string_with_formatting_args lint emitted when it should not #13953

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
17f9344
Fix `literal_string_with_formatting_args` lint emitted when it should…
GuillaumeGomez Jan 6, 2025
a7fb37c
Correctly handle expanded macros for `literal_string_with_formatting_…
GuillaumeGomez Jan 7, 2025
00104a4
Add more detailed explanations for #13885 regression test
GuillaumeGomez Jan 15, 2025
e35330c
Use `is_from_proc_macro` instead of checking if the snippet contains …
GuillaumeGomez Jan 15, 2025
277bf08
Fix regression #14007
GuillaumeGomez Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion clippy_lints/src/literal_string_with_formatting_args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use rustc_span::{BytePos, Span};

use clippy_utils::diagnostics::span_lint;
use clippy_utils::mir::enclosing_mir;
use clippy_utils::source::snippet_opt;

declare_clippy_lint! {
/// ### What it does
Expand Down Expand Up @@ -81,7 +82,7 @@ fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, spans: &[(Span, Option<Strin

impl LateLintPass<'_> for LiteralStringWithFormattingArg {
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
if expr.span.from_expansion() {
if expr.span.from_expansion() || expr.span.is_dummy() {
return;
}
if let ExprKind::Lit(lit) = expr.kind {
Expand All @@ -95,7 +96,15 @@ impl LateLintPass<'_> for LiteralStringWithFormattingArg {
},
_ => return,
};
let Some(snippet) = snippet_opt(cx, expr.span) else {
return;
};
let fmt_str = symbol.as_str();
// If the literal has been generated by the macro, the snippet should not contain it,
// allowing us to skip it.
if !snippet.contains(fmt_str) {
return;
}
Copy link
Member

@y21 y21 Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will add false negatives for escaped strings (eg "{foo} \n bar") and similar, but as a temporary solution to fix FPs it's probably fine

Copy link
Member

@y21 y21 Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth trying to use is_from_proc_macro here instead, because that also solves the same issue of a node not matching up with its snippet. It seems like it should also fix the FP while not running into this FN since it doesn't actually check the string contents.

(If we do go with the current approach, a small nit: it might be a good use for the newer SpanRange API with if !expr.span.check_source_text(|snippet| snippet.contains(fmt_str)) { return; }. Slightly simpler and avoids allocating a temporary string)

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why is_from_proc_macro didn't work last time I tried but this time it behaves as expected, so switching back to it.

let lo = expr.span.lo();
let mut current = fmt_str;
let mut diff_len = 0;
Expand Down
19 changes: 19 additions & 0 deletions tests/ui/literal_string_with_formatting_arg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,24 @@
#![warn(clippy::literal_string_with_formatting_args)]
#![allow(clippy::unnecessary_literal_unwrap)]

// Regression test for <https://github.com/rust-lang/rust-clippy/issues/13885>.
// It's not supposed to emit the lint in this case (in `assert!` expansion).
fn compiler_macro() {
fn parse(_: &str) -> Result<(), i32> {
unimplemented!()
}

assert!(
parse(
#[allow(clippy::literal_string_with_formatting_args)]
"foo {:}"
)
.is_err()
);
let value = 0;
assert!(format!("{value}").is_ascii());
}
Copy link
Member

@samueltardieu samueltardieu Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add another true positive test such as

Suggested change
}
assert!("{value}".is_ascii());
}

so that we can check that the lint properly triggers in non-formatting-macro arguments?


fn main() {
let x: Option<usize> = None;
let y = "hello";
Expand All @@ -13,6 +31,7 @@ fn main() {
x.expect(r"{y:?} {y:?} "); //~ literal_string_with_formatting_args
x.expect(r"{y:?} y:?}"); //~ literal_string_with_formatting_args
x.expect(r##" {y:?} {y:?} "##); //~ literal_string_with_formatting_args
assert!("{y}".is_ascii()); //~ literal_string_with_formatting_args
// Ensure that it doesn't try to go in the middle of a unicode character.
x.expect("———{:?}"); //~ literal_string_with_formatting_args

Expand Down
30 changes: 18 additions & 12 deletions tests/ui/literal_string_with_formatting_arg.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:7:15
--> tests/ui/literal_string_with_formatting_arg.rs:25:15
|
LL | x.expect("{y} {}");
| ^^^
Expand All @@ -8,64 +8,70 @@ LL | x.expect("{y} {}");
= help: to override `-D warnings` add `#[allow(clippy::literal_string_with_formatting_args)]`

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:8:16
--> tests/ui/literal_string_with_formatting_arg.rs:26:16
|
LL | x.expect(" {y} bla");
| ^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:9:15
--> tests/ui/literal_string_with_formatting_arg.rs:27:15
|
LL | x.expect("{:?}");
| ^^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:10:15
--> tests/ui/literal_string_with_formatting_arg.rs:28:15
|
LL | x.expect("{y:?}");
| ^^^^^

error: these look like formatting arguments but are not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:11:16
--> tests/ui/literal_string_with_formatting_arg.rs:29:16
|
LL | x.expect(" {y:?} {y:?} ");
| ^^^^^ ^^^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:12:23
--> tests/ui/literal_string_with_formatting_arg.rs:30:23
|
LL | x.expect(" {y:..} {y:?} ");
| ^^^^^

error: these look like formatting arguments but are not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:13:16
--> tests/ui/literal_string_with_formatting_arg.rs:31:16
|
LL | x.expect(r"{y:?} {y:?} ");
| ^^^^^ ^^^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:14:16
--> tests/ui/literal_string_with_formatting_arg.rs:32:16
|
LL | x.expect(r"{y:?} y:?}");
| ^^^^^

error: these look like formatting arguments but are not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:15:19
--> tests/ui/literal_string_with_formatting_arg.rs:33:19
|
LL | x.expect(r##" {y:?} {y:?} "##);
| ^^^^^ ^^^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:17:18
--> tests/ui/literal_string_with_formatting_arg.rs:34:14
|
LL | assert!("{y}".is_ascii());
| ^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:36:18
|
LL | x.expect("———{:?}");
| ^^^^

error: this looks like a formatting argument but it is not part of a formatting macro
--> tests/ui/literal_string_with_formatting_arg.rs:27:19
--> tests/ui/literal_string_with_formatting_arg.rs:46:19
|
LL | x.expect(r##" {x:?} "##); // `x` doesn't exist so we shoud not lint
| ^^^^^

error: aborting due to 11 previous errors
error: aborting due to 12 previous errors

14 changes: 14 additions & 0 deletions tests/ui/{literal_string_with_formatting_args}.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Regression test for <https://github.com/rust-lang/rust-clippy/issues/13885>.
// The `dbg` macro generates a literal with the name of the current file, so
// we need to ensure the lint is not emitted in this case.

#![crate_name = "foo"]
#![allow(unused)]
#![warn(clippy::literal_string_with_formatting_args)]

fn another_bad() {
let literal_string_with_formatting_args = 0;
dbg!("something");
Copy link
Member

@y21 y21 Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that the expr.span.from_expansion() check doesn't already prevent this. Looking at -Zunpretty=hir-tree output it does seem like the span of the string with the file name is properly marked as being from the non-root context

Lit(
    Spanned {
        node: Str(
            "[/app/example.rs:2:5] \"something\" = ",
            Cooked,
        ),
        span: /rustc/eb54a50837ad4bcc9842924f27e7287ca66e294c/library/std/src/macros.rs:365:35: 365:58 (#4),
    },
),

Odd

Copy link
Member

@y21 y21 Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what's happening. Clippy sets -Zflatten_format_args=no, which changes the default behavior of how format args are lowered and only that one has this non-macro span. Adding the flag makes it repro on godbolt and shows a root context span for the file name string.

Lit(
    Spanned {
        node: Str(
            "/app/example.rs",
            Cooked,
        ),
        span: /app/example.rs:2:5: 2:22 (#0),
    },
)

Maybe add to the comment that clippy sets this flag and only with that flag does it have a non-macro span which gets around the from_expansion check, in case anyone wants to look into this in the future and doesn't have to go down this rabbit hole?

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into it but couldn't figure out why it didn't work. Thanks a lot for the detailed explanations! Adding it as a comment.

}

fn main() {}
Loading