Skip to content

Commit

Permalink
Rollup merge of #100277 - m-ou-se:format-args-1, r=compiler-errors
Browse files Browse the repository at this point in the history 
Simplify format_args builtin macro implementation.

Instead of a FxHashMap<Symbol, (usize, Span)> for the named arguments, this now includes the name and span in the elements of the Vec<FormatArg> directly. The FxHashMap still exists to look up the index, but no longer contains the span. Looking up the name or span of an argument is now trivial and does not need the map anymore.
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Aug 15, 2022
2 parents d6b6503 + a639fdb commit 965ed81
Showing 1 changed file with 64 additions and 90 deletions.
154 changes: 64 additions & 90 deletions compiler/rustc_builtin_macros/src/format.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,64 +130,46 @@ impl PositionalNamedArgsLint {
/// CountIsParam, which contains an index into the arguments.
fn maybe_add_positional_named_arg(
&mut self,
current_positional_arg: usize,
total_args_length: usize,
format_argument_index: usize,
arg: Option<&FormatArg>,
ty: PositionalNamedArgType,
cur_piece: usize,
inner_span_to_replace: Option<rustc_parse_format::InnerSpan>,
names: &FxHashMap<Symbol, (usize, Span)>,
has_formatting: bool,
) {
let start_of_named_args = total_args_length - names.len();
if current_positional_arg >= start_of_named_args {
self.maybe_push(
format_argument_index,
ty,
cur_piece,
inner_span_to_replace,
names,
has_formatting,
)
if let Some(arg) = arg {
if let Some(name) = arg.name {
self.push(name, ty, cur_piece, inner_span_to_replace, has_formatting)
}
}
}

/// Try constructing a PositionalNamedArg struct and pushing it into the vec of positional
/// named arguments. If a named arg associated with `format_argument_index` cannot be found,
/// a new item will not be added as the lint cannot be emitted in this case.
fn maybe_push(
/// Construct a PositionalNamedArg struct and push it into the vec of positional
/// named arguments.
fn push(
&mut self,
format_argument_index: usize,
arg_name: Ident,
ty: PositionalNamedArgType,
cur_piece: usize,
inner_span_to_replace: Option<rustc_parse_format::InnerSpan>,
names: &FxHashMap<Symbol, (usize, Span)>,
has_formatting: bool,
) {
let named_arg = names
.iter()
.find(|&(_, &(index, _))| index == format_argument_index)
.map(|found| found.clone());

if let Some((&replacement, &(_, positional_named_arg_span))) = named_arg {
// In FormatSpec, `precision_span` starts at the leading `.`, which we want to keep in
// the lint suggestion, so increment `start` by 1 when `PositionalArgumentType` is
// `Precision`.
let inner_span_to_replace = if ty == PositionalNamedArgType::Precision {
inner_span_to_replace
.map(|is| rustc_parse_format::InnerSpan { start: is.start + 1, end: is.end })
} else {
inner_span_to_replace
};
self.positional_named_args.push(PositionalNamedArg {
ty,
cur_piece,
inner_span_to_replace,
replacement,
positional_named_arg_span,
has_formatting,
});
}
// In FormatSpec, `precision_span` starts at the leading `.`, which we want to keep in
// the lint suggestion, so increment `start` by 1 when `PositionalArgumentType` is
// `Precision`.
let inner_span_to_replace = if ty == PositionalNamedArgType::Precision {
inner_span_to_replace
.map(|is| rustc_parse_format::InnerSpan { start: is.start + 1, end: is.end })
} else {
inner_span_to_replace
};
self.positional_named_args.push(PositionalNamedArg {
ty,
cur_piece,
inner_span_to_replace,
replacement: arg_name.name,
positional_named_arg_span: arg_name.span,
has_formatting,
});
}
}

Expand All @@ -211,15 +193,15 @@ struct Context<'a, 'b> {
/// * `arg_types` (in JSON): `[[0, 1, 0], [0, 1, 1], [0, 1]]`
/// * `arg_unique_types` (in simplified JSON): `[["o", "x"], ["o", "x"], ["o", "x"]]`
/// * `names` (in JSON): `{"foo": 2}`
args: Vec<P<ast::Expr>>,
args: Vec<FormatArg>,
/// The number of arguments that were added by implicit capturing.
num_captured_args: usize,
/// Placeholder slot numbers indexed by argument.
arg_types: Vec<Vec<usize>>,
/// 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, Span)>,
names: FxHashMap<Symbol, usize>,

/// The latest consecutive literal strings, or empty if there weren't any.
literal: String,
Expand Down Expand Up @@ -282,7 +264,7 @@ struct Context<'a, 'b> {

pub struct FormatArg {
expr: P<ast::Expr>,
named: bool,
name: Option<Ident>,
}

/// Parses the arguments from the given list of tokens, returning the diagnostic
Expand All @@ -298,9 +280,9 @@ fn parse_args<'a>(
ecx: &mut ExtCtxt<'a>,
sp: Span,
tts: TokenStream,
) -> PResult<'a, (P<ast::Expr>, Vec<FormatArg>, FxHashMap<Symbol, (usize, Span)>)> {
) -> PResult<'a, (P<ast::Expr>, Vec<FormatArg>, FxHashMap<Symbol, usize>)> {
let mut args = Vec::<FormatArg>::new();
let mut names = FxHashMap::<Symbol, (usize, Span)>::default();
let mut names = FxHashMap::<Symbol, usize>::default();

let mut p = ecx.new_parser_from_tts(tts);

Expand Down Expand Up @@ -365,9 +347,9 @@ 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].expr.span, "previously here")
.span_label(args[prev].expr.span, "previously here")
.span_label(e.span, "duplicate argument")
.emit();
continue;
Expand All @@ -378,8 +360,8 @@ 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, ident.span));
args.push(FormatArg { expr: e, named: true });
names.insert(ident.name, slot);
args.push(FormatArg { expr: e, name: Some(ident) });
}
_ => {
let e = p.parse_expr()?;
Expand All @@ -389,12 +371,12 @@ fn parse_args<'a>(
"positional arguments cannot follow named arguments",
);
err.span_label(e.span, "positional arguments must be before named arguments");
for pos in names.values() {
err.span_label(args[pos.0].expr.span, "named argument");
for &pos in names.values() {
err.span_label(args[pos].expr.span, "named argument");
}
err.emit();
}
args.push(FormatArg { expr: e, named: false });
args.push(FormatArg { expr: e, name: None });
}
}
}
Expand All @@ -410,8 +392,7 @@ impl<'a, 'b> Context<'a, 'b> {
fn resolve_name_inplace(&mut 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, Span::default())).0;
let lookup = |s: &str| self.names.get(&Symbol::intern(s)).copied().unwrap_or(0);

match *p {
parse::String(_) => {}
Expand Down Expand Up @@ -457,27 +438,21 @@ impl<'a, 'b> Context<'a, 'b> {
let pos = match arg.position {
parse::ArgumentIs(i) => {
self.unused_names_lint.maybe_add_positional_named_arg(
i,
self.args.len(),
i,
self.args.get(i),
PositionalNamedArgType::Arg,
self.curpiece,
Some(arg.position_span),
&self.names,
has_precision || has_width,
);

Exact(i)
}
parse::ArgumentImplicitlyIs(i) => {
self.unused_names_lint.maybe_add_positional_named_arg(
i,
self.args.len(),
i,
self.args.get(i),
PositionalNamedArgType::Arg,
self.curpiece,
None,
&self.names,
has_precision || has_width,
);
Exact(i)
Expand Down Expand Up @@ -563,13 +538,10 @@ impl<'a, 'b> Context<'a, 'b> {
parse::CountImplied | parse::CountIs(..) => {}
parse::CountIsParam(i) => {
self.unused_names_lint.maybe_add_positional_named_arg(
i,
self.args.len(),
i,
self.args.get(i),
named_arg_type,
self.curpiece,
*inner_span,
&self.names,
true,
);
self.verify_arg_type(Exact(i), Count);
Expand Down Expand Up @@ -622,7 +594,7 @@ impl<'a, 'b> Context<'a, 'b> {
);
for arg in &self.args {
// Point at the arguments that will be formatted.
e.span_label(arg.span, "");
e.span_label(arg.expr.span, "");
}
} else {
let (mut refs, spans): (Vec<_>, Vec<_>) = refs.unzip();
Expand Down Expand Up @@ -692,7 +664,7 @@ impl<'a, 'b> Context<'a, 'b> {
);
if let Some(arg) = self.args.get(pos) {
e.span_label(
arg.span,
arg.expr.span,
"this parameter corresponds to the precision flag",
);
}
Expand Down Expand Up @@ -771,7 +743,7 @@ impl<'a, 'b> Context<'a, 'b> {
match self.names.get(&name) {
Some(&idx) => {
// Treat as positional arg.
self.verify_arg_type(Capture(idx.0), ty)
self.verify_arg_type(Capture(idx), ty)
}
None => {
// For the moment capturing variables from format strings expanded from macros is
Expand All @@ -787,8 +759,11 @@ impl<'a, 'b> Context<'a, 'b> {
self.fmtsp
};
self.num_captured_args += 1;
self.args.push(self.ecx.expr_ident(span, Ident::new(name, span)));
self.names.insert(name, (idx, span));
self.args.push(FormatArg {
expr: self.ecx.expr_ident(span, Ident::new(name, span)),
name: Some(Ident::new(name, span)),
});
self.names.insert(name, idx);
self.verify_arg_type(Capture(idx), ty)
} else {
let msg = format!("there is no argument named `{}`", name);
Expand Down Expand Up @@ -1054,11 +1029,11 @@ impl<'a, 'b> Context<'a, 'b> {
// evaluated a single time each, in the order written by the programmer,
// and that the surrounding future/generator (if any) is Send whenever
// possible.
let no_need_for_match =
nicely_ordered && !original_args.iter().skip(1).any(|e| may_contain_yield_point(e));
let no_need_for_match = nicely_ordered
&& !original_args.iter().skip(1).any(|arg| may_contain_yield_point(&arg.expr));

for (arg_index, arg_ty) in fmt_arg_index_and_ty {
let e = &mut original_args[arg_index];
let e = &mut original_args[arg_index].expr;
let span = e.span;
let arg = if no_need_for_match {
let expansion_span = e.span.with_ctxt(self.macsp.ctxt());
Expand Down Expand Up @@ -1087,7 +1062,9 @@ impl<'a, 'b> Context<'a, 'b> {
// span is otherwise unavailable in the MIR used by borrowck).
let heads = original_args
.into_iter()
.map(|e| self.ecx.expr_addr_of(e.span.with_ctxt(self.macsp.ctxt()), e))
.map(|arg| {
self.ecx.expr_addr_of(arg.expr.span.with_ctxt(self.macsp.ctxt()), arg.expr)
})
.collect();

let pat = self.ecx.pat_ident(self.macsp, Ident::new(sym::args, self.macsp));
Expand Down Expand Up @@ -1220,7 +1197,7 @@ pub fn expand_preparsed_format_args(
sp: Span,
efmt: P<ast::Expr>,
args: Vec<FormatArg>,
names: FxHashMap<Symbol, (usize, Span)>,
names: FxHashMap<Symbol, usize>,
append_newline: bool,
) -> P<ast::Expr> {
// NOTE: this verbose way of initializing `Vec<Vec<ArgumentType>>` is because
Expand Down Expand Up @@ -1312,16 +1289,17 @@ pub fn expand_preparsed_format_args(
if err.should_be_replaced_with_positional_argument {
let captured_arg_span =
fmt_span.from_inner(InnerSpan::new(err.span.start, err.span.end));
let positional_args = args.iter().filter(|arg| !arg.named).collect::<Vec<_>>();
let n_positional_args =
args.iter().rposition(|arg| arg.name.is_none()).map_or(0, |i| i + 1);
if let Ok(arg) = ecx.source_map().span_to_snippet(captured_arg_span) {
let span = match positional_args.last() {
let span = match args[..n_positional_args].last() {
Some(arg) => arg.expr.span,
None => fmt_sp,
};
e.multipart_suggestion_verbose(
"consider using a positional formatting argument instead",
vec![
(captured_arg_span, positional_args.len().to_string()),
(captured_arg_span, n_positional_args.to_string()),
(span.shrink_to_hi(), format!(", {}", arg)),
],
Applicability::MachineApplicable,
Expand All @@ -1338,11 +1316,9 @@ 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().map(|(i, _)| i).collect();

let mut cx = Context {
ecx,
args: args.into_iter().map(|arg| arg.expr).collect(),
args,
num_captured_args: 0,
arg_types,
arg_unique_types,
Expand Down Expand Up @@ -1410,14 +1386,12 @@ pub fn expand_preparsed_format_args(
.enumerate()
.filter(|(i, ty)| ty.is_empty() && !cx.count_positions.contains_key(&i))
.map(|(i, _)| {
let msg = if named_pos.contains(&i) {
// named argument
let msg = if cx.args[i].name.is_some() {
"named argument never used"
} else {
// positional argument
"argument never used"
};
(cx.args[i].span, msg)
(cx.args[i].expr.span, msg)
})
.collect::<Vec<_>>();

Expand Down

0 comments on commit 965ed81

Please sign in to comment.