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.

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

On format!() arg count mismatch provide extra info #63121

Merged
merged 5 commits into from
Aug 3, 2019

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jul 30, 2019

When positional width and precision formatting flags are present in a
formatting string that has an argument count mismatch, provide extra
information pointing at them making it easiser to understand where the
problem may lay:

error: 4 positional arguments in format string, but there are 3 arguments
  --> $DIR/ifmt-bad-arg.rs:78:15
   |
LL |     println!("{} {:.*} {}", 1, 3.2, 4);
   |               ^^ ^^--^ ^^      --- this parameter corresponds to the precision flag
   |                    |
   |                    this precision flag adds an extra required argument at position 1, which is why there are 4 arguments expected
   |
   = note: positional arguments are zero-based
   = note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html

error: 4 positional arguments in format string, but there are 3 arguments
  --> $DIR/ifmt-bad-arg.rs:81:15
   |
LL |     println!("{} {:07$.*} {}", 1, 3.2, 4);
   |               ^^ ^^-----^ ^^      --- this parameter corresponds to the precision flag
   |                    |  |
   |                    |  this precision flag adds an extra required argument at position 1, which is why there are 4 arguments expected
   |                    this width flag expects an `usize` argument at position 7, but there are 3 arguments
   |
   = note: positional arguments are zero-based
   = note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html

error: invalid reference to positional argument 7 (there are 3 arguments)
  --> $DIR/ifmt-bad-arg.rs:84:18
   |
LL |     println!("{} {:07$} {}", 1, 3.2, 4);
   |                  ^^^--^
   |                     |
   |                     this width flag expects an `usize` argument at position 7, but there are 3 arguments
   |
   = note: positional arguments are zero-based
   = note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html

Fix #49384.

When positional width and precision formatting flags are present in a
formatting string that has an argument count mismatch, provide extra
information pointing at them making it easiser to understand where the
problem may lay:

```
error: 4 positional arguments in format string, but there are 3 arguments
  --> $DIR/ifmt-bad-arg.rs:78:15
   |
LL |     println!("{} {:.*} {}", 1, 3.2, 4);
   |               ^^ ^^--^ ^^      --- this parameter corresponds to the precision flag
   |                    |
   |                    this precision flag adds an extra required argument at position 1, which is why there are 4 arguments expected
   |
   = note: positional arguments are zero-based
   = note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html

error: 4 positional arguments in format string, but there are 3 arguments
  --> $DIR/ifmt-bad-arg.rs:81:15
   |
LL |     println!("{} {:07$.*} {}", 1, 3.2, 4);
   |               ^^ ^^-----^ ^^      --- this parameter corresponds to the precision flag
   |                    |  |
   |                    |  this precision flag adds an extra required argument at position 1, which is why there are 4 arguments expected
   |                    this width flag expects an `usize` argument at position 7, but there are 3 arguments
   |
   = note: positional arguments are zero-based
   = note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html

error: 3 positional arguments in format string, but there are 3 arguments
  --> $DIR/ifmt-bad-arg.rs:84:15
   |
LL |     println!("{} {:07$} {}", 1, 3.2, 4);
   |               ^^ ^^---^ ^^
   |                    |
   |                    this width flag expects an `usize` argument at position 7, but there are 3 arguments
   |
   = note: positional arguments are zero-based
   = note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html
```
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2019
@estebank
Copy link
Contributor Author

r? @Centril

src/libfmt_macros/lib.rs Show resolved Hide resolved
src/libfmt_macros/lib.rs Show resolved Hide resolved
src/libsyntax_ext/format.rs Outdated Show resolved Hide resolved
src/libfmt_macros/lib.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Jul 30, 2019

Unfortunately I'm not really familiar with this part of the compiler so I don't feel comfortable doing the main review but I left some comments above.

r? @alexcrichton cc @Mark-Simulacrum

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me! I tend to not review the error-generation code too too carefully, but the error messages look fantastic and I've largely just got one stylistic request on the fmt_macros crate

@@ -554,24 +577,25 @@ impl<'a> Parser<'a> {
/// Parses a Count parameter at the current position. This does not check
/// for 'CountIsNextParam' because that is only used in precision, not
/// width.
fn count(&mut self) -> Count {
fn count(&mut self, start: usize) -> (Count, Option<InnerSpan>) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it's pretty tailored in the implementation, but perhaps this could be somewhat more generalized? For example instead of modifying existing methods and having some which return a span and some which don't (in addition to passing around start spans and such) could one new function be added liek:

fn span<R>(&mut self, f: impl FnOnce(&mut Self) -> R) -> (R, Span) {
    // ...
}

and that's called like let (count, span) = self.span(|me| me.count()); or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that the actual span for the thing being composed requires some passing of location between different methods. count in particular is only used in two places and both care about the span. These methods are meant to only be used from a single module, which is why I didn't bother with an externally pleasing api surface for them.

src/libsyntax_ext/format.rs Show resolved Hide resolved
@alexcrichton
Copy link
Member

@bors: r+

Ok, sounds reasonable to me!

@bors
Copy link
Contributor

bors commented Aug 2, 2019

📌 Commit 22ea38d has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 2, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 2, 2019
…hton

On `format!()` arg count mismatch provide extra info

When positional width and precision formatting flags are present in a
formatting string that has an argument count mismatch, provide extra
information pointing at them making it easiser to understand where the
problem may lay:

```
error: 4 positional arguments in format string, but there are 3 arguments
  --> $DIR/ifmt-bad-arg.rs:78:15
   |
LL |     println!("{} {:.*} {}", 1, 3.2, 4);
   |               ^^ ^^--^ ^^      --- this parameter corresponds to the precision flag
   |                    |
   |                    this precision flag adds an extra required argument at position 1, which is why there are 4 arguments expected
   |
   = note: positional arguments are zero-based
   = note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html

error: 4 positional arguments in format string, but there are 3 arguments
  --> $DIR/ifmt-bad-arg.rs:81:15
   |
LL |     println!("{} {:07$.*} {}", 1, 3.2, 4);
   |               ^^ ^^-----^ ^^      --- this parameter corresponds to the precision flag
   |                    |  |
   |                    |  this precision flag adds an extra required argument at position 1, which is why there are 4 arguments expected
   |                    this width flag expects an `usize` argument at position 7, but there are 3 arguments
   |
   = note: positional arguments are zero-based
   = note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html

error: invalid reference to positional argument 7 (there are 3 arguments)
  --> $DIR/ifmt-bad-arg.rs:84:18
   |
LL |     println!("{} {:07$} {}", 1, 3.2, 4);
   |                  ^^^--^
   |                     |
   |                     this width flag expects an `usize` argument at position 7, but there are 3 arguments
   |
   = note: positional arguments are zero-based
   = note: for information about formatting flags, visit https://doc.rust-lang.org/std/fmt/index.html
```

Fix rust-lang#49384.
bors added a commit that referenced this pull request Aug 2, 2019
Rollup of 7 pull requests

Successful merges:

 - #63107 (Added support for armv7-unknown-linux-gnueabi/musleabi)
 - #63121 (On `format!()` arg count mismatch provide extra info)
 - #63196 (build_helper: try less confusing method names)
 - #63206 (remove unsupported test case)
 - #63208 (Round generator sizes to a multiple of their alignment)
 - #63212 (Pretty print attributes in `print_arg`)
 - #63215 (Clarify semantics of mem::zeroed)

Failed merges:

r? @ghost
@bors bors merged commit 22ea38d into rust-lang:master Aug 3, 2019
phansch added a commit to phansch/rust-clippy that referenced this pull request Aug 3, 2019
Broken due to:

* rust-lang/rust#63180 (`Existential` -> `OpaqueTy`)
* rust-lang/rust#63121 (New fields for `FormatSpec`)
bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 3, 2019
Rustup to latest rustc master

Broken due to:

* rust-lang/rust#63180 (`Existential` -> `OpaqueTy`)
* rust-lang/rust#63121 (New fields for `FormatSpec`)
bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 3, 2019
Rustup to latest rustc master

Broken due to:

* rust-lang/rust#63180 (`Existential` -> `OpaqueTy`)
* rust-lang/rust#63121 (New fields for `FormatSpec`)
bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 3, 2019
Rustup to latest rustc master

Broken due to:

changelog: none

* rust-lang/rust#63180 (`Existential` -> `OpaqueTy`)
* rust-lang/rust#63121 (New fields for `FormatSpec`)
bors added a commit to rust-lang/rust-clippy that referenced this pull request Aug 3, 2019
Rustup to latest rustc master

Broken due to:

* rust-lang/rust#63180 (`Existential` -> `OpaqueTy`)
* rust-lang/rust#63121 (New fields for `FormatSpec`)

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing error message if you forget the width of the * format specifier
6 participants