-
Notifications
You must be signed in to change notification settings - Fork 99
Dev Meeting 2024 05 28
- 5.2 Release
- Released during compiler's beta, went smoothly
- 5.3 trunk support
- Reused the work from @hhugo and adapted it
- We have an open PR with 5.3 support that needs review
- External contributors already started adding support for new features: @nojb added support for the effects patterns and an internal change to location reports
- How to maintain trunk support on our main branch
-
ppx_derivingandppx_deriving_yojsonppxlib ports- PRs open for the release of both
- A few bug fixes were required but it should be good to go now
- Next: Deprecating ppx_deriving's API and ppx_derivers
- 5.2 internal AST bump
- Now that the 5.2 support has been released, we can discuss the plan for this
- Unused code/type/module warnings countermeasures
- We have code to generate attributes or actual AST nodes that are only here to silence warnings
- They are emitted unconditionally whereas it's pretty clear that should not be the case
- Some of them mess with code coverage tools such as bisect_ppx
- Nathan
- Sonja
- Paul-Elliot
- Mathieu Barbin
The release went smoothly and no changes were needed for 5.2 compat after the compiler's beta release.
The process of merging trunk-support into main could have used a bit more documentation but that won't be a problem anymore since we'll gradually add compiler support into main.
Action-points:
- Nathan: Write an entry for the OCaml Changelog. ✔️
Nathan opened a PR to add 5.3 trunk support. He reused the work from @hhugo that only needed a couple of small fixes as it had some bugs copied over from previous migrations.
@nojb has since then contributed to the PR, adding support for location_report changes and the new efect patterns in the parsetree.
Since we are moving the trunk support to the main branch we'll need a CI to check that we can build with trunk. ocaml-ci can't be used for this unfortunately so we'll likely use github's own to add a trunk build. The advantage is that it should be easier to make it clear it's a separate build from the rest and that it is not require to pass for PRs to be merged. There is an old PR on ppxlib that attempted to add such a build but wasn't merged because the build couldn't be run for a non default branch. We can probably reuse this work. It also contained a cron job running the trunk builds once a week. That would be helpful in detecting breakage even when no new PRs are opened on ppxlib.
We'd also like to receive notifications when one opens a PR with parsetree changes on the compiler. That would allow to anticipate a bit on such change and to work with the authors to write the migrations. It could also eventually help identify how some parsetree changes could be improved to ease migrations and compat in general.
We'll need documentation and/or scripts to assist with releases. Since we're adding the trunk support gradually we're using a < 5.4 compiler upper bound on main but we want a < 5.3 upper bound for released version between now and at least the 5.3 beta.
One downside of having the trunk support on main is that if we introduce breaking changes, it will make it harder to test new compilers with ppxlib. We rarely introduce breaking changes outside of AST bumps which generally happens right after a compiler release and we do the extra work to fix our reverse dependencies. All in all it shouldn't be too hard to deal with but it might require a bit of discipline on our side.
Action-points:
- Nathan to add CI scripts to build with trunk ✔️
- Nathan to look into adding pings to parsetree touching PRs on the compiler
The ppxlib driver generates items and/or wraps code generated by derivers to silence warnings 32, 34 and 60, repesctively unused value, type declaration and module. To be a bit more specific, the driver will wrap generated code in an include struct ... end where it disables warnings 32 and 60. It will also generate a let _ = fun (_: t) -> () for each type in a set of type declarations which has a [@@deriving ...] attached to prevent those types as being reported as unused by the compiler.
This is an historical comfort feature that was added to avoid having to deal with those warnings that can appear as a result of the use of some derivers.
The feature is a bit too aggressive though and it can silence some meaningful warnings. Janestreet recently started an effort to remove unused generated code internally and to do so added a flag to the driver and an optional argument to the deriver registering functions to allow the driver not to automatically silence unused value/module warnings. If both the user (via the driver flag) and the deriver authors (via the optional argument) allow this, the driver does not silence warnings 32 and 60.
There also exists an older flag that turns off the warning 32 silencing, regardless of any deriver configuration but it is poorly scoped and also controls other things, such as the warning 34 silencing.
That summarizes where we are at right now.
Mathieu recently reported problems with the anti warning 34 feature messing his coverage reports, which lead us to question the feature entirely.
It seems that, same as with the other warnings, this would be better dealt with by individual derivers or by users directly. Silencing a warning shouldn't be done lightly and therefore always doing so by default is probably not the best behaviour here.
We considered what's the use case for this feature as it's unusual for a deriver's generated code not to consume or produce values of the type it is derived from. Mathieu pointed out that it could happen with structural types, such as polymorphic variants, that the compiler would consider the type itself unused if the generated code does not use coercion.
It seems that if the type is only used to be passed to a deriver, it should be used as a payload in an extension point rather than declared as an actual type, e.g., the deriver could also support the following:
[%%derive_x type t = [`A | `B]]Alternatively, the user could add a [@@warning "-34"] if they are explicitly using a type declaration simply as a deriver's input.
That being said, we cannot simply change that behaviour overnight and need to provide a transition scenario, similar to what's being done for the other two warnings. We discussed the following solutions:
- Disable coverage for the driver generated items
- Change the nature of the generated item so it interacts better with
bisect_ppx, e.g.let _ : t = Obj.magic () - Add a flag to disable the generation of this item
We agreed the last option is good since we will want to drop this behaviour in the long run and it's good enough for Mathieu's usecase and likely for a lot of users that don't need that feature with the set of derivers they use. It also allow use to provide a smooth transition scenario by adding an optional argument for derivers to opt in to this.
Adding such a flag will also allow us to gather a bit of data on why this feature was added in the first place and who is still relying on it.
We also discussed the possibility to allow a force value to be passed to those CLI options so that the features can be disabled regardless of the derivers being used and wheter they opted in or not.
This whole discussion also lead to the question of whether generated code should be taken into account by coverage tools. It's a broader topic that should also be discussed with bisect_ppx maintainers but the answer is not simple and depends on what one's using coverage for. If the coverage is used for testing, it's likely not the deriver's user responsibility to test the generated code and therefore, we probably don't want to include it in coverage reports. If the coverage is used to detect dead code that the compiler's missing, then it's a different matter. It's likely something that needs to be configured by users in the end but that means we'd need to allow such configuration.
Action-points:
- Mathieu: Implement the approach (via a new flag on the user side with 2 or 4 values) we've discussed and update his PR.
- Nathan/Mathieu: Try approach. Nathan also update his branch. ✔️
- Nathan: Poke Carl to find out where at Jane Street the silencing of the warning is needed needed (to decide)
We finally released the ppxlib ports for ppx_deriving's standard derivers and ppx_deriving_yojson.
We should post an anouncement on discuss as this is a historical moment and users will benefit from this. In particular this will fix the merlin bug with some of those derivers that were breaking the location invariant and won't anymore, providing a much better user experience. They can also now be used with [@@deriving_inline]!
The next steps here are to deprecate ppx_deriving.api. In the long run, being able to remove this part of ppx_deriving will allow us to drop ppx_derivers which will greatly simplify how we deal with [@@deriving ...] in ppxlib.
We still need to look into how and where it is still used, in particular there might be an issue with toplevels/ocamlfind and how ppx can de dynamically linked there. We also need to look into dune's and META files ppx_rewriter vs ppx_deriver distinction as it might be related to this.
This is not an urgent matter and the most important part of the work is done but at least we can start seriously looking into this now.
Action-points:
- Nathan to reach out to Simmo to see if he wants to do post the announce ✔️
- Nathan to post the announce otherwise. ✔️