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

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

Open
Centril opened this Issue Oct 7, 2018 · 12 comments

Comments

Projects
None yet
5 participants
@Centril
Copy link
Contributor

Centril commented Oct 7, 2018

This is a tracking issue for the RFC "Or patterns, i.e Foo(Bar(x) | Baz(x))" (rust-lang/rfcs#2535).

This issue also tracks the changes in rust-lang/rfcs#2175 and rust-lang/rfcs#2530 since RFC 2535 subsume those.

Steps:

Unresolved questions:

  • Should we allow top_pat or pat<allow_top_alt> in inferrable_param such that closures permit |Ok(x) | Err(x)| without first wrapping in parenthesis?

    We defer this decision to stabilization as it may depend on experimentation. Our current inclination is to keep the RFC as-is because the ambiguity is not just for the compiler; for humans, it is likely also ambiguous and thus harder to read.

    This also applies to functions which, although do not look as ambiguous, benefit from better consistency with closures. With respect to function arguments there's also the issue that not disambiguating with parenthesis makes it less clear whether the type ascription applies to the or-pattern as a whole or just the last alternative.

  • Should the pat macro fragment specifier match top_pat in different
    Rust editions or should it match pat<no_top_alt> as currently specified? We defer such decisions to stabilization because it depends on the outcome of crater runs to see what the extent of the breakage would be.

The benefit of avoiding pat<no_top_alt> in as many places as possible would both be grammatical consistency and fewer surprises for uses. The drawbacks would be possible ambiguity or backtracking for closures and breakage for macros.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Oct 7, 2018

I've closed #48215 in favor of this issue to track all related changes since rust-lang/rfcs#2535 subsumes RFC 2175.

I would suggest that #![feature(or_patterns)] should imply #![feature(if_while_or_patterns)] and then we will eventually stabilize both gates if/once we stabilize or_patterns.

@CAD97

This comment has been minimized.

Copy link
Contributor

CAD97 commented Oct 18, 2018

Minor thing that I just noticed: anonymous enums with the syntax of "tuple-but-|-instead-of-,", (A|B), is syntactically ambiguous with this feature, since we have parenthesized patterns.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Oct 18, 2018

@CAD97 example of a snippet that would show the ambiguity?

@CAD97

This comment has been minimized.

Copy link
Contributor

CAD97 commented Oct 18, 2018

I confused things between patterns and types:

match it {
    it: (A|B) => (), // it with type ascription (A|B)
    it @ (A|B) => (), // it binding to pattern (A|B)
}

So long as patterns and types aren't valid in the same position, this is ok. (I confused myself since tuples are valid patterns.)

@varkor varkor self-assigned this Oct 18, 2018

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Dec 11, 2018

I've got a branch for this that I think is close, but as I'm quite busy at the moment, I'm not sure how soon I'll finish it; if anyone else would like to take this over in the meantime, just leave a comment.

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Dec 23, 2018

@varkor Where's your branch? I'm happy to take over if you like... especially if it gets const generics to land any sooner. ;-)

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Dec 24, 2018

@alexreg: it's at https://github.com/varkor/rust/tree/or-patterns. I think where I left it, there was some crash when you tried using or-patterns, but most of the boilerplate is in. Feel free to play around with it to see if you can get it working. (Though I'm not prioritising this over const generics, don't worry 😉)

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Jan 27, 2019

@dlrobertson BTW, have you taken into account the unresolved questions whilst working on this?

@dlrobertson

This comment has been minimized.

Copy link
Contributor

dlrobertson commented Jan 27, 2019

@alexreg thanks for the ping. Haven't really thought about it too much. I've mostly been experimenting with the code, but at this point I am not keen on changing more than the pattern matching code for the following reasons:

  • I don't see much of a benefit in allowing |Ok(x) | Err(x)|
  • | feels very ambiguous here. When we hit a | when parsing a closure, is it a or-pattern
    or the end of the closure argument patterns? We would have to parse past the end to find out.

That being said, I'm still new, so I might be missing something.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Jan 27, 2019

@dlrobertson Thanks for working on this. The more important part to test with crater is whether top_pat can be used for pat macro fragments or if it has regressions so it would be good to start with top_pat and maybe switch to pat<no_top_alt> based on the data.

I think it's also worth considering fn foo(Ok(x) | Err(x): Result<A, B>) { .. } without parens since that doesn't look ambiguous like with closures (the part about type ascription is also irrelevant since both p and q must have the same type in p | q so it shouldn't matter what the ascription applies to). But we can leave that for later.

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Jan 28, 2019

@dlrobertson That's fair enough. What @Centril said is good advice.

@dlrobertson

This comment has been minimized.

Copy link
Contributor

dlrobertson commented Feb 11, 2019

Back to looking at this. An update for anyone following this (and a note to self so I don't forget things).

TODO:

  • Figure out lowering (particularly figure out if lowering should occur in HAIR -> MIR or HIR -> HAIR).
  • be sure to remove the Vec<Pat> from match and while/if let since we now have a p | q pattern form. - @Centril
  • ensure parsing conforms to the RFC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.