Fully implement or-pattern parsing #63693

Centril · 2019-08-18T22:45:51Z

Builds upon the initial parsing in #61708 to fully implement or-pattern (p | q) parsing as specified in the grammar section of RFC 2535.

Noteworthy:

We allow or-patterns in [p | q, ...].
We allow or-patterns in let statements and for expressions including with leading |.
We improve recovery for p || q (+ tests for that in multiple-pattern-typo.rs).
We improve recovery for | p | q in inner patterns (tests in or-patterns-syntactic-fail.rs).
We rigorously test or-pattern parsing (in or-patterns-syntactic-{pass,fail}.rs).
We harden the feature gating tests.
We do not change ast.rs. That is, ExprKind::Let.0 and Arm.pats still accept Vec<P<Pat>>.
I was starting work on that but it would be cleaner to do this in a separate PR so this one has a narrower scope.

cc @dlrobertson
cc the tracking issue #54883.

r? @estebank

dlrobertson · 2019-08-19T14:42:21Z

src/libsyntax/parse/parser/expr.rs

@@ -1448,7 +1450,7 @@ impl<'a> Parser<'a> {

        Ok(ast::Arm {
            attrs,
-            pats,
+            pats: pat, // FIXME(or_patterns, Centril | dlrobertson): this should just be `pat,`.


Why would this just be a singular pat? Foo(A | B) will now be a single pattern that contains an or-pattern, but woudn't we still want to parse Foo(A) | Bar(B) as multiple patterns?

Having arm.pat: P<Pat> instead of arm.pats: Vec<P<Pat>> simplifies the overall structure of the compiler by using a uniform mechanism for or-patterns instead of having a distinction between or-patterns at the top-level and in nested positions. As an example, you can see that the parser pat.rs parser has, in this PR, a hack to unpack an or-pattern at the top level. There are many other places (e.g. in resolve, save-analysis, etc.) in which it would simplify (and reduce duplication of logic) to have one uniform mechanism. So we should represent p1 | p2 at the top of a match arm simply as PatKind::Or.

Edit: As a further consideration, using Vec<P<Pat>> pessimizes what I think is the most common case of an arm with just Foo => . Moreover, let now permits or-patterns so there's no good reason to have a top-level distinction sometimes and sometimes not. In terms of preventing bugs, it is also better to have one mechanism since you can ensure that way that the semantics at the top and inner levels do not diverge.

Got it! This makes a lot of sense and I'd love to see this happen.The lowering in MIR will require quite a bit of work and thought in order to get to this point, but AFAIK unifying the two could also greatly simplify librustc_mir/build/matches.

Oh absolutely. I think basically we want to work from both sides and meet in the middle. That is, we fix the AST and add a hack to lowering. Then we fix lowering + HIR and add a hack to HAIR. Eventually we get down to MIR and the hack is no more.

src/libsyntax/parse/parser/pat.rs

estebank · 2019-08-20T21:56:24Z

src/libsyntax/parse/parser/pat.rs

+
+        // If the next token is not a `|`,
+        // this is not an or-pattern and we should exit here.
+        if !self.check(&token::BinOp(token::Or)) && self.token != token::OrOr {


I can see this becoming a diagnostics problem in a similar vein to { foo() } * bar.

I guess you can look-ahead for the absence of various tokens like , ] } ) and try to recover? I'd prefer to do more diagnostics polish in a follow up tho.

I don't disagree, it's just that I noticed how hard it was to deal on expression parsing recovery because of similar things so I wanted to bring it up in case you might have some ideas on how to restrict it. Not critical for this PR.

Thanks for the heads-up. :)

A cheap solution code-wise would be to reuse the generic list parsing infra and use | as the separator. Then you also need to check that trailing == false and error otherwise. (that's actually a good opportunity for recovery in case of a trailing |). The drawback to this is that it will unconditionally allocate a Vec<P<Pat>> which you will need to then convert back to P<Pat> in the case of a single element. That might pessimize parsing perf but it is probably negligible in the grand scheme of things... But let's revisit in a different PR. ^^

src/libsyntax/parse/parser/pat.rs

src/test/ui/or-patterns/or-patterns-syntactic-fail.stderr

src/test/ui/parser/mut-patterns.stderr

…_or`.

…lace.

Centril · 2019-08-25T04:41:48Z

@estebank I've addressed the comments (see #63693 (comment) for moving things to diagnostics.rs which I think isn't the best idea).

I'm not super happy with the introduction of fn parse_fn_param_pat and that the pattern parser now has 4 entry points (well 3 after refactoring...). I think it should probably just be replaced with a call to parse_top_pat and accepting fn foo(A | B: typ) since there are no technical drawbacks nor readability ambiguities in that... but let's leave that for a different PR.

Centril · 2019-08-26T20:15:27Z

@estebank Added a commit just now to fix an issue (that I just noticed) with a wrong span.

estebank · 2019-08-26T20:33:19Z

src/test/ui/or-patterns/or-patterns-syntactic-fail.stderr

+  --> $DIR/or-patterns-syntactic-fail.rs:50:17
+   |
+LL |     let NS { f: || A | B };
+   |                 ^^ help: remove the `||`


Shouldn't these two last at the end be parsed as a closure?

No. This is in a pattern context so $field_name: $pat and there is no way to parse a closure expression in a pattern context.

estebank · 2019-08-26T20:37:57Z

@Centril I think it looks good. I'm only slightly worried about the S { f: || A | B } case, where I would expect it to be identified as a malformed closure as opposed to an or-pattern, particularly because I would expect S { f: || A } to be an S with a field f that holds an Fn() -> A, and I'm not 100% sure that is the case.

Beyond that, r=me 🎉

Centril · 2019-08-26T21:00:31Z

Thanks for the review. :)

I think we can always revisit S { f: || A } but imo it is too much of a special case to deserve special logic (which I think will be invasive to the overall parsing logic here) and you'd also need to mix up expression and pattern contexts to confuse let S { f: || A } for a closure.

@bors r=estebank

bors · 2019-08-26T21:00:32Z

📌 Commit 2bd27fb has been approved by estebank

@dlrobertson

…ebank Fully implement or-pattern parsing Builds upon the initial parsing in rust-lang#61708 to fully implement or-pattern (`p | q`) parsing as specified in [the grammar section of RFC 2535](https://github.com/rust-lang/rfcs/blob/master/text/2535-or-patterns.md#grammar). Noteworthy: - We allow or-patterns in `[p | q, ...]`. - We allow or-patterns in `let` statements and `for` expressions including with leading `|`. - We improve recovery for `p || q` (+ tests for that in `multiple-pattern-typo.rs`). - We improve recovery for `| p | q` in inner patterns (tests in `or-patterns-syntactic-fail.rs`). - We rigorously test or-pattern parsing (in `or-patterns-syntactic-{pass,fail}.rs`). - We harden the feature gating tests. - We do **_not_** change `ast.rs`. That is, `ExprKind::Let.0` and `Arm.pats` still accept `Vec<P<Pat>>`. I was starting work on that but it would be cleaner to do this in a separate PR so this one has a narrower scope. cc @dlrobertson cc the tracking issue rust-lang#54883. r? @estebank

@ghost

Rollup of 6 pull requests Successful merges: - #63317 (Do not complain about unused code when used in `impl` `Self` type) - #63693 (Fully implement or-pattern parsing) - #63836 (VxWorks does not provide a way to set the task name except at creation time) - #63845 (Removed a confusing FnOnce example) - #63855 (Refactor feature gates) - #63921 (add link to FileCheck docs) Failed merges: r? @ghost

@dlrobertson

…ochenkov or-patterns: Uniformly use `PatKind::Or` in AST & Fix/Cleanup resolve Following up on work in rust-lang#63693 and rust-lang#61708, in this PR we: - Uniformly use `PatKind::Or(...)` in AST: - Change `ast::Arm.pats: Vec<P<Pat>>` => `ast::Arm.pat: P<Pat>` - Change `ast::ExprKind::Let.0: Vec<P<Pat>>` => `ast::ExprKind::Let.0: P<Pat>` - Adjust `librustc_resolve/late.rs` to correctly handle or-patterns at any level of nesting as a result. In particular, the already-bound check which rejects e.g. `let (a, a);` now accounts for or-patterns. The consistency checking (ensures no missing bindings and binding mode consistency) also now accounts for or-patterns. In the process, a bug was found in the current compiler which allowed: ```rust enum E<T> { A(T, T), B(T) } use E::*; fn foo() { match A(0, 1) { B(mut a) | A(mut a, mut a) => {} } } ``` The new algorithms took a few iterations to get right. I tried several clever schemes but ultimately a version based on a stack of hashsets and recording product/sum contexts was chosen since it is more clearly correct. - Clean up `librustc_resolve/late.rs` by, among other things, using a new `with_rib` function to better ensure stack dicipline. - Do not push the change in AST to HIR for now to avoid doing too much in this PR. To cope with this, we introduce a temporary hack in `rustc::hir::lowering` (clearly marked in the diff). cc rust-lang#54883 cc @dlrobertson @matthewjasper r? @petrochenkov

@dlrobertson

…ochenkov or-patterns: Uniformly use `PatKind::Or` in AST & Fix/Cleanup resolve Following up on work in rust-lang#63693 and rust-lang#61708, in this PR we: - Uniformly use `PatKind::Or(...)` in AST: - Change `ast::Arm.pats: Vec<P<Pat>>` => `ast::Arm.pat: P<Pat>` - Change `ast::ExprKind::Let.0: Vec<P<Pat>>` => `ast::ExprKind::Let.0: P<Pat>` - Adjust `librustc_resolve/late.rs` to correctly handle or-patterns at any level of nesting as a result. In particular, the already-bound check which rejects e.g. `let (a, a);` now accounts for or-patterns. The consistency checking (ensures no missing bindings and binding mode consistency) also now accounts for or-patterns. In the process, a bug was found in the current compiler which allowed: ```rust enum E<T> { A(T, T), B(T) } use E::*; fn foo() { match A(0, 1) { B(mut a) | A(mut a, mut a) => {} } } ``` The new algorithms took a few iterations to get right. I tried several clever schemes but ultimately a version based on a stack of hashsets and recording product/sum contexts was chosen since it is more clearly correct. - Clean up `librustc_resolve/late.rs` by, among other things, using a new `with_rib` function to better ensure stack dicipline. - Do not push the change in AST to HIR for now to avoid doing too much in this PR. To cope with this, we introduce a temporary hack in `rustc::hir::lowering` (clearly marked in the diff). cc rust-lang#54883 cc @dlrobertson @matthewjasper r? @petrochenkov

@dlrobertson

…ochenkov or-patterns: Uniformly use `PatKind::Or` in AST & Fix/Cleanup resolve Following up on work in rust-lang#63693 and rust-lang#61708, in this PR we: - Uniformly use `PatKind::Or(...)` in AST: - Change `ast::Arm.pats: Vec<P<Pat>>` => `ast::Arm.pat: P<Pat>` - Change `ast::ExprKind::Let.0: Vec<P<Pat>>` => `ast::ExprKind::Let.0: P<Pat>` - Adjust `librustc_resolve/late.rs` to correctly handle or-patterns at any level of nesting as a result. In particular, the already-bound check which rejects e.g. `let (a, a);` now accounts for or-patterns. The consistency checking (ensures no missing bindings and binding mode consistency) also now accounts for or-patterns. In the process, a bug was found in the current compiler which allowed: ```rust enum E<T> { A(T, T), B(T) } use E::*; fn foo() { match A(0, 1) { B(mut a) | A(mut a, mut a) => {} } } ``` The new algorithms took a few iterations to get right. I tried several clever schemes but ultimately a version based on a stack of hashsets and recording product/sum contexts was chosen since it is more clearly correct. - Clean up `librustc_resolve/late.rs` by, among other things, using a new `with_rib` function to better ensure stack dicipline. - Do not push the change in AST to HIR for now to avoid doing too much in this PR. To cope with this, we introduce a temporary hack in `rustc::hir::lowering` (clearly marked in the diff). cc rust-lang#54883 cc @dlrobertson @matthewjasper r? @petrochenkov

@dlrobertson

…ochenkov or-patterns: Uniformly use `PatKind::Or` in AST & Fix/Cleanup resolve Following up on work in rust-lang#63693 and rust-lang#61708, in this PR we: - Uniformly use `PatKind::Or(...)` in AST: - Change `ast::Arm.pats: Vec<P<Pat>>` => `ast::Arm.pat: P<Pat>` - Change `ast::ExprKind::Let.0: Vec<P<Pat>>` => `ast::ExprKind::Let.0: P<Pat>` - Adjust `librustc_resolve/late.rs` to correctly handle or-patterns at any level of nesting as a result. In particular, the already-bound check which rejects e.g. `let (a, a);` now accounts for or-patterns. The consistency checking (ensures no missing bindings and binding mode consistency) also now accounts for or-patterns. In the process, a bug was found in the current compiler which allowed: ```rust enum E<T> { A(T, T), B(T) } use E::*; fn foo() { match A(0, 1) { B(mut a) | A(mut a, mut a) => {} } } ``` The new algorithms took a few iterations to get right. I tried several clever schemes but ultimately a version based on a stack of hashsets and recording product/sum contexts was chosen since it is more clearly correct. - Clean up `librustc_resolve/late.rs` by, among other things, using a new `with_rib` function to better ensure stack dicipline. - Do not push the change in AST to HIR for now to avoid doing too much in this PR. To cope with this, we introduce a temporary hack in `rustc::hir::lowering` (clearly marked in the diff). cc rust-lang#54883 cc @dlrobertson @matthewjasper r? @petrochenkov

Rust got full-blown support for or-patterns (see [RFC 2535][0]). This means a couple changes: * `OrP` is a new variant of `Pat` * `WhileLet`, `IfLet`, `Arm` now just take a `Pat` (instead of a list) * in the parser, or-patterns are not allowed everywhere that regular patterns are! Tests cases were heavily inspired by [the PR that implemented the RFC][1]. [0]: https://github.com/rust-lang/rfcs/blob/master/text/2535-or-patterns.md#grammar [1]: rust-lang/rust#63693

@dlrobertson

or-patterns: Push `PatKind/PatternKind::Or` at top level to HIR & HAIR Following up on work in rust-lang#64111, rust-lang#63693, and rust-lang#61708, in this PR: - We change `hair::Arm.patterns: Vec<Pattern<'_>>` into `hir::Arm.pattern: Pattern<'_>`. - `fn hair::Arm::top_pats_hack` is introduced as a temporary crutch in MIR building to avoid more changes. - We change `hir::Arm.pats: HirVec<P<Pat>>` into `hir::Arm.pat: P<Pat>`. - The hacks in `rustc::hir::lowering` are removed since the representation hack is no longer necessary. - In some places, `fn hir::Arm::top_pats_hack` is introduced to leave some things as future work. - Misc changes: HIR pretty printing is adjusted to behave uniformly wrt. top/inner levels, rvalue promotion is adjusted, regionck, and dead_code is also. - Type checking is adjusted to uniformly handle or-patterns at top/inner levels. To make things compile, `p_0 | ... | p_n` is redefined as a "reference pattern" in [`fn is_non_ref_pat`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_typeck/check/struct.FnCtxt.html#method.is_non_ref_pat) for now. This is done so that reference types are not eagerly stripped from the `expected: Ty<'tcx>`. - Liveness is adjusted wrt. the `unused_variables` and `unused_assignments` lints to handle top/inner levels uniformly and the handling of `fn` parameters, `let` locals, and `match` arms are unified in this respect. This is not tested for now as exhaustiveness checks are reachable and will ICE. - In `check_match`, checking `@` and by-move bindings is adjusted. However, exhaustiveness checking is not adjusted the moment and is handled by @dlrobertson in rust-lang#63688. - AST borrowck (`construct.rs`) is not adjusted as AST borrowck will be removed soon. r? @matthewjasper cc @dlrobertson @varkor @oli-obk

@dlrobertson

or-patterns: Push `PatKind/PatternKind::Or` at top level to HIR & HAIR Following up on work in #64111, #63693, and #61708, in this PR: - We change `hair::Arm.patterns: Vec<Pattern<'_>>` into `hir::Arm.pattern: Pattern<'_>`. - `fn hair::Arm::top_pats_hack` is introduced as a temporary crutch in MIR building to avoid more changes. - We change `hir::Arm.pats: HirVec<P<Pat>>` into `hir::Arm.pat: P<Pat>`. - The hacks in `rustc::hir::lowering` are removed since the representation hack is no longer necessary. - In some places, `fn hir::Arm::top_pats_hack` is introduced to leave some things as future work. - Misc changes: HIR pretty printing is adjusted to behave uniformly wrt. top/inner levels, rvalue promotion is adjusted, regionck, and dead_code is also. - Type checking is adjusted to uniformly handle or-patterns at top/inner levels. To make things compile, `p_0 | ... | p_n` is redefined as a "reference pattern" in [`fn is_non_ref_pat`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_typeck/check/struct.FnCtxt.html#method.is_non_ref_pat) for now. This is done so that reference types are not eagerly stripped from the `expected: Ty<'tcx>`. - Liveness is adjusted wrt. the `unused_variables` and `unused_assignments` lints to handle top/inner levels uniformly and the handling of `fn` parameters, `let` locals, and `match` arms are unified in this respect. This is not tested for now as exhaustiveness checks are reachable and will ICE. - In `check_match`, checking `@` and by-move bindings is adjusted. However, exhaustiveness checking is not adjusted the moment and is handled by @dlrobertson in #63688. - AST borrowck (`construct.rs`) is not adjusted as AST borrowck will be removed soon. r? @matthewjasper cc @dlrobertson @varkor @oli-obk

@dlrobertson

or-patterns: Push `PatKind/PatternKind::Or` at top level to HIR & HAIR Following up on work in rust-lang#64111, rust-lang#63693, and rust-lang#61708, in this PR: - We change `hair::Arm.patterns: Vec<Pattern<'_>>` into `hir::Arm.pattern: Pattern<'_>`. - `fn hair::Arm::top_pats_hack` is introduced as a temporary crutch in MIR building to avoid more changes. - We change `hir::Arm.pats: HirVec<P<Pat>>` into `hir::Arm.pat: P<Pat>`. - The hacks in `rustc::hir::lowering` are removed since the representation hack is no longer necessary. - In some places, `fn hir::Arm::top_pats_hack` is introduced to leave some things as future work. - Misc changes: HIR pretty printing is adjusted to behave uniformly wrt. top/inner levels, rvalue promotion is adjusted, regionck, and dead_code is also. - Type checking is adjusted to uniformly handle or-patterns at top/inner levels. To make things compile, `p_0 | ... | p_n` is redefined as a "reference pattern" in [`fn is_non_ref_pat`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_typeck/check/struct.FnCtxt.html#method.is_non_ref_pat) for now. This is done so that reference types are not eagerly stripped from the `expected: Ty<'tcx>`. - Liveness is adjusted wrt. the `unused_variables` and `unused_assignments` lints to handle top/inner levels uniformly and the handling of `fn` parameters, `let` locals, and `match` arms are unified in this respect. This is not tested for now as exhaustiveness checks are reachable and will ICE. - In `check_match`, checking `@` and by-move bindings is adjusted. However, exhaustiveness checking is not adjusted the moment and is handled by @dlrobertson in rust-lang#63688. - AST borrowck (`construct.rs`) is not adjusted as AST borrowck will be removed soon. r? @matthewjasper cc @dlrobertson @varkor @oli-obk

rust-highfive assigned estebank Aug 18, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 18, 2019

Centril added the F-or_patterns `#![feature(or_patterns)]` label Aug 18, 2019

dlrobertson reviewed Aug 19, 2019

View reviewed changes

estebank reviewed Aug 20, 2019

View reviewed changes

estebank mentioned this pull request Aug 20, 2019

Custom diagnostic for bad mut in pattern with sub-bindings #63764

Closed

Centril added 24 commits August 24, 2019 21:31

parser: extract ban_unexpected_or_or.

5299d8a

parser: type alias type Expected = Option<&'static str>;.

1ba7550

parser: refactor parse_pat_with_or + use it in [p0, p1, ..] pats.

0bbea47

parser: improve or-patterns recovery.

30b841d

parser: move multiple-pattern-typo -> or-patterns directory.

d34ee76

parser: multiple-pattern-typo: cover more or-pattern places.

5f57fee

parser: simplify parse_pat_with_or.

f852c7c

parser: extract maybe_recover_unexpected_comma.

21d9b85

parser: integrate maybe_recover_unexpected_comma in `parse_pat_with…

a4a34ab

…_or`.

parser: extract eat_or_separator.

7b59b4f

parser: improve parse_pat_with_or docs.

dc5bbaf

parser: use eat_or_separator for leading vert.

6498959

parser: move maybe_recover_unexpected_comma to a more appropriate p…

39f5e5b

…lace.

parser: document ban_unexpected_or_or.

8f6a0cd

parser: parse_pats -> parse_top_pat{_unpack}.

b7178ef

parser: document parse_pat.

92d66a1

parser: let stmts & for exprs: allow or-patterns.

95792b4

parser: drive-by: simplify parse_arg_general.

5f6bec8

move ui/or-pattern-mismatch -> ui/or-patterns/.

a9af18b

or-patterns: add syntactic tests.

f35432e

move feature-gate-or_patterns.* -> ui/or-patterns/

d3b3bce

or-patterns: harden feature gating tests.

1ffea18

parser: better recovery for || in inner pats.

b205055

parser: bool -> GateOr.

b2966e6

Centril added 3 commits August 25, 2019 05:45

parser: gracefully handle fn foo(A | B: type).

1caaa40

or_patterns: add run-rustfix tests.

6a73199

parser: TopLevel -> RecoverComma.

acb1130

Centril force-pushed the polish-parse-or-pats branch from 585bb3c to acb1130 Compare August 25, 2019 04:21

parser: fix span for leading vert.

2bd27fb

estebank reviewed Aug 26, 2019

View reviewed changes

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 26, 2019

Centril mentioned this pull request Aug 26, 2019

Rollup of 6 pull requests #63926

Merged

bors merged commit 2bd27fb into rust-lang:master Aug 27, 2019

Centril deleted the polish-parse-or-pats branch August 27, 2019 03:56

This was referenced Aug 27, 2019

Tracking issue for RFC 2535, 2530, 2175, "Or patterns, i.e Foo(Bar(x) | Baz(x))" #54883

Closed

or-patterns: Uniformly use PatKind::Or in AST & Fix/Cleanup resolve #64111

Merged

Centril mentioned this pull request Sep 16, 2019

or-patterns: Push PatKind/PatternKind::Or at top level to HIR & HAIR #64508

Merged

dlrobertson mentioned this pull request Sep 27, 2019

WIP: Initial implementation of or-pattern handling in MIR #63688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully implement or-pattern parsing #63693

Fully implement or-pattern parsing #63693

Centril commented Aug 18, 2019 •

edited

dlrobertson Aug 19, 2019

Centril Aug 19, 2019 •

edited

dlrobertson Aug 20, 2019

Centril Aug 20, 2019

estebank Aug 20, 2019

Centril Aug 20, 2019

estebank Aug 21, 2019

Centril Aug 21, 2019

Centril commented Aug 25, 2019

Centril commented Aug 26, 2019

estebank Aug 26, 2019

Centril Aug 26, 2019

estebank commented Aug 26, 2019

Centril commented Aug 26, 2019

bors commented Aug 26, 2019

Fully implement or-pattern parsing #63693

Fully implement or-pattern parsing #63693

Conversation

Centril commented Aug 18, 2019 • edited

Choose a reason for hiding this comment

Centril Aug 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril commented Aug 25, 2019

Centril commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Aug 26, 2019

Centril commented Aug 26, 2019

bors commented Aug 26, 2019

Centril commented Aug 18, 2019 •

edited

Centril Aug 19, 2019 •

edited