Conversation
Our migrations can now support modular explicits. This also required a fix to how we were migrating Ppat_unpack too. Signed-off-by: Patrick Ferris <patrick@sirref.org>
Signed-off-by: Patrick Ferris <patrick@sirref.org>
patricoferris
requested review from
NathanReb,
ceastlund and
panglesd
as code owners
NathanReb
left a comment
NathanReb
left a comment
There was a problem hiding this comment.
I'll sum up here some of the offline discussions we had:
- Encode
Ppat_unpack (_, Some _)as an extension point instead as it is semantically different from 5.4'sPpat_constraint (Ppat_unpack _, Ptyp_package _)and the attribute encoded version could too easily be tempered with in a breaking way. - external type declarations should be encoded in an extension point at the closest parent. We can keep the attribute based encoding inside the extension's payload but it has to be wrapped to avoid tempering or misinterpretation by existing ppx-es.
- I think I like the encoding (as in a node gets encoded into an extension point) to be tested separately from regular migration.
Other than that it all looks good, I appreciate the detailed explanation in the test that really helped understanding the migrations. I wonder if we should add comments to non trivial migration code to provide context right next to it without having to jump elsewhere.
| let mk_core_type ?(ptyp_attributes = []) ptyp_desc = | ||
| { | ||
| ptyp_desc; | ||
| ptyp_loc = Location.none; |
There was a problem hiding this comment.
Just to err on the safe side, I think we should use the node to encode's loc here!
| > EOF | ||
|
|
||
| For now, we do not support these and raise an error. In the future we may wish | ||
| to encode this feature into attributes and this test, along with this comment, |
There was a problem hiding this comment.
This comment should be removed!
|
We realized encoding See the discussions in ocaml/ocaml#14149 for
This preserves semantics and in the event that a ppx would temper with the attribute, the worst possible scenario doesn't lead to a compiler error anymore and instead turns an ill-typed program into a well typed one. |
2 participants
This adds further support for the 5.5 compiler (#620). In particular it adds full migrations for modular explicits and external type declarations. This PR will require ocaml/ocaml#14518 to fix the tests for external types.