diff options
| -rw-r--r-- | compiler/rustc_builtin_macros/src/format.rs | 85 | ||||
| -rw-r--r-- | compiler/rustc_expand/src/base.rs | 5 | ||||
| -rw-r--r-- | compiler/rustc_interface/src/passes.rs | 7 | ||||
| -rw-r--r-- | compiler/rustc_lint/src/context.rs | 12 | ||||
| -rw-r--r-- | compiler/rustc_lint_defs/src/builtin.rs | 31 | ||||
| -rw-r--r-- | compiler/rustc_lint_defs/src/lib.rs | 1 | ||||
| -rw-r--r-- | src/test/ui/macros/issue-98466-allow.rs | 16 | ||||
| -rw-r--r-- | src/test/ui/macros/issue-98466.fixed | 51 | ||||
| -rw-r--r-- | src/test/ui/macros/issue-98466.rs | 51 | ||||
| -rw-r--r-- | src/test/ui/macros/issue-98466.stderr | 81 |
10 files changed, 324 insertions, 16 deletions
diff --git a/compiler/rustc_builtin_macros/src/format.rs b/compiler/rustc_builtin_macros/src/format.rs index 6c2ac343544..4791151c7d3 100644 --- a/compiler/rustc_builtin_macros/src/format.rs +++ b/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,6 +971,49 @@ 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( @@ -974,7 +1021,7 @@ pub fn expand_preparsed_format_args( 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() diff --git a/compiler/rustc_expand/src/base.rs b/compiler/rustc_expand/src/base.rs index bfe2d773788..e1f19064d52 100644 --- a/compiler/rustc_expand/src/base.rs +++ b/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![], } } diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs index 97b8139f9da..0529fff5a6b 100644 --- a/compiler/rustc_interface/src/passes.rs +++ b/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(); }); diff --git a/compiler/rustc_lint/src/context.rs b/compiler/rustc_lint/src/context.rs index 5725c240320..13e3bb9a363 100644 --- a/compiler/rustc_lint/src/context.rs +++ b/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)); diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 6d2cb63c1d7..39690851d1e 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/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" +} diff --git a/compiler/rustc_lint_defs/src/lib.rs b/compiler/rustc_lint_defs/src/lib.rs index 48f441e69d6..1bc7e7de660 100644 --- a/compiler/rustc_lint_defs/src/lib.rs +++ b/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 diff --git a/src/test/ui/macros/issue-98466-allow.rs b/src/test/ui/macros/issue-98466-allow.rs new file mode 100644 index 00000000000..c260148c148 --- /dev/null +++ b/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); +} diff --git a/src/test/ui/macros/issue-98466.fixed b/src/test/ui/macros/issue-98466.fixed new file mode 100644 index 00000000000..e46e22f001f --- /dev/null +++ b/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); +} diff --git a/src/test/ui/macros/issue-98466.rs b/src/test/ui/macros/issue-98466.rs new file mode 100644 index 00000000000..2c3b099afde --- /dev/null +++ b/src/test/ui/macros/issue-98466.rs @@ -0,0 +1,51 @@ +// check-pass +// run-rustfix + +fn main() { + let mut _x: usize; + _x = 1; + println!("_x is {}", _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 = _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 {}", 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 = 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 = _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 {}", 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); +} diff --git a/src/test/ui/macros/issue-98466.stderr b/src/test/ui/macros/issue-98466.stderr new file mode 100644 index 00000000000..ad11d181b62 --- /dev/null +++ b/src/test/ui/macros/issue-98466.stderr @@ -0,0 +1,81 @@ +warning: named argument `_x` is not used by name + --> $DIR/issue-98466.rs:7:26 + | +LL | println!("_x is {}", _x = 5); + | -- ^^ this named argument is only referred to by position in formatting string + | | + | this formatting argument uses named argument `_x` by position + | + = note: `#[warn(named_arguments_used_positionally)]` on by default +help: use the named argument by name to avoid ambiguity + | +LL | println!("_x is {_x}", _x = 5); + | ~~~~ + +warning: named argument `y` is not used by name + --> $DIR/issue-98466.rs:10:26 + | +LL | println!("_x is {}", y = _x); + | -- ^ this named argument is only referred to by position in formatting string + | | + | this formatting argument uses named argument `y` by position + | +help: use the named argument by name to avoid ambiguity + | +LL | println!("_x is {y}", y = _x); + | ~~~ + +warning: named argument `y` is not used by name + --> $DIR/issue-98466.rs:13:83 + | +LL | println!("first positional arg {}, second positional arg {}, _x is {}", 1, 2, y = _x); + | -- ^ this named argument is only referred to by position in formatting string + | | + | this formatting argument uses named argument `y` by position + | +help: use the named argument by name to avoid ambiguity + | +LL | println!("first positional arg {}, second positional arg {}, _x is {y}", 1, 2, y = _x); + | ~~~ + +warning: named argument `_x` is not used by name + --> $DIR/issue-98466.rs:19:34 + | +LL | let _f = format!("_x is {}", _x = 5); + | -- ^^ this named argument is only referred to by position in formatting string + | | + | this formatting argument uses named argument `_x` by position + | +help: use the named argument by name to avoid ambiguity + | +LL | let _f = format!("_x is {_x}", _x = 5); + | ~~~~ + +warning: named argument `y` is not used by name + --> $DIR/issue-98466.rs:22:34 + | +LL | let _f = format!("_x is {}", y = _x); + | -- ^ this named argument is only referred to by position in formatting string + | | + | this formatting argument uses named argument `y` by position + | +help: use the named argument by name to avoid ambiguity + | +LL | let _f = format!("_x is {y}", y = _x); + | ~~~ + +warning: named argument `y` is not used by name + --> $DIR/issue-98466.rs:25:91 + | +LL | let _f = format!("first positional arg {}, second positional arg {}, _x is {}", 1, 2, y = _x); + | -- ^ this named argument is only referred to by position in formatting string + | | + | this formatting argument uses named argument `y` by position + | +help: use the named argument by name to avoid ambiguity + | +LL | let _f = format!("first positional arg {}, second positional arg {}, _x is {y}", 1, 2, y = _x); + | ~~~ + +warning: 6 warnings emitted + |