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

in which parentheses are suggested for should-have-been-tuple-patterns #48527

Merged
merged 1 commit into from Mar 9, 2018

Conversation

Projects
None yet
6 participants
@zackmdavis
Member

zackmdavis commented Feb 25, 2018

destructure_suggest_parens

Programmers used to working in some other languages (such as Python or
Go) might expect to be able to destructure values with comma-separated
identifiers but no parentheses on the left side of an assignment.

Previously, the first name in such code would get parsed as a
single-indentifier pattern—recognizing, for example, the
let a in let a, b = (1, 2);—whereupon we would have a fatal syntax
error on seeing an unexpected comma rather than the expected semicolon
(all the way nearer to the end of parse_full_stmt).

Instead, let's look for that comma when parsing the pattern, and if we
see it, make-believe that we're parsing the remaining elements in a
tuple pattern, so that we can suggest wrapping it all in parentheses. We
need to do this in a separate wrapper method called on a "top-level"
pattern, rather than within
parse_pat itself, because parse_pat gets called recursively to parse
the sub-patterns within a tuple pattern.

We could also do this for match arms, if let, and while let, but
we elect not to in this patch, as it seems less likely for users to make
the mistake in those contexts.

Resolves #48492.

r? @petrochenkov

// suggestion-enhanced error here rather than choking on the comma
// later.
if let PatKind::Ident(_, _, ref sub) = pat.node {
if sub.is_none() { // just `ident`, not `ident @ pattern`

This comment has been minimized.

@petrochenkov

petrochenkov Feb 25, 2018

Contributor

PatKind::Ident and sub.is_none here are artificial restrictions.
What we want is to parse PAT, PAT, ..., PAT instead of just PAT and return PatKind::Tuple containing those patterns.
It's not very important if those pattern are identifiers or not - if we can easily recover more, we should do it.
(You may want to modify parse_pat_tuple_elements to return partial result even in case of failure.)

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Feb 25, 2018

We could also do this for match arms, if let, and while let, but
we elect not to in this patch

If supporting if let and other location means simply using parse_pat_fancy instead of parse_pat, I'd rather use parse_pat_fancy everywhere by default unless it conflicts with something.
Supporting "top level" patterns like if let and while let seems to be a part of the "baseline" to me.

@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Feb 28, 2018

@petrochenkov updated (force-push)

artificial restrictions [...] If supporting if let and other location means simply using parse_pat_fancy instead of parse_pat, I'd rather use parse_pat_fancy everywhere by default

You're right; fixed.

(You may want to modify parse_pat_tuple_elements to return partial result even in case of failure.)

The second commit here (d615130) revives the partial-result type from the abandoned #46721. This lets us check subsequent code that makes use of the destructured variables, which I think is exciting!—

zmd@ReflectiveCoherence:~/Code/rust$ rustc +stage1 recover.rs 
error: unexpected `,` in pattern
 --> recover.rs:2:10
  |
2 |     let a, b = (1, 2);
  |         -^-- help: try adding parentheses: `(a, b)`

error[E0277]: cannot divide `{integer}` by `{float}`
 --> recover.rs:3:15
  |
3 |     let c = a / 1.5;
  |               ^ no implementation for `{integer} / {float}`
  |
  = help: the trait `std::ops::Div<{float}>` is not implemented for `{integer}`

error: aborting due to 2 previous errors

(On master, we never get to the type error because parsing was interrupted by the bad pattern. With the first commit (ed6679c) but without d615130, the compiler will think that a has type ({integer}, {integer}) and that b doesn't exist.)

But there are other situations where autotupling can be misleading:

zmd@ReflectiveCoherence:~/Code/rust$ rustc +stage1 slice.rs 
error: unexpected `,` in pattern
 --> slice.rs:4:10
  |
4 |     let a, b = &[1, 2];
  |         -^-- help: try adding parentheses: `(a, b)`

error[E0658]: non-reference pattern used to match a reference (see issue #42640)
 --> slice.rs:4:9
  |
4 |     let a, b = &[1, 2];
  |         ^^^^ help: consider using a reference: `&a, b`
  |
  = help: add #![feature(match_default_bindings)] to the crate attributes to enable

error[E0308]: mismatched types
 --> slice.rs:4:9
  |
4 |     let a, b = &[1, 2];
  |         ^^^^ expected array of 2 elements, found tuple
  |
  = note: expected type `[{integer}; 2]`
             found type `(_, _)`

error: aborting due to 3 previous errors

(The E0658 suggestion is wrong, and the E0308 diagnostic in isolation makes it look like we support Python-like parenless tuples, which we don't.)

What do you think?

@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Feb 28, 2018

(Travis failure is only because I rebased on master without being aware of #48449.)

@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Feb 28, 2018

On further thought, I'm leaning toward the position that the incongruous type errors "downstream" are bad enough that we actually shouldn't try to recover more than the original error did. That is: parse_top_level_pat should return an Err (preventing us from parsing further) rather than just emitting a diagnostic and returning Ok ... still eager for reviewers' thoughts, though.

@estebank

This comment has been minimized.

Contributor

estebank commented Feb 28, 2018

@zackmdavis I agree with your assessment. Another (more involved) option would be to set the type of the tuple elements to TyErr and continue, but doing this would be non-trivial. If we try to go down that route, I'd rather do so in a follow up PR as part of a full revamp (piece meal improvement towards this in #46732).

In the meantime, I think it is very reasonable to bail out on the enclosing scope and continue (what you're doing). That way the rest of the code will be type checked.

@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Feb 28, 2018

(force-pushed a more conservative version that returns Err in accordance with previous two comments)

r? @estebank

@petrochenkov petrochenkov self-assigned this Mar 1, 2018

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Mar 1, 2018

Recovery is not required to be 100% correct, it's guessing by definition, it should only be correct in most cases.
If the let a, b = [1, 2] mistake is relatively rare in practice compared to let a, b = (1, 2) mistake, then we can issue extra diagnostics for it.

That said, even without recovery this is a nice improvement.
@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Mar 1, 2018

📌 Commit a36e77c has been approved by petrochenkov

@bors

This comment has been minimized.

Contributor

bors commented Mar 1, 2018

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #48338
@bors

This comment has been minimized.

Contributor

bors commented Mar 1, 2018

📌 Commit a36e77c has been approved by petrochenkov

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 2, 2018

Rollup merge of rust-lang#48527 - zackmdavis:and_the_social_construct…
…ion_of_tuples, r=petrochenkov

in which parentheses are suggested for should-have-been-tuple-patterns

![destructure_suggest_parens](https://user-images.githubusercontent.com/1076988/36638335-48b082d4-19a7-11e8-9726-0d043544df2f.png)

Programmers used to working in some other languages (such as Python or
Go) might expect to be able to destructure values with comma-separated
identifiers but no parentheses on the left side of an assignment.

Previously, the first name in such code would get parsed as a
single-indentifier pattern—recognizing, for example, the
`let a` in `let a, b = (1, 2);`—whereupon we would have a fatal syntax
error on seeing an unexpected comma rather than the expected semicolon
(all the way nearer to the end of `parse_full_stmt`).

Instead, let's look for that comma when parsing the pattern, and if we
see it, make-believe that we're parsing the remaining elements in a
tuple pattern, so that we can suggest wrapping it all in parentheses. We
need to do this in a separate wrapper method called on a "top-level"
pattern, rather than within
`parse_pat` itself, because `parse_pat` gets called recursively to parse
the sub-patterns within a tuple pattern.

~~We could also do this for `match` arms, `if let`, and `while let`, but
we elect not to in this patch, as it seems less likely for users to make
the mistake in those contexts.~~

Resolves rust-lang#48492.

r? @petrochenkov

bors added a commit that referenced this pull request Mar 2, 2018

Auto merge of #48652 - Manishearth:rollup, r=Manishearth
Rollup of 5 pull requests

- Successful merges: #48338, #48527, #48592, #48629, #48635
- Failed merges:

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 2, 2018

@kennytm kennytm referenced this pull request Mar 2, 2018

Closed

Another rollup #2 #48659

@kennytm

This comment has been minimized.

Member

kennytm commented Mar 2, 2018

@bors r-

The parse_pat_tuple_elements function was recently removed in #48500, replaced by parse_parenthesized_pat_list. Please rebase and update the code.

@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Mar 3, 2018

(rebased)

@estebank

This comment has been minimized.

Contributor

estebank commented Mar 3, 2018

The rebase seems to have messed the parsing up:

[01:12:28] 2	  --> $DIR/issue-48492-tuple-destructure-missing-parens.rs:48:17
[01:12:28] 3	   |
[01:12:28] 4	LL |     while let b1, b2, b3 = reading_frame.next().expect("there should be a start codon") {
[01:12:28] -	   |               --^------- help: try adding parentheses: `(b1, b2, b3)`
[01:12:28] +	   |               --^
[01:12:28] +	   |               |
[01:12:28] +	   |               help: try adding parentheses: `(b1,)`
// later.
let comma_span = self.span;
self.bump();
if let Err(mut err) = self.parse_parenthesized_pat_list() {

This comment has been minimized.

@petrochenkov

petrochenkov Mar 3, 2018

Contributor

I'd keep fn parse_parenthesized_pat_list but factor out fn parse_pat_list out of it and use it here.

This comment has been minimized.

@zackmdavis

zackmdavis Mar 8, 2018

Member

Thanks.

in which parentheses are suggested for should-have-been-tuple-patterns
Programmers used to working in some other languages (such as Python or
Go) might expect to be able to destructure values with comma-separated
identifiers but no parentheses on the left side of an assignment.

Previously, the first name in such code would get parsed as a
single-indentifier pattern—recognizing, for example, the
`let a` in `let a, b = (1, 2);`—whereupon we would have a fatal syntax
error on seeing an unexpected comma rather than the expected semicolon
(all the way nearer to the end of `parse_full_stmt`).

Instead, let's look for that comma when parsing the pattern, and if we
see it, momentarily make-believe that we're parsing the remaining
elements in a tuple pattern, so that we can suggest wrapping it all in
parentheses. We need to do this in a separate wrapper method called on
the top-level pattern (or `|`-patterns) in a `let` statement, `for`
loop, `if`- or `while let` expression, or match arm rather than within
`parse_pat` itself, because `parse_pat` gets called recursively to parse
the sub-patterns within a tuple pattern.

Resolves #48492.
@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Mar 8, 2018

(updated)

@estebank

This comment has been minimized.

Contributor

estebank commented Mar 8, 2018

@bors r+ rollup

@bors

This comment has been minimized.

Contributor

bors commented Mar 8, 2018

📌 Commit 1f04597 has been approved by estebank

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 9, 2018

Rollup merge of rust-lang#48527 - zackmdavis:and_the_social_construct…
…ion_of_tuples, r=estebank

in which parentheses are suggested for should-have-been-tuple-patterns

![destructure_suggest_parens](https://user-images.githubusercontent.com/1076988/36638335-48b082d4-19a7-11e8-9726-0d043544df2f.png)

Programmers used to working in some other languages (such as Python or
Go) might expect to be able to destructure values with comma-separated
identifiers but no parentheses on the left side of an assignment.

Previously, the first name in such code would get parsed as a
single-indentifier pattern—recognizing, for example, the
`let a` in `let a, b = (1, 2);`—whereupon we would have a fatal syntax
error on seeing an unexpected comma rather than the expected semicolon
(all the way nearer to the end of `parse_full_stmt`).

Instead, let's look for that comma when parsing the pattern, and if we
see it, make-believe that we're parsing the remaining elements in a
tuple pattern, so that we can suggest wrapping it all in parentheses. We
need to do this in a separate wrapper method called on a "top-level"
pattern, rather than within
`parse_pat` itself, because `parse_pat` gets called recursively to parse
the sub-patterns within a tuple pattern.

~~We could also do this for `match` arms, `if let`, and `while let`, but
we elect not to in this patch, as it seems less likely for users to make
the mistake in those contexts.~~

Resolves rust-lang#48492.

r? @petrochenkov

bors added a commit that referenced this pull request Mar 9, 2018

Auto merge of #48860 - Manishearth:rollup, r=Manishearth
Rollup of 5 pull requests

- Successful merges: #48527, #48588, #48801, #48856, #48857
- Failed merges:

@bors bors merged commit 1f04597 into rust-lang:master Mar 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment