Versioned API

[ghost]

Following the discussion in https://github.com/whitequark/ppx_deriving_yojson/pull/49, I'd like to look a adding some kind of versioned API to ppx_deriving. But I'd like to discuss the final picture before starting.

Current state

At the tip of the master branch, Ppx_deriving is using the 4.05 Ast from ocaml-migrate-parsetree explicitly. This means that it breaks all packages using ppx_deriving, except on 4.05. I guess this is a bit abrupt?

Proposal

Initial setup

We restore the Ppx_deriving as it was before #123, i.e. using the Ast from the compiler with some cppo to keep it working with several versions of the compiler. However, we deprecate it. We also keep the registration with the omp driver.

The Ppx_deriving module as it is at the tip of master is copied to Ppx_deriving_405 and distributed as ppx_deriving.405. At the same time, we take the opportunity to upgrade the API:

• add a rec_flag argument to the type_decl_{str,sig} callbacks
• replace string by string Location.loc in various places where a location was added

Gluing thing together

At this point we have two independent rewriters interpreting [@@deriving] and [%derive.foo]. To solve this problem, we can reuse the idea of register_external: we add a module Ppx_deriving_attribute whose only function is to register the various derivers implemented by the various passes. Each pass will ignore the derivers that are interpreted by another pass.

Upgrade story

In order to upgrade the version of the AST used by ppx_deriving, for instance from 4.05 to 4.07, one proceeds as follow:

• copy Ppx_deriving_405 to Ppx_deriving_407
• deprecate Ppx_deriving_405
• upgrade Ppx_deriving_407

Rationale

It the end it is better if ppx rewriters follow new versions of the Ast, since omp won't support all versions forever. With this plan, people have a window of time in which they can upgrade their code, without having to upgrade all ppx rewriters once.

Tweak

If we do exactly as described, it seems that we'll regularly loose the history of the Ppx_deriving_XXX module since there is no git cp. To workaround this, we can keep the latest version of the code in Ppx_deriving_latest and then setup a rule to copy it to Ppx_deriving_XXX or use includePpx_deriving_latestinPpx_deriving_XXX`.

[whitequark]

We restore the Ppx_deriving as it was before #123, i.e. using the Ast from the compiler with some cppo to keep it working with several versions of the compiler. However, we deprecate it. We also keep the registration with the omp driver.

We could just bump the major version. Given that we should not need to bump the major version afterwards hopefully for a long time, that seems fine.

The Ppx_deriving module as it is at the tip of master is copied to Ppx_deriving_405 and distributed as ppx_deriving.405.

I'm unhappy about it. Can we have exactly one copy of the Ppx_deriving logic and perform the AST conversion to/from whatever the specific deriver wants? It's not the entire AST of the file but just select type declarations, so that should not pose a performance issue. And it seems like this way the possibility of the copies diverging is much lower.

If we do exactly as described, it seems that we'll regularly loose the history of the Ppx_deriving_XXX module since there is no git cp.

I think git should be able to track this by itself (maybe it needs a little help via -C).

[ghost]

We could just bump the major version. Given that we should not need to bump the major version afterwards hopefully for a long time, that seems fine.

Perfect, this does make things simpler. Do you agree that Ppx_deriving should be renamed Ppx_deriving_405 thought? If not, the upgrade story has to be different.

I'm unhappy about it. Can we have exactly one copy of the Ppx_deriving logic and perform the AST conversion to/from whatever the specific deriver wants? It's not the entire AST of the file but just select type declarations, so that should not pose a performance issue. And it seems like this way the possibility of the copies diverging is much lower.

We certainly can, that's what ppx_type_conv is doing at the moment to interoperate with ppx_deriving. However it does make the code more complicated. In practice, ppx_deriving plugins will have to use one of the AST versions supported by ppx_deriving if they want to use the helpers, which is likely. With the solution I suggest, you have two passes to maintain with one being deprecated, so it doesn't seem too bad to me.

Moreover, it's not clear that having two independent passes will be slower on average. Depending on the input file, the current version of OCaml and the ppx rewriters that are linked in, it might be slower or faster.

In any case it's going to be more refactoring to setup a single pass. I'm happy to help setting things up so that we have the new APIs in place, but I'll leave the question of a better implementation to someone else.

BTW, this is a different request, but moving forward such a Ppx_deriving_attribute is all ppx_type_conv would need. Would you be happy to replace Ppx_deriving.register_external by this module as a separate package? This would simplify ppx_type_conv since I could replace the optional dependency on ppx_deriving by a hard dependency on ppx_deriving_attribute. This would also improve the experience for users since they wouldn't have to recompile packages if they install ppx_deriving after ppx_type_conv.

I think git should be able to track this by itself (maybe it needs a little help via -C).

Cool, I didn't know about this option

[whitequark]

Do you agree that Ppx_deriving should be renamed Ppx_deriving_405 thought? If not, the upgrade story has to be different.

If we go ahead with two passes, then yes. Since you do not want to refactor it into one pass and I probably cannot commit to it right now either, that's what we should use, I think. The documentation will be messy. (Also, I think it should be called Ppx_deriving_4_05, but that's minor.)

Would you be happy to replace Ppx_deriving.register_external by this module as a separate package?

Absolutely. Do I understand it right that this module would:

• let various packages register themselves as handlers of [@@deriving];
• pass the native AST to the registered handlers;
• include the received native AST back after/instead of the type definition;
• check that every invoked deriver is present and has ran.

In that case, wouldn't ppx_deriving itself not need two passes anymore, but would merely forward every deriver registered through it to ppx_deriving_attribute with the necessary AST back- and forward-conversion?

[ghost]

If we go ahead with two passes, then yes. Since you do not want to refactor it into one pass and I probably cannot commit to it right now either, that's what we should use, I think. The documentation will be messy. (Also, I think it should be called Ppx_deriving_4_05, but that's minor.)

I would have thought the two choices are independent. What would you do differently in terms of API?

Absolutely. Do I understand it right that this module would:

I was thinking of something much simpler:

type tool = ..

type deriver =
  { tool : tool
  ; name : string
  }

val register : deriver -> unit
val lookup : string -> deriver option

then in Ppx_deriving_4_05 we'd have:

type Ppx_deriving_attribute.tool += Ppx_deriving_4_05

let register d =
  Ppx_deriving_attribute.register { tool = Ppx_deriving_4_05; name = d.name };
  ...

let lookup_internal_or_external name =
  try Some (Internal (Hashtbl.find registry name))
  with Not_found ->
    match Ppx_deriving_attribute.lookup name with
    | Some d -> Some (External d.name)
    | None -> None

[whitequark]

Then I don't understand how is this sufficient...

[ghost]

Well, each pass ignores the derivers that are not register for this pass

[whitequark]

Glossing over some other details that I think are troublesome, this still means that leftover derivers will be ignored. This is unacceptable, and ppx_deriving was specifically designed in a way that does not let this happen.

[whitequark]

(The reason this is unacceptable is, of course, you can use [@@deriving] in a .mli, and if there's some sort of build system mishap, you could have your public interface silently changed, and undetected for a long time, perhaps.)

[ghost]

No. This is exactly the same as the current Ppx_deriving.register_external and no derivers are ignored. Technically they'll be ignored by a single pass, but if they are registered by another one, you'll know that they'll be interpreted by another pass. Each pass will still fail on every completely unknown deriver.

[whitequark]

Okay, I'll need to spend some more time looking into this design.

[ghost]

Btw, a much simpler solution is to just break the compatibility every time ppx_deriving upgrades the version of the AST it's based on. That's basically the current story for ppx_type_conv.

Then there is nothing to change right now, just upgrade a bit the API to follow the evolution of the 4.05 parsetree.

[whitequark]

@diml We've already considered and rejected this option back when 4.03 came out.

[ghost]

BTW, we have similar problems with ppx_core regarding versionning. Ppx_core uses one specific version of the AST (current 4.03) and when we'll upgrade this version this will break the code for users of ppx_core. Providing a versionned API for ppx_core would be too much work since it's quite big.

I had another idea recently for a ppx that would make it easy to use pattern matching on abstract types and wrote some experiments here.

That could be an alternative to versioning the various ppx libraries (ppx_tools, ppx_deriving, ppx_core, ...). I believe it would provide a much better experience for users in the end.

[whitequark]

@diml This is an excellent idea. Please keep me updated.

I'm not quite sure about integrating the other API-breaking changes in ppx_deriving now though, since breaking the API in a major way twice seems very troublesome...

[ghost]

Please keep me updated.

Sure

I'm not quite sure about integrating the other API-breaking changes in ppx_deriving now though, since breaking the API in a major way twice seems very troublesome...

Makes sense

[ghost]

Just to follow up on this, @xclerc has been working on ppx_view_pattern. The project is in a good state and it has the dual of Ast_helper as view patterns, which makes it easy to use if you already know Ast_helper. We should be able to start our internal code review/release process soon and the project should be on github in a few weeks.