Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify format_args builtin macro implementation. #100277

Merged
merged 2 commits into from
Aug 15, 2022
Merged

Simplify format_args builtin macro implementation. #100277

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2808e07
Simplify format_args builtin macro implementation.
m-ou-se Aug 8, 2022
a639fdb
Get rid of named_pos in format_args impl.
m-ou-se Aug 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 64 additions & 90 deletions compiler/rustc_builtin_macros/src/format.rs
View file
Open in desktop
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