Rollup merge of #109664 - m-ou-se:format-args-placeholder-span, r=oli-obk

Use span of placeholders in format_args!() expansion. `format_args!("{}", x)` expands to something that contains `Argument::new_display(&x)`. That entire expression was generated with the span of `x`. After this PR, `&x` uses the span of `x`, but the `new_display` call uses the span of the `{}` placeholder within the format string. If an implicitly captured argument was used like in `format_args!("{x}")`, both use the span of the `{x}` placeholder. This fixes https://github.com/rust-lang/rust/issues/109576, and also allows for more improvements to similar diagnostics in the future, since the usage of `x` can now be traced to the exact `{}` placeholder that required it to be `Display` (or `Debug` etc.)
2024-11-22 14:55:26 +00:00 · 2023-03-29 14:07:28 +05:30 · 2023-03-29 14:07:28 +05:30 · a3eb2f0f22
commit a3eb2f0f22
parent 39f93d3c98 6c72a002a6
8 changed files with 153 additions and 60 deletions
--- a/compiler/rustc_ast_lowering/src/format.rs
+++ b/compiler/rustc_ast_lowering/src/format.rs
@ -2,7 +2,7 @@ use super::LoweringContext;
 use rustc_ast as ast;
 use rustc_ast::visit::{self, Visitor};
 use rustc_ast::*;
-use rustc_data_structures::fx::FxIndexSet;
+use rustc_data_structures::fx::FxIndexMap;
 use rustc_hir as hir;
 use rustc_span::{
    sym,
@ -238,7 +238,7 @@ fn make_count<'hir>(
    ctx: &mut LoweringContext<'_, 'hir>,
    sp: Span,
    count: &Option<FormatCount>,
-    argmap: &mut FxIndexSet<(usize, ArgumentType)>,
+    argmap: &mut FxIndexMap<(usize, ArgumentType), Option<Span>>,
 ) -> hir::Expr<'hir> {
    match count {
        Some(FormatCount::Literal(n)) => {
@ -252,7 +252,7 @@ fn make_count<'hir>(
        }
        Some(FormatCount::Argument(arg)) => {
            if let Ok(arg_index) = arg.index {
-                let (i, _) = argmap.insert_full((arg_index, ArgumentType::Usize));
+                let (i, _) = argmap.insert_full((arg_index, ArgumentType::Usize), arg.span);
                let count_param = ctx.arena.alloc(ctx.expr_lang_item_type_relative(
                    sp,
                    hir::LangItem::FormatCount,
@ -291,12 +291,14 @@ fn make_format_spec<'hir>(
    ctx: &mut LoweringContext<'_, 'hir>,
    sp: Span,
    placeholder: &FormatPlaceholder,
-    argmap: &mut FxIndexSet<(usize, ArgumentType)>,
+    argmap: &mut FxIndexMap<(usize, ArgumentType), Option<Span>>,
 ) -> hir::Expr<'hir> {
    let position = match placeholder.argument.index {
        Ok(arg_index) => {
-            let (i, _) =
-                argmap.insert_full((arg_index, ArgumentType::Format(placeholder.format_trait)));
+            let (i, _) = argmap.insert_full(
+                (arg_index, ArgumentType::Format(placeholder.format_trait)),
+                placeholder.span,
+            );
            ctx.expr_usize(sp, i)
        }
        Err(_) => ctx.expr(
@ -386,7 +388,7 @@ fn expand_format_args<'hir>(

    // Create a list of all _unique_ (argument, format trait) combinations.
    // E.g. "{0} {0:x} {0} {1}" -> [(0, Display), (0, LowerHex), (1, Display)]
-    let mut argmap = FxIndexSet::default();
+    let mut argmap = FxIndexMap::default();
    for piece in &fmt.template {
        let FormatArgsPiece::Placeholder(placeholder) = piece else { continue };
        if placeholder.format_options != Default::default() {
@ -394,7 +396,10 @@ fn expand_format_args<'hir>(
            use_format_options = true;
        }
        if let Ok(index) = placeholder.argument.index {
-            if !argmap.insert((index, ArgumentType::Format(placeholder.format_trait))) {
+            if argmap
+                .insert((index, ArgumentType::Format(placeholder.format_trait)), placeholder.span)
+                .is_some()
+            {
                // Duplicate (argument, format trait) combination,
                // which we'll only put once in the args array.
                use_format_options = true;
@ -438,7 +443,7 @@ fn expand_format_args<'hir>(
    // This is an optimization, speeding up compilation about 1-2% in some cases.
    // See https://github.com/rust-lang/rust/pull/106770#issuecomment-1380790609
    let use_simple_array = argmap.len() == arguments.len()
-        && argmap.iter().enumerate().all(|(i, &(j, _))| i == j)
+        && argmap.iter().enumerate().all(|(i, (&(j, _), _))| i == j)
        && arguments.iter().skip(1).all(|arg| !may_contain_yield_point(&arg.expr));

    let args = if use_simple_array {
@ -452,14 +457,19 @@ fn expand_format_args<'hir>(
        let elements: Vec<_> = arguments
            .iter()
            .zip(argmap)
-            .map(|(arg, (_, ty))| {
-                let sp = arg.expr.span.with_ctxt(macsp.ctxt());
+            .map(|(arg, ((_, ty), placeholder_span))| {
+                let placeholder_span =
+                    placeholder_span.unwrap_or(arg.expr.span).with_ctxt(macsp.ctxt());
+                let arg_span = match arg.kind {
+                    FormatArgumentKind::Captured(_) => placeholder_span,
+                    _ => arg.expr.span.with_ctxt(macsp.ctxt()),
+                };
                let arg = ctx.lower_expr(&arg.expr);
                let ref_arg = ctx.arena.alloc(ctx.expr(
-                    sp,
+                    arg_span,
                    hir::ExprKind::AddrOf(hir::BorrowKind::Ref, hir::Mutability::Not, arg),
                ));
-                make_argument(ctx, sp, ref_arg, ty)
+                make_argument(ctx, placeholder_span, ref_arg, ty)
            })
            .collect();
        ctx.expr_array_ref(macsp, ctx.arena.alloc_from_iter(elements))
@ -475,16 +485,26 @@ fn expand_format_args<'hir>(
        //     }
        let args_ident = Ident::new(sym::args, macsp);
        let (args_pat, args_hir_id) = ctx.pat_ident(macsp, args_ident);
-        let args = ctx.arena.alloc_from_iter(argmap.iter().map(|&(arg_index, ty)| {
-            let arg = &arguments[arg_index];
-            let sp = arg.expr.span.with_ctxt(macsp.ctxt());
-            let args_ident_expr = ctx.expr_ident(macsp, args_ident, args_hir_id);
-            let arg = ctx.arena.alloc(ctx.expr(
-                sp,
-                hir::ExprKind::Field(args_ident_expr, Ident::new(sym::integer(arg_index), macsp)),
-            ));
-            make_argument(ctx, sp, arg, ty)
-        }));
+        let args = ctx.arena.alloc_from_iter(argmap.iter().map(
+            |(&(arg_index, ty), &placeholder_span)| {
+                let arg = &arguments[arg_index];
+                let placeholder_span =
+                    placeholder_span.unwrap_or(arg.expr.span).with_ctxt(macsp.ctxt());
+                let arg_span = match arg.kind {
+                    FormatArgumentKind::Captured(_) => placeholder_span,
+                    _ => arg.expr.span.with_ctxt(macsp.ctxt()),
+                };
+                let args_ident_expr = ctx.expr_ident(macsp, args_ident, args_hir_id);
+                let arg = ctx.arena.alloc(ctx.expr(
+                    arg_span,
+                    hir::ExprKind::Field(
+                        args_ident_expr,
+                        Ident::new(sym::integer(arg_index), macsp),
+                    ),
+                ));
+                make_argument(ctx, placeholder_span, arg, ty)
+            },
+        ));
        let elements: Vec<_> = arguments
            .iter()
            .map(|arg| {
--- a/tests/mir-opt/sroa/lifetimes.foo.ScalarReplacementOfAggregates.diff
+++ b/tests/mir-opt/sroa/lifetimes.foo.ScalarReplacementOfAggregates.diff
@ -19,12 +19,12 @@
      let mut _17: &[core::fmt::ArgumentV1<'_>; 2]; // in scope 0 at $SRC_DIR/std/src/macros.rs:LL:COL
      let _18: &[core::fmt::ArgumentV1<'_>; 2]; // in scope 0 at $SRC_DIR/std/src/macros.rs:LL:COL
      let _19: [core::fmt::ArgumentV1<'_>; 2]; // in scope 0 at $SRC_DIR/std/src/macros.rs:LL:COL
-      let mut _20: core::fmt::ArgumentV1<'_>; // in scope 0 at $DIR/lifetimes.rs:+10:21: +10:22
-      let mut _21: &std::boxed::Box<dyn std::fmt::Display>; // in scope 0 at $DIR/lifetimes.rs:+10:21: +10:22
-      let _22: &std::boxed::Box<dyn std::fmt::Display>; // in scope 0 at $DIR/lifetimes.rs:+10:21: +10:22
-      let mut _23: core::fmt::ArgumentV1<'_>; // in scope 0 at $DIR/lifetimes.rs:+10:25: +10:26
-      let mut _24: &u32;                   // in scope 0 at $DIR/lifetimes.rs:+10:25: +10:26
-      let _25: &u32;                       // in scope 0 at $DIR/lifetimes.rs:+10:25: +10:26
+      let mut _20: core::fmt::ArgumentV1<'_>; // in scope 0 at $DIR/lifetimes.rs:+10:20: +10:23
+      let mut _21: &std::boxed::Box<dyn std::fmt::Display>; // in scope 0 at $DIR/lifetimes.rs:+10:20: +10:23
+      let _22: &std::boxed::Box<dyn std::fmt::Display>; // in scope 0 at $DIR/lifetimes.rs:+10:20: +10:23
+      let mut _23: core::fmt::ArgumentV1<'_>; // in scope 0 at $DIR/lifetimes.rs:+10:24: +10:27
+      let mut _24: &u32;                   // in scope 0 at $DIR/lifetimes.rs:+10:24: +10:27
+      let _25: &u32;                       // in scope 0 at $DIR/lifetimes.rs:+10:24: +10:27
      let mut _27: bool;                   // in scope 0 at $DIR/lifetimes.rs:+12:1: +12:2
      let mut _28: isize;                  // in scope 0 at $DIR/lifetimes.rs:+12:1: +12:2
      let mut _29: isize;                  // in scope 0 at $DIR/lifetimes.rs:+12:1: +12:2
@ -108,34 +108,34 @@
          StorageLive(_17);                // scope 4 at $SRC_DIR/std/src/macros.rs:LL:COL
          StorageLive(_18);                // scope 4 at $SRC_DIR/std/src/macros.rs:LL:COL
          StorageLive(_19);                // scope 4 at $SRC_DIR/std/src/macros.rs:LL:COL
-          StorageLive(_20);                // scope 4 at $DIR/lifetimes.rs:+10:21: +10:22
-          StorageLive(_21);                // scope 4 at $DIR/lifetimes.rs:+10:21: +10:22
-          StorageLive(_22);                // scope 4 at $DIR/lifetimes.rs:+10:21: +10:22
-          _22 = &_8;                       // scope 4 at $DIR/lifetimes.rs:+10:21: +10:22
-          _21 = &(*_22);                   // scope 4 at $DIR/lifetimes.rs:+10:21: +10:22
-          _20 = core::fmt::ArgumentV1::<'_>::new_display::<Box<dyn std::fmt::Display>>(move _21) -> bb3; // scope 4 at $DIR/lifetimes.rs:+10:21: +10:22
+          StorageLive(_20);                // scope 4 at $DIR/lifetimes.rs:+10:20: +10:23
+          StorageLive(_21);                // scope 4 at $DIR/lifetimes.rs:+10:20: +10:23
+          StorageLive(_22);                // scope 4 at $DIR/lifetimes.rs:+10:20: +10:23
+          _22 = &_8;                       // scope 4 at $DIR/lifetimes.rs:+10:20: +10:23
+          _21 = &(*_22);                   // scope 4 at $DIR/lifetimes.rs:+10:20: +10:23
+          _20 = core::fmt::ArgumentV1::<'_>::new_display::<Box<dyn std::fmt::Display>>(move _21) -> bb3; // scope 4 at $DIR/lifetimes.rs:+10:20: +10:23
                                           // mir::Constant
-                                           // + span: $DIR/lifetimes.rs:27:21: 27:22
+                                           // + span: $DIR/lifetimes.rs:27:20: 27:23
                                           // + user_ty: UserType(4)
                                           // + literal: Const { ty: for<'b> fn(&'b Box<dyn std::fmt::Display>) -> core::fmt::ArgumentV1<'b> {core::fmt::ArgumentV1::<'_>::new_display::<Box<dyn std::fmt::Display>>}, val: Value(<ZST>) }
      }
  
      bb3: {
-          StorageDead(_21);                // scope 4 at $DIR/lifetimes.rs:+10:21: +10:22
-          StorageLive(_23);                // scope 4 at $DIR/lifetimes.rs:+10:25: +10:26
-          StorageLive(_24);                // scope 4 at $DIR/lifetimes.rs:+10:25: +10:26
-          StorageLive(_25);                // scope 4 at $DIR/lifetimes.rs:+10:25: +10:26
-          _25 = &_6;                       // scope 4 at $DIR/lifetimes.rs:+10:25: +10:26
-          _24 = &(*_25);                   // scope 4 at $DIR/lifetimes.rs:+10:25: +10:26
-          _23 = core::fmt::ArgumentV1::<'_>::new_display::<u32>(move _24) -> bb4; // scope 4 at $DIR/lifetimes.rs:+10:25: +10:26
+          StorageDead(_21);                // scope 4 at $DIR/lifetimes.rs:+10:22: +10:23
+          StorageLive(_23);                // scope 4 at $DIR/lifetimes.rs:+10:24: +10:27
+          StorageLive(_24);                // scope 4 at $DIR/lifetimes.rs:+10:24: +10:27
+          StorageLive(_25);                // scope 4 at $DIR/lifetimes.rs:+10:24: +10:27
+          _25 = &_6;                       // scope 4 at $DIR/lifetimes.rs:+10:24: +10:27
+          _24 = &(*_25);                   // scope 4 at $DIR/lifetimes.rs:+10:24: +10:27
+          _23 = core::fmt::ArgumentV1::<'_>::new_display::<u32>(move _24) -> bb4; // scope 4 at $DIR/lifetimes.rs:+10:24: +10:27
                                           // mir::Constant
-                                           // + span: $DIR/lifetimes.rs:27:25: 27:26
+                                           // + span: $DIR/lifetimes.rs:27:24: 27:27
                                           // + user_ty: UserType(5)
                                           // + literal: Const { ty: for<'b> fn(&'b u32) -> core::fmt::ArgumentV1<'b> {core::fmt::ArgumentV1::<'_>::new_display::<u32>}, val: Value(<ZST>) }
      }
  
      bb4: {
-          StorageDead(_24);                // scope 4 at $DIR/lifetimes.rs:+10:25: +10:26
+          StorageDead(_24);                // scope 4 at $DIR/lifetimes.rs:+10:26: +10:27
          _19 = [move _20, move _23];      // scope 4 at $SRC_DIR/std/src/macros.rs:LL:COL
          StorageDead(_23);                // scope 4 at $SRC_DIR/std/src/macros.rs:LL:COL
          StorageDead(_20);                // scope 4 at $SRC_DIR/std/src/macros.rs:LL:COL
--- a/tests/ui/consts/const-eval/format.stderr
+++ b/tests/ui/consts/const-eval/format.stderr
@ -1,8 +1,8 @@
 error[E0015]: cannot call non-const formatting macro in constant functions
-  --> $DIR/format.rs:2:20
+  --> $DIR/format.rs:2:13
   |
 LL |     panic!("{:?}", 0);
-   |                    ^
+   |             ^^^^
   |
   = note: calls in constant functions are limited to constant functions, tuple structs and tuple variants
   = note: this error originates in the macro `$crate::const_format_args` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info)
@ -17,10 +17,10 @@ LL |     panic!("{:?}", 0);
   = note: this error originates in the macro `$crate::const_format_args` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info)

 error[E0015]: cannot call non-const formatting macro in constant functions
-  --> $DIR/format.rs:8:22
+  --> $DIR/format.rs:8:15
   |
 LL |     println!("{:?}", 0);
-   |                      ^
+   |               ^^^^
   |
   = note: calls in constant functions are limited to constant functions, tuple structs and tuple variants
   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
--- a/tests/ui/fmt/format-args-argument-span.rs
+++ b/tests/ui/fmt/format-args-argument-span.rs
@ -0,0 +1,22 @@
+// check-compile
+
+struct DisplayOnly;
+
+impl std::fmt::Display for DisplayOnly {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        unimplemented!()
+    }
+}
+
+fn main() {
+    let x = Some(1);
+    println!("{x:?} {x} {x:?}");
+    //~^ ERROR: `Option<{integer}>` doesn't implement `std::fmt::Display`
+    println!("{x:?} {x} {x:?}", x = Some(1));
+    //~^ ERROR: `Option<{integer}>` doesn't implement `std::fmt::Display`
+    let x = DisplayOnly;
+    println!("{x} {x:?} {x}");
+    //~^ ERROR: `DisplayOnly` doesn't implement `Debug`
+    println!("{x} {x:?} {x}", x = DisplayOnly);
+    //~^ ERROR: `DisplayOnly` doesn't implement `Debug`
+}
--- a/tests/ui/fmt/format-args-argument-span.stderr
+++ b/tests/ui/fmt/format-args-argument-span.stderr
@ -0,0 +1,51 @@
+error[E0277]: `Option<{integer}>` doesn't implement `std::fmt::Display`
+  --> $DIR/format-args-argument-span.rs:13:21
+   |
+LL |     println!("{x:?} {x} {x:?}");
+   |                     ^^^ `Option<{integer}>` cannot be formatted with the default formatter
+   |
+   = help: the trait `std::fmt::Display` is not implemented for `Option<{integer}>`
+   = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
+   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: `Option<{integer}>` doesn't implement `std::fmt::Display`
+  --> $DIR/format-args-argument-span.rs:15:37
+   |
+LL |     println!("{x:?} {x} {x:?}", x = Some(1));
+   |                                     ^^^^^^^ `Option<{integer}>` cannot be formatted with the default formatter
+   |
+   = help: the trait `std::fmt::Display` is not implemented for `Option<{integer}>`
+   = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
+   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: `DisplayOnly` doesn't implement `Debug`
+  --> $DIR/format-args-argument-span.rs:18:19
+   |
+LL |     println!("{x} {x:?} {x}");
+   |                   ^^^^^ `DisplayOnly` cannot be formatted using `{:?}`
+   |
+   = help: the trait `Debug` is not implemented for `DisplayOnly`
+   = note: add `#[derive(Debug)]` to `DisplayOnly` or manually `impl Debug for DisplayOnly`
+   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
+help: consider annotating `DisplayOnly` with `#[derive(Debug)]`
+   |
+LL | #[derive(Debug)]
+   |
+
+error[E0277]: `DisplayOnly` doesn't implement `Debug`
+  --> $DIR/format-args-argument-span.rs:20:35
+   |
+LL |     println!("{x} {x:?} {x}", x = DisplayOnly);
+   |                                   ^^^^^^^^^^^ `DisplayOnly` cannot be formatted using `{:?}`
+   |
+   = help: the trait `Debug` is not implemented for `DisplayOnly`
+   = note: add `#[derive(Debug)]` to `DisplayOnly` or manually `impl Debug for DisplayOnly`
+   = note: this error originates in the macro `$crate::format_args_nl` which comes from the expansion of the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
+help: consider annotating `DisplayOnly` with `#[derive(Debug)]`
+   |
+LL | #[derive(Debug)]
+   |
+
+error: aborting due to 4 previous errors
+
+For more information about this error, try `rustc --explain E0277`.
--- a/tests/ui/fmt/ifmt-bad-arg.stderr
+++ b/tests/ui/fmt/ifmt-bad-arg.stderr
@ -300,10 +300,9 @@ error[E0308]: mismatched types
  --> $DIR/ifmt-bad-arg.rs:78:32
   |
 LL |     println!("{} {:.*} {}", 1, 3.2, 4);
-   |                                ^^^
-   |                                |
-   |                                expected `&usize`, found `&{float}`
-   |                                arguments to this function are incorrect
+   |                    --          ^^^ expected `&usize`, found `&{float}`
+   |                    |
+   |                    arguments to this function are incorrect
   |
   = note: expected reference `&usize`
              found reference `&{float}`
@ -315,10 +314,9 @@ error[E0308]: mismatched types
  --> $DIR/ifmt-bad-arg.rs:81:35
   |
 LL |     println!("{} {:07$.*} {}", 1, 3.2, 4);
-   |                                   ^^^
-   |                                   |
-   |                                   expected `&usize`, found `&{float}`
-   |                                   arguments to this function are incorrect
+   |                       --          ^^^ expected `&usize`, found `&{float}`
+   |                       |
+   |                       arguments to this function are incorrect
   |
   = note: expected reference `&usize`
              found reference `&{float}`
--- a/tests/ui/fmt/ifmt-unimpl.stderr
+++ b/tests/ui/fmt/ifmt-unimpl.stderr
@ -2,7 +2,9 @@ error[E0277]: the trait bound `str: UpperHex` is not satisfied
  --> $DIR/ifmt-unimpl.rs:2:21
   |
 LL |     format!("{:X}", "3");
-   |                     ^^^ the trait `UpperHex` is not implemented for `str`
+   |              ----   ^^^ the trait `UpperHex` is not implemented for `str`
+   |              |
+   |              required by a bound introduced by this call
   |
   = help: the following other types implement trait `UpperHex`:
             &T
--- a/tests/ui/suggestions/issue-97760.stderr
+++ b/tests/ui/suggestions/issue-97760.stderr
@ -1,8 +1,8 @@
 error[E0277]: `<impl IntoIterator as IntoIterator>::Item` doesn't implement `std::fmt::Display`
-  --> $DIR/issue-97760.rs:4:20
+  --> $DIR/issue-97760.rs:4:19
   |
 LL |         println!("{x}");
-   |                    ^ `<impl IntoIterator as IntoIterator>::Item` cannot be formatted with the default formatter
+   |                   ^^^ `<impl IntoIterator as IntoIterator>::Item` cannot be formatted with the default formatter
   |
   = help: the trait `std::fmt::Display` is not implemented for `<impl IntoIterator as IntoIterator>::Item`
   = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead