Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assertion failed on mutual recursion with optional arguments #1001

Open
Bastacyclop opened this issue Sep 19, 2023 · 6 comments
Open

Assertion failed on mutual recursion with optional arguments #1001

Bastacyclop opened this issue Sep 19, 2023 · 6 comments

Comments

@Bastacyclop
Copy link

Minimal example and triggered assertion:

type t = int option

let rec f ?(optional : t) () = f ?optional ()
and g () = f ?optional:(Some 1) ()
> dune build @doc
odoc: internal error, uncaught exception:
      File "src/loader/cmi.ml", line 420, characters 21-27: Assertion failed
      Raised at Odoc_loader__Cmi.read_type_expr in file "src/loader/cmi.ml", line 420, characters 21-33
      Called from Odoc_loader__Cmt.read_pattern in file "src/loader/cmt.ml", line 40, characters 22-57
      Called from Odoc_loader__Cmt.read_value_bindings.(fun) in file "src/loader/cmt.ml", line 92, characters 18-50
      Called from Stdlib__List.fold_left in file "list.ml", line 121, characters 24-34
      Called from Odoc_loader__Cmt.read_value_bindings in file "src/loader/cmt.ml", line 86, characters 4-360
      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 121, 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
> dune --version
3.10.0
> odoc --version
2.2.1

Type annotations avoid hitting the assertion:

type t = int option

let rec f ?(optional : t) () = (f : ?optional:int -> unit -> unit) ?optional ()
and g () = (f : ?optional:int -> unit -> unit) ?optional:(Some 1) ()
@gpetiot
Copy link
Contributor

gpetiot commented Oct 16, 2023

I cannot reproduce this bug with the master (2.3.0+) version of odoc (same dune version), do you still observe it after upgrading odoc?

@jonludlam
Copy link
Member

Hmm, this is still failing on 2.4.1 for me.

@jonludlam
Copy link
Member

Slightly smaller test case:

type t = int option
let rec f ?(optional : t) () = f ?optional ()

So, this turns out to be quite interesting. The problem is that we're reading the type of the function, and have found an optional argument. We then ask for the representation of the type and expect to find a arg option so we can label the argument as optional with type arg:

let arg =
            if Btype.is_optional lbl then
              match Compat.get_desc (Compat.repr arg) with
              | Tconstr(_option, [arg], _) -> read_type_expr env arg
              | _ -> assert false
            else read_type_expr env arg

In this case, we're finding the constructor t which doesn't have any arguments, so the match fails and we hit the assert. I'm not quite sure what calling repr is supposed to do - it's a no-op from OCaml 4.14 onwards, but I tried the test case on 4.12 and it failed there too - also, since I happened to have a 4.02 switch around, I tried it on that and it failed there too.

In a further test, the following did not fail:

type t = int option

let rec f ?(optional : t) () = ()

@jonludlam
Copy link
Member

It'd be handy to have someone who knows how the compiler handles this for an opinion - @Octachron, would you be able to take a look and suggest how to handle this?

@Octachron
Copy link
Member

Octachron commented Feb 16, 2024

At first glance, the current code is too syntactic. The compiler version (which may also fail in corner cases(?)) looks like this:

let extract_option_type env ty =
  match get_desc (expand_head env ty) with
    Tconstr(path, [ty], _) when Path.same path Predef.path_option -> ty
  | _ -> assert false

The important difference is the expand_head env ty part before matching the type expression description which will expand type abbreviations before trying to compare the result to the built-in option type path.

@jonludlam
Copy link
Member

Hum, I don't have an Env.t at this point. This'll require something a bit different I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants