Skip to content

Add support for OCaml 4.13#4352

Merged
1 commit merged intoocaml:2.8from
kit-ty-kate:413
Mar 15, 2021
Merged

Add support for OCaml 4.13#4352
1 commit merged intoocaml:2.8from
kit-ty-kate:413

Conversation

@kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Mar 13, 2021

Record disambiguation behaviour changed when using |> in ocaml/ocaml#10081

cc @alainfrisch

@jberdine
Copy link
Contributor

I'm sorry, I noted this change here but failed to make sure that it was seen by anyone working on dune.

@ghost
Copy link

ghost commented Mar 15, 2021

No worries and thanks @kit-ty-kate for the patch. Nice to know about this OCaml PR BTW, seems like we might finally be able to get rid of ppx_pipebang at Jane Street.

BTW, I'm curious how the OCaml PR relates to this change. I'm guessing this is because of the |> in let output = ... in, but I don't see the link to the t variable.

@ghost ghost merged commit 0078756 into ocaml:2.8 Mar 15, 2021
@jberdine
Copy link
Contributor

@jeremiedimino there was some discussion of the need to add this annotation in the ocaml PR comments, see e.g. ocaml/ocaml#10081 (comment) . TL;DR: it seems that this code was relying on (non-principal) horizontal type propagation.

@kit-ty-kate
Copy link
Member Author

i forgot to specify, this probably needs a cherry-pick in main as well

ghost pushed a commit that referenced this pull request Mar 15, 2021
@ghost
Copy link

ghost commented Mar 15, 2021

@jberdine thanks for the pointer. Without digging too much into this, my initial interpretation is: "where type annotations are needed depend on implementation details in the compiler". Am I far from the truth? :)

@kit-ty-kate, cherry-picked in main.

@jberdine
Copy link
Contributor

@jberdine thanks for the pointer. Without digging too much into this, my initial interpretation is: "where type annotations are needed depend on implementation details in the compiler". Am I far from the truth? :)

Yeah, that's right. Except that if you compile with -principal it is AFAIU deemed a bug in the compiler for such details to matter.

kit-ty-kate added a commit to rgrinberg/opam-repository that referenced this pull request Mar 29, 2021
@kit-ty-kate kit-ty-kate deleted the 413 branch April 26, 2021 09:54
This pull request was closed.
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