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

Merged
merged 26 commits into from Oct 7, 2018

Conversation

@Centril
Copy link
Contributor

Centril commented Aug 30, 2018

🖼️ Rendered

Tracking issue

📝 Summary

Allow | to be arbitrarily nested within a pattern such that Some(A(0) | B(1 | 2)) becomes a valid pattern.

💖 Thanks

To @Nemo157 and @joshtriplett for reviewing the draft version of this RFC.

@Centril Centril added the T-lang label Aug 30, 2018

@Centril Centril self-assigned this Aug 30, 2018

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Aug 30, 2018

👍 I've wanted this for a while, and I look forward to using it.

@Centril Centril referenced this pull request Aug 31, 2018

Closed

Or patterns #43

@whataloadofwhat

This comment has been minimized.

Copy link

whataloadofwhat commented Sep 2, 2018

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!()
}
@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Sep 2, 2018

@whataloadofwhat Yeah, | already doesn't mean bitwise or in the context of patterns.


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:

This comment has been minimized.

@scottmcm

scottmcm Sep 3, 2018

Member

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.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Sep 5, 2018

+1 on the general goal.

What is the impact on pat fragment in macro_rules? Unlike other some other combinations forbidden by RFC 550 Macro future-proofing, | is allowed to follow a pattern today and this runs without asserting in Rust 1.28.0:

macro_rules! foo {
    ($a: pat) => { false };
    ($a: pat | $b: pat) => { true };
}

fn main() {
    assert!(foo!(Some(_) | None));
}
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Sep 5, 2018

RFC 550’s "Edit history" section links to #1336.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Sep 5, 2018

We can and have disallowed some characters in macros before (e.g. ? in separators), but we would need to act fast.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Sep 5, 2018

“Fast” as in before May 2015?

+ the type inferred for `p` does not unify with the type inferred for `q`, or
+ the same set of bindings are not introduced in `p` and `q`, or
+ the type of any two bindings with the same name in `p` and `q` do not unify
with respect to types or binding modes.

This comment has been minimized.

@scottmcm

scottmcm Sep 25, 2018

Member

Do coercions apply, or do they need to unify exactly?

This comment has been minimized.

@Centril

Centril Sep 25, 2018

Contributor

Summary from Discord conversation:

...So according to the coercion rules, if match is a coercion site, then the following should work according to the type coercion rules:

fn main() {
    enum E<'a> {
        A(&'a u8),
        B(&'a mut u8),
    }

    match unimplemented!() {
        E::A(x) | E::B(x) => {
            let x: &u8 = x;
        }
    }
}

However; match is not coercion site, so possibly the coercions would apply for let.
For now, I think we can be conservative and require an exact match and relax it later if need be / it makes sense.

In relation to type ascription, one could reasonably expect that E::A(x : τ) | E::B(x: τ) should work if x on both sides are implicitly coercible to τ but I don't think we need to specify that interaction just yet.

This comment has been minimized.

@Centril

Centril Sep 25, 2018

Contributor

Amended the RFC with a clarification according to the above notes.

This comment has been minimized.

@eddyb

eddyb Sep 26, 2018

Member

match wouldn't be the coercion site here, the individual patterns would need to be. Currently, the x binding there is the same variable (initialized on two possible disjoint execution paths), and can't possibly have two types (but we can relax that in the future).

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Sep 25, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Oct 2, 2018

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.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Oct 2, 2018

@durka

It was not added to the alternatives or drawbacks section.

It wasn't added to the alternatives because it is the proposal ;)

Finally, pat macro fragment specifiers will also match the pat<no_top_alt> production as opposed to pat<allow_top_alt>.

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Oct 2, 2018

Oh, okay great! That should be in the guide-level docs as well, not just buried in the grammar definition.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Oct 5, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@Centril Centril merged commit df1ea7e into rust-lang:master Oct 7, 2018

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Oct 7, 2018

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#54883

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment