Make positional argument error in format! clearer #45807
Conversation
|
|
||
| format!("{} {value} {} {}", 1, value=2); | ||
| //~^ ERROR: invalid reference to positional argument 2 (there are only 2 arguments) | ||
| format!("{name} {value} {} {} {} {} {} {}", 0, name=1, value=2); |
There was a problem hiding this comment.
Could you add a case with multiple named arguments that are not used, mixed with positional arguments? I'm curious about the new output.
src/libsyntax_ext/format.rs
Outdated
| .map(|r| r.to_string()) | ||
| .collect(); | ||
|
|
||
| let msg = if self.names.is_empty() { |
There was a problem hiding this comment.
Add another check for self.args.iter().filter_map(|arg| /* Some(arg) if numbered positional argument */).count() == 0 here, so that if there are arguments like {1}, it goes to the else branch and it is explicit about the arguments that are wrong.
There was a problem hiding this comment.
Seems like the position of numbered and implicit placeholder is already resolved in the parser, should we change it so we still have that information here?
There was a problem hiding this comment.
If you prefer, that's also a reasonable change.
src/libsyntax_ext/format.rs
Outdated
| self.describe_num_args()) | ||
| }; | ||
|
|
||
| self.ecx.span_err(self.fmtsp, &msg[..]); |
There was a problem hiding this comment.
We should probably add a note making it explicit that positional arguments are 0 indexed, probably only when it is needed (I think only for the else branch).
kennytm
added
the
S-waiting-on-review
|
@estebank this should work now.
Yeah the output is incorrect, fixed that in the latest commit (test case: https://github.com/tommyip/rust/blob/1be9cc76daa1a5047fadb0bb0e826b47a63d8f1b/src/test/compile-fail/ifmt-bad-arg.rs#L37) |
|
@bors r+ |
|
📌 Commit b577b9a has been approved by |
kennytm
added
S-waiting-on-bors
|
☀️ Test successful - status-appveyor, status-travis |
r? @estebank
Fixes #44954