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

[Request] Overlapping match statements #2003

Closed
Nokel81 opened this issue May 18, 2017 · 48 comments
Closed

[Request] Overlapping match statements #2003

Nokel81 opened this issue May 18, 2017 · 48 comments
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.

Comments

@Nokel81
Copy link
Contributor

Nokel81 commented May 18, 2017

The ability to specify in a match statement that given some results the execute in top down order of all the statements that match.

Example:
Current:

match cmp.compare(&array[left], &array[right]) {
    Less => {
        merged.push(array[left]);
        left += 1;
    },
    Equal => {
        merged.push(array[left]);
        merged.push(array[right]);
        left += 1;
        right += 1;
    },
    Greater => {
        merged.push(array[right]);
        right += 1;
    }
}

my proposal for the new syntax:

match all cmp.compare(&array[left], &array[right]) {
    Less, Equal => {
        merged.push(array[left]);
        left += 1;
    },
    Greater, Equal => {
        merged.push(array[right]);
        right += 1;
    }
}

Basically this would be an extension to the current patterns where a comma signifies an overlapping qualifier meaning that if the value matches the overlap then it executes the code and then continues on searching.

@mark-i-m
Copy link
Member

I think the syntax would need to be different... maybe match any instead of match? Otherwise, the behavior of things like this would change:

match some_value {
    Less => {...}
    Equal => {...}
    _ => {...} // does this execute always?
}

@burdges
Copy link

burdges commented May 18, 2017

I think match needs checks for exhaustiveness and exclusivity, which this breaks. You could ask for | in if let instead though?

let x = cmp.compare(&array[left], &array[right]);
if let Less | Equal = x { .. }
if let Greater | Equal = x { .. }

I'd just use the ordering related impls for Ordering here though :

let x = cmp.compare(&array[left], &array[right]);
if x <= Equal { .. }
if x >= Equal { .. }

@Nokel81
Copy link
Contributor Author

Nokel81 commented May 19, 2017

@mark-i-m Yes, I would say that the last line would always execute.

@burdges Yes, I do see that in this case a dual if check could work, but I would still say that the match would be easier to read even if it is sugar.

@mark-i-m
Copy link
Member

I agree with @burdges if this becomes a feature, it should be clearly distinct from normal match because of the very different behavior... I think exhaustiveness checks could still be done in the same way they are done for current match statements more or less.

I do wonder how commonly used such a feature would be though...

@burdges
Copy link

burdges commented May 19, 2017

There is zero reason to use a match here except for wanting exhaustiveness checks. Yet, there are more situations in which you want exhaustiveness checks with if let constructions than situations in which you want this construct, like say

if let Less = x { .. }
foo();
if let Equal = x { .. }
bar();
if let Greater = x { .. }

I think the right solution is flexible manual exhaustiveness checks and | in if let, so roughly

let mut done = false;
let x = cmp.compare(&array[left], &array[right]);
if let Less | Equal = x { done=true; ..}
if let Greater | Equal = x { done=true; .. }
proof_assert!(done);

where proof_assert! asks the compiler to prove an assertion at compile time.

@glaebhoerl
Copy link
Contributor

How hard would it be to write a macro for this? It seems too niche to warrant a dedicated language feature to be honest.

@Nokel81
Copy link
Contributor Author

Nokel81 commented May 19, 2017

I agree that match would only be used with exhaustiveness checks but I think that a match any would be clearer then a new macro and if let

@oli-obk
Copy link
Contributor

oli-obk commented Jun 6, 2017

The advantage of the macro is that the feature can be experimented on without changing the language. I think having an existing impl in form of a macro will help the discussion significantly.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jun 6, 2017

What sort of thing is this macro for? So that I can build it.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jun 20, 2017

After looking into it I do not believe that a match all can be made as a macro

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jun 22, 2017

I have updated the RFC to change it to say match all

@mark-i-m
Copy link
Member

@Nokel81 Would something like this suit your purposes?

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jun 24, 2017

Yes it would if it did exhaustiveness testing

@mark-i-m
Copy link
Member

I think you could make a procedural macro for that... It would generate the code generated by the macro I posted above. Then, it could also generate a normal match state with the patterns. This will use the normal compiler exhaustiveness checking. The reason I think it should be a procedural macro is that I think that's the only way to de-duplicate the match arms...

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jun 24, 2017

Sorry but I don't know enough about macros to do that

@mark-i-m
Copy link
Member

mark-i-m commented Jun 24, 2017 via email

@glaebhoerl
Copy link
Contributor

It seems fairly straightforward to add a dummy match just for the purpose of exhaustiveness checking: https://is.gd/zGrXsy

Is there any reason that wouldn't be sufficient?

I also added a let binding for the discriminant to avoid repeating any side effects. Of course, this means it will only work with Copy types. I'm not sure what to do about that. (I think, if we had #2005, we could add an & there and it would just work out of the box?)

@mark-i-m
Copy link
Member

mark-i-m commented Jun 24, 2017 via email

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jun 26, 2017

@glaebhoerl The link that you provided does not compile.

@glaebhoerl
Copy link
Contributor

(@Nokel81 Yes, that's the point - the match is not exhaustive.)

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jun 26, 2017

Good point, oops. So this works amazingly, I just need to find out how the else case might work

@mark-i-m
Copy link
Member

mark-i-m commented Jun 27, 2017

You might also want to add the attribute on the match:

#[allow(unreachable_patterns)]

Since the match will have some duplicate patterns... That's what I was trying to say above...

EDIT: it should be allow not warn

@mark-i-m
Copy link
Member

or rather, I was doing something more complicate with the procedural macro, but this works better...

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jun 27, 2017

I do have a question about the macro though. Is there a specific reason why it has to be None/Some?

@mark-i-m
Copy link
Member

mark-i-m commented Jun 27, 2017 via email

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 6, 2017

How would we write the macro so that the _ pattern only matches if none of the other patterns match? Is it possible to add a counter?

@mark-i-m
Copy link
Member

mark-i-m commented Jul 7, 2017 via email

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 8, 2017

What I really meant was for the existence of an else clause which would act kind of like the _ case for normal matches but only match if there were no other matches

@mark-i-m
Copy link
Member

mark-i-m commented Jul 8, 2017 via email

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 8, 2017

I agree that that is more intuitive and I will try and make a macro when I have time (in about a week or so)

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 20, 2017

Actually, given the format of a match statement the , separateness is not required since using | already basically does that. So what is really required is

match_all val {
    val => expr,
    ....
} else {
    expr
}

Since the _ still matches everything, I would suggest that having both _ and else { } would be a compiler error of type Unreachable code on the else statement since it can never run.

@coder543
Copy link

coder543 commented Jul 21, 2017

Just to throw this into the discussion, this seems to be a functionally complete implementation of match_all, even if it is not an aesthetically complete implementation. The commented out line is an optional special-case that handles if no match occurred. If you would prefer that every match_all handles the condition of no match, this version provides that. Glancing through the discussion, it doesn't look like anyone has come up with these modifications to @glaebhoerl's code.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 24, 2017

Do you know why using | results in a compilation error? If the syntax was instead using | instead of , then the bug of a result being done multiple times could be fixed. I though that pat | pat was also a pat

@coder543
Copy link

coder543 commented Jul 24, 2017

If the syntax was instead using | instead of , then the bug of a result being done multiple times could be fixed.

Can you provide an example of both the bug and what the match_all would look like with that change? I'm not sure I understand what bug would be avoided, or what the final match_all would look like. As for why it's a compilation error, macro_rules! has some annoying limitations on what is acceptable, but this isn't one of those times. Should $x:pat | $y:pat being fed Some(3) | None have $x = Some(3) | None such that $y doesn't have a value? If you have more than one | bar, how do you divide the pattern up? If pat | pat => pat is true, there's no clear solution for when $y would ever receive a value. It would be either impossible or random for $y to receive a value.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 24, 2017

Here is an example, what I mean is that if a pattern appears twice in the same pat/expr pair then the expr will be executed multiple times.

And when I was saying that | doesn't work as a pattern, I had removed the ($p:pat),+ and made it just $p:pat which I thought would except |

@coder543
Copy link

I understand now. It's worth nothing that the if let syntax only allows for one pattern, not multiple, so this is a syntax error:

let value = Some(4);
if let Some(4) | None = value {
    println!("yay");
}

@coder543
Copy link

coder543 commented Jul 24, 2017

This version uses or-bars instead of commas for aesthetics. (does not fix the repeated condition issue)

@coder543
Copy link

And now, this other version uses or-bars and prevents duplicate execution of a match branch.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 24, 2017

I was just about to post that 😀 Though my version just uses a boolean check

@coder543
Copy link

I would guess that using break makes it clearer to the optimizer that it can jump straight to the end, rather than checking each of the other conditions, as soon as one test case passes. i have been wrong about optimizers many times in the past, but that's my guess.

@coder543
Copy link

coder543 commented Jul 24, 2017

With and without optimizations using my version of the code. I just love how Rust and LLVM are able to optimize the high-level abstraction to just three instructions necessary to call both test3 and test4, in this case where it unrealistically has full knowledge of everything.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 24, 2017

That. Is. Insane. But no matter, I think that this macro enables everything in the rfc so now to put it onto cargo. Do you care who does?

@coder543
Copy link

I don't care who publishes it, feel free to! And I don't really care if I get listed as a contributor or not, but it's always fun to pump up my count of crates I've helped author! XD

If you want to list me as an author, I'm the first author here, and you can get the author string there, but I really don't care either way! I only did a little bit.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 25, 2017

Here is the crate. I have added the ability to use the match_all with IfNoMatch branch for variable assignment

@coder543
Copy link

Where is the repository? It would be nice to have some example code in the docs, so I was thinking about sending a pull request.

@Nokel81
Copy link
Contributor Author

Nokel81 commented Jul 25, 2017

https://github.com/nokel81/match_all. The readme does but I cannot find a place to put the readme as the description

@coder543
Copy link

PR sent

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Dec 6, 2017
@Centril
Copy link
Contributor

Centril commented Apr 26, 2018

Closing as the associated and linked RFC was closed.

@Centril Centril closed this as completed Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

7 participants