Rollup merge of #98580 - PrestonFrom:issue_98466, r=estebank

Emit warning when named arguments are used positionally in format

Addresses Issue 98466 by emitting an error if a named argument
is used like a position argument (i.e. the name is not used in
the string to be formatted).

Fixes #98466

compiler/rustc_builtin_macros/src/format.rs

@@ -14,6 +14,9 @@ use rustc_span::symbol::{sym, Ident, Symbol};
 use rustc_span::{InnerSpan, Span};
 use smallvec::SmallVec;
 
+use rustc_lint_defs::builtin::NAMED_ARGUMENTS_USED_POSITIONALLY;
+use rustc_lint_defs::{BufferedEarlyLint, BuiltinLintDiagnostics, LintId};
+use rustc_parse_format::{Count, FormatSpec};
 use std::borrow::Cow;
 use std::collections::hash_map::Entry;
 
@@ -57,7 +60,7 @@ struct Context<'a, 'b> {
     /// Unique format specs seen for each argument.
     arg_unique_types: Vec<Vec<ArgumentType>>,
     /// Map from named arguments to their resolved indices.
-    names: FxHashMap<Symbol, usize>,
+    names: FxHashMap<Symbol, (usize, Span)>,
 
     /// The latest consecutive literal strings, or empty if there weren't any.
     literal: String,
@@ -130,9 +133,9 @@ fn parse_args<'a>(
     ecx: &mut ExtCtxt<'a>,
     sp: Span,
     tts: TokenStream,
-) -> PResult<'a, (P<ast::Expr>, Vec<P<ast::Expr>>, FxHashMap<Symbol, usize>)> {
+) -> PResult<'a, (P<ast::Expr>, Vec<P<ast::Expr>>, FxHashMap<Symbol, (usize, Span)>)> {
     let mut args = Vec::<P<ast::Expr>>::new();
-    let mut names = FxHashMap::<Symbol, usize>::default();
+    let mut names = FxHashMap::<Symbol, (usize, Span)>::default();
 
     let mut p = ecx.new_parser_from_tts(tts);
 
@@ -197,7 +200,7 @@ fn parse_args<'a>(
                 p.bump();
                 p.expect(&token::Eq)?;
                 let e = p.parse_expr()?;
-                if let Some(prev) = names.get(&ident.name) {
+                if let Some((prev, _)) = names.get(&ident.name) {
                     ecx.struct_span_err(e.span, &format!("duplicate argument named `{}`", ident))
                         .span_label(args[*prev].span, "previously here")
                         .span_label(e.span, "duplicate argument")
@@ -210,7 +213,7 @@ fn parse_args<'a>(
                 // if the input is valid, we can simply append to the positional
                 // args. And remember the names.
                 let slot = args.len();
-                names.insert(ident.name, slot);
+                names.insert(ident.name, (slot, ident.span));
                 args.push(e);
             }
             _ => {
@@ -222,7 +225,7 @@ fn parse_args<'a>(
                     );
                     err.span_label(e.span, "positional arguments must be before named arguments");
                     for pos in names.values() {
-                        err.span_label(args[*pos].span, "named argument");
+                        err.span_label(args[pos.0].span, "named argument");
                     }
                     err.emit();
                 }
@@ -242,7 +245,8 @@ impl<'a, 'b> Context<'a, 'b> {
     fn resolve_name_inplace(&self, p: &mut parse::Piece<'_>) {
         // NOTE: the `unwrap_or` branch is needed in case of invalid format
         // arguments, e.g., `format_args!("{foo}")`.
-        let lookup = |s: &str| *self.names.get(&Symbol::intern(s)).unwrap_or(&0);
+        let lookup =
+            |s: &str| self.names.get(&Symbol::intern(s)).unwrap_or(&(0, Span::default())).0;
 
         match *p {
             parse::String(_) => {}
@@ -548,7 +552,7 @@ impl<'a, 'b> Context<'a, 'b> {
                 match self.names.get(&name) {
                     Some(&idx) => {
                         // Treat as positional arg.
-                        self.verify_arg_type(Capture(idx), ty)
+                        self.verify_arg_type(Capture(idx.0), ty)
                     }
                     None => {
                         // For the moment capturing variables from format strings expanded from macros is
@@ -565,7 +569,7 @@ impl<'a, 'b> Context<'a, 'b> {
                             };
                             self.num_captured_args += 1;
                             self.args.push(self.ecx.expr_ident(span, Ident::new(name, span)));
-                            self.names.insert(name, idx);
+                            self.names.insert(name, (idx, span));
                             self.verify_arg_type(Capture(idx), ty)
                         } else {
                             let msg = format!("there is no argument named `{}`", name);
@@ -967,14 +971,57 @@ pub fn expand_format_args_nl<'cx>(
     expand_format_args_impl(ecx, sp, tts, true)
 }
 
+fn lint_named_arguments_used_positionally(
+    names: FxHashMap<Symbol, (usize, Span)>,
+    cx: &mut Context<'_, '_>,
+    unverified_pieces: Vec<parse::Piece<'_>>,
+) {
+    let mut used_argument_names = FxHashSet::<&str>::default();
+    for piece in unverified_pieces {
+        if let rustc_parse_format::Piece::NextArgument(a) = piece {
+            match a.position {
+                rustc_parse_format::Position::ArgumentNamed(arg_name, _) => {
+                    used_argument_names.insert(arg_name);
+                }
+                _ => {}
+            };
+            match a.format {
+                FormatSpec { width: Count::CountIsName(s, _), .. }
+                | FormatSpec { precision: Count::CountIsName(s, _), .. } => {
+                    used_argument_names.insert(s);
+                }
+                _ => {}
+            };
+        }
+    }
+
+    for (symbol, (index, span)) in names {
+        if !used_argument_names.contains(symbol.as_str()) {
+            let msg = format!("named argument `{}` is not used by name", symbol.as_str());
+            let arg_span = cx.arg_spans[index];
+            cx.ecx.buffered_early_lint.push(BufferedEarlyLint {
+                span: MultiSpan::from_span(span),
+                msg: msg.clone(),
+                node_id: ast::CRATE_NODE_ID,
+                lint_id: LintId::of(&NAMED_ARGUMENTS_USED_POSITIONALLY),
+                diagnostic: BuiltinLintDiagnostics::NamedArgumentUsedPositionally(
+                    arg_span,
+                    span,
+                    symbol.to_string(),
+                ),
+            });
+        }
+    }
+}
+
 /// Take the various parts of `format_args!(efmt, args..., name=names...)`
 /// and construct the appropriate formatting expression.
 pub fn expand_preparsed_format_args(
     ecx: &mut ExtCtxt<'_>,
     sp: Span,
     efmt: P<ast::Expr>,
     args: Vec<P<ast::Expr>>,
-    names: FxHashMap<Symbol, usize>,
+    names: FxHashMap<Symbol, (usize, Span)>,
     append_newline: bool,
 ) -> P<ast::Expr> {
     // NOTE: this verbose way of initializing `Vec<Vec<ArgumentType>>` is because
@@ -1073,7 +1120,12 @@ pub fn expand_preparsed_format_args(
         .map(|span| fmt_span.from_inner(InnerSpan::new(span.start, span.end)))
         .collect();
 
-    let named_pos: FxHashSet<usize> = names.values().cloned().collect();
+    let named_pos: FxHashSet<usize> = names.values().cloned().map(|(i, _)| i).collect();
+
+    // Clone `names` because `names` in Context get updated by verify_piece, which includes usages
+    // of the names of named arguments, resulting in incorrect errors if a name argument is used
+    // but not declared, such as: `println!("x = {x}");`
+    let named_arguments = names.clone();
 
     let mut cx = Context {
         ecx,
@@ -1101,9 +1153,11 @@ pub fn expand_preparsed_format_args(
         is_literal: parser.is_literal,
     };
 
-    // This needs to happen *after* the Parser has consumed all pieces to create all the spans
+    // This needs to happen *after* the Parser has consumed all pieces to create all the spans.
+    // unverified_pieces is used later to check named argument names are used, so clone each piece.
     let pieces = unverified_pieces
-        .into_iter()
+        .iter()
+        .cloned()
         .map(|mut piece| {
             cx.verify_piece(&piece);
             cx.resolve_name_inplace(&mut piece);
@@ -1265,6 +1319,11 @@ pub fn expand_preparsed_format_args(
         }
 
         diag.emit();
+    } else if cx.invalid_refs.is_empty() && !named_arguments.is_empty() {
+        // Only check for unused named argument names if there are no other errors to avoid causing
+        // too much noise in output errors, such as when a named argument is entirely unused.
+        // We also only need to perform this check if there are actually named arguments.
+        lint_named_arguments_used_positionally(named_arguments, &mut cx, unverified_pieces);
     }
 
     cx.into_expr()

compiler/rustc_expand/src/base.rs

@@ -12,7 +12,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_data_structures::sync::{self, Lrc};
 use rustc_errors::{Applicability, DiagnosticBuilder, ErrorGuaranteed, MultiSpan, PResult};
 use rustc_lint_defs::builtin::PROC_MACRO_BACK_COMPAT;
-use rustc_lint_defs::BuiltinLintDiagnostics;
+use rustc_lint_defs::{BufferedEarlyLint, BuiltinLintDiagnostics};
 use rustc_parse::{self, parser, MACRO_ARGUMENTS};
 use rustc_session::{parse::ParseSess, Limit, Session, SessionDiagnostic};
 use rustc_span::def_id::{CrateNum, DefId, LocalDefId};
@@ -988,6 +988,8 @@ pub struct ExtCtxt<'a> {
     pub expansions: FxHashMap<Span, Vec<String>>,
     /// Used for running pre-expansion lints on freshly loaded modules.
     pub(super) lint_store: LintStoreExpandDyn<'a>,
+    /// Used for storing lints generated during expansion, like `NAMED_ARGUMENTS_USED_POSITIONALLY`
+    pub buffered_early_lint: Vec<BufferedEarlyLint>,
     /// When we 'expand' an inert attribute, we leave it
     /// in the AST, but insert it here so that we know
     /// not to expand it again.
@@ -1020,6 +1022,7 @@ impl<'a> ExtCtxt<'a> {
             force_mode: false,
             expansions: FxHashMap::default(),
             expanded_inert_attrs: MarkedAttrs::new(),
+            buffered_early_lint: vec![],
         }
     }
 

compiler/rustc_interface/src/passes.rs

@@ -14,7 +14,7 @@ use rustc_errors::{Applicability, ErrorGuaranteed, MultiSpan, PResult};
 use rustc_expand::base::{ExtCtxt, LintStoreExpand, ResolverExpand};
 use rustc_hir::def_id::{StableCrateId, LOCAL_CRATE};
 use rustc_hir::definitions::Definitions;
-use rustc_lint::{EarlyCheckNode, LintStore};
+use rustc_lint::{BufferedEarlyLint, EarlyCheckNode, LintStore};
 use rustc_metadata::creader::CStore;
 use rustc_metadata::{encode_metadata, EncodedMetadata};
 use rustc_middle::arena::Arena;
@@ -336,12 +336,15 @@ pub fn configure_and_expand(
 
         let lint_store = LintStoreExpandImpl(lint_store);
         let mut ecx = ExtCtxt::new(sess, cfg, resolver, Some(&lint_store));
-
         // Expand macros now!
         let krate = sess.time("expand_crate", || ecx.monotonic_expander().expand_crate(krate));
 
         // The rest is error reporting
 
+        sess.parse_sess.buffered_lints.with_lock(|buffered_lints: &mut Vec<BufferedEarlyLint>| {
+            buffered_lints.append(&mut ecx.buffered_early_lint);
+        });
+
         sess.time("check_unused_macros", || {
             ecx.check_unused_macros();
         });

compiler/rustc_lint/src/context.rs

@@ -857,6 +857,18 @@ pub trait LintContext: Sized {
                         Applicability::MachineApplicable,
                     );
                 },
+                BuiltinLintDiagnostics::NamedArgumentUsedPositionally(positional_arg, named_arg, name) => {
+                    db.span_label(named_arg, "this named argument is only referred to by position in formatting string");
+                    let msg = format!("this formatting argument uses named argument `{}` by position", name);
+                    db.span_label(positional_arg, msg);
+                    db.span_suggestion_verbose(
+                        positional_arg,
+                        "use the named argument by name to avoid ambiguity",
+                        format!("{{{}}}", name),
+                        Applicability::MaybeIncorrect,
+                    );
+
+                }
             }
             // Rewrap `db`, and pass control to the user.
             decorate(LintDiagnosticBuilder::new(db));

compiler/rustc_lint_defs/src/builtin.rs

@@ -3292,6 +3292,7 @@ declare_lint_pass! {
         TEST_UNSTABLE_LINT,
         FFI_UNWIND_CALLS,
         REPR_TRANSPARENT_EXTERNAL_PRIVATE_FIELDS,
+        NAMED_ARGUMENTS_USED_POSITIONALLY,
     ]
 }
 
@@ -3996,3 +3997,33 @@ declare_lint! {
     "call to foreign functions or function pointers with FFI-unwind ABI",
     @feature_gate = sym::c_unwind;
 }
+
+declare_lint! {
+    /// The `named_arguments_used_positionally` lint detects cases where named arguments are only
+    /// used positionally in format strings. This usage is valid but potentially very confusing.
+    ///
+    /// ### Example
+    ///
+    /// ```rust,compile_fail
+    /// #![deny(named_arguments_used_positionally)]
+    /// fn main() {
+    ///     let _x = 5;
+    ///     println!("{}", _x = 1); // Prints 1, will trigger lint
+    ///
+    ///     println!("{}", _x); // Prints 5, no lint emitted
+    ///     println!("{_x}", _x = _x); // Prints 5, no lint emitted
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Rust formatting strings can refer to named arguments by their position, but this usage is
+    /// potentially confusing. In particular, readers can incorrectly assume that the declaration
+    /// of named arguments is an assignment (which would produce the unit type).
+    /// For backwards compatibility, this is not a hard error.
+    pub NAMED_ARGUMENTS_USED_POSITIONALLY,
+    Warn,
+    "named arguments in format used positionally"
+}

compiler/rustc_lint_defs/src/lib.rs

@@ -467,6 +467,7 @@ pub enum BuiltinLintDiagnostics {
         /// If true, the lifetime will be fully elided.
         use_span: Option<(Span, bool)>,
     },
+    NamedArgumentUsedPositionally(Span, Span, String),
 }
 
 /// Lints that are buffered up early on in the `Session` before the

src/test/ui/macros/issue-98466-allow.rs

@@ -0,0 +1,16 @@
+// check-pass
+#![allow(named_arguments_used_positionally)]
+
+fn main() {
+    let mut _x: usize;
+    _x = 1;
+    println!("_x is {}", _x = 5);
+    println!("_x is {}", y = _x);
+    println!("first positional arg {}, second positional arg {}, _x is {}", 1, 2, y = _x);
+
+    let mut _x: usize;
+    _x = 1;
+    let _f = format!("_x is {}", _x = 5);
+    let _f = format!("_x is {}", y = _x);
+    let _f = format!("first positional arg {}, second positional arg {}, _x is {}", 1, 2, y = _x);
+}

src/test/ui/macros/issue-98466.fixed

@@ -0,0 +1,51 @@
+// check-pass
+// run-rustfix
+
+fn main() {
+    let mut _x: usize;
+    _x = 1;
+    println!("_x is {_x}", _x = 5);
+    //~^ WARNING named argument `_x` is not used by name [named_arguments_used_positionally]
+    //~| HELP use the named argument by name to avoid ambiguity
+    println!("_x is {y}", y = _x);
+    //~^ WARNING named argument `y` is not used by name [named_arguments_used_positionally]
+    //~| HELP use the named argument by name to avoid ambiguity
+    println!("first positional arg {}, second positional arg {}, _x is {y}", 1, 2, y = _x);
+    //~^ WARNING named argument `y` is not used by name [named_arguments_used_positionally]
+    //~| HELP use the named argument by name to avoid ambiguity
+
+    let mut _x: usize;
+    _x = 1;
+    let _f = format!("_x is {_x}", _x = 5);
+    //~^ WARNING named argument `_x` is not used by name [named_arguments_used_positionally]
+    //~| HELP use the named argument by name to avoid ambiguity
+    let _f = format!("_x is {y}", y = _x);
+    //~^ WARNING named argument `y` is not used by name [named_arguments_used_positionally]
+    //~| HELP use the named argument by name to avoid ambiguity
+    let _f = format!("first positional arg {}, second positional arg {}, _x is {y}", 1, 2, y = _x);
+    //~^ WARNING named argument `y` is not used by name [named_arguments_used_positionally]
+    //~| HELP use the named argument by name to avoid ambiguity
+
+    let s = "0.009";
+    // Confirm that named arguments used in formatting are correctly considered.
+    println!(".{:0<width$}", s, width = _x);
+
+    let region = "abc";
+    let width = 8;
+    let ls = "abcde";
+    let full = "abcde";
+    // Confirm that named arguments used in formatting are correctly considered.
+    println!(
+        "| {r:rw$?} | {ui:4?} | {v}",
+        r = region,
+        rw = width,
+        ui = ls,
+        v = full,
+    );
+
+    // Confirm that named arguments used in formatting are correctly considered.
+    println!("{:.a$}", "aaaaaaaaaaaaaaaaaa", a = 4);
+
+    // Confirm that named arguments used in formatting are correctly considered.
+    println!("{:._a$}", "aaaaaaaaaaaaaaaaaa", _a = 4);
+}