Limit literal_string_with_formatting_args to known variables

GuillaumeGomez · GuillaumeGomez · commit ffce1c1f2cd4 · 2024-11-19T18:43:07.000+01:00
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -962,7 +962,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(move |_| Box::new(manual_div_ceil::ManualDivCeil::new(conf)));
     store.register_late_pass(|_| Box::new(manual_is_power_of_two::ManualIsPowerOfTwo));
     store.register_late_pass(|_| Box::new(non_zero_suggestions::NonZeroSuggestions));
-    store.register_early_pass(|| Box::new(literal_string_with_formatting_args::LiteralStringWithFormattingArg));
+    store.register_late_pass(|_| Box::new(literal_string_with_formatting_args::LiteralStringWithFormattingArg));
     store.register_late_pass(move |_| Box::new(unused_trait_names::UnusedTraitNames::new(conf)));
     store.register_late_pass(|_| Box::new(manual_ignore_case_cmp::ManualIgnoreCaseCmp));
     store.register_late_pass(|_| Box::new(unnecessary_literal_bound::UnnecessaryLiteralBound));
diff --git a/clippy_lints/src/literal_string_with_formatting_args.rs b/clippy_lints/src/literal_string_with_formatting_args.rs
@@ -1,11 +1,13 @@
-use rustc_ast::ast::{Expr, ExprKind};
-use rustc_ast::token::LitKind;
-use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_ast::{LitKind, StrStyle};
+use rustc_hir::{Expr, ExprKind};
+use rustc_lexer::is_ident;
+use rustc_lint::{LateContext, LateLintPass};
 use rustc_parse_format::{ParseMode, Parser, Piece};
 use rustc_session::declare_lint_pass;
-use rustc_span::BytePos;
+use rustc_span::{BytePos, Span};
 
 use clippy_utils::diagnostics::span_lint;
+use clippy_utils::mir::enclosing_mir;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -35,18 +37,63 @@ declare_clippy_lint! {
 
 declare_lint_pass!(LiteralStringWithFormattingArg => [LITERAL_STRING_WITH_FORMATTING_ARGS]);
 
-impl EarlyLintPass for LiteralStringWithFormattingArg {
-    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
+impl LiteralStringWithFormattingArg {
+    fn emit_lint(&self, cx: &LateContext<'_>, expr: &Expr<'_>, spans: Vec<(Span, Option<String>)>) {
+        if !spans.is_empty()
+            && let Some(mir) = enclosing_mir(cx.tcx, expr.hir_id)
+        {
+            let spans = spans
+                .iter()
+                .filter_map(|(span, name)| {
+                    if let Some(name) = name {
+                        // We need to check that the name is a local.
+                        if !mir.var_debug_info.iter().any(|local| local.name.as_str() == name) {
+                            return None;
+                        }
+                    }
+                    Some(*span)
+                })
+                .collect::<Vec<_>>();
+            match spans.len() {
+                0 => {},
+                1 => {
+                    span_lint(
+                        cx,
+                        LITERAL_STRING_WITH_FORMATTING_ARGS,
+                        spans,
+                        "this looks like a formatting argument but it is not part of a formatting macro",
+                    );
+                },
+                _ => {
+                    span_lint(
+                        cx,
+                        LITERAL_STRING_WITH_FORMATTING_ARGS,
+                        spans,
+                        "these look like formatting arguments but are not part of a formatting macro",
+                    );
+                },
+            }
+        }
+    }
+}
+
+impl LateLintPass<'_> for LiteralStringWithFormattingArg {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
         if expr.span.from_expansion() {
             return;
         }
         if let ExprKind::Lit(lit) = expr.kind {
-            let add = match lit.kind {
-                LitKind::Str => 1,
-                LitKind::StrRaw(nb) => nb as usize + 2,
+            let (add, symbol) = match lit.node {
+                LitKind::Str(symbol, style) => {
+                    let add = match style {
+                        StrStyle::Cooked => 1,
+                        StrStyle::Raw(nb) => nb as usize + 2,
+                    };
+                    (add, symbol)
+                },
                 _ => return,
             };
-            let fmt_str = lit.symbol.as_str();
+            let fmt_str = symbol.as_str();
             let lo = expr.span.lo();
             let mut current = fmt_str;
             let mut diff_len = 0;
@@ -82,32 +129,33 @@ impl EarlyLintPass for LiteralStringWithFormattingArg {
                         .find('}')
                         .map_or(pos.end, |found| start + 1 + found)
                         + 1;
-                    spans.push(
+                    let ident_start = start + 1;
+                    let colon_pos = fmt_str[ident_start..end].find(':');
+                    let ident_end = colon_pos.unwrap_or(end - 1);
+                    let mut name = None;
+                    if ident_start < ident_end
+                        && let arg = &fmt_str[ident_start..ident_end]
+                        && !arg.is_empty()
+                    {
+                        if is_ident(arg) {
+                            name = Some(arg.to_string());
+                        } else if colon_pos.is_none() && !arg.chars().all(|c| c.is_ascii_digit()) {
+                            // Not a `{0}` or a `{0:?}`.
+                            continue;
+                        }
+                    } else if colon_pos.is_none() {
+                        // Not a `{:?}`.
+                        continue;
+                    }
+                    spans.push((
                         expr.span
                             .with_hi(lo + BytePos((start + add).try_into().unwrap()))
                             .with_lo(lo + BytePos((end + add).try_into().unwrap())),
-                    );
+                        name,
+                    ));
                 }
             }
-            match spans.len() {
-                0 => {},
-                1 => {
-                    span_lint(
-                        cx,
-                        LITERAL_STRING_WITH_FORMATTING_ARGS,
-                        spans,
-                        "this looks like a formatting argument but it is not part of a formatting macro",
-                    );
-                },
-                _ => {
-                    span_lint(
-                        cx,
-                        LITERAL_STRING_WITH_FORMATTING_ARGS,
-                        spans,
-                        "these look like formatting arguments but are not part of a formatting macro",
-                    );
-                },
-            }
+            self.emit_lint(cx, expr, spans);
         }
     }
 }
diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs
@@ -18,7 +18,7 @@
     clippy::collapsible_else_if,
     clippy::needless_borrows_for_generic_args,
     clippy::module_name_repetitions,
-    clippy::literal_string_with_formatting_arg
+    clippy::literal_string_with_formatting_args
 )]
 
 mod config;
diff --git a/tests/ui/literal_string_with_formatting_arg.rs b/tests/ui/literal_string_with_formatting_arg.rs
@@ -6,6 +6,7 @@ fn main() {
     let y = "hello";
     x.expect("{y} {}"); //~ literal_string_with_formatting_args
     x.expect(" {y} bla"); //~ literal_string_with_formatting_args
+    x.expect(" {0}"); //~ literal_string_with_formatting_args
     x.expect("{:?}"); //~ literal_string_with_formatting_args
     x.expect("{y:?}"); //~ literal_string_with_formatting_args
     x.expect(" {y:?} {y:?} "); //~ literal_string_with_formatting_args
@@ -23,6 +24,7 @@ fn main() {
     x.expect(" {   } "); // For now we ignore `{}` to limit false positives.
     x.expect("{{y} {x");
     x.expect("{{y:?}");
+    x.expect(r##" {x:?} "##); // `x` doesn't exist so we shoud not lint
     x.expect("{y:...}");
     let _ = "fn main {\n\
     }";
diff --git a/tests/ui/literal_string_with_formatting_arg.stderr b/tests/ui/literal_string_with_formatting_arg.stderr
@@ -1,65 +1,77 @@
 error: this looks like a formatting argument but it is not part of a formatting macro
-  --> tests/ui/literal_string_with_formatting_args.rs:7:15
+  --> tests/ui/literal_string_with_formatting_arg.rs:7:15
    |
 LL |     x.expect("{y} {}");
    |               ^^^
    |
-   = note: `-D clippy::literal-string-with-formatting-arg` implied by `-D warnings`
+   = 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_args.rs:8:16
+  --> tests/ui/literal_string_with_formatting_arg.rs:8: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_args.rs:9:15
+  --> tests/ui/literal_string_with_formatting_arg.rs:9:16
+   |
+LL |     x.expect(" {0}");
+   |                ^^^
+
+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
    |
 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_args.rs:10:15
+  --> tests/ui/literal_string_with_formatting_arg.rs:11: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_args.rs:11:16
+  --> tests/ui/literal_string_with_formatting_arg.rs:12: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_args.rs:12:23
+  --> tests/ui/literal_string_with_formatting_arg.rs:13: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_args.rs:13:16
+  --> tests/ui/literal_string_with_formatting_arg.rs:14: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_args.rs:14:16
+  --> tests/ui/literal_string_with_formatting_arg.rs:15: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_args.rs:15:19
+  --> tests/ui/literal_string_with_formatting_arg.rs:16: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_args.rs:17:18
+  --> tests/ui/literal_string_with_formatting_arg.rs:18:18
    |
 LL |     x.expect("———{:?}");
    |                  ^^^^
 
-error: aborting due to 10 previous errors
+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
+   |
+LL |     x.expect(r##" {x:?} "##); // `x` doesn't exist so we shoud not lint
+   |                   ^^^^^
+
+error: aborting due to 12 previous errors
 
diff --git a/tests/ui/regex.stderr b/tests/ui/regex.stderr
@@ -196,51 +196,51 @@ LL |     let binary_trivial_empty = BRegex::new("^$");
    = help: consider using `str::is_empty`
 
 error: compiling a regex in a loop
-  --> tests/ui/regex.rs:125:21
+  --> tests/ui/regex.rs:126:21
    |
 LL |         let regex = Regex::new("a.b");
    |                     ^^^^^^^^^^
    |
 help: move the regex construction outside this loop
-  --> tests/ui/regex.rs:122:5
+  --> tests/ui/regex.rs:123:5
    |
 LL |     loop {
    |     ^^^^
    = note: `-D clippy::regex-creation-in-loops` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::regex_creation_in_loops)]`
 
 error: compiling a regex in a loop
-  --> tests/ui/regex.rs:127:21
+  --> tests/ui/regex.rs:128:21
    |
 LL |         let regex = BRegex::new("a.b");
    |                     ^^^^^^^^^^^
    |
 help: move the regex construction outside this loop
-  --> tests/ui/regex.rs:122:5
+  --> tests/ui/regex.rs:123:5
    |
 LL |     loop {
    |     ^^^^
 
 error: compiling a regex in a loop
-  --> tests/ui/regex.rs:133:25
+  --> tests/ui/regex.rs:134:25
    |
 LL |             let regex = Regex::new("a.b");
    |                         ^^^^^^^^^^
    |
 help: move the regex construction outside this loop
-  --> tests/ui/regex.rs:122:5
+  --> tests/ui/regex.rs:123:5
    |
 LL |     loop {
    |     ^^^^
 
 error: compiling a regex in a loop
-  --> tests/ui/regex.rs:138:32
+  --> tests/ui/regex.rs:139:32
    |
 LL |             let nested_regex = Regex::new("a.b");
    |                                ^^^^^^^^^^
    |
 help: move the regex construction outside this loop
-  --> tests/ui/regex.rs:137:9
+  --> tests/ui/regex.rs:138:9
    |
 LL |         for _ in 0..10 {
    |         ^^^^^^^^^^^^^^

Original file line number	Diff line number	Diff line change
`@@ -196,51 +196,51 @@ LL \| let binary_trivial_empty = BRegex::new("^$");`
`196`	`196`	= help: consider using `str::is_empty`
`197`	`197`
`198`	`198`	`error: compiling a regex in a loop`
`199`		`- --> tests/ui/regex.rs:125:21`
	`199`	`+ --> tests/ui/regex.rs:126:21`
`200`	`200`	`\|`
`201`	`201`	`LL \| let regex = Regex::new("a.b");`
`202`	`202`	`\| ^^^^^^^^^^`
`203`	`203`	`\|`
`204`	`204`	`help: move the regex construction outside this loop`
`205`		`- --> tests/ui/regex.rs:122:5`
	`205`	`+ --> tests/ui/regex.rs:123:5`
`206`	`206`	`\|`
`207`	`207`	`LL \| loop {`
`208`	`208`	`\| ^^^^`
`209`	`209`	= note: `-D clippy::regex-creation-in-loops` implied by `-D warnings`
`210`	`210`	= help: to override `-D warnings` add `#[allow(clippy::regex_creation_in_loops)]`
`211`	`211`
`212`	`212`	`error: compiling a regex in a loop`
`213`		`- --> tests/ui/regex.rs:127:21`
	`213`	`+ --> tests/ui/regex.rs:128:21`
`214`	`214`	`\|`
`215`	`215`	`LL \| let regex = BRegex::new("a.b");`
`216`	`216`	`\| ^^^^^^^^^^^`
`217`	`217`	`\|`
`218`	`218`	`help: move the regex construction outside this loop`
`219`		`- --> tests/ui/regex.rs:122:5`
	`219`	`+ --> tests/ui/regex.rs:123:5`
`220`	`220`	`\|`
`221`	`221`	`LL \| loop {`
`222`	`222`	`\| ^^^^`
`223`	`223`
`224`	`224`	`error: compiling a regex in a loop`
`225`		`- --> tests/ui/regex.rs:133:25`
	`225`	`+ --> tests/ui/regex.rs:134:25`
`226`	`226`	`\|`
`227`	`227`	`LL \| let regex = Regex::new("a.b");`
`228`	`228`	`\| ^^^^^^^^^^`
`229`	`229`	`\|`
`230`	`230`	`help: move the regex construction outside this loop`
`231`		`- --> tests/ui/regex.rs:122:5`
	`231`	`+ --> tests/ui/regex.rs:123:5`
`232`	`232`	`\|`
`233`	`233`	`LL \| loop {`
`234`	`234`	`\| ^^^^`
`235`	`235`
`236`	`236`	`error: compiling a regex in a loop`
`237`		`- --> tests/ui/regex.rs:138:32`
	`237`	`+ --> tests/ui/regex.rs:139:32`
`238`	`238`	`\|`
`239`	`239`	`LL \| let nested_regex = Regex::new("a.b");`
`240`	`240`	`\| ^^^^^^^^^^`
`241`	`241`	`\|`
`242`	`242`	`help: move the regex construction outside this loop`
`243`		`- --> tests/ui/regex.rs:137:9`
	`243`	`+ --> tests/ui/regex.rs:138:9`
`244`	`244`	`\|`
`245`	`245`	`LL \| for _ in 0..10 {`
`246`	`246`	`\| ^^^^^^^^^^^^^^`