Fix migration of Pexp_function type_constraints

[NathanReb]

In 5.2 the representation of function expressions has been simplified. Among the simplifications, a Pexp_function now has a type_constraint argument that captures a type constraint or coercion on the function's return type.

E.g. in 5.01 the following:

let f x : string = x

was roughly:

Pexp_fun
  Nolabel
  None
  (Ppat_var "x")
  (Pexp_constraint (Pexp_ident "x") (Ptyp_constr "t")

In 5.02 it becomes:

Pexp_function
  [(Pparam_val Nolabel None (Ppat_var "x"))]
  (Some (Pconstraint (Ptyp_constr "t")))
  (Pfunction_body (Pexp_ident "x"))

It's worth noting that it's still possible to have the coercion or constraint in the function body. We chose to always extract it from the body and promote it to the Pexp_function argument when migrating from 5.1 to 5.2.

There was a bug in the coercion case where we were promoting it AND keeping it in the body which lead to compilation errors as the expression wasn't typing correctly anymore.

[NathanReb]

I'm trying to figure out if there is any value in trying to make this round-trip properly as it could easily be achieved with attributes but I don't want to go down that road if it's not necessary.

[NathanReb]

cc @Octachron @kit-ty-kate

This fixes the bug in https://github.com/Octachron/bug-reproduction-archives/pull/1.

I'm still a bit puzzled as to why it didn't trigger when manually compiling with -ppx but at least now it's not triggering with dune anymore and the migration is correct!

[panglesd]

Thanks @NathanReb !

I would appreciate a little help to review, if you still have the issue freshish in your mind.

In particular, your example treats about constraints, but your modification is on the coercion migration... Could you confirm that the actual code where the migration was wrong, and was fixed by this PR is:

let f x :> string = x

?

(We need a way to easily add tests for migrations to review those kind of changes...)

[NathanReb]

The issue was with with coercion which is one of the two possible type of type_constraints, see:

• Pexp_function definition
• type_constraint definition

What happens in the migration is that we want to move those constraint up from the function body to the Pexp_function as described in the PR description above.

We were doing it correctly in the Pexp_constraint case, see here and here, you can see we're calling take_body on the expression wrapped in the Pexp_constraint.

In the Pexp_coerce case though we were not. It's likely just a typo but you can see here that we call take_body e, e not being the expression wrapped in the Pexp_coerce but instead the expression we are matching over, i.e. the Pexp_coerce itself.

Roughly, instead of producing:

let f x : t :> string = body

We were produing:

let f x : t :> string = (body : t :> string)

Which was not compiling because one of those coercion happened first and made the other incorrect as t was not visible as the return type anymore.

[kit-ty-kate]

Should the preview package in opam-repository be updated? If so I can do if you want.

I've tested trunk-support locally and it seems to work just fine and has fixed the two issues reported

[NathanReb]

Sorry I was off on friday! Yes I will release a new preview version! I just need to check if the ocaml.ppx.context thing has been fixed upstream!