From 076d9ca7f79b740577396896a1bdc330e69ff305 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 19 Nov 2024 18:25:12 +0100 Subject: [PATCH] Limit `literal_string_with_formatting_args` to known variables --- clippy_lints/src/lib.rs | 2 +- .../literal_string_with_formatting_args.rs | 108 +++++++++++++----- lintcheck/src/main.rs | 2 +- .../ui/literal_string_with_formatting_arg.rs | 2 + .../literal_string_with_formatting_arg.stderr | 36 ++++-- tests/ui/regex.stderr | 16 +-- 6 files changed, 113 insertions(+), 53 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 4f003377a5e5..017e6e2a1423 100644 --- 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 index 6514a72a44da..1ef4924f54f4 100644 --- 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,61 @@ 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) { +fn emit_lint(cx: &LateContext<'_>, expr: &Expr<'_>, spans: &[(Span, Option)]) { + 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::>(); + 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 +127,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", - ); - }, - } + emit_lint(cx, expr, &spans); } } } diff --git a/lintcheck/src/main.rs b/lintcheck/src/main.rs index db7710a7a381..03e2a24f6f98 100644 --- 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 index fdb8ba606210..b4b136b58632 100644 --- 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 index 515ae978f7a8..33b3b4d8d9df 100644 --- 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 index 919bac8f52f0..50ebc8a53388 100644 --- a/tests/ui/regex.stderr +++ b/tests/ui/regex.stderr @@ -196,13 +196,13 @@ 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 { | ^^^^ @@ -210,37 +210,37 @@ LL | loop { = 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 { | ^^^^^^^^^^^^^^