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

Fix import of signatures with mutually recursive types #35

Merged
merged 3 commits into from
Apr 27, 2019

Conversation

thierry-martinez
Copy link
Contributor

@thierry-martinez thierry-martinez commented Apr 26, 2019

Import of signatures with mutually recursive types was broken.

If a signature with mutually recursive types was defined in a.ml:

module type S = sig
  type t = A of u
  and u = B of t
end

then the compilation of the following compilation unit b.ml fails:

module type S = [%import: (module A.S)]

The reason is that the following pattern matching branch was broken in
two ways:

| (Sig_type (_, _, Trec_first) | _) :: _ when trec <> [] ->
  1. _ is a catch all, so it matches in particular Sig_type with
    rec_flag other than Trec_first, i.e. the left hand side branch
    of pattern disjunction is useless;

  2. trec should be appended to the result in the final case of the
    empty list (tsig = []).

Import of signatures with mutually recursive types was broken.

If a signature with mutually recursive types was defined in `a.ml`:

```
module type S = sig
  type t = A of u
  and u = B of t
end
```

then the compilation of the following compilation unit `b.ml` fails:

```
module type S = [%import: (module A.S)]
```

The reason is that the following pattern matching branch was broken in
two ways:

```
| (Sig_type (_, _, Trec_first) | _) :: _ when trec <> [] ->
```

(1) `_` is a catch all, so it matches in particular `Sig_type` with
    rec_flag other than Trec_first, i.e. the left hand side branch
    of pattern disjunction is useless;

(2) trec should be appended to the result in the final case of the
    empty list (tsig = []).
@ejgallego
Copy link
Collaborator

Should a test-case be added?

@whitequark
Copy link
Contributor

Ideally, yes.

@gasche
Copy link
Contributor

gasche commented Apr 26, 2019

This is a very nice catch, but I would like the new code to be more readable than your proposal. Before reading your patch, I had no idea that | _ -> true -> was valid OCaml code. Some ideas to make things more readable:

  1. Avoid when, which is generally a tool to make code less readable.
  2. More ambitious change, have a recursive function psig_of_type_block or psig_of_type_next dedicated to parsing recursive blocks of type declarations, mutually-recursive with psig_of_tsig. This lets you remove the optional paramteer of psig_of_tsig, which also contributes in making this code more complex than it could be.

@thierry-martinez
Copy link
Contributor Author

@ejgallego Test-case added, thanks!

@gasche Thanks for your suggestion! Indeed, it could be better written: I was misguided by the idea of making as little change as possible. I did not follow your suggestion of two mutually recursive functions as I did not know how not to repeat the call to ptype_decl_of_ttype_decl twice (one time for Trec_first and another time for the other items). I think that my proposal of gathering mutually recursive definitions before transforming them is quite natural at least.

Copy link
Contributor

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your new implementation is fine, thanks!

@gasche gasche merged commit 4596f50 into ocaml-ppx:master Apr 27, 2019
kit-ty-kate pushed a commit to ocaml/opam-repository that referenced this pull request May 1, 2019
CHANGES:

* Fix import of signatures with mutually recursive types
    (Thierry Martinez ocaml-ppx/ppx_import#35, review and tweaks by Gabriel Scherer)
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

Successfully merging this pull request may close these issues.

4 participants