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

or-patterns: disallow in let bindings #82048

Merged
merged 2 commits into from
Mar 9, 2021

Conversation

mark-i-m
Copy link
Member

@mark-i-m mark-i-m commented Feb 13, 2021

Blocked on #81869

Disallows top-level or-patterns before type ascription. We want to reserve this syntactic space for possible future generalized type ascription.

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 13, 2021
@mark-i-m
Copy link
Member Author

Actually, I'm going to rebase this to not be based on the other PR... no need...

@rust-log-analyzer

This comment has been minimized.

@mark-i-m mark-i-m marked this pull request as ready for review February 13, 2021 00:37
@petrochenkov
Copy link
Contributor

I think this is indeed better blocked on #81869 because the logic here should be shared with fn parse_fn_param_pat.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2021
@petrochenkov
Copy link
Contributor

parse_fn_param_pat is not used for closures, so or-patterns in both function parameters and let bindings can be parsed with fn parse_pat_allow_top_alt and then reported as errors post-factum.

@mark-i-m mark-i-m marked this pull request as draft February 13, 2021 15:29
@mark-i-m
Copy link
Member Author

parse_fn_param_pat is not used for closures, so or-patterns in both function parameters and let bindings can be parsed with fn parse_pat_allow_top_alt and then reported as errors post-factum.

Hmm.. so this made sense initially, but as I'm thinking about it, it seems that they are different... Specifically, in the case of let, I think we want to allow top-level or-patterns without a type, right? That requires first parsing the pattern and then looking for a Colon token after it... I think I can share the error generation itself, but not much more...

@petrochenkov
Copy link
Contributor

I mean, in both cases we parse parse_pat_allow_top_alt and then look for a colon, except that it's mandatory in one case and optional in another.

@mark-i-m
Copy link
Member Author

Hmm... I see. I'll take a closer look when I implement the changes in #79278 (comment)

@bors

This comment has been minimized.

@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-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Feb 18, 2021
@mark-i-m
Copy link
Member Author

@petrochenkov I've pushed a commit which does a few things:

  • Attempts to address your review comments.
  • Changes let bindings to require no_top_alt, as per the Lang Team's request (Stabilize or_patterns (RFC 2535, 2530, 2175) #79278 (comment))
  • Creates a new feature gate or_patterns_with_ty that allows the use of top-level or-patterns in let if there is not type annotation.

Please let me know what you think.

@rust-log-analyzer

This comment has been minimized.

@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 Feb 18, 2021
@mark-i-m

This comment has been minimized.

compiler/rustc_feature/src/active.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/item.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 20, 2021

I'm skeptical about the decision to not stabilize let P1 | P2 = E (without type annotation), but it's a conservative error so it's not a big deal.

What I would worry about more is nested | patterns neighboring type ascription in situations like

let b @ P1 | P2: T = E;

I'm not sure whether code like this is impossible with the currently implemented parsing priorities, this needs an investigation.
EDIT: In any case, the fix is to go through the top pattern recursively in search of or-patterns in fn parse_pat_before_ty, but without recursing into parentheses, tuples and other "closed" groups.

@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 Feb 20, 2021
@bors

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 5, 2021

📌 Commit e64138c 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 5, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Mar 6, 2021
…petrochenkov

or-patterns: disallow in `let` bindings

~~Blocked on rust-lang#81869

Disallows top-level or-patterns before type ascription. We want to reserve this syntactic space for possible future generalized type ascription.

r? `@petrochenkov`
@JohnTitor
Copy link
Member

Failed in rollup: #82840 (comment)
Looks like we need a tweak to clippy.
@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 Mar 6, 2021
@mark-i-m
Copy link
Member Author

mark-i-m commented Mar 8, 2021

I think clippy should compile now.

@rust-log-analyzer

This comment has been minimized.

@estebank
Copy link
Contributor

estebank commented Mar 8, 2021

@mark-i-m would you mind filing a ticket to change the unnecessary parens lint ahead of stabilization? I was recently touching this area and should be straightforward to make it not trigger for let (A | B) = C;.

We should probably have lints, in rustc or clippy for match expressions that can be reduced to matches! or irrefutable let bindings (I though we already had the later).

@mark-i-m
Copy link
Member Author

mark-i-m commented Mar 8, 2021

@estebank I had thought I already fixed the lint in this PR, though I'm not sure why it was firing for clippy...

@estebank
Copy link
Contributor

estebank commented Mar 8, 2021

@mark-i-m ah! Interesting.

@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 Mar 8, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 8, 2021

📌 Commit 402a00a 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Mar 9, 2021
…petrochenkov

or-patterns: disallow in `let` bindings

~~Blocked on rust-lang#81869

Disallows top-level or-patterns before type ascription. We want to reserve this syntactic space for possible future generalized type ascription.

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 9, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#81127 (Improve sift_down performance in BinaryHeap)
 - rust-lang#81879 (Added #[repr(transparent)] to core::cmp::Reverse)
 - rust-lang#82048 (or-patterns: disallow in `let` bindings)
 - rust-lang#82731 (Bump libc dependency of std to 0.2.88.)
 - rust-lang#82799 (Add regression test for rust-lang#75525)
 - rust-lang#82841 (Change x64 size checks to not apply to x32.)
 - rust-lang#82883 (Update Cargo)
 - rust-lang#82887 (Update CONTRIBUTING.md)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0d97f9b into rust-lang:master Mar 9, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 9, 2021
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 11, 2021
…petrochenkov

or-patterns: disallow in `let` bindings

~~Blocked on rust-lang#81869

Disallows top-level or-patterns before type ascription. We want to reserve this syntactic space for possible future generalized type ascription.

r? ``@petrochenkov``
@mark-i-m mark-i-m deleted the or-pat-type-ascription branch March 28, 2021 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants