From 17f9344a9689d57813d56ee35d95c97f05e6c5e0 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Mon, 6 Jan 2025 19:03:55 +0100 Subject: [PATCH 1/5] Fix `literal_string_with_formatting_args` lint emitted when it should not --- .../literal_string_with_formatting_args.rs | 2 +- .../ui/literal_string_with_formatting_arg.rs | 19 ++++++++++++ .../literal_string_with_formatting_arg.stderr | 30 +++++++++++-------- 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/literal_string_with_formatting_args.rs b/clippy_lints/src/literal_string_with_formatting_args.rs index 49353a1b76be..4c8063ee6e63 100644 --- a/clippy_lints/src/literal_string_with_formatting_args.rs +++ b/clippy_lints/src/literal_string_with_formatting_args.rs @@ -81,7 +81,7 @@ fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, spans: &[(Span, Option 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 { diff --git a/tests/ui/literal_string_with_formatting_arg.rs b/tests/ui/literal_string_with_formatting_arg.rs index f257c66f59d3..c9731e592e59 100644 --- a/tests/ui/literal_string_with_formatting_arg.rs +++ b/tests/ui/literal_string_with_formatting_arg.rs @@ -1,6 +1,24 @@ #![warn(clippy::literal_string_with_formatting_args)] #![allow(clippy::unnecessary_literal_unwrap)] +// Regression test for . +// 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()); +} + fn main() { let x: Option = None; let y = "hello"; @@ -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 diff --git a/tests/ui/literal_string_with_formatting_arg.stderr b/tests/ui/literal_string_with_formatting_arg.stderr index 32a84f600da7..1898f4f57b82 100644 --- a/tests/ui/literal_string_with_formatting_arg.stderr +++ b/tests/ui/literal_string_with_formatting_arg.stderr @@ -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} {}"); | ^^^ @@ -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 From a7fb37c1d832bea651f29b4676a816635dc11894 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 7 Jan 2025 15:55:41 +0100 Subject: [PATCH 2/5] Correctly handle expanded macros for `literal_string_with_formatting_args` lint --- .../src/literal_string_with_formatting_args.rs | 9 +++++++++ tests/ui/{literal_string_with_formatting_args}.rs | 14 ++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 tests/ui/{literal_string_with_formatting_args}.rs diff --git a/clippy_lints/src/literal_string_with_formatting_args.rs b/clippy_lints/src/literal_string_with_formatting_args.rs index 4c8063ee6e63..c0d016ce08b0 100644 --- a/clippy_lints/src/literal_string_with_formatting_args.rs +++ b/clippy_lints/src/literal_string_with_formatting_args.rs @@ -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 @@ -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; + } let lo = expr.span.lo(); let mut current = fmt_str; let mut diff_len = 0; diff --git a/tests/ui/{literal_string_with_formatting_args}.rs b/tests/ui/{literal_string_with_formatting_args}.rs new file mode 100644 index 000000000000..8a06d0c7b360 --- /dev/null +++ b/tests/ui/{literal_string_with_formatting_args}.rs @@ -0,0 +1,14 @@ +// Regression test for . +// 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"); +} + +fn main() {} From 00104a4167d2dd295e3639163dea393f7cd310d1 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 15 Jan 2025 16:42:56 +0100 Subject: [PATCH 3/5] Add more detailed explanations for #13885 regression test --- .../{literal_string_with_formatting_args}.rs | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/ui/{literal_string_with_formatting_args}.rs b/tests/ui/{literal_string_with_formatting_args}.rs index 8a06d0c7b360..0f5b36e67503 100644 --- a/tests/ui/{literal_string_with_formatting_args}.rs +++ b/tests/ui/{literal_string_with_formatting_args}.rs @@ -2,6 +2,38 @@ // 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. +// 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. +// +// So instead of having: +// +// ``` +// Lit( +// Spanned { +// node: Str( +// "[/app/example.rs:2:5] \"something\" = ", +// Cooked, +// ), +// span: /rustc/eb54a50837ad4bcc9842924f27e7287ca66e294c/library/std/src/macros.rs:365:35: 365:58 (#4), +// }, +// ), +// ``` +// +// We get: +// +// ``` +// Lit( +// Spanned { +// node: Str( +// "/app/example.rs", +// Cooked, +// ), +// span: /app/example.rs:2:5: 2:22 (#0), +// }, +// ) +// ``` + #![crate_name = "foo"] #![allow(unused)] #![warn(clippy::literal_string_with_formatting_args)] From e35330cabeaf312cefb4370560fd473d47eabe2d Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 15 Jan 2025 16:52:46 +0100 Subject: [PATCH 4/5] Use `is_from_proc_macro` instead of checking if the snippet contains the literal --- .../src/literal_string_with_formatting_args.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/literal_string_with_formatting_args.rs b/clippy_lints/src/literal_string_with_formatting_args.rs index c0d016ce08b0..21066a00eaf5 100644 --- a/clippy_lints/src/literal_string_with_formatting_args.rs +++ b/clippy_lints/src/literal_string_with_formatting_args.rs @@ -7,8 +7,8 @@ use rustc_session::declare_lint_pass; use rustc_span::{BytePos, Span}; use clippy_utils::diagnostics::span_lint; +use clippy_utils::is_from_proc_macro; use clippy_utils::mir::enclosing_mir; -use clippy_utils::source::snippet_opt; declare_clippy_lint! { /// ### What it does @@ -80,8 +80,8 @@ fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, spans: &[(Span, Option for LiteralStringWithFormattingArg { - fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { +impl<'tcx> LateLintPass<'tcx> for LiteralStringWithFormattingArg { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { if expr.span.from_expansion() || expr.span.is_dummy() { return; } @@ -96,15 +96,10 @@ 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) { + if is_from_proc_macro(cx, expr) { return; } + let fmt_str = symbol.as_str(); let lo = expr.span.lo(); let mut current = fmt_str; let mut diff_len = 0; From 277bf089b36db59dbc2c2efa279fbe52a3fe1c4b Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 18 Jan 2025 17:13:53 +0100 Subject: [PATCH 5/5] Fix regression #14007 --- .../literal_string_with_formatting_args.rs | 6 +++- .../ui/literal_string_with_formatting_arg.rs | 7 ++++ .../literal_string_with_formatting_arg.stderr | 36 +++++++++++-------- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/literal_string_with_formatting_args.rs b/clippy_lints/src/literal_string_with_formatting_args.rs index 21066a00eaf5..7e3cc444a817 100644 --- a/clippy_lints/src/literal_string_with_formatting_args.rs +++ b/clippy_lints/src/literal_string_with_formatting_args.rs @@ -128,7 +128,11 @@ impl<'tcx> LateLintPass<'tcx> for LiteralStringWithFormattingArg { pos.start += diff_len; pos.end += diff_len; - let start = fmt_str[..pos.start].rfind('{').unwrap_or(pos.start); + let mut start = pos.start; + while start < fmt_str.len() && !fmt_str.is_char_boundary(start) { + start += 1; + } + let start = fmt_str[..start].rfind('{').unwrap_or(start); // If this is a unicode character escape, we don't want to lint. if start > 1 && fmt_str[..start].ends_with("\\u") { continue; diff --git a/tests/ui/literal_string_with_formatting_arg.rs b/tests/ui/literal_string_with_formatting_arg.rs index c9731e592e59..b9a6654d4276 100644 --- a/tests/ui/literal_string_with_formatting_arg.rs +++ b/tests/ui/literal_string_with_formatting_arg.rs @@ -19,6 +19,13 @@ fn compiler_macro() { assert!(format!("{value}").is_ascii()); } +// Regression test for . +fn regression_14007() { + let s = "{и}"; + let ш = 12; + let s = "{ш}"; //~ literal_string_with_formatting_args +} + fn main() { let x: Option = None; let y = "hello"; diff --git a/tests/ui/literal_string_with_formatting_arg.stderr b/tests/ui/literal_string_with_formatting_arg.stderr index 1898f4f57b82..021983056bf6 100644 --- a/tests/ui/literal_string_with_formatting_arg.stderr +++ b/tests/ui/literal_string_with_formatting_arg.stderr @@ -1,77 +1,83 @@ error: this looks like a formatting argument but it is not part of a formatting macro - --> tests/ui/literal_string_with_formatting_arg.rs:25:15 + --> tests/ui/literal_string_with_formatting_arg.rs:26:14 | -LL | x.expect("{y} {}"); - | ^^^ +LL | let s = "{ш}"; + | ^^^ | = note: `-D clippy::literal-string-with-formatting-args` implied by `-D warnings` = 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:26:16 + --> tests/ui/literal_string_with_formatting_arg.rs:32:15 + | +LL | x.expect("{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:33: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:27:15 + --> tests/ui/literal_string_with_formatting_arg.rs:34: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:28:15 + --> tests/ui/literal_string_with_formatting_arg.rs:35: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:29:16 + --> tests/ui/literal_string_with_formatting_arg.rs:36: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:30:23 + --> tests/ui/literal_string_with_formatting_arg.rs:37: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:31:16 + --> tests/ui/literal_string_with_formatting_arg.rs:38: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:32:16 + --> tests/ui/literal_string_with_formatting_arg.rs:39: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:33:19 + --> tests/ui/literal_string_with_formatting_arg.rs:40: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:34:14 + --> tests/ui/literal_string_with_formatting_arg.rs:41: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 + --> tests/ui/literal_string_with_formatting_arg.rs:43: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:46:19 + --> tests/ui/literal_string_with_formatting_arg.rs:53:19 | LL | x.expect(r##" {x:?} "##); // `x` doesn't exist so we shoud not lint | ^^^^^ -error: aborting due to 12 previous errors +error: aborting due to 13 previous errors