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

Jump to bottom

Improve error message when typo is made in format! #75779

Merged
merged 1 commit into from
Aug 30, 2020
Merged

Improve error message when typo is made in format! #75779

merged 1 commit into from
Aug 30, 2020

Conversation

scrabsha
Copy link
Contributor

The expansion of the format! built-in macro is roughly done in two steps:

  • the format expression is parsed, the arguments are parsed,
  • the format expression is checked to be a string literal, code is expanded.

The problem is that the expression parser can eat too much tokens, which invalidates the parsing of the next format arguments. As the format expression check happens next, the error emitted concerns the format arguments, whereas the problem is about the format expression.

This PR contains two commits. The first one actually checks that the formatting expression is a string literal before raising any error about the formatting arguments, and the second one contains some simple heuristics which allow to suggest, when the format expression is followed by a dot instead of a comma, to suggest to replace the dot with a comma.

This pull request should fix #75492.

Note: this is my first non-doc contribution to the rust ecosystem. Feel free to make any comment about my code, or whatever. I'll be very happy to fix it :)

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

(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 Aug 21, 2020
@scrabsha scrabsha force-pushed the fix-issue-75492 branch 3 times, most recently from 5cf5197 to 0d16b39 Compare August 24, 2020 15:21
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Aug 24, 2020 •
edited
Loading

r? @petrochenkov

I thought highfive had some heuristic based on who has touched the files in question.

@petrochenkov
Copy link
Contributor

So, this PR looks significantly more complicated than it needs to be.

There are two orthogonal issues here:

  • the format expression is parsed over-eagerly
  • recovery for confusable separators . and , is not performed

I suggest addressing them independently starting from the first one.
Simple implementation for it:

  • Before parsing the format expression use one token look_ahead
    • if the next token is a string literal make a format expression out of it, bump() and proceed parsing other arguments
    • otherwise use parse_expr to make a format expression, and proceed parsing other arguments

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2020
@scrabsha
Copy link
Contributor Author

Thanks for your feedback, I'm working on it :)

Side question: should I open a new PR, or is it okay to modify this one?

@petrochenkov
Copy link
Contributor

@scileo
Whichever is more convenient, both ways are ok.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 29, 2020
@scrabsha
Copy link
Contributor Author

@petrochenkov
I have applied all the changes you suggested. Emitted errors are way nicer now!

There are a few changes that may need to be discussed:

  • when a string literal is detected, but the string literal itself is incorrect (eg: this branch), a fake string literal is generated. I came up with this solution by comparing what parse_expr emits in this case. However, its symbol field slightly differs from what parse_expr creates: opening and closing quotes are not included. Still all pass, but I am not sure if this is correct.
  • in order to create the format expression, I ended up using roughly the same code as Parser::mk_expr. Is it correct?
  • your suggestion does not detect cases when the format expression is not a string literal, such as format!(concat!("{", "}"). 42). This looks acceptable to me, as non-string literal format expression is pretty rare.
  • should we also handle raw strings too?
  • you talked earlier about error recovery. Is it needed now?

petrochenkov
petrochenkov reviewed Aug 29, 2020
View reviewed changes
src/librustc_builtin_macros/format.rs Outdated Show resolved Hide resolved
src/librustc_builtin_macros/format.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

you talked earlier about error recovery. Is it needed now?

Yes, but it's better to do it in a separate PR later.
We basically need to treat . as , (but with a non-fatal error) if we encounter it as a separator in the format! arguments list.
See the use of similar_tokens in parse_seq_to_before_tokens for an example.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2020
@scrabsha
Copy link
Contributor Author

you talked earlier about error recovery. Is it needed now?

Yes, but it's better to do it in a separate PR later.
We basically need to treat . as , (but with a non-fatal error) if we encounter it as a separator in the format! arguments list.
See the use of similar_tokens in parse_seq_to_before_tokens for an example.

Thanks for the clarification. My initial understandings about how to do it were wrong. I'll work on it once this PR is merged.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 30, 2020
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Aug 30, 2020

📌 Commit 1001c76c6c449e27d90409ff5cd8865d510f5341 has been approved by petrochenkov

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2020
@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 30, 2020 •
edited
Loading

@bors retry
(Prioritizing #74862.)

@bors
Copy link
Contributor

bors commented Aug 30, 2020

⌛ Testing commit 1001c76c6c449e27d90409ff5cd8865d510f5341 with merge 1a5c36a0e53086af131b47592bbaaea4cf254c62...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Aug 30, 2020

⌛ Testing commit 1001c76c6c449e27d90409ff5cd8865d510f5341 with merge c57047b5fd90f5aaafb6028350ba44356c20f56f...

@bors bors mentioned this pull request Aug 30, 2020
Merged
@bors

This comment has been minimized.

Sign in to view
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 30, 2020
@Aaron1011
Copy link
Member

@bors retry

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 30, 2020
@Aaron1011
Copy link
Member

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 30, 2020
@petrochenkov
Copy link
Contributor

Needs a rebase.
(Also, it's preferable to squash the commits into one.)

@scrabsha
Use string literal directly when available in format
f6d18db 
Previous implementation used the `Parser::parse_expr` function in order
to extract the format expression. If the first comma following the
format expression was mistakenly replaced with a dot, then the next
format expression was eaten by the function, because it looked as a
syntactically valid expression, which resulted in incorrectly spanned
error messages.

The way the format expression is exctracted is changed: we first look at
the first available token in the first argument supplied to the
`format!` macro call. If it is a string literal, then it is promoted as
a format expression immediatly, otherwise we fall back to the original
`parse_expr`-related method.

This allows us to ensure that the parser won't consume too much tokens
when a typo is made.

A test has been created so that it is ensured that the issue is properly
fixed.
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 30, 2020

📌 Commit f6d18db has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 30, 2020
@scrabsha
Copy link
Contributor Author

Thank you very much for your help @petrochenkov!

@bors
Copy link
Contributor

bors commented Aug 30, 2020

⌛ Testing commit f6d18db with merge 36b0d7e...

@bors
Copy link
Contributor

bors commented Aug 30, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing 36b0d7e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 30, 2020
@bors bors merged commit 36b0d7e into rust-lang:master Aug 30, 2020
@cuviper cuviper added this to the 1.48.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov Awaiting requested review from petrochenkov

Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.

Typoing dot instead of comma in println! gives a warning at the wrong place
8 participants
@scrabsha @rust-highfive @ecstatic-morse @petrochenkov @bors @rust-log-analyzer @Aaron1011 @cuviper