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 precedence of stack_ keyword #92

Open
wants to merge 2 commits into
base: jane
Choose a base branch
from

Conversation

dvulakh
Copy link

@dvulakh dvulakh commented Nov 27, 2024

#85 added support for the stack_ keyword. Internal adoption has revealed some bugs in the parenthesization behavior. Two examples:

Foo (stack_ ((), ()))

becomes

Foo stack_ ((), ())

(invalid syntax)

stack_ (() :: [])

becomes

stack_ () :: []

(parsetree change)

This PR fixes the precedence associated with Pexp_stack in the extended AST for these two patterns, as well as for a number of patterns that should be uncommon (because there stack_ does not enclose an allocation, and so the code is rejected by the typechecker). We should still aim to support these cases, because:

  1. We should succeed in formatting any code that parses. If someone writes one of these forbidden patterns, they should get a (ostensibly helpful) error from the typechecker, not a mysterious formatting failure.
  2. These patterns could be used in ppx payloads and then transformed to code that actually compiles.

It might be worth testing ocamlformat's treatment of stack_ more thoroughly in the near future, but I think it makes sense to merge this PR before doing so, since it fixes problems people have actually started to encounter.

these tests produce a runtime error in ocamlformat

Signed-off-by: David Vulakh <[email protected]>
the two main bugs in the PR description, plus some expressions nobody
should write, but might write anyway

Signed-off-by: David Vulakh <[email protected]>
@dvulakh dvulakh requested a review from ccasin November 27, 2024 19:21
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.

1 participant