Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds ppx support for the new language features in OCaml 5.5.
Polymorphic function parameters and modular explicits in OCaml 5.5 both require type annotations, so the eta-expansion done by the ppx is now nontrivial; it must preserve type annotations. With type annotations come optional arguments with default values, which contain expressions that may depend on previous arguments, so previous arguments should not be renamed in order for type annotations to work. Thankfully, as far as I can tell, polymorphic parameters cannot be optional, and there is no syntax for optional or labelled modular explicits. Hence, neither needs to be handled.
The code in this PR does not depend on OCaml 5.5, and it works for previous versions with only one small regression: unpacked module arguments cannot shadow each other, which seems like a silly programming pattern anyways.
Reviewing
This PR is implemented in four commits, the first two of which are not actually required for 5.5 support and are just general upgrades. This PR seems best reviewed commit-by-commit.
max_listthat actually returned a minimum list. It seemed like a typo, and this commit fixes it and adds a test.opam; see testing below) so that the new value binding continues to silence such warnings.I comment the code quite extensively because I did not exactly have an easy time figuring out how to eta-expand these two new features. It was simpler than I first expected, but since the details are quite implicit in the parse tree, it felt necessary to explain what's happening.
Testing
I have added tests to cover the features. Since optional arguments seemed to be an edge case, I used Sherlocode (see link below) to look for optional module arguments, and it seems no code will be broken by these updates.
opamuses optional first class module arguments frequently, so I instrumented all ofopamwith--auto(nearly 60_000 lines), and it all compiled without issue.The tests include some classic modular explicits code, but I did not fully stress test because I don't have a wealth of examples for this new feature.
The tests pass with
dune test(and instrumentedopambuilds) on OCaml 5.5.0~alpha1 with ppxlib release 0.38.0, and also on OCaml 5.4 with ppxlib release 0.37.0.Optional module arguments in the ecosystem: https://sherlocode.com/?q=%3F%5Cw%5C*%3A%5C((module