Skip to content

Add format specifier suggestion #76443

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

Closed
macdonaldo wants to merge 1 commit into from
Closed

Add format specifier suggestion #76443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Add format specifier suggestion 
When format specifier is missing from the format string:
- Found format specifiers are pointed at
- Suggest format specifier if none is present

Fix #68293
  • Loading branch information
Macdonald Umoren authored and Macdonald Umoren committed Sep 29, 2020
commit faf70d9d4765508e271adebbcd045d927b3e1c0e
53 changes: 47 additions & 6 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 @@ -1090,20 +1090,21 @@ pub fn expand_preparsed_format_args(
if !errs.is_empty() {
let args_used = cx.arg_types.len() - errs_len;
let args_unused = errs_len;
let found_fmt_specs = &cx.arg_spans;

let mut diag = {
if let [(sp, msg)] = &errs[..] {
let mut diag = cx.ecx.struct_span_err(*sp, *msg);
diag.span_label(*sp, *msg);
diag
} else {
let mut diag = cx.ecx.struct_span_err(
errs.iter().map(|&(sp, _)| sp).collect::<Vec<Span>>(),
"multiple unused formatting arguments",
);
let mut diag = cx
.ecx
.struct_span_err(errs.iter().map(|&(sp, _)| sp).collect::<Vec<Span>>(),
"multiple unused formatting arguments");
diag.span_label(cx.fmtsp, "multiple missing formatting specifiers");
for (sp, msg) in errs {
diag.span_label(sp, msg);
for (sp, msg) in &errs {
diag.span_label(*sp, *msg);
}
diag
}
Expand Down Expand Up @@ -1178,6 +1179,46 @@ pub fn expand_preparsed_format_args(
if !found_foreign && errs_len == 1 {
diag.span_label(cx.fmtsp, "formatting specifier missing");
}
if !found_foreign && found_fmt_specs.is_empty() {
diag.note("format specifiers use curly braces: `{}`");
}

if !found_foreign && !found_fmt_specs.is_empty() {
let mut note_span: MultiSpan = cx.arg_spans.clone().into();
let used_args = cx
.arg_types
.iter()
.enumerate()
.filter(|(_i, ty)| !ty.is_empty())
.map(|(i, _)| {
(cx.args[i].span, i)
})
.collect::<Vec<_>>();

let names_reversed: FxHashMap<usize, Symbol> = cx.names.iter().map(|(symbol, pos)| {
(*pos, *symbol)
}).collect();

for (span, index) in &used_args {
let mut fmt_arg_type = "positional";
let mut fmt_arg_ident = (*index).to_string();

if let Some(&symbol) = names_reversed.get(index) {
fmt_arg_type = "named";
fmt_arg_ident = symbol.to_ident_string();
}

note_span.push_span_label(*span, format!("used {} formatting argument `{}`", fmt_arg_type, fmt_arg_ident));

// for each reference to a formatting argument(identified by index), get the span of its formatting specifier
// for &ref in &cx.arg_index_map[*index] {
// let sp = &cx.arg_spans[ref];
// note_span.push_span_label(*sp, format!("{} argument `{}` used here: ",fmt_arg_type, fmt_arg_ident));
// }
}

diag.span_note(note_span, "the following existing formatting specifiers are used");
}

diag.emit();
}
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/fmt/format-args-capture-missing-variables.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: named argument never used
--> $DIR/format-args-capture-missing-variables.rs:10:51
--> $DIR/format-args-capture-missing-variables.rs:10:14
|
LL | format!("{valuea} {valueb}", valuea=5, valuec=7);
| ------------------- ^ named argument never used
| -^^^^^^^^-^^^^^^^^- ^ named argument never used
| |
| formatting specifier missing

Expand Down
32 changes: 18 additions & 14 deletions src/test/ui/if/ifmt-bad-arg.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ LL | format!("{1}", 1);
= note: positional arguments are zero-based

error: argument never used
--> $DIR/ifmt-bad-arg.rs:9:20
--> $DIR/ifmt-bad-arg.rs:9:14
|
LL | format!("{1}", 1);
| ----- ^ argument never used
| -^^^- ^ argument never used
| |
| formatting specifier missing

Expand Down Expand Up @@ -90,36 +90,38 @@ LL | format!("", 1, 2);
| | |
| | argument never used
| multiple missing formatting specifiers
|
= note: format specifiers use curly braces: `{}`

error: argument never used
--> $DIR/ifmt-bad-arg.rs:33:22
--> $DIR/ifmt-bad-arg.rs:33:14
|
LL | format!("{}", 1, 2);
| ---- ^ argument never used
| -^^- ^ argument never used
| |
| formatting specifier missing
Comment on lines 96 to 102
Copy link
Contributor

@estebank estebank Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that ideally this would look like

error: argument never used
  --> $DIR/ifmt-bad-arg.rs:33:22
   |
LL |     format!("{}", 1, 2);
   |             ----     ^ argument never used
   |             |
   |             formatting specifier missing
note: the following existing formatting specifiers are used
   |
LL |     format!("{}", 1, 2);
   |              ^^      - used by positional formatting specifier `0`
   |              |
   |              this is positional formatting specifier `0`

In order to accomplish that we would need to

  • keep track of the spans for each formatting specifier and a way to refer to them ("positional specifier 0", "named specifier foo", etc.)
  • look at the mapping for which formatting specifier is used by which argument
  • we'll need to account for named and positional specifiers that are not used

For the proposed output we would need to do something like (simplified):

let note_span: MultiSpan = cx.arg_spans.into();
for span in cx.arg_spans {
    note_span.push_span_label(span, "used argument");
}
for span in errs.iter().map(|&(sp, _)| sp) {
  note_span.push_span_label(span, "argument used here");
}
err.span_note(note_span, "the following existing formatting specifiers are used");

Would you be ok with taking on trying to accomplish this?

Copy link
Author

@macdonaldo macdonaldo Sep 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, I'll work on making these changes. Thanks for the feedback!


error: argument never used
--> $DIR/ifmt-bad-arg.rs:34:20
--> $DIR/ifmt-bad-arg.rs:34:14
|
LL | format!("{1}", 1, 2);
| ----- ^ argument never used
| -^^^- ^ argument never used
| |
| formatting specifier missing

error: named argument never used
--> $DIR/ifmt-bad-arg.rs:35:26
--> $DIR/ifmt-bad-arg.rs:35:14
|
LL | format!("{}", 1, foo=2);
| ---- ^ named argument never used
| -^^- ^ named argument never used
| |
| formatting specifier missing

error: argument never used
--> $DIR/ifmt-bad-arg.rs:36:22
--> $DIR/ifmt-bad-arg.rs:36:14
|
LL | format!("{foo}", 1, foo=2);
| ------- ^ argument never used
| -^^^^^- ^ argument never used
| |
| formatting specifier missing

Expand All @@ -130,12 +132,14 @@ LL | format!("", foo=2);
| -- ^ named argument never used
| |
| formatting specifier missing
|
= note: format specifiers use curly braces: `{}`

error: multiple unused formatting arguments
--> $DIR/ifmt-bad-arg.rs:38:32
--> $DIR/ifmt-bad-arg.rs:38:14
|
LL | format!("{} {}", 1, 2, foo=1, bar=2);
| ------- ^ ^ named argument never used
| -^^-^^- ^ ^ named argument never used
| | |
| | named argument never used
| multiple missing formatting specifiers
Expand Down Expand Up @@ -165,10 +169,10 @@ LL | format!("{valuea} {valueb}", valuea=5, valuec=7);
= help: if you intended to capture `valueb` from the surrounding scope, add `#![feature(format_args_capture)]` to the crate attributes

error: named argument never used
--> $DIR/ifmt-bad-arg.rs:45:51
--> $DIR/ifmt-bad-arg.rs:45:14
|
LL | format!("{valuea} {valueb}", valuea=5, valuec=7);
| ------------------- ^ named argument never used
| -^^^^^^^^-^^^^^^^^- ^ named argument never used
| |
| formatting specifier missing

Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/macros/format-foreign.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ LL | {}!\n
|

error: argument never used
--> $DIR/format-foreign.rs:12:30
--> $DIR/format-foreign.rs:12:15
|
LL | println!("{} %f", "one", 2.0);
| ------- ^^^ argument never used
| -^^---- ^^^ argument never used
| |
| formatting specifier missing

Expand Down
6 changes: 6 additions & 0 deletions src/test/ui/macros/format-unused-lables.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ LL | println!("Test", 123, 456, 789);
| | | argument never used
| | argument never used
| multiple missing formatting specifiers
|
= note: format specifiers use curly braces: `{}`

error: multiple unused formatting arguments
--> $DIR/format-unused-lables.rs:6:9
Expand All @@ -19,6 +21,8 @@ LL | 456,
| ^^^ argument never used
LL | 789
| ^^^ argument never used
|
= note: format specifiers use curly braces: `{}`

error: named argument never used
--> $DIR/format-unused-lables.rs:11:35
Expand All @@ -27,6 +31,8 @@ LL | println!("Some stuff", UNUSED="args");
| ------------ ^^^^^^ named argument never used
| |
| formatting specifier missing
|
= note: format specifiers use curly braces: `{}`

error: multiple unused formatting arguments
--> $DIR/format-unused-lables.rs:14:9
Expand Down
38 changes: 38 additions & 0 deletions src/test/ui/suggestions/issue-68293-missing-format-specifier.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Issue 68293: This tests that the following changes work:
// the suggestion "format specifiers use curly braces: `{}`" is made
// found format specifiers are pointed at

fn no_format_specifiers_one_unused_argument() {
println!("list: ", 1);
//~^ ERROR argument never used
//~| NOTE formatting specifier missing
//~| NOTE format specifiers use curly braces: `{}`
//~| NOTE argument never used
}

fn no_format_specifiers_multiple_unused_arguments() {
println!("list: ", 3, 4, 5);
//~^ ERROR multiple unused formatting arguments
//~| NOTE multiple missing formatting specifiers
//~| NOTE format specifiers use curly braces: `{}`
//~| NOTE argument never used
//~| NOTE argument never used
//~| NOTE argument never used
}

fn missing_format_specifiers_one_unused_argument() {
println!("list: a{}, b{}", 1, 2, 3);
//~^ ERROR argument never used
//~| NOTE formatting specifier missing
//~| NOTE argument never used
}

fn missing_format_specifiers_multiple_unused_arguments() {
println!("list: a{}, b{} c{}", 1, 2, 3, 4, 5);
//~^ ERROR multiple unused formatting arguments
//~| NOTE multiple missing formatting specifiers
//~| NOTE argument never used
//~| NOTE argument never used
}

fn main() {}
41 changes: 41 additions & 0 deletions src/test/ui/suggestions/issue-68293-missing-format-specifier.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
error: argument never used
--> $DIR/issue-68293-missing-format-specifier.rs:6:22
|
LL | println!("list: ", 1);
| -------- ^ argument never used
| |
| formatting specifier missing
|
= note: format specifiers use curly braces: `{}`

error: multiple unused formatting arguments
--> $DIR/issue-68293-missing-format-specifier.rs:14:22
|
LL | println!("list: ", 3, 4, 5);
| -------- ^ ^ ^ argument never used
| | | |
| | | argument never used
| | argument never used
| multiple missing formatting specifiers
|
= note: format specifiers use curly braces: `{}`

error: argument never used
--> $DIR/issue-68293-missing-format-specifier.rs:24:20
|
LL | println!("list: a{}, b{}", 1, 2, 3);
| --------^^---^^- ^ argument never used
| |
| formatting specifier missing

error: multiple unused formatting arguments
--> $DIR/issue-68293-missing-format-specifier.rs:31:20
|
LL | println!("list: a{}, b{} c{}", 1, 2, 3, 4, 5);
| --------^^---^^--^^- ^ ^ argument never used
| | |
| | argument never used
| multiple missing formatting specifiers

error: aborting due to 4 previous errors