Skip to content

Dev meeting 19 02 2024

Jump to bottom
Sonja Heinze edited this page Feb 21, 2024 · 1 revision

Agenda

  • 5.2 support
    • The complicated compiler AST changes for 5.2. Among others, let's discuss if we're testing their support.
      • Syntactic support for functions with arity > 1
      • Others?
      • The compiler ocaml.ppx.context change
        • This affects the PPX world iff people want to build their project with a different OCaml version than their PPX is built with. Do we want to keep on supporting that workflow?
        • Why was the change of ocaml.ppx.context in the compiler done in a breaking way? Is it worth for us to discuss that with them?
      • The ppxlib ~preview release with 5.2 support
  • Issue on ppx_deriving plug-in
  • Upcoming things to do on ppxlib
    • When to start with the work to bump ppxlib's AST to 5.2?
    • When to start with the upstream Astlib efforts?
  • If we have time, let's also discuss how to improve the repo for people to get involved easier (issue labels, contribution guidelines etc)

Attendees

  • Nathan / @NathanReb
  • Paul-Elliot / @panglesd
  • Florian / @Octachron
  • Shon / @shonfeder
  • Sonja / @pitag-ha
  • Carl

Notes

Independently, both Florian and Shon have joined the meeting 🎉

5.2 support

Kate / @kit-ty-kate and Florian / @Octachron have found a bug coming up when building projects with 5.2~alpha that seems clearly PPX related: An unexpected type error on some projects with PPXs.

We're wondering if the bug appears totally independently from the PPX, and also if it appears totally independent from the ppx pipeline (staged_pps vs pps).

One guess is that the bug is related to the ocaml.ppx.context change. However, the question is how it would be related to that change, given that in the test situation the PPX is built with the same version the project is built with.

Action-point Florian and other compiler folks: Pinpoint this further.

Action-point Nathan: Find out what's happening on the ppxlib side.

Non AST-related changes that can affect PPXs

Florian has pointed out that there are two changes in 5.2 that don't affect ppxlib directly, but can be relevant in the PPX context (thanks for giving us that context!):

  1. Improvement of "unused attributes" warning coming from the compiler.
  2. From 5.2 on, it's possible to use reserved keywords such as "if" as an identifier. This could affect the PPX world if a keyword is overriden in the scope a PPX using that keyword is used in. Very unlikely, but good to know!

Relevant AST changes

Are there changes that add syntax and change the parsetree? - No. So: Should we bump the AST at all this time?

In contra: Usually, the reason to bump the AST is to allow people to use the new compiler syntax in projects with PPXs. No new syntax -> no reason to bump the AST. We're expecting this AST bump to break a lot of PPXs. We shouldn't break them for "nothing". Also, Florian is poitning out that 5.3 will come with syntax support for effects, so for 5.3, we'll bump the AST for sure.

In favor: Change of perspective: As pointed out, bumping the AST won't have a benefit for PPX users. However, it would have a benefit for PPX authors: The new function arity representation makes it a lot easier to handle function nodes. Side-note: We could also improve Ast_builder to facilitate manipulating function nodes.

Conclusion: Not sure yet, but probably we again won't bump the AST this time.

Arity change

Side-note for the future: For the arity change, we expect lot's of PPXs to break once we bump the AST.

For now, let's focus on the migrations. For certain functions with GADTs, the migrations might be wrong: If we start from 5.1 and go to 5.2, we might be grouping too many function paramters.

Let's write a test for

let ex2 : type a . (a, unit -> unit) eqtest -> a = fun Equal () -> ()

Thanks, Florian for pointing this out and coming up with the example!

We're expecting the round-trip 5.2 -> 5.1 -> 5.2 to work, but the other one might be wrong (we're just guessing).

Action-point Nathan: Write this test and have a look.

New location PRs

We're talking about

  • new locations on RHS of Ptyp_alias
  • New parameter location information to parsetree/typedtree

We don't remember atm if we did those migrations correctly. Possibly, we're just adding a none location. That'd be pretty bad once we bump the AST: From then on, a PPX would remove the added locations.

Action-point Nathan: Have a look and potentially open an issue to at least not forget.

Change on ocaml.ppx.context

Our guess: The compiler folks just didn't notice that that change would affect PPX. Florian: One of the reasons we have the compiler alpha release is to communicate those problems 🎉 Side-note: The change is only a porblem if someone builds the PPX with a different compiler version than the project. We used to support that for bucklescript. Nowadays, we don't know of any JS compilers or anyone else who needs that workdlow. Nathan: Migrations for the ppx context change are simple and almost written, so we'll keep on supporting that workflow for now.

The ppxlib ~preview release with 5.2 support

The 0.32.1~5.2preview is a preview release for ppxlib comptibility with OCaml 5.2. It doesn't have a dedicated GH release. The opam release uses the tarball from the last commit on the trunk-support branch we've used (every commit on GH has a tarball!) (thanks, Kate, for coming up with this preview release workflow!).

Issue on the ppx_deriving standard plug-in make

Shon has asked about a bug report about one of the ppx_deriving standard plug-ins (about make). Bug when using the deriver on a recursive type: https://github.com/ocaml-ppx/ppx_deriving/issues/272

We haven't looked at it, yet. Standard plug-in situation is quite bad. We'd have loved to make a good and hygienic set of standard derivers directly based on ppxlib. We didn't have the time. Now, we should at least maintain the "good" old ppx_deriving ones! Very important: review Simmo's PR on porting those plug-ins directly to the ppxlib API (ppx_derving already registers everything to the ppxlib driver anyways, but ppxlib API directly a lot better)! We guess that should even directly fix the bug we're talking about!

Action-point Shon: Check out Simmo's branch that ports the plug-ins to the ppxlib API and report back if that fixes the problem.

Action-point Nathan: Review Simmo's PR.

Shon: Also available for other small pieces of help 🎉 E.g. one idea: More utility functions for Ast_builder. Ideally directly with API documentation for the new functions.

We appreciate every help we can get from external contributors! 🙏

Clone this wiki locally