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

Implement (if-let) guards in pattern matchings #821

Merged
merged 6 commits into from
Aug 5, 2024
Merged

Conversation

maximebuyse
Copy link
Contributor

@maximebuyse maximebuyse commented Aug 1, 2024

Closes #814

This adds support for guards in pattern matchings. They are added to the AST (with a corresponding feature guard), and handled in the THIR import. A phase rewrites them as an equivalent nested pattern matching. Apart from supporting guards as specified in #807, this will also be useful for future phases that will generate guards (see #806).

Example of code with if-let guard (which is currently a nightly feature) and Rust code for the equivalent after the new phase:

pub fn if_let_guard(x: Option<Result<i32, i32>>) -> i32 {
    match x {
        None => 0,
        Some(v) if let Ok(y) = v => y,
        Some(Err(y)) => y,
        _ => 1,
    }
}

pub fn equivalent(x: Option<Result<i32, i32>>) -> i32 {
    match x {
        None => 0,
        _ => match match x {
            Some(v) => match v {
                Ok(y) => Some(y),
                _ => None,
            },
            _ => None,
        } {
            Some(y) => y,
            None => match x {
                Some(Err(y)) => y,
                _ => 1,
            },
        },
    }
}

Regular if guards are also supported (but for now desugared as if-let guards):

pub fn if_guard(x: Option<i32>) -> i32 {
    match x {
        Some(v) if v > 0 => v,
        _ => 0,
    }
}

pub fn equivalent(x: Option<i32>) -> i32 {
    match x {
        Some(v) if let true = v > 0 => v,
        _ => 0,
    }
}

@maximebuyse maximebuyse requested a review from W95Psp August 1, 2024 07:57
@maximebuyse
Copy link
Contributor Author

This also closes #818 and #807

Copy link
Collaborator

@W95Psp W95Psp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, the output looks like what we want!
However, we need:

  • more tests (e.g. a match where more than one arm as an if/if let, exercise if, exercise a non-trivial if let with a computation in the RHS... you don't need to have the equivalent desugar for everything)
  • some description on the PR. Your PR should:
    • explain the problem you solve (referring to an issue is fine, iff the issue has a nice and intelligible description about the exact problem we are solving, with examples --but in your case the issue has no description, please add some as requested in the issue)
    • then give an overview of technically what is our approach
    • then give examples of what one can do if your PR is merged (possibly with links to playground)
    • note that those points above should basically be copy pastes from code documentations or tests cases

@maximebuyse
Copy link
Contributor Author

Thank you, the output looks like what we want! However, we need:

* more tests (e.g. a match where more than one arm as an `if`/`if let`, exercise `if`, exercise a non-trivial `if let` with a computation in the RHS... you don't need to have the equivalent desugar for everything)

* some description on the PR. Your PR should:
  
  * explain the problem you solve (referring to an issue is fine, iff the issue has a nice and intelligible description about the exact problem we are solving, with examples --but in your case the issue has no description, please add some as requested in the issue)
  * then give an overview of technically what is our approach
  * then give examples of what one can do if your PR is merged (possibly with links to playground)
  * note that those points above should basically be copy pastes from code documentations or tests cases

Thank you. I addressed most of your feedback but I can add more tests / documentation if you think it is not clear enough.

@W95Psp W95Psp self-requested a review August 1, 2024 15:12
Copy link
Collaborator

@W95Psp W95Psp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you, that's great! :)
I don't know how you want to do this optimization for "one wildcard arm" matches: in this PR or in a subsequent one?

@maximebuyse
Copy link
Contributor Author

Thanks you, that's great! :) I don't know how you want to do this optimization for "one wildcard arm" matches: in this PR or in a subsequent one?

I'll do it in this one

@maximebuyse
Copy link
Contributor Author

Thanks you, that's great! :) I don't know how you want to do this optimization for "one wildcard arm" matches: in this PR or in a subsequent one?

@W95Psp That's done now

@W95Psp
Copy link
Collaborator

W95Psp commented Aug 5, 2024

Thanks, I think that's great now! Let's merge that PR :)

@W95Psp W95Psp enabled auto-merge August 5, 2024 09:08
@W95Psp W95Psp added this pull request to the merge queue Aug 5, 2024
Merged via the queue into main with commit aa0d1c9 Aug 5, 2024
13 checks passed
@W95Psp W95Psp deleted the implement-814 branch August 5, 2024 09:38
@W95Psp W95Psp mentioned this pull request Aug 5, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support if let guards in pattern matchings
2 participants