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

Add type constraint to binding_op #2486

Merged
merged 4 commits into from
Dec 14, 2023
Merged

Conversation

gpetiot
Copy link
Collaborator

@gpetiot gpetiot commented Dec 7, 2023

Contributing to #2401

The main positive is removing Sugar.type_cstr (all this code is inconvenient for the polynewtypes shenanigans) by doing this work in the parser.

A minor side effect is that we don't add useless parentheses around:

  • let* ({ state; server; pool } as t) = xxx
  • let+ (Ok x) = xxx

It's a fix, but not important enough to add a line in the changelog.

@gpetiot gpetiot added the no changelog set this to bypass the CI check for changelog entries label Dec 7, 2023
@gpetiot gpetiot requested a review from Julow December 7, 2023 08:47
Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Very happy that Sugar.type_cstr goes away! The parser change is minimal too.

I think the parentheses issue should be fixed though.

lib/Ast.ml Outdated
@@ -1893,14 +1905,14 @@ end = struct
| Fp {pparam_desc= Pparam_val (_, _, _); _}, Ppat_cons _ -> true
| Pat {ppat_desc= Ppat_construct _; _}, Ppat_cons _ -> true
| _, Ppat_constraint (_, {ptyp_desc= Ptyp_poly _; _}) -> false
| ( Exp {pexp_desc= Pexp_letop _; _}
| ( (Exp {pexp_desc= Pexp_letop _; _} | Bo {pbop_typ= Some _; _})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Parenthesing of pattern is not consistent, maybe some AST rules are missing ?

let _ =
  let (_ as foo) = foo in
  let+ _ as foo = foo in
  ()

These parentheses are intended for disambiguation, I think.

@gpetiot gpetiot force-pushed the binding-op-constraint branch from e8769df to 5ecdbf3 Compare December 14, 2023 09:25
@gpetiot
Copy link
Collaborator Author

gpetiot commented Dec 14, 2023

I think I fixed all regressions, test_branch doesn't report any diff.

@gpetiot gpetiot requested a review from Julow December 14, 2023 10:19
Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Looks good :)

@Julow Julow merged commit 3b3f88a into ocaml-ppx:main Dec 14, 2023
8 of 9 checks passed
@gpetiot gpetiot deleted the binding-op-constraint branch December 15, 2023 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog set this to bypass the CI check for changelog entries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants