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

Fix pretty printing an AST representing &(mut ident) #80205

Merged
merged 1 commit into from
Dec 21, 2020
Merged

Fix pretty printing an AST representing &(mut ident) #80205

merged 1 commit into from
Dec 21, 2020

Conversation

tomprogrammer
Copy link
Contributor

@tomprogrammer tomprogrammer commented Dec 19, 2020

The PR fixes a misguiding help diagnostic in the parser that I reported in #80186. I discovered that the parsers recovery and reporting logic was correct but the pretty printer produced wrong code for the example. (Details in #80186 (comment))

Example:

#![allow(unused_variables)]
fn main() {
    let mut &x = &0;
}

The AST fragment

PatKind::Ref(PatKind::Ident(BindingMode::ByValue(Mutability::Mut), ..), Mutability::Not)

was printed to be &mut ident. But this wouldn't round trip through parsing again, because then it would be:

PatKind::Ref(PatKind::Ident(BindingMode::ByValue(Mutability::Not), ..), Mutability::Mut)

Now the pretty-printer prints &(mut ident). Reparsing that code results in the AST fragment

PatKind::Ref(PatKind::Paren(PatKind::Ident(BindingMode::ByValue(Mutability::Mut), ..)), Mutability::Not)

which I think should behave like the original pattern.

Old diagnostic:

error: `mut` must be attached to each individual binding
 --> src/main.rs:3:9
  |
3 |     let mut &x = &0;
  |         ^^^^^^ help: add `mut` to each binding: `&mut x`
  |
  = note: `mut` may be followed by `variable` and `variable @ pattern`

New diagnostic:

error: `mut` must be attached to each individual binding
 --> src/main.rs:3:9
  |
3 |     let mut &x = &0;
  |         ^^^^^^ help: add `mut` to each binding: `&(mut x)`
  |
  = note: `mut` may be followed by `variable` and `variable @ pattern`

Fixes #80186

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 Dec 19, 2020
@tomprogrammer
Copy link
Contributor Author

I didn't find a place to test the help diagnostics of the parser. Are these actually tested?
The change in the pretty-printer can be tested without the parser. I don't know where to add this test, so I appreciate hints where to put the test.

@camelid
Copy link
Member

camelid commented Dec 20, 2020

I didn't find a place to test the help diagnostics of the parser. Are these actually tested?
The change in the pretty-printer can be tested without the parser. I don't know where to add this test, so I appreciate hints where to put the test.

Yes, diagnostics are thoroughly tested. Just add code (the code from the issue you opened) to a file in src/test/ui/pattern (or somewhere in src/test/ui). Then run ./x.py test src/test/ui/pattern/<your-file>.rs --bless.

Note that you will need to add //~ ERROR and potentially //~ HELP annotations; read this for help with that.

@camelid camelid added A-diagnostics Area: Messages for errors, warnings, and lints A-patterns Relating to patterns and pattern matching A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` labels Dec 20, 2020
@tomprogrammer
Copy link
Contributor Author

I added a minimal test case that should cover the entire issue. All other code in the issue can be reduced to this test case.

`PatKind::Ref(PatKind::Ident(BindingMode::ByValue(Mutability::Mut), ..), ..)`
is an AST representing `&(mut ident)`. It was errorneously printed as
`&mut ident` which reparsed into a syntactically different AST.

This affected help diagnostics in the parser.
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 21, 2020

📌 Commit b05ab18 has been approved by davidtwco

@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 Dec 21, 2020
@bors
Copy link
Contributor

bors commented Dec 21, 2020

⌛ Testing commit b05ab18 with merge 1e88a17...

@bors
Copy link
Contributor

bors commented Dec 21, 2020

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 1e88a17 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 21, 2020
@bors bors merged commit 1e88a17 into rust-lang:master Dec 21, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 21, 2020
@tomprogrammer tomprogrammer deleted the prettyprint-pattern-mut-binding branch December 21, 2020 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-patterns Relating to patterns and pattern matching A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` 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
Development

Successfully merging this pull request may close these issues.

Misguiding help on error "mut must be attached to each individual binding"
6 participants