Make expect tests compiler-independent

[pitag-ha]

Using Pprintast from the compiler, as was done before, has the problem that the test output can differ on different compiler versions. See failing CI on #327.

[panglesd]

I'm only wondering, why ppxlib's Pprintast behaves like ocaml<4.14 and not ocaml 4.14?

What would happen if at some point, we want to update this function? (for instance, to behave like in 4.14, which seems a better behaviour (see ocaml/ocaml#10565).

In conclusion, I think that this PR should be merged, but independently I think we should have a story on when to update Pprintast.

[pitag-ha]

Thanks @panglesd!

I'm only wondering, why ppxlib's Pprintast behaves like ocaml<4.14 and not ocaml 4.14?

That's a very good point! And in fact, Pprintast does behave like OCaml 4.14 (I've updated it when bumping the AST to 4.14). So your observation that the #327 CI is failing precisely on 4.14 shows that the the first commit of this PR (the one you've reviewed) wouldn't fix the CI of #327. Still, I think that commit is an improvement, so I've left it.

I've also added a second commit now which should indeed fix the CI in #327. See commit message of that commit for more explanation. I admit that inlining print_out_value (i.e. copying a lot of code from the compiler) isn't a great approach, but I can only see three approaches to this problem:

• Ideally, the compiler would expose a reference to set the maximum length for string truncation, so that we would only need to set that one integer instead of copying all the code of print_out_value in order to have a stable print_out_value. However, that's not on us and even if the compiler devs agree, that would take a while.
• We can't write any expect tests in which an output string gets truncated. That doesn't sound like a viable solution.
• We inline print_out_value to make sure it stays stable, which is what I've done.

Let me know if you have an idea for a better solution and, if not, let me know what you think about this!

[panglesd]

I think we should only keep the first commit.

Promoting the test in #327 will fix the CI test for all versions of OCaml.
Let's always use the printer from the last version, it should be better than the previous one!

[pitag-ha]

Promoting the test in #327 will fix the CI test for all versions of OCaml.

Are you sure that that's the case if we only keep the first commit? I think that's the case if we only keep the second commit (although, also in that case, I'd keep the first commit, since the first commit is a simple improvement)

[panglesd]

Sorry for my previous comment, you are right it's the second commit that will make CI pass and that the first one is still good to keep!

However I have a suggestion: instead of inlining, why not modify the out_value so that the behaviour is the same as in OCaml 4.14?
I would say it is equally bad as your suggestion in terms of maintenance, but much less code :)

  (* To achieve 4.14 printing behaviour, see
     https://github.com/ocaml/ocaml/pull/10565 *)
  let rec map_tree = function
    | Outcometree.Oval_constr (name, params) ->
        Outcometree.Oval_constr (name, List.map ~f:map_tree params)
    | Oval_variant (name, Some param) ->
        Oval_variant (name, Some (map_tree param))
    | Oval_string (s, maxlen, kind) ->
        Oval_string (s, (if maxlen < 8 then 8 else maxlen), kind)
    | Oval_tuple tl -> Oval_tuple (List.map ~f:map_tree tl)
    | Oval_array tl -> Oval_array (List.map ~f:map_tree tl)
    | Oval_list tl -> Oval_list (List.map ~f:map_tree tl)
    | Oval_record fel ->
        Oval_record
          (List.map ~f:(fun (name, tree) -> (name, map_tree tree)) fel)
    | tree -> tree
  in
  let print_out_value = !Toploop.print_out_value in
  (Toploop.print_out_value :=
     fun ppf tree -> print_out_value ppf (map_tree tree));

[pitag-ha]

Ooh nice, I love that idea, @panglesd!

Side-note: I haven't checked carefully if all variant constructors are recursed correctly. If not, the worst case would be that the CI breaks again and it would be very easy to fix it.