FormatArgsExpn: Find comma spans and ignore weird proc macro spans

Alexendoo · Alexendoo · commit 465e2972eb95 · 2022-10-03T20:46:30.000Z
diff --git a/clippy_lints/src/format_args.rs b/clippy_lints/src/format_args.rs
@@ -1,13 +1,13 @@
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::macros::FormatParamKind::{Implicit, Named, Numbered, Starred};
 use clippy_utils::macros::{is_format_macro, FormatArgsExpn, FormatParam, FormatParamUsage};
-use clippy_utils::source::{expand_past_previous_comma, snippet_opt};
+use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::implements_trait;
 use clippy_utils::{is_diag_trait_item, meets_msrv, msrvs};
 use if_chain::if_chain;
 use itertools::Itertools;
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind, HirId, Path, QPath};
+use rustc_hir::{Expr, ExprKind, HirId, QPath};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::adjustment::{Adjust, Adjustment};
 use rustc_middle::ty::Ty;
@@ -169,7 +169,7 @@ fn check_uninlined_args(cx: &LateContext<'_>, args: &FormatArgsExpn<'_>, call_si
     // we cannot remove any other arguments in the format string,
     // because the index numbers might be wrong after inlining.
     // Example of an un-inlinable format:  print!("{}{1}", foo, 2)
-    if !args.params().all(|p| check_one_arg(cx, &p, &mut fixes)) || fixes.is_empty() {
+    if !args.params().all(|p| check_one_arg(args, &p, &mut fixes)) || fixes.is_empty() {
         return;
     }
 
@@ -196,19 +196,18 @@ fn check_uninlined_args(cx: &LateContext<'_>, args: &FormatArgsExpn<'_>, call_si
     );
 }
 
-fn check_one_arg(cx: &LateContext<'_>, param: &FormatParam<'_>, fixes: &mut Vec<(Span, String)>) -> bool {
+fn check_one_arg(args: &FormatArgsExpn<'_>, param: &FormatParam<'_>, fixes: &mut Vec<(Span, String)>) -> bool {
     if matches!(param.kind, Implicit | Starred | Named(_) | Numbered)
         && let ExprKind::Path(QPath::Resolved(None, path)) = param.value.kind
-        && let Path { span, segments, .. } = path
-        && let [segment] = segments
+        && let [segment] = path.segments
+        && let Some(arg_span) = args.value_with_prev_comma_span(param.value.hir_id)
     {
         let replacement = match param.usage {
             FormatParamUsage::Argument => segment.ident.name.to_string(),
             FormatParamUsage::Width => format!("{}$", segment.ident.name),
             FormatParamUsage::Precision => format!(".{}$", segment.ident.name),
         };
         fixes.push((param.span, replacement));
-        let arg_span = expand_past_previous_comma(cx, *span);
         fixes.push((arg_span, String::new()));
         true  // successful inlining, continue checking
     } else {
diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs
@@ -475,11 +475,11 @@ fn check_literal(cx: &LateContext<'_>, format_args: &FormatArgsExpn<'_>, name: &
                 value.span,
                 "literal with an empty format string",
                 |diag| {
-                    if let Some(replacement) = replacement {
+                    if let Some(replacement) = replacement
                         // `format!("{}", "a")`, `format!("{named}", named = "b")
                         //              ~~~~~                      ~~~~~~~~~~~~~
-                        let value_span = expand_past_previous_comma(cx, value.span);
-
+                        && let Some(value_span) = format_args.value_with_prev_comma_span(value.hir_id)
+                    {
                         let replacement = replacement.replace('{', "{{").replace('}', "}}");
                         diag.multipart_suggestion(
                             "try this",
diff --git a/clippy_utils/src/macros.rs b/clippy_utils/src/macros.rs
@@ -16,6 +16,7 @@ use rustc_parse_format::{self as rpf, Alignment};
 use rustc_span::def_id::DefId;
 use rustc_span::hygiene::{self, MacroKind, SyntaxContext};
 use rustc_span::{sym, BytePos, ExpnData, ExpnId, ExpnKind, Pos, Span, SpanData, Symbol};
+use std::iter::{once, zip};
 use std::ops::ControlFlow;
 
 const FORMAT_MACRO_DIAG_ITEMS: &[Symbol] = &[
@@ -412,7 +413,8 @@ impl FormatString {
 }
 
 struct FormatArgsValues<'tcx> {
-    /// See `FormatArgsExpn::value_args`
+    /// Values passed after the format string and implicit captures. `[1, z + 2, x]` for
+    /// `format!("{x} {} {y}", 1, z + 2)`.
     value_args: Vec<&'tcx Expr<'tcx>>,
     /// Maps an `rt::v1::Argument::position` or an `rt::v1::Count::Param` to its index in
     /// `value_args`
@@ -675,6 +677,68 @@ impl<'tcx> Count<'tcx> {
     }
 }
 
+/// Gets the spans of the commas inbetween the format string and explicit args, not including any
+/// trailing comma
+///
+/// ```ignore
+/// format!("{} {}", a, b)
+/// //             ^  ^
+/// ```
+///
+/// Ensures that the format string and values aren't coming from a proc macro that sets the output
+/// span to that of its input
+fn comma_spans(cx: &LateContext<'_>, explicit_values: &[&Expr<'_>], fmt_span: Span) -> Option<Vec<Span>> {
+    // `format!("{} {} {c}", "one", "two", c = "three")`
+    //                       ^^^^^  ^^^^^      ^^^^^^^
+    let value_spans = explicit_values
+        .iter()
+        .map(|val| hygiene::walk_chain(val.span, fmt_span.ctxt()));
+
+    // `format!("{} {} {c}", "one", "two", c = "three")`
+    //                     ^^     ^^     ^^^^^^
+    let between_spans = once(fmt_span)
+        .chain(value_spans)
+        .tuple_windows()
+        .map(|(start, end)| start.between(end));
+
+    let mut comma_spans = Vec::new();
+    for between_span in between_spans {
+        let mut offset = 0;
+        let mut seen_comma = false;
+
+        for token in tokenize(&snippet_opt(cx, between_span)?) {
+            match token.kind {
+                TokenKind::LineComment { .. } | TokenKind::BlockComment { .. } | TokenKind::Whitespace => {},
+                TokenKind::Comma if !seen_comma => {
+                    seen_comma = true;
+
+                    let base = between_span.data();
+                    comma_spans.push(Span::new(
+                        base.lo + BytePos(offset),
+                        base.lo + BytePos(offset + 1),
+                        base.ctxt,
+                        base.parent,
+                    ));
+                },
+                // named arguments, `start_val, name = end_val`
+                //                            ^^^^^^^^^ between_span
+                TokenKind::Ident | TokenKind::Eq if seen_comma => {},
+                // An unexpected token usually indicates the format string or a value came from a proc macro output that
+                // sets the span of its output to an input, e.g. `println!(some_proc_macro!("input"), ..)` that emits a
+                // string literal with the span set to that of `"input"`
+                _ => return None,
+            }
+            offset += token.len;
+        }
+
+        if !seen_comma {
+            return None;
+        }
+    }
+
+    Some(comma_spans)
+}
+
 /// Specification for the formatting of an argument in the format string. See
 /// <https://doc.rust-lang.org/std/fmt/index.html#formatting-parameters> for the precise meanings.
 #[derive(Debug)]
@@ -765,9 +829,17 @@ pub struct FormatArgsExpn<'tcx> {
     /// Has an added newline due to `println!()`/`writeln!()`/etc. The last format string part will
     /// include this added newline.
     pub newline: bool,
-    /// Values passed after the format string and implicit captures. `[1, z + 2, x]` for
+    /// Spans of the commas between the format string and explicit values, excluding any trailing
+    /// comma
+    ///
+    /// ```ignore
+    /// format!("..", 1, 2, 3,)
+    /// //          ^  ^  ^
+    /// ```
+    comma_spans: Vec<Span>,
+    /// Explicit values passed after the format string, ignoring implicit captures. `[1, z + 2]` for
     /// `format!("{x} {} {y}", 1, z + 2)`.
-    value_args: Vec<&'tcx Expr<'tcx>>,
+    explicit_values: Vec<&'tcx Expr<'tcx>>,
 }
 
 impl<'tcx> FormatArgsExpn<'tcx> {
@@ -845,11 +917,22 @@ impl<'tcx> FormatArgsExpn<'tcx> {
                 })
                 .collect::<Option<Vec<_>>>()?;
 
+            let mut explicit_values = values.value_args;
+            // remove values generated for implicitly captured vars
+            let len = explicit_values
+                .iter()
+                .take_while(|val| !format_string.span.contains(val.span))
+                .count();
+            explicit_values.truncate(len);
+
+            let comma_spans = comma_spans(cx, &explicit_values, format_string.span)?;
+
             Some(Self {
                 format_string,
                 args,
-                value_args: values.value_args,
                 newline,
+                comma_spans,
+                explicit_values,
             })
         } else {
             None
@@ -875,7 +958,7 @@ impl<'tcx> FormatArgsExpn<'tcx> {
 
     /// Source callsite span of all inputs
     pub fn inputs_span(&self) -> Span {
-        match *self.value_args {
+        match *self.explicit_values {
             [] => self.format_string.span,
             [.., last] => self
                 .format_string
@@ -884,6 +967,22 @@ impl<'tcx> FormatArgsExpn<'tcx> {
         }
     }
 
+    /// Get the span of a value expanded to the previous comma, e.g. for the value `10`
+    ///
+    /// ```ignore
+    /// format("{}.{}", 10, 11)
+    /// //            ^^^^
+    /// ```
+    pub fn value_with_prev_comma_span(&self, value_id: HirId) -> Option<Span> {
+        for (comma_span, value) in zip(&self.comma_spans, &self.explicit_values) {
+            if value.hir_id == value_id {
+                return Some(comma_span.to(hygiene::walk_chain(value.span, comma_span.ctxt())));
+            }
+        }
+
+        None
+    }
+
     /// Iterator of all format params, both values and those referenced by `width`/`precision`s.
     pub fn params(&'tcx self) -> impl Iterator<Item = FormatParam<'tcx>> {
         self.args
diff --git a/tests/ui/expect_fun_call.fixed b/tests/ui/expect_fun_call.fixed
@@ -101,4 +101,10 @@ fn main() {
         let opt_ref = &opt;
         opt_ref.unwrap_or_else(|| panic!("{:?}", opt_ref));
     }
+
+    let format_capture: Option<i32> = None;
+    format_capture.unwrap_or_else(|| panic!("{error_code}"));
+
+    let format_capture_and_value: Option<i32> = None;
+    format_capture_and_value.unwrap_or_else(|| panic!("{error_code}, {}", 1));
 }
diff --git a/tests/ui/expect_fun_call.rs b/tests/ui/expect_fun_call.rs
@@ -101,4 +101,10 @@ fn main() {
         let opt_ref = &opt;
         opt_ref.expect(&format!("{:?}", opt_ref));
     }
+
+    let format_capture: Option<i32> = None;
+    format_capture.expect(&format!("{error_code}"));
+
+    let format_capture_and_value: Option<i32> = None;
+    format_capture_and_value.expect(&format!("{error_code}, {}", 1));
 }
diff --git a/tests/ui/expect_fun_call.stderr b/tests/ui/expect_fun_call.stderr
@@ -78,5 +78,17 @@ error: use of `expect` followed by a function call
 LL |         opt_ref.expect(&format!("{:?}", opt_ref));
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{:?}", opt_ref))`
 
-error: aborting due to 13 previous errors
+error: use of `expect` followed by a function call
+  --> $DIR/expect_fun_call.rs:106:20
+   |
+LL |     format_capture.expect(&format!("{error_code}"));
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{error_code}"))`
+
+error: use of `expect` followed by a function call
+  --> $DIR/expect_fun_call.rs:109:30
+   |
+LL |     format_capture_and_value.expect(&format!("{error_code}, {}", 1));
+   |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{error_code}, {}", 1))`
+
+error: aborting due to 15 previous errors
 
diff --git a/tests/ui/uninlined_format_args.fixed b/tests/ui/uninlined_format_args.fixed
@@ -1,3 +1,4 @@
+// aux-build:proc_macro_with_span.rs
 // run-rustfix
 
 #![allow(clippy::eq_op)]
@@ -8,18 +9,15 @@
 #![warn(clippy::uninlined_format_args)]
 #![feature(custom_inner_attributes)]
 
+extern crate proc_macro_with_span;
+use proc_macro_with_span::with_span;
+
 macro_rules! no_param_str {
     () => {
         "{}"
     };
 }
 
-macro_rules! pass_through {
-    ($expr:expr) => {
-        $expr
-    };
-}
-
 macro_rules! my_println {
    ($($args:tt),*) => {{
         println!($($args),*)
@@ -144,11 +142,14 @@ fn tester(fn_arg: i32) {
     );
     println!(no_param_str!(), local_i32);
 
-    // FIXME: bugs!
-    // println!(pass_through!("foo={local_i32}"), local_i32 = local_i32);
-    // println!(pass_through!("foo={}"), local_i32);
-    // println!(indoc!("foo={}"), local_i32);
-    // printdoc!("foo={}", local_i32);
+    println!(
+        "{val}",
+    );
+    println!("{val}");
+    println!("{val}");
+
+    println!(with_span!("{0} {1}" "{1} {0}"), local_i32, local_f64);
+    println!("{}", with_span!(span val));
 }
 
 fn main() {
diff --git a/tests/ui/uninlined_format_args.rs b/tests/ui/uninlined_format_args.rs
@@ -1,3 +1,4 @@
+// aux-build:proc_macro_with_span.rs
 // run-rustfix
 
 #![allow(clippy::eq_op)]
@@ -8,18 +9,15 @@
 #![warn(clippy::uninlined_format_args)]
 #![feature(custom_inner_attributes)]
 
+extern crate proc_macro_with_span;
+use proc_macro_with_span::with_span;
+
 macro_rules! no_param_str {
     () => {
         "{}"
     };
 }
 
-macro_rules! pass_through {
-    ($expr:expr) => {
-        $expr
-    };
-}
-
 macro_rules! my_println {
    ($($args:tt),*) => {{
         println!($($args),*)
@@ -147,11 +145,16 @@ fn tester(fn_arg: i32) {
     );
     println!(no_param_str!(), local_i32);
 
-    // FIXME: bugs!
-    // println!(pass_through!("foo={local_i32}"), local_i32 = local_i32);
-    // println!(pass_through!("foo={}"), local_i32);
-    // println!(indoc!("foo={}"), local_i32);
-    // printdoc!("foo={}", local_i32);
+    println!(
+        "{}",
+        // comment with a comma , in it
+        val,
+    );
+    println!("{}", /* comment with a comma , in it */ val);
+    println!("{named}", named = val);
+
+    println!(with_span!("{0} {1}" "{1} {0}"), local_i32, local_f64);
+    println!("{}", with_span!(span val));
 }
 
 fn main() {
diff --git a/tests/ui/uninlined_format_args.stderr b/tests/ui/uninlined_format_args.stderr

Original file line number	Diff line number	Diff line change
`@@ -101,4 +101,10 @@ fn main() {`
`101`	`101`	`let opt_ref = &opt;`
`102`	`102`	`opt_ref.unwrap_or_else(\|\| panic!("{:?}", opt_ref));`
`103`	`103`	`}`
	`104`	`+`
	`105`	`+ let format_capture: Option<i32> = None;`
	`106`	`+ format_capture.unwrap_or_else(\|\| panic!("{error_code}"));`
	`107`	`+`
	`108`	`+ let format_capture_and_value: Option<i32> = None;`
	`109`	`+ format_capture_and_value.unwrap_or_else(\|\| panic!("{error_code}, {}", 1));`
`104`	`110`	`}`