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

Guard Limitations in let #317

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

milesfrain
Copy link
Contributor

No description provided.

@hdgarrood
Copy link
Collaborator

From what I can tell from looking at the parser, it’s not just constructor patterns that don’t work with guards, it’s any pattern other than a simple identifier. For example, it looks to me like record and array patterns and at-patterns (foo@(...)) don’t work in this context either. I also think we ought to be able to come up with a better justification for this. I’ll have a think.

@natefaubion
Copy link
Contributor

natefaubion commented May 17, 2020

It's that way "because". See purescript/purescript#3200. Basically multiway-if would be more general, but there's nothing fundamentally preventing it's inclusion.

@milesfrain
Copy link
Contributor Author

Should we add the caveat, then open a new issue to remove it?

Comment on lines 182 to 193
Guards are incompatable with Constructor Patterns in `let`. For example, the following function using a `Tuple` constructor pattern will not compile:
```purs
-- This doesn't work
f1 :: Int
f1 =
let
(Tuple a b)
| false = Tuple 1 2
| otherwise = Tuple 3 4
in
a
```
Copy link
Member

@thomashoneyman thomashoneyman Aug 21, 2020

Choose a reason for hiding this comment

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

Some of the capitalization is a bit odd -- we don't usually capitalize "guard" or "constructor". What about:

Guards are not supported with patterns other than simple identifiers in let expressions. For example, this does not compile:

I don't know about linking to purescript/purescript#3200 here, but it's at least accurate to say that this isn't supported right now.

Copy link
Member

Choose a reason for hiding this comment

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

Other than this I think it's sensible to merge this while the restriction does exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied your suggestion and also added that link. I'm a fan of linking to relevant issues in the docs.

@milesfrain
Copy link
Contributor Author

Co-authored-by: Thomas Honeyman <[email protected]>
@JordanMartinez
Copy link
Contributor

This PR hasn't been merged because it's a bit incomplete, I believe.

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.

5 participants