Raise an error when type nonrec is encountered

[emillon]

This feature has two different implementations in the AST:

• in 4.02.3, it is an annotation on a type,
• in >= 4.03.0 it is straight in the constructor.

Note that this can break existing programs; however relying on ppx_deriving to do the right thing in that case seems dangerous.
Maybe an escape hatch could be added so that the recursive version is derived nonetheless.

It is an intermediate solution to #116. Solving it is tricky because it would be necessary to expose this to the plugins, breaking the API. It is probably easier to do so once 4.02.3 support is dropped, since the per-type semantics are difficult to handle.

Let me know what you think about these changes.
Thanks!

[whitequark]

Solving it is tricky because it would be necessary to expose this to the plugins, breaking the API. It is probably easier to do so once 4.02.3 support is dropped, since the per-type semantics are difficult to handle.

What API do you propose?

[emillon]

I was thinking about extending the type of the type_decl_str argument (now options:(string * expression) list -> path:string list -> type_declaration list -> structure) so that it takes an extra recursive:bool argument.

Alternatively, maybe this could be encoded in the options list (with ppx_deriving adding something like ("ppx_deriving.recursive", [%expr true]).

So far I haven't tried to make a plugin nonrec-aware, so I'm not sure about the implications on that side.

[whitequark]

I was thinking about extending the type of the type_decl_str argument (now options:(string * expression) list -> path:string list -> type_declaration list -> structure) so that it takes an extra recursive:bool argument.

How about adding a ?recursive:bool? This should preserve source compatibility.

[emillon]

Absolutely, that would probably work too.

[whitequark]

@emillon poke? Any chance you could implement this?

[emillon]

Hi,

I had a look at this, and there are two challenges:

• it's a bit tricky to add an optional argument, because of polymorphism in functions such as Ppx_deriving.derive. It might be possible to use some partial application tricks which are not pretty, but it's probably worth preserving source compatibility as you said.
• there are visibility issues in some cases. At first I thought that the rec_flag for the value declaration should be the same as the type declaration, but that's not the case: for example in show, it seems that pp and show should be mutually recursive.

These do not seem to be show stoppers but this is a bit more involved than I initially thought.

[whitequark]

Thanks for the analysis. I would really appreciate any implementation, yours or by anyone else.

[gasche]

Regarding code generation, the scheme I would propose is to turn

let rec fn_foo = ...
and fn_bar = ...

into

let rec fn_foo_nonrec = ...
and fn_bar_nonrec = ...
let fn_foo = fn_foo_nonrec
and fn_bar = fn_bar_nonrec

or a variant of this that hides the _nonrec declaration from the global scope, such as

let fn_foo, fn_bar =
  let rec fn_foo_nonrec = ...
  and fn_bar_nonrec = ....
in fn_foo_nonrec, fn_bar_nonrec

Note in particular that occurrences of foo, bar in the declaration will lookup a fn_{foo,bar} in the environment, as expected, while mutual recursion of the two functions is still possible.

It is not immediate to implement this, and I think there is no solution that is both good and backward-compatible with the previous API. (One reason in particular is that currently it is the plugin's duty to generate the whole str-item, so this has to be changed plugin-wise and we need to pass them more information as @emillon suggests).
I would personally give up on backward-compatibility: plugins can easily use version bounds to advertise the version of ppx_deriving they support, allowing you to keep a clean API close to the current parsetree. One could encapsulate the logic of this code-generation scheme (which is not simple) in a Ppx_deriving function, so that user plugins can be updated by just calling this function in their type_decl_str component -- type_decl_sig does not change, as the external interface is not affected by nonrec.

[whitequark]

I agree on giving up backwards compatibility.