Skip to content

Commit

Permalink
Auto merge of #99660 - PrestonFrom:issue_99265, r=compiler-errors
Browse files Browse the repository at this point in the history 
Generate correct suggestion with named arguments used positionally

Address issue #99265 by checking each positionally used argument
to see if the argument is named and adding a lint to use the name
instead. This way, when named arguments are used positionally in a
different order than their argument order, the suggested lint is
correct.

For example:
```
println!("{b} {}", a=1, b=2);
```
This will now generate the suggestion:
```
println!("{b} {a}", a=1, b=2);
```

Additionally, this check now also correctly replaces or inserts
only where the positional argument is (or would be if implicit).
Also, width and precision are replaced with their argument names
when they exists.

Since the issues were so closely related, this fix for issue #99265
also fixes issue #99266.

Fixes #99265
Fixes #99266
  • Loading branch information
@bors
bors committed Jul 29, 2022
2 parents 9de7474 + 1b2e05e commit ea6ab1b
Show file tree
Hide file tree
Showing 12 changed files with 1,120 additions and 102 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_builtin_macros/src/asm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ fn expand_preparsed_asm(ecx: &mut ExtCtxt<'_>, args: AsmArgs) -> Option<ast::Inl
let span = arg_spans.next().unwrap_or(template_sp);

let operand_idx = match arg.position {
parse::ArgumentIs(idx) | parse::ArgumentImplicitlyIs(idx) => {
parse::ArgumentIs(idx, _) | parse::ArgumentImplicitlyIs(idx) => {
if idx >= args.operands.len()
|| named_pos.contains_key(&idx)
|| args.reg_args.contains(&idx)
Expand Down
274 changes: 214 additions & 60 deletions compiler/rustc_builtin_macros/src/format.rs
View file

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions compiler/rustc_lint/src/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -861,10 +861,10 @@ pub trait LintContext: Sized {
if let Some(positional_arg) = positional_arg {
let msg = format!("this formatting argument uses named argument `{}` by position", name);
db.span_label(positional_arg, msg);
db.span_suggestion_verbose(
db.span_suggestion_verbose(
positional_arg,
"use the named argument by name to avoid ambiguity",
format!("{{{}}}", name),
name,
Applicability::MaybeIncorrect,
);
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_lint_defs/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ pub enum BuiltinLintDiagnostics {
/// If true, the lifetime will be fully elided.
use_span: Option<(Span, bool)>,
},
NamedArgumentUsedPositionally(Option<Span>, Span, Symbol),
NamedArgumentUsedPositionally(Option<Span>, Span, String),
}

/// Lints that are buffered up early on in the `Session` before the
Expand Down
20 changes: 16 additions & 4 deletions compiler/rustc_parse_format/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,16 +104,16 @@ pub struct FormatSpec<'a> {
pub enum Position<'a> {
/// The argument is implied to be located at an index
ArgumentImplicitlyIs(usize),
/// The argument is located at a specific index given in the format
ArgumentIs(usize),
/// The argument is located at a specific index given in the format,
ArgumentIs(usize, Option<InnerSpan>),
/// The argument has a name.
ArgumentNamed(&'a str, InnerSpan),
}

impl Position<'_> {
pub fn index(&self) -> Option<usize> {
match self {
ArgumentIs(i) | ArgumentImplicitlyIs(i) => Some(*i),
ArgumentIs(i, ..) | ArgumentImplicitlyIs(i) => Some(*i),
_ => None,
}
}
Expand Down Expand Up @@ -502,8 +502,15 @@ impl<'a> Parser<'a> {
/// Returns `Some(parsed_position)` if the position is not implicitly
/// consuming a macro argument, `None` if it's the case.
fn position(&mut self) -> Option<Position<'a>> {
let start_position = self.cur.peek().map(|item| item.0);
if let Some(i) = self.integer() {
Some(ArgumentIs(i))
let inner_span = start_position.and_then(|start| {
self.cur
.peek()
.cloned()
.and_then(|item| Some(self.to_span_index(start).to(self.to_span_index(item.0))))
});
Some(ArgumentIs(i, inner_span))
} else {
match self.cur.peek() {
Some(&(start, c)) if rustc_lexer::is_id_start(c) => {
Expand Down Expand Up @@ -574,6 +581,10 @@ impl<'a> Parser<'a> {
// no '0' flag and '0$' as the width instead.
if let Some(end) = self.consume_pos('$') {
spec.width = CountIsParam(0);

if let Some((pos, _)) = self.cur.peek().cloned() {
spec.width_span = Some(self.to_span_index(pos - 2).to(self.to_span_index(pos)));
}
havewidth = true;
spec.width_span = Some(self.to_span_index(end - 1).to(self.to_span_index(end + 1)));
} else {
Expand All @@ -586,6 +597,7 @@ impl<'a> Parser<'a> {
spec.width = w;
spec.width_span = sp;
}

if let Some(start) = self.consume_pos('.') {
if let Some(end) = self.consume_pos('*') {
// Resolve `CountIsNextParam`.
Expand Down
28 changes: 20 additions & 8 deletions compiler/rustc_parse_format/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,18 +62,30 @@ fn format_nothing() {
}
#[test]
fn format_position() {
same("{3}", &[NextArgument(Argument { position: ArgumentIs(3), format: fmtdflt() })]);
same(
"{3}",
&[NextArgument(Argument {
position: ArgumentIs(3, Some(InnerSpan { start: 2, end: 3 })),
format: fmtdflt(),
})],
);
}
#[test]
fn format_position_nothing_else() {
same("{3:}", &[NextArgument(Argument { position: ArgumentIs(3), format: fmtdflt() })]);
same(
"{3:}",
&[NextArgument(Argument {
position: ArgumentIs(3, Some(InnerSpan { start: 2, end: 3 })),
format: fmtdflt(),
})],
);
}
#[test]
fn format_type() {
same(
"{3:x}",
&[NextArgument(Argument {
position: ArgumentIs(3),
position: ArgumentIs(3, Some(InnerSpan { start: 2, end: 3 })),
format: FormatSpec {
fill: None,
align: AlignUnknown,
Expand All @@ -93,7 +105,7 @@ fn format_align_fill() {
same(
"{3:>}",
&[NextArgument(Argument {
position: ArgumentIs(3),
position: ArgumentIs(3, Some(InnerSpan { start: 2, end: 3 })),
format: FormatSpec {
fill: None,
align: AlignRight,
Expand All @@ -110,7 +122,7 @@ fn format_align_fill() {
same(
"{3:0<}",
&[NextArgument(Argument {
position: ArgumentIs(3),
position: ArgumentIs(3, Some(InnerSpan { start: 2, end: 3 })),
format: FormatSpec {
fill: Some('0'),
align: AlignLeft,
Expand All @@ -127,7 +139,7 @@ fn format_align_fill() {
same(
"{3:*<abcd}",
&[NextArgument(Argument {
position: ArgumentIs(3),
position: ArgumentIs(3, Some(InnerSpan { start: 2, end: 3 })),
format: FormatSpec {
fill: Some('*'),
align: AlignLeft,
Expand Down Expand Up @@ -181,7 +193,7 @@ fn format_counts() {
same(
"{1:0$.10x}",
&[NextArgument(Argument {
position: ArgumentIs(1),
position: ArgumentIs(1, Some(InnerSpan { start: 2, end: 3 })),
format: FormatSpec {
fill: None,
align: AlignUnknown,
Expand Down Expand Up @@ -291,7 +303,7 @@ fn format_mixture() {
&[
String("abcd "),
NextArgument(Argument {
position: ArgumentIs(3),
position: ArgumentIs(3, Some(InnerSpan { start: 7, end: 8 })),
format: FormatSpec {
fill: None,
align: AlignUnknown,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/traits/on_unimplemented.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ impl<'tcx> OnUnimplementedFormatString {
}
}
// `{:1}` and `{}` are not to be used
Position::ArgumentIs(_) | Position::ArgumentImplicitlyIs(_) => {
Position::ArgumentIs(..) | Position::ArgumentImplicitlyIs(_) => {
let reported = struct_span_err!(
tcx.sess,
span,
Expand Down
48 changes: 24 additions & 24 deletions src/test/ui/macros/issue-98466.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,80 +2,80 @@ 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
| - ^^ 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
| - ^ 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
| - ^ 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
| - ^^ 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
| - ^ 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
| - ^ 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

Loading

0 comments on commit ea6ab1b

Please sign in to comment.