Fix for handle differences in sig_class for OCaml 5.1 vs OCaml 4.14

[tmcgilchrist]

Fixes the issue reported as #1007

4979052 is a direct fix for the exception
b4f11fc pins the version of odoc-parser as per meta-data changes in opam-repository see https://github.com/ocaml/opam-repository/blob/master/packages/odoc/odoc.2.2.1/opam

cc @jonludlam I can cherry pick 4979052 onto 2.3.0. For ocaml-docs-ci we could use a new release of 2.2.x and 2.3.x.

[Octachron]

You could restrict the ifdef to the pattern rather than duplicate the code, since we only need to ignore ghost items.

[tmcgilchrist]

I've changed this to just ifdef the pattern.

[Octachron]

I forgot about this iterator on signature items when updating for 5.1, sorry about this.

The compiler has a specialized iterator on visible signature items with the aim to avoid similar issues, I am not sure if would make to vendor this iterator in odoc?

[jonludlam]

@Octachron we need to keep track of where in the structure we are, and I think that's quite hard with the iterators, isn't it?

[tmcgilchrist]

After more testing I found an issue with this change against timmy.1.0.4 using OCaml.5.1.0.

odoc: internal error, uncaught exception:
      File "src/loader/cmi.cppo.ml", line 939, characters 21-27: Assertion failed
      Raised at Odoc_loader__Cmi.read_module_type in file "src/loader/cmi.cppo.ml", line 939, characters 21-33
      Called from Odoc_loader__Cmt.read_module_expr in file "src/loader/cmt.ml", line 371, characters 18-85
      Called from Odoc_loader__Cmt.read_module_expr_maybe_canonical in file "src/loader/cmt.ml", line 414, characters 10-53
      Called from Odoc_loader__Cmt.read_module_binding in file "src/loader/cmt.ml", line 435, characters 10-81
      Called from Odoc_loader__Cmt.read_structure_item in file "src/loader/cmt.ml", line 501, characters 14-47
      Called from Odoc_loader__Cmt.read_structure.(fun) in file "src/loader/cmt.ml", line 583, characters 24-61
      Called from Stdlib__List.fold_left in file "list.ml", line 123, characters 24-34
      Called from Odoc_loader__Cmt.read_structure in file "src/loader/cmt.ml", line 581, characters 4-127
      Called from Odoc_loader__Cmt.read_implementation in file "src/loader/cmt.ml", line 596, characters 4-74
      Called from Odoc_loader.read_cmt in file "src/loader/odoc_loader.ml", line 145, characters 34-74
      Called from Odoc_loader.wrap_errors.(fun) in file "src/loader/odoc_loader.ml", line 164, characters 10-14
      Called from Odoc_model__Error.catch in file "src/model/error.ml", line 54, characters 21-27
      Called from Odoc_model__Error.catch_warnings.(fun) in file "src/model/error.ml", line 89, characters 18-22
      Called from Odoc_model__Error.with_ref in file "src/model/error.ml", line 67, characters 12-16
      Re-raised at Odoc_model__Error.with_ref in file "src/model/error.ml", line 72, characters 4-11
      Called from Odoc_odoc__Compile.resolve_and_substitute in file "src/odoc/compile.ml", line 87, characters 4-31
      Called from Odoc_model__Error.catch in file "src/model/error.ml", line 54, characters 21-27
      Called from Odoc_model__Error.catch_warnings.(fun) in file "src/model/error.ml", line 89, characters 18-22
      Called from Odoc_model__Error.with_ref in file "src/model/error.ml", line 67, characters 12-16
      Re-raised at Odoc_model__Error.with_ref in file "src/model/error.ml", line 72, characters 4-11
      Called from Odoc_odoc__Compile.compile.(fun) in file "src/odoc/compile.ml", line 239, characters 6-136
      Called from Cmdliner_term.app.(fun) in file "cmdliner_term.ml", line 24, characters 19-24
      Called from Cmdliner_term.app.(fun) in file "cmdliner_term.ml", line 22, characters 12-19
      Called from Cmdliner_eval.run_parser in file "cmdliner_eval.ml", line 34, characters 37-44

https://github.com/tmcgilchrist/odoc/blob/ocaml_5_1_fix/src/loader/cmi.cppo.ml#L939 inside the read_module_type function. The match statement in that function looks the same between 2.2 branch and master. Are there changes missing for type module_type in compat.cppo.ml ?

Happily nothing else has failed with the same error in ocaml-docs-ci :-)

[panglesd]

Thanks!

We need this change in the other branches (2.3 and master) and to cut patch releases.

But, cmi.ml was already rewritten by cppo, so I would prefer not to rename it, I think it is easier to inspect the git history when there is less file renaming. What do you think @jonludlam, since you merged this PR?

[tmcgilchrist]

@panglesd shall I prep a PR for the other branches this change need to exist on?
A patch release for 2.2 and 2.3 would be very welcome.

[panglesd]

Thanks @tmcgilchrist ! I was going to open the PRs myself, since I also want to investigate the timmy failure, and hopefully fix it at the same time.

[panglesd]

It seems that the following command triggers the timmy failure:

$ (cd _build/default/timmy/lib/.timmy.objs/byte && /home/panglesd/.opam/5.1.0/bin/odoc compile -I . -I ../../../../_doc/_odoc/pkg/timmy --pkg timmy -o timmy__Type_schema.odoc timmy__Type_schema.cmt)
odoc: internal error, uncaught exception:
      File "src/loader/cmi.ml", line 941, characters 21-27: Assertion failed
      Raised at Odoc_loader__Cmi.read_module_type in file "src/loader/cmi.ml", line 941, characters 21-33
      Called from Odoc_loader__Cmt.read_module_expr in file "src/loader/cmt.ml", line 380, characters 18-85
...

[panglesd]

Here is a minimal repro:

(* file a.ml *)
module M = struct end

(* file b.ml *)
[@@@warning "-unused-value-declaration"]
module M (Make : sig
  val v : string
end) =
  A.M

Note that the following version does not raise:

(* file b.ml *)
[@@@warning "-unused-value-declaration"]

module A = struct module M = struct end end

module M (Make : sig
  val v : string
end) =
  A.M

which is very strange.

I compared the typedtree for the two versions and, keeping only the relevant part, and, here is the result:

Failing version:

  structure_item
    Tstr_module
    M/275
      module_expr
        Tmod_functor "Make/272"
        module_type
          Tmty_signature
          [
            signature_item
              Tsig_value
              value_description v/271
                core_type
                  Ttyp_constr "string/15!"
                  []
                []
          ]
        module_expr
          module_expr
            Tmod_ident "Test_moduletype_alias!.A.M"

Passing version

  structure_item
    Tstr_module
    M/275
      module_expr
        Tmod_functor "Make/274"
        module_type
          Tmty_signature
          [
            signature_item
              Tsig_value
              value_description v/273
                core_type
                  Ttyp_constr "string/15!"
                  []
                []
          ]
        module_expr
          Tmod_ident "A/272.M"

Notice the two nesting of module_expr in the raising version. This is very strange to me...
Since I don't understand, I see two possibilities:

1. The assertion was a valid one in some previous version (or, never) but not anymore in 5.1.
2. The assertion should still be valid in 5.1, but this is a regression introduced in 5.1 or some previous version

@Octachron do you have any idea? I tend to think it's 1.

[panglesd]

Ok, duplicate of #905 where @Julow already has a minimal repro.

Also, not linked to OCaml 5.1 vs 4.14, I though the reported error was new in 5.1.

[tmcgilchrist]

@panglesd How did you get the tree structure in #1018 (comment)? That would be helpful to diagnose the issues in future.

[panglesd]

I used the -dtypedtree flag of ocaml or ocamlc to get the typedtree output! I also removed the location information which take quite a lot of space.

The ocaml toplevel is not so user-friendly, and utop has no -dtypedtree, but you can use utop-full and type:

Clflags.dump_typedtree := true ;;

to get the typedtree output!

[hhugo]

Can we have a new release with this change ?

[tmcgilchrist]

A release for 2.2 series would be appreciated. That would cross off a few more failing package for ocaml-docs-ci.

[Julow]

This has been released in 2.2.2 and 2.3.1.