Auto merge of #9586 - Alexendoo:format-args-commas, r=Manishearth

FormatArgsExpn: Find comma spans and ignore weird proc macro spans

Fixes the following cases:

A missing `, 1` from the `expect_fun_call` suggestion:

```rust
Some(()).expect(&format!("{x} {}", 1));
```
```
warning: use of `expect` followed by a function call
 --> t.rs:7:14
  |
7 |     Some(()).expect(&format!("{x} {}", 1));
  |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{x} {}"))`
```

The suggestion removing from the comma in the comment rather than the one after the format string:

```rust
println!(
    "{}",
    // a comment, with a comma in it
    x
);
```
```
warning: variables can be used directly in the `format!` string
  --> t.rs:9:5
   |
9  | /     println!(
10 | |         "{}",
11 | |         // a comment, with a comma in it
12 | |         x
13 | |     );
   | |_____^
   |
help: change this to
   |
10 ~         "{x}",
11 ~         // a comment
   |
```

It also no longer accepts expansions where a format string or argument has a "weird" proc macro span, that is one where the literal/expression it outputs has the span of one of its inputs. Kind of like a `format_args` specific `clippy_utils::is_from_proc_macro`, e.g. `format!(indoc! {" ... "})`

changelog: [`expect_fun_call`]: Fix suggestion for `format!` using captured variables
changelog: [`print_literal`], [`write_literal`], [`uninlined_format_args`]: Fix suggestion when following a comment including a comma

Author: bors

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 {

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",

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`
@@ -765,12 +767,82 @@ 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> {
+    /// 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)
+    }
+
     pub fn parse(cx: &LateContext<'_>, expr: &'tcx Expr<'tcx>) -> Option<Self> {
         let macro_name = macro_backtrace(expr.span)
             .map(|macro_call| cx.tcx.item_name(macro_call.def_id))
@@ -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 = Self::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

tests/ui/expect_fun_call.fixed

@@ -100,4 +100,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));
 }

tests/ui/expect_fun_call.rs

@@ -100,4 +100,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));
 }

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:105: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:108: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
 

tests/ui/uninlined_format_args.fixed

@@ -1,21 +1,19 @@
+// aux-build:proc_macro_with_span.rs
 // run-rustfix
 #![feature(custom_inner_attributes)]
 #![warn(clippy::uninlined_format_args)]
 #![allow(named_arguments_used_positionally, unused_imports, unused_macros, unused_variables)]
 #![allow(clippy::eq_op, clippy::format_in_format_args, clippy::print_literal)]
 
+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),*)
@@ -140,11 +138,13 @@ 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!(with_span!("{0} {1}" "{1} {0}"), local_i32, local_f64);
+    println!("{}", with_span!(span val));
 }
 
 fn main() {

tests/ui/uninlined_format_args.rs

@@ -1,21 +1,19 @@
+// aux-build:proc_macro_with_span.rs
 // run-rustfix
 #![feature(custom_inner_attributes)]
 #![warn(clippy::uninlined_format_args)]
 #![allow(named_arguments_used_positionally, unused_imports, unused_macros, unused_variables)]
 #![allow(clippy::eq_op, clippy::format_in_format_args, clippy::print_literal)]
 
+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),*)
@@ -143,11 +141,15 @@ 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!(with_span!("{0} {1}" "{1} {0}"), local_i32, local_f64);
+    println!("{}", with_span!(span val));
 }
 
 fn main() {