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

Curly braces are allowed nested in patterns #1380

Merged
merged 9 commits into from
Jul 20, 2022

Conversation

janmasrovira
Copy link
Collaborator

@janmasrovira janmasrovira commented Jul 15, 2022

Fixes #1337.

@janmasrovira janmasrovira self-assigned this Jul 15, 2022
@lukaszcz lukaszcz self-requested a review July 15, 2022 15:24
@janmasrovira janmasrovira force-pushed the 258-curly-braces-are-allowed-nested-in-patterns branch 2 times, most recently from 195296c to 7a95e21 Compare July 15, 2022 17:19
@janmasrovira janmasrovira marked this pull request as ready for review July 18, 2022 09:46
@lukaszcz lukaszcz force-pushed the 258-curly-braces-are-allowed-nested-in-patterns branch from abeb1f4 to a27470c Compare July 19, 2022 08:21
@lukaszcz
Copy link
Collaborator

Compiling:

module braces;

f : {A : Type} -> A -> A;
f {Do you wanna stack trace?} x := x;

end;

results in:

juvix: constructor expected on the left of a pattern application
CallStack (from HasCallStack):
  error, called at src/Juvix/Prelude/Base.hs:264:9 in juvix-0.2.1-CZT06Rzh5GNEZIQRWHpfSc:Juvix.Prelude.Base
  error, called at src/Juvix/Translation/ScopedToAbstract.hs:382:11 in juvix-0.2.1-CZT06Rzh5GNEZIQRWHpfSc:Juvix.Translation.ScopedToAbstract

Should be a user-level error message.

@lukaszcz
Copy link
Collaborator

Maybe I'll just open the above as a separate issue. It fails also for explicit arguments in ordinary braces.

@lukaszcz
Copy link
Collaborator

Maybe it's fine to leave it as it is for now in the current PR, but I think ultimately PatternBraces should be removed from the scoped concrete syntax. It's not a kind of pattern, but an indication that the argument is implicit. The information on which patterns (arguments) are implicit should perhaps be kept in FunctionClause? Analogously, you don't have PatternParens in the scoped concrete syntax, PatternAtomParens seems to be removed at scoping.

@janmasrovira
Copy link
Collaborator Author

Maybe it's fine to leave it as it is for now in the current PR, but I think ultimately PatternBraces should be removed from the scoped concrete syntax. It's not a kind of pattern, but an indication that the argument is implicit. The information on which patterns (arguments) are implicit should perhaps be kept in FunctionClause? Analogously, you don't have PatternParens in the scoped concrete syntax, PatternAtomParens seems to be removed at scoping.

Good point. I think it is better if I remove PatternBraces from scoped syntax in this PR

@janmasrovira
Copy link
Collaborator Author

Maybe it's fine to leave it as it is for now in the current PR, but I think ultimately PatternBraces should be removed from the scoped concrete syntax. It's not a kind of pattern, but an indication that the argument is implicit. The information on which patterns (arguments) are implicit should perhaps be kept in FunctionClause? Analogously, you don't have PatternParens in the scoped concrete syntax, PatternAtomParens seems to be removed at scoping.

Good point. I think it is better if I remove PatternBraces from scoped syntax in this PR

Done in 1108b7f

@janmasrovira janmasrovira modified the milestones: 0.2.0, 0.2.2 Jul 19, 2022
@janmasrovira janmasrovira force-pushed the 258-curly-braces-are-allowed-nested-in-patterns branch from 1108b7f to de606c1 Compare July 19, 2022 13:20
@lukaszcz
Copy link
Collaborator

Do you intend to fix "proper error messages" in another commit to this PR, or should this be done in another issue?

@janmasrovira
Copy link
Collaborator Author

janmasrovira commented Jul 19, 2022

Do you intend to fix "proper error messages" in another commit to this PR, or should this be done in another issue?

Let's do it in a separate pr.
I have already started working on it, but I think it would be a good idea for you to join me to get familiar with creating new errors

@lukaszcz
Copy link
Collaborator

Do you intend to fix "proper error messages" in another commit to this PR, or should this be done in another issue?

Let's do it in a separate pr. I have already started working on it, but I think it would be a good idea for you to join me to get familiar with creating new errors

Okay, I'll approve this PR then.

@lukaszcz lukaszcz self-requested a review July 19, 2022 14:39
@jonaprieto jonaprieto force-pushed the 258-curly-braces-are-allowed-nested-in-patterns branch from 6e2aefe to bd57aa2 Compare July 20, 2022 08:05
@janmasrovira janmasrovira merged commit a8f4aca into main Jul 20, 2022
@janmasrovira janmasrovira deleted the 258-curly-braces-are-allowed-nested-in-patterns branch July 20, 2022 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Curly braces are allowed nested in patterns
2 participants