Skip to content

Make bind & tighter than mod in jkind parsing #3076

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

Merged
merged 8 commits into from
Sep 26, 2024

Conversation

dvulakh
Copy link
Contributor

@dvulakh dvulakh commented Sep 25, 2024

Currently,

type t = j & k mod m

parses as

type t = j & (k mod m)

The crowdsourced concensus is that this is confusing. This PR makes it parse as

type t = (j & k) mod m

instead.

Why not require the user to be explicit and write parentheses either way? We'll likely make ocamlformat add more parentheses than are necessary, but feel that requiring parentheses in the parser would hurt approachability / discoverability of the new syntax.

Signed-off-by: David Vulakh <dvulakh@janestreet.com>
Signed-off-by: David Vulakh <dvulakh@janestreet.com>
Signed-off-by: David Vulakh <dvulakh@janestreet.com>
Signed-off-by: David Vulakh <dvulakh@janestreet.com>
Note that
```
type t3 : any & (any mod non_null) = #(t1 * t2);;
```
Does not have the same behavior as
```
type t3 : any & any mod non_null = #(t1 * t2);;
```

Signed-off-by: David Vulakh <dvulakh@janestreet.com>
@dvulakh dvulakh requested a review from ccasin September 25, 2024 14:30
@ccasin
Copy link
Contributor

ccasin commented Sep 25, 2024

(We discussed in person and David is also going to add a test to pprintast_unconditional.ml to check if we need to modify any printers)

@mshinwell mshinwell added the lexer/parser Changes to the lexer and parser label Sep 25, 2024
the test currently fails, so yes, we need to update the kind printer

Signed-off-by: David Vulakh <dvulakh@janestreet.com>
there are sometimes unnecessary parentheses, but that seems fine in
the name of both clarity of printed syntax and printer simplicity

added a test with [with] along the way

Signed-off-by: David Vulakh <dvulakh@janestreet.com>
and change the jkind bits of pprint ast to use [Misc.pp_parens_if] now
that it's exposed

Signed-off-by: David Vulakh <dvulakh@janestreet.com>
Copy link
Contributor

@ccasin ccasin left a comment

Choose a reason for hiding this comment

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

LGTM

@dvulakh dvulakh merged commit 058c893 into main Sep 26, 2024
16 checks passed
@dvulakh dvulakh deleted the dvulakh.jkind-parsing-precedence branch September 26, 2024 02:55
lukemaurer pushed a commit to lukemaurer/flambda-backend that referenced this pull request Oct 23, 2024
…caml-flambda#3076)

* make `&` tighter than `mod` in jkind parsing

Signed-off-by: David Vulakh <dvulakh@janestreet.com>

* forbid `k mod m & l` without parens around `mod`

Signed-off-by: David Vulakh <dvulakh@janestreet.com>

* reflect new syntax in tests

Signed-off-by: David Vulakh <dvulakh@janestreet.com>

* revert 1eea0c2

Signed-off-by: David Vulakh <dvulakh@janestreet.com>

* add typing test to demo `&` and `mod` precedence

Note that
```
type t3 : any & (any mod non_null) = #(t1 * t2);;
```
Does not have the same behavior as
```
type t3 : any & any mod non_null = #(t1 * t2);;
```

Signed-off-by: David Vulakh <dvulakh@janestreet.com>

* add test to [pprintast_unconditional.ml]

the test currently fails, so yes, we need to update the kind printer

Signed-off-by: David Vulakh <dvulakh@janestreet.com>

* add more parentheses in printer

there are sometimes unnecessary parentheses, but that seems fine in
the name of both clarity of printed syntax and printer simplicity

added a test with [with] along the way

Signed-off-by: David Vulakh <dvulakh@janestreet.com>

* fix oprint

and change the jkind bits of pprint ast to use [Misc.pp_parens_if] now
that it's exposed

Signed-off-by: David Vulakh <dvulakh@janestreet.com>

---------

Signed-off-by: David Vulakh <dvulakh@janestreet.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lexer/parser Changes to the lexer and parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants