Ppxlib.0.26.0 compatibility

[pitag-ha]

This is a patch PR to make the PPX compatible with ppxlib.0.26.0 which has bumped the AST to 4.14/5.00.

[gasche]

If I read the code correctly, the change to the AST corresponds the new possibility of writing 'c 'd. in

type ('a, 'b) t += Foo : 'c 'd. foo -> (bar, baz) t

The proposal in this PR is to reject this new form -- extension constructors that explicitly bind GADT existential type variables.

Question: why are we rejecting this new form, instead of ignoring it? This makes the behavior inconsistent with non-extension constructor, such as

type ('a, 'b) t = Foo : 'c 'd. foo -> (bar, baz) t

where 'c 'd. is silently ignored (which sounds like a better choice).

[pitag-ha]

Hey, thanks for the review! In these patch PRs, I simply preserve the old behavior and error out when encountering a new feature in the AST that isn't accounted for (I should make that clear in the PR message). The reason for that is that I myself don't know all >100 ppxlib reverse dependencies well enough to decide myself what a good behavior is when encountering the new feature in the AST. So currently the strategy when bumping the AST is: I simply write a patch PR that brings the PPX up-to-date with the PPX ecosystem.

It's true though that it makes sense to update the PR when the maintainers point out what the good new behavior would look like. So I've just updated the PR to ignore bindings of existential type variables by extension constructors. Thanks for pointing out that that would be the good behavior!

[gasche]

Thanks! I merged.

[gasche]

In these patch PRs, I simply preserve the old behavior and error out when encountering a new feature in the AST that isn't accounted for (I should make that clear in the PR message).

But then the behavior, if the PR gets merged, is a codebase that compiles against the new OCaml/ppxlib AST, but does not work correctly when fed source using its new syntactic features. So we get compile-time compatibility with new ppxlib versions, but runtime incompatibility. And there isn't a place for the user to get documentation on which features are actually supported (except by reading the source or trying the extension on various examples).

This is something that we would already have (without a source change on the extension side) with the old ocaml-migrate-parsetree workflow that would insert a (partial) conversion to the extension-supported AST. But here the PR also serves as a notification to the extension' maintainers to ask them to update their code to follow the ppxlib update, and it lets maintainers update the code incrementally (support some features but not others).

[pitag-ha]

So we get compile-time compatibility with new ppxlib versions, but runtime incompatibility.

I think it's important to mention that the runtime incompatibility only exists if the new feature is used on a node on which the deriver is applied. Anywhere else in the code, new features can be used.

This is something that we would already have (without a source change on the extension side) with the old ocaml-migrate-parsetree workflow that would insert a (partial) conversion to the extension-supported AST.

The old ocaml-migrate-parsetree workflow gave runtime incompatibility always when a new feature was used anywhere in the code.

And there isn't a place for the user to get documentation on which features are actually supported (except by reading the source or trying the extension on various examples).

That's a good point! I think I should at least add the word "currently" to the "isn't supported" message, so that it's clear that the missing support isn't necessarily due to the nature of the deriver (and it might be worth filing an issue if there's interest in having it supported).

But here the PR also serves as a notification to the extension' maintainers to ask them to update their code to follow the ppxlib update, and it lets maintainers update the code incrementally (support some features but not others).

Yes, exactly! And I think it doesn't matter if that's done in follow-up PRs or if maintainers let me know what the good behavior would be when encountering a new feature so that I adapt the patch PR to that. But in any case, the maintainers need to make sure the code is updated to implement the expected behavior on new features (of course, that's only the case in patch PRs similar to this one).