Do not deduplicate captured args while expanding format_args!

Author: ShoyuVanilla

compiler/rustc_ast/src/format.rs

@@ -77,14 +77,14 @@ pub struct FormatArguments {
     arguments: Vec<FormatArgument>,
     num_unnamed_args: usize,
     num_explicit_args: usize,
-    names: FxHashMap<Symbol, usize>,
+    explicit_names: FxHashMap<Symbol, usize>,
 }
 
 impl FormatArguments {
     pub fn new() -> Self {
         Self {
             arguments: Vec::new(),
-            names: FxHashMap::default(),
+            explicit_names: FxHashMap::default(),
             num_unnamed_args: 0,
             num_explicit_args: 0,
         }
@@ -93,13 +93,16 @@ impl FormatArguments {
     pub fn add(&mut self, arg: FormatArgument) -> usize {
         let index = self.arguments.len();
         if let Some(name) = arg.kind.ident() {
-            self.names.insert(name.name, index);
-        } else if self.names.is_empty() {
+            self.explicit_names.insert(name.name, index);
+        } else if let FormatArgumentKind::Normal = arg.kind
+            && self.explicit_names.is_empty()
+            && self.arguments.len() == self.num_unnamed_args
+        {
             // Only count the unnamed args before the first named arg.
             // (Any later ones are errors.)
             self.num_unnamed_args += 1;
         }
-        if !matches!(arg.kind, FormatArgumentKind::Captured(..)) {
+        if !matches!(arg.kind, FormatArgumentKind::Captured) {
             // This is an explicit argument.
             // Make sure that all arguments so far are explicit.
             assert_eq!(
@@ -113,8 +116,8 @@ impl FormatArguments {
         index
     }
 
-    pub fn by_name(&self, name: Symbol) -> Option<(usize, &FormatArgument)> {
-        let i = *self.names.get(&name)?;
+    pub fn by_explicit_name(&self, name: Symbol) -> Option<(usize, &FormatArgument)> {
+        let i = *self.explicit_names.get(&name)?;
         Some((i, &self.arguments[i]))
     }
 
@@ -156,15 +159,15 @@ pub enum FormatArgumentKind {
     /// `format_args(…, arg = 1)`
     Named(Ident),
     /// `format_args("… {arg} …")`
-    Captured(Ident),
+    Captured,
 }
 
 impl FormatArgumentKind {
     pub fn ident(&self) -> Option<Ident> {
         match self {
             &Self::Normal => None,
             &Self::Named(id) => Some(id),
-            &Self::Captured(id) => Some(id),
+            &Self::Captured => None,
         }
     }
 }

compiler/rustc_ast_lowering/src/format.rs

@@ -462,7 +462,7 @@ fn expand_format_args<'hir>(
                     let placeholder_span =
                         placeholder_span.unwrap_or(arg.expr.span).with_ctxt(macsp.ctxt());
                     let arg_span = match arg.kind {
-                        FormatArgumentKind::Captured(_) => placeholder_span,
+                        FormatArgumentKind::Captured => placeholder_span,
                         _ => arg.expr.span.with_ctxt(macsp.ctxt()),
                     };
                     let args_ident_expr = ctx.expr_ident(macsp, args_ident, args_hir_id);

compiler/rustc_builtin_macros/src/format.rs

@@ -121,7 +121,7 @@ fn parse_args<'a>(ecx: &ExtCtxt<'a>, sp: Span, tts: TokenStream) -> PResult<'a,
                 p.bump();
                 p.expect(exp!(Eq))?;
                 let expr = p.parse_expr()?;
-                if let Some((_, prev)) = args.by_name(ident.name) {
+                if let Some((_, prev)) = args.by_explicit_name(ident.name) {
                     ecx.dcx().emit_err(errors::FormatDuplicateArg {
                         span: ident.span,
                         prev: prev.kind.ident().unwrap().span,
@@ -374,12 +374,11 @@ fn make_format_args(
             }
             Name(name, span) => {
                 let name = Symbol::intern(name);
-                if let Some((index, _)) = args.by_name(name) {
+                if let Some((index, _)) = args.by_explicit_name(name) {
                     // Name found in `args`, so we resolve it to its index.
-                    if index < args.explicit_args().len() {
-                        // Mark it as used, if it was an explicit argument.
-                        used[index] = true;
-                    }
+                    assert!(index < args.explicit_args().len());
+                    // Mark it as used, as this is an explicit argument.
+                    used[index] = true;
                     Ok(index)
                 } else {
                     // Name not found in `args`, so we add it as an implicitly captured argument.
@@ -394,7 +393,7 @@ fn make_format_args(
                         unnamed_arg_after_named_arg = true;
                         DummyResult::raw_expr(span, Some(guar))
                     };
-                    Ok(args.add(FormatArgument { kind: FormatArgumentKind::Captured(ident), expr }))
+                    Ok(args.add(FormatArgument { kind: FormatArgumentKind::Captured, expr }))
                 }
             }
         };

src/tools/clippy/clippy_lints/src/format_args.rs

@@ -447,7 +447,7 @@ impl<'tcx> FormatArgsExpr<'_, 'tcx> {
         let index = pos.index.unwrap();
         let arg = &self.format_args.arguments.all_args()[index];
 
-        if !matches!(arg.kind, FormatArgumentKind::Captured(_))
+        if !matches!(arg.kind, FormatArgumentKind::Captured)
             && let rustc_ast::ExprKind::Path(None, path) = &arg.expr.kind
             && let [segment] = path.segments.as_slice()
             && segment.args.is_none()
@@ -467,7 +467,7 @@ impl<'tcx> FormatArgsExpr<'_, 'tcx> {
             // * if we can't inline a numbered argument, e.g. `print!("{0} ...", foo.bar, ...)`
             // * if allow_mixed_uninlined_format_args is false and this arg hasn't been inlined already
             pos.kind != FormatArgPositionKind::Number
-                && (!self.ignore_mixed || matches!(arg.kind, FormatArgumentKind::Captured(_)))
+                && (!self.ignore_mixed || matches!(arg.kind, FormatArgumentKind::Captured))
         }
     }
 

src/tools/clippy/clippy_lints/src/unit_types/let_unit_value.rs

@@ -202,7 +202,7 @@ impl<'tcx> Visitor<'tcx> for UnitVariableCollector<'_, 'tcx> {
         {
             if let Some(macro_call) = self.macro_call
                 && macro_call.arguments.all_args().iter().any(|arg| {
-                    matches!(arg.kind, FormatArgumentKind::Captured(_)) && find_format_arg_expr(ex, arg).is_some()
+                    matches!(arg.kind, FormatArgumentKind::Captured) && find_format_arg_expr(ex, arg).is_some()
                 })
             {
                 self.spans.push(VariableUsage::FormatCapture);

tests/pretty/hir-format-args-duplicated-captures.pp

@@ -0,0 +1,20 @@
+#[attr = MacroUse {arguments: UseAll}]
+extern crate std;
+#[prelude_import]
+use ::std::prelude::rust_2015::*;
+//@ pretty-compare-only
+//@ pretty-mode:hir
+//@ pp-exact:hir-format-args-duplicated-captures.pp
+
+const X: i32 = 42;
+
+fn main() {
+    let _ =
+        {
+            super let args = (&X, &X);
+            super let args =
+                [format_argument::new_display(args.0),
+                        format_argument::new_display(args.1)];
+            unsafe { format_arguments::new(b"\xc0\x01 \xc0\x00", &args) }
+        };
+}

tests/pretty/hir-format-args-duplicated-captures.rs

@@ -0,0 +1,9 @@
+//@ pretty-compare-only
+//@ pretty-mode:hir
+//@ pp-exact:hir-format-args-duplicated-captures.pp
+
+const X: i32 = 42;
+
+fn main() {
+    let _ = format_args!("{X} {X}");
+}

tests/ui/drop/format-args-duplicated-captured-const-drops.rs

@@ -0,0 +1,53 @@
+//! A regression test for https://github.com/rust-lang/rust/issues/145739.
+//! `format_args!` should not deduplicate implicitly captured arguments.
+//! They must be evaluated once per occurrence, whereas explicitly
+//! provided arguments are evaluated only once, regardless of how many
+//! times they appear in the format string.
+//@ run-pass
+
+use std::sync::atomic::{AtomicUsize, Ordering};
+
+static DROP_COUNTER: AtomicUsize = AtomicUsize::new(0);
+
+#[derive(Debug)]
+struct Foo;
+
+impl Drop for Foo {
+    fn drop(&mut self) {
+        DROP_COUNTER.fetch_add(1, Ordering::Relaxed);
+    }
+}
+
+fn main() {
+    assert_eq!(DROP_COUNTER.load(Ordering::Relaxed), 0);
+
+    {
+        let _x = format_args!("{Foo:?}");
+    }
+    assert_eq!(DROP_COUNTER.load(Ordering::Relaxed), 1);
+
+    {
+        let _x = format_args!("{Foo:?}{Foo:?}");
+    }
+    // Increased by 2, as `Foo` is constructed for each captured `Foo`.
+    assert_eq!(DROP_COUNTER.load(Ordering::Relaxed), 3);
+
+    {
+        let _x = format_args!("{:?}{0:?}", Foo);
+    }
+    // Increased by 1, as `foo` is constructed just once as an explicit argument.
+    assert_eq!(DROP_COUNTER.load(Ordering::Relaxed), 4);
+
+    {
+        let _x = format_args!("{foo:?}{foo:?}{bar:?}{Foo:?}{Foo:?}", foo = Foo, bar = Foo);
+    }
+    // Increased by 4, as `foo` is constructed twice for captured `Foo`, and once for each `foo` and
+    // `bar`.
+    assert_eq!(DROP_COUNTER.load(Ordering::Relaxed), 8);
+
+    {
+        let _x = format_args!("{Foo:?}{Foo:?}", Foo = Foo);
+    }
+    // Increased by 1, as `Foo` is shadowed by an explicit argument.
+    assert_eq!(DROP_COUNTER.load(Ordering::Relaxed), 9);
+}