-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Or patterns, i.e Foo(Bar(x) | Baz(x))
#2535
Conversation
π I've wanted this for a while, and I look forward to using it. |
I think a drawback is that it may be suprising with bitwise or let x = Some(1 | 2);
match x {
Some(1 | 2) => (), // won't happen, but intuition says it would
_ => panic!()
} ... But I guess that it's kind of already an issue right now anyway let x = 1 | 2;
match x {
1 | 2 => (), // won't happen
_ => panic!()
} |
@whataloadofwhat Yeah, |
text/0000-or-patterns.md
Outdated
|
||
The only real choice that we do have to make is whether the new addition to the | ||
pattern grammar should be `pat : .. | pat "|" pat ;` or if it instead should be | ||
`pat : .. | "|"? pat "|" pat ;`. We have chosen the former for 4 reasons: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: pat ::= "|"? pat "|" pat;
is definitely bad, since it would allow patterns like | | A | B | C
via the interpretation | (| A | B) | C
. So I'm absolutely in favour of the current plan.
+1 on the general goal. What is the impact on macro_rules! foo {
($a: pat) => { false };
($a: pat | $b: pat) => { true };
}
fn main() {
assert!(foo!(Some(_) | None));
} |
RFC 550βs "Edit history" section links to #1336. |
We can and have disallowed some characters in macros before (e.g. |
βFastβ as in before May 2015? |
Was serious consideration given to avoiding breakage by requiring parentheses when doing alternation at the top level? It was not added to the alternatives or drawbacks section... and this isn't the first RFC to ignore/dismiss macro breakage. Other than that I strongly approve. |
Oh, okay great! That should be in the guide-level docs as well, not just buried in the grammar definition. |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
Huzzah! This RFC has been merged! Tracking issue: rust-lang/rust#54883 |
I agree, and I think it's horrible. I read the release notes today, and especially the example there reads highly ambiguously:
It may have been ambiguous before, but now it is way worse. I'm expecting this to cause a bug or two down the line. I don't have many issues with Rust's design, but this is just plain awful. |
There are many situations where the new syntax makes writing robust code a lot easier since it avoids tons of repetition. It is already used plenty of times inside the Rust compiler, for example. But I agree that in the very special case of integer patterns, this is not great. Note however that the issue exists before this RFC already, so it's not really a problem introduced by the new syntax (even though it might be amplified). Would you like to open an issue describing the problem and possible solutions? (Discussions on merged RFCs are not going to lead anywhere, and are probably just going to get lost.) |
Some confusion is possible at a glance, but pattern context v.s. expression context is enough to resolve it. Most expression operators are syntax errors in patterns. For example |
A lint for the specific case that @mortarroad pointed out seems like a good idea. Frankly, it never occurred to me that one might use a bitwise or in that context (and I do a lot of bit-twiddling). Personally, I find that nested or-patterns are major ergonomics improvement, especially when right something like a state machine. |
πΌοΈ Rendered
β Tracking issue
π Summary
Allow
|
to be arbitrarily nested within a pattern such thatSome(A(0) | B(1 | 2))
becomes a valid pattern.π Thanks
To @Nemo157 and @joshtriplett for reviewing the draft version of this RFC.