Fix pprintast with ppat_constraint in let bindings

[NathanReb]

We recently updated our internal copy of pprintast to reflect our internal bump from the 4.14 AST to the 5.2 one.

We used a strict copy of the compiler's 5.2 pprintast with a few minor modification:

• We added two extra entry points to pretty print type declaration and class signature which are exposed in our pprintast copy
• We made the code compatible with older compilers by using our custom function instead of Lexer.is_keyword

In 5.1, the AST changed so that type constraint in let bindings are attached to the whole value binding node rather than to the pattern part. That means that the following source code:

let f : int -> int = fun x -> x + 1

is now represented slightly differently, the int -> int type constraint is not embedded in the pattern as a

Ppat_constraint (Ppat_var "f", Ptyp_arrow ...)

but separately, in the pvb_constraint field, leaving the pattern to be a simple Ppat_var ....

In addition, the Ptyp_poly ... core type construction is only allowed in specific places in the AST, though it is not restricted to those places by the AST types. With the change in the 5.1 AST, it is not expected to be found in the pattern part of a value binding but in the pvb_constraint field.

As stated though, one can still build an AST node with the constraint embedded in the pattern. Trying to pretty print this with Ocaml 5.1 or above pprintast will produce code looking like this:

let (f : 'a . 'a -> unit) = fun _ -> ()

as opposed to the following form:

let f : 'a . 'a -> unit = fun _ -> ()

which is how those nodes were printed pre 5.1 and how the new nodes, embedding the type constraint in the pvb_constraint field are printed now.

It's important to note that the former is not and never was valid ocaml syntax and gets rejected by the parser:

File "test.ml", line 1, characters 12-13:
1 | let (f : 'a . 'a -> unit) = fun _ -> ()
                ^
Error: Syntax error: ) expected
File "test.ml", line 1, characters 4-5:
1 | let (f : 'a . 'a -> unit) = fun _ -> ()
        ^
  This ( might be unmatched

I think it's safe to assume that such constructions (Ptyp_poly in a Ppat_constraint rather than in the pvb_constraint field) are not supported by the compiler anymore and should be replaced in favor of the new representation.

That being said, it seems some part of the compiler are still able to process those correctly and ppx that generate such nodes seem to still function, as long as they don't rely on the unmigrated AST to printed as source code.

For that reason I propose this PR which adds custom support for this construction and prints it with the valid syntax. It's important to note that doing so, the source code won't parse back to it's original AST but I consider this okay as there is no way to produce source code that would parse to this construction anyway.

We will probably drop support for this in the long run as it's likely that the compiler itself will entirely reject those at some point as well.

I'm also considering changing Ast_builder.value_binding so that it detects Ppat_constraint in the pattern part and moves it to the pvb_constraint to avoid users the need to update their code.

Let me know what you think!

[NathanReb]

@patricoferris if you have some time to review this that would be appreciated, I don't want to oversee anything here!

@hhugo if you want to try this one out with gen_js_api it should fix the syntax error thing but I don't expect it will make the test suite pass as some other changes in pprintast seem to be responsible for the diffs.

[hhugo]

@patricoferris if you have some time to review this that would be appreciated, I don't want to oversee anything here!

@hhugo if you want to try this one out with gen_js_api it should fix the syntax error thing but I don't expect it will make the test suite pass as some other changes in pprintast seem to be responsible for the diffs.

I've opened LexiFi/gen_js_api#181 to better use the 5.2 ast

[patricoferris]

Thanks @NathanReb for this fix, not a very obvious one (at least to me)!

[Octachron]

That being said, it seems some part of the compiler are still able to process those correctly and ppx that generate such nodes seem to still function, as long as they don't rely on the unmigrated AST to printed as source code.

To be more precise, the compiler is still going internally through a translation let f: 'a. 'a -> 'a = ... to let (f:'a. 'a -> 'a) internally during typechecking. Thus it is currently fine to produce such AST. However, this is brittle because it is possible that in the future the parsetree will be changed to remove Ptyp_poly.