Fix: Making pa_sexp_conv compatible with the record disambiguation feature in OCaml 4.01

[murmour]

Currently, pa_sexp_conv doesn't handle a case like this correctly:

type a = { fl: float } and b = { fl: int } with sexp

The provided fix involves generating additional type annotations. It's been used in production for 3 months with no problems whatsoever.

[murmour]

Jeremie Dimino

When I try to build the pa_sexp_test.ml with your patch I get this error:

File "lib_test/pa_sexp_test.ml", line 194, characters 13-18:
Error: This expression has type [> `True ]
       but an expression was expected of type v
       The second variant type does not allow tag(s) `True

Could you check your patch?

The patch is OK.

Compiling pa_sexp_test fails even before applying my patch because of a funny bug in Camlp4, which I also fixed recently: https://github.com/ocaml/camlp4/pull/1. It has nothing to do with record disambiguation.

By the way, it isn't integrated into the make test suite. I had no clue that it didn't compile.

[ghost]

By the way, it isn't integrated into the make test suite. I had no clue that it didn't compile.

Yes, sorry for that, we do build it internally but the public repositories are not setup to build tests (yet).

Compiling pa_sexp_test fails even before applying my patch

We are able to build it in our internal build, I'll have another look.

because of a funny bug in Camlp4, which I also fixed recently: ocaml/camlp4#1. It has nothing to do with record disambiguation.

Thanks for that, this hack of Camlp4 is an horrible mess.

[murmour]

We are able to build it in our internal build, I'll have another look.

Oh. My bad. It actually does typecheck successfully when there is no type annotation. Sorry :-) The bug was still there though, just invisible.

[ghost]

OK, so we need first the camlp4 fix before we can include this patch?

[murmour]

OK, so we need first the camlp4 fix before we can include this patch?

Yes. Which means we need to wait a couple of years until the next release of OCaml. ... Unless we temporarily modify the test case like this:

  (* todo: Underscores were added as a temporary workaround against a known Camlp4 bug *)
  type v = [ `_True | `_False of int ] with sexp
  let () = assert (Sexp.to_string (sexp_of_v `_True) = "True")
  let () = assert (Sexp.to_string (sexp_of_v (`_False 2)) = "(False 2)")

I think it's totally OK from the engineering point of view, as the generated code was broken anyway.

[ghost]

OK, I submitted the patch internally. I disabled this test as it indeed seems broken.