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

Fix two labeled tuple bugs. #56

Merged
merged 2 commits into from
Jan 24, 2024
Merged

Fix two labeled tuple bugs. #56

merged 2 commits into from
Jan 24, 2024

Conversation

ccasin
Copy link

@ccasin ccasin commented Jan 23, 2024

Bug one

(), ~x:(fun y -> y) was being misformatted as (), ~x:fun y -> y, which doesn't parse. I thought this was avoided by the precedence changes in the original labeled tuples PR, but I was wrong.

Now that I've added similar logic and tests to labeled tuple patterns and expressions to make parenthesization coherent, you might ask whether I should pre-emptively do it for types. I believe this isn't needed because labeled tuple type elements use the same non-terminal as normal tuple type elements (atomic_type, see here). On the other hand, when parsing patterns and expressions, a different parser is used for the labeled and unlabeled cases (pattern vs simple_pattern, and expr vs simple_expr).

Bug two

unit -> local_ (x:int * y:bool) was being misformatted as unit -> local_ x:int * y:bool, which doesn't parse.

The parens are needed only if the return is local_ and the tuple's first element is labeled. This is a limitation in the parser. The way ocamlformat handles local_ makes the location of the fix awkward, but ultimately fairly understandable. Hopefully one day we'll rework the local_ printing.

Copy link

@riaqn riaqn left a comment

Choose a reason for hiding this comment

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

LGTM - feel free to merge once git signature is fixed.

@ccasin ccasin merged commit d693e9d into janestreet:jane Jan 24, 2024
7 of 8 checks passed
@riaqn
Copy link

riaqn commented Jan 24, 2024

For future reference (after discussion with @ccasin )
Labelled tuple types (whose first field is labelled) always need parens on the LHS of arrow types, regardless of local_. This is done in parenze_typ in Ast.ml.
That's also where we should check parens for the RHS of arrow types. But because the RHS depends on local_, which has been stripped away before parenze_typ, we check it elsewhere as done in this PR.

@ccasin thinks we should move local syntax to jane_syntax for maintainability. I want to note that the new mode encoding
ocaml-flambda/flambda-backend#2231
won't help, as it retains the spirit of the old encoding and only uses part of jane_syntax.

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.

2 participants