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

RFC: or-patterns in let and if / while let expressions #2175

Merged
merged 7 commits into from
Feb 14, 2018

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Oct 16, 2017

Rendered.
Tracking issue

Enables the use of or-patterns in let, if let, and while let constructs as in:

if let A(x) | B(x) | ... = expr {
    do_stuff_with(x);
}

while let A(x) | B(x) | ... = expr {
    do_stuff_with(x);
}

let Ok(index) | Err(index) = slice.binary_search(&x);

Acknowledgements & ping

  • @scottmcm, fixing typos, and helping me with the RFC in general.

@Centril Centril changed the title RFC: or-patterns in if / while let A(x) | B(x) | ... = expr RFC: or-patterns in if / while let Oct 16, 2017
@Centril Centril changed the title RFC: or-patterns in if / while let RFC: or-patterns in if / while let expressions Oct 16, 2017
@scottmcm scottmcm added the T-lang Relevant to the language team, which will review and decide on the RFC. label Oct 16, 2017
@Centril
Copy link
Contributor Author

Centril commented Oct 16, 2017

This change was also discussed in #2144.

@petrochenkov
Copy link
Contributor

Looks like a stripped-down version of #1882, which was recently postponed.

This RFC adds significantly less new stuff though, since | is still a special construction, not a part of a pattern.
Since if let and while let are desugared into match anyway and match already supports this special construction, I see no reasons to not accept this.

@Centril
Copy link
Contributor Author

Centril commented Oct 16, 2017

@petrochenkov I wouldn't say this and #1882 are related tho. This only makes match, if let and while let consistent by letting the latter two do what the former already can do =)

@nugend
Copy link

nugend commented Oct 18, 2017

I just saw this RFC in TWIR and funnily enough, a related crate was nominated for crate of the week: https://docs.rs/if_chain/0.1.2/if_chain/#multiple-patterns

@Centril
Copy link
Contributor Author

Centril commented Oct 18, 2017

@nugend Interesting! Perhaps worthy of mention in motivation, and possibly alternatives.

@cramertj
Copy link
Member

It'd be cool to also have this in plain-old let bindings if the match is exhaustive:

#[derive(Copy, Clone)]
enum Foo {
    Bar(i32),
    Baz(i32),
}

impl Foo {
    fn inner(&self) -> i32 {
        let Bar(x) | Baz(x) = *self;
        x
    }
}

@Centril
Copy link
Contributor Author

Centril commented Oct 18, 2017

@cramertj I'm not opposed to this.. But is that a common enough use case tho? Do you have some real world examples I could work into the motivation perhaps?

@cramertj
Copy link
Member

@Centril I'm not sure how common it is-- it also only saves a few characters compared to a match. For me, the motivation would be primarily about making patterns consistent.

@Centril
Copy link
Contributor Author

Centril commented Oct 18, 2017

@cramertj I guess I buy the consistency argument since I use it under motivation already - but it would be even more compelling with some actual example.

If anyone finds one, please let us know =)

@cramertj
Copy link
Member

cramertj commented Oct 18, 2017

@Centril There're some functions like the example I gave in rustc, but usually they have one or two variants that don't bind the variable and bug! instead. I think the reason it's tough to come up with examples is that usually you'd write an enum like my Foo above as a struct with a kind enum:

// Instead of this:

#[derive(Copy, Clone)]
enum Foo {
    Bar(i32),
    Baz(i32),
}

impl Foo {
    fn inner(&self) -> i32 {
        let Bar(x) | Baz(x) = *self;
        x
    }
}


// People write this:

struct Foo {
    val: i32,
    kind: FooKind,
}

enum FooKind { Bar, Baz }

@Centril
Copy link
Contributor Author

Centril commented Oct 18, 2017

@cramertj Right. And isn't that the recommended way of writing it?

Devil's advocate: Wouldn't we risk encouraging the "wrong" way of writing this by allowing:

let Bar(x) | Baz(x) = *self;

?

@cramertj
Copy link
Member

@Centril Yeah, that seems like a reasonable objection.

@Centril
Copy link
Contributor Author

Centril commented Oct 18, 2017

@cramertj Hmm.. Perhaps this can be made into a separate RFC since it might be more controversial? - so that this RFC can be merged faster (tho we have several months to discuss it before the impl period is over...)

@petrochenkov
Copy link
Contributor

@cramertj
Instead of doing #2175 (comment) I'd suggest to go full #1882 (after the end of the implementation period).
The nice point of this RFC is that it's really minimal (if let and while let desugars into match supporting |) while let doesn't desugar into match increasing the scope of changes.

@Centril Centril mentioned this pull request Jan 1, 2018
@nikomatsakis
Copy link
Contributor

My take is that we definitely want this in let.

I have definitely encountered cases where I want to do:

let Foo(x) | Bar(x) = value;

An example would be in Chalk, where I have this enum:

enum ParameterKind<T, L = T> {
    Ty(T),
    Lifetime(L),
}

indicating e.g. the value of a parameter in a list like Foo<'a, T>. But note the default L = T, this is because often I don't need different data for the two cases. As it happens now, I have an impl like:

impl<T> ParameterKind<T> {
    fn into_inner(self) -> T { ... }
}

there are few times that I wished I could use let syntax to destructure.

It doesn't come up a lot, but as @cramertj says, to me having the syntax of if let and let diverge is pretty confusing.

@nikomatsakis
Copy link
Contributor

Nominating for discussin @rust-lang/lang meeting. Seems like one of those "tiny extension to the syntax" sorts of RFCs that are always hard to judge. Obviously not a major blocker for anyone, but maybe a useful thing to go ahead and do? This seems to have pretty minimal implementation cost, I would think, since everything winds up desugared in the compiler to a match anyway. It's just about the "user cost". (Right?)

@Centril
Copy link
Contributor Author

Centril commented Jan 26, 2018

@nikomatsakis

An example would be in Chalk, where I have this enum:

That's an interesting use case! And I agree that diverging would be unfortunate.
Should this be fixed in this RFC, or revisited later?

@nikomatsakis
Copy link
Contributor

@Centril I say fix it in the RFC now =)

@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

Subjection to the proviso that the RFC is amended to permit | patterns in let, I move that we merge it. It can be hard to judge when it makes sense to do these sorts of small extensions to the syntax. This particular extension seems worthwhile primarily because it is mostly echoes syntax already found elsewhere in the language, so it makes things slightly more expressive without really making people learn new things they don't already have to know.

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 26, 2018

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jan 26, 2018
@Centril
Copy link
Contributor Author

Centril commented Jan 29, 2018

@nikomatsakis Amended =)

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jan 30, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Jan 30, 2018

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

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jan 30, 2018
@aturon aturon removed the I-nominated label Feb 8, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Feb 9, 2018

The final comment period is now complete.

@aturon
Copy link
Member

aturon commented Feb 14, 2018

This RFC has been merged!

Tracking issue

@aturon aturon merged commit 2431a9a into rust-lang:master Feb 14, 2018
@Centril Centril changed the title RFC: or-patterns in if / while let expressions RFC: or-patterns in let / if / while let expressions Feb 15, 2018
@Centril Centril changed the title RFC: or-patterns in let / if / while let expressions RFC: or-patterns in let and if / while let expressions Feb 15, 2018
@Centril Centril deleted the rfc/if-while-or-patterns branch February 15, 2018 06:21
@nox
Copy link
Contributor

nox commented Feb 23, 2018

This seems quite unfortunate to me that this landed. Other forms of sugar around if let and whatnot that actually brought more expressiveness to the table were rejected with "just use match", why are or-patterns considered differently?

@eddyb
Copy link
Member

eddyb commented Feb 23, 2018

@nox IMO it's a language design bug that pattern | pattern is not itself a pattern, that can be nested in other patterns, etc. This is a bandaid, sure, but eventually we should fix it fully.

@nox
Copy link
Contributor

nox commented Feb 23, 2018

@eddyb Mmmh, I guess that makes sense if you frame it this way. Thanks for the answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-expressions Term language related proposals & ideas A-patterns Pattern matching related proposals & ideas A-syntax Syntax related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants