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

Do not allow recursive modules in `with module` #1626

Merged
merged 2 commits into from Aug 2, 2018

Conversation

Projects
None yet
4 participants
@lpw25
Copy link
Contributor

commented Feb 21, 2018

Currently, recursive modules can be referred to in with module within their own signatures. This leads to some disconcerting module types:

# module type S = sig module M : sig end end;;
module type S = sig module M : sig  end end

# module rec M : S with module M = M = M;;
module rec M :
  sig module M : sig module M : sig module M : sig  end end end end

# module rec M : sig
    module N : S with module M = M
    module A : module type of N
    module B : module type of A
  end = M;;
        module rec M :
  sig
    module N :
      sig
        module M :
          sig
            module N :
              sig
                module M :
                  sig
                    module N : sig module M : sig  end end
                    module A : sig module M : sig  end end
                    module B : sig module M : sig  end end
                  end
              end
            module A :
              sig
                module M :
                  sig
                    module N : sig module M : sig  end end
                    module A : sig module M : sig  end end
                    module B : sig module M : sig  end end
                  end
              end
            module B :
              sig
                module M :
                  sig
                    module N : sig module M : sig  end end
                    module A : sig module M : sig  end end
                    module B : sig module M : sig  end end
                  end
              end
          end
      end
    module A :
      sig
        module M :
          sig
            module N :
              sig
                module M :
                  sig
                    module N : sig module M : sig  end end
                    module A : sig module M : sig  end end
                    module B : sig module M : sig  end end
                  end
              end
            module A :
              sig
                module M :
                  sig
                    module N : sig module M : sig  end end
                    module A : sig module M : sig  end end
                    module B : sig module M : sig  end end
                  end
              end
            module B :
              sig
                module M :
                  sig
                    module N : sig module M : sig  end end
                    module A : sig module M : sig  end end
                    module B : sig module M : sig  end end
                  end
              end
          end
      end
    module B :
      sig
        module M :
          sig
            module N :
              sig
                module M :
                  sig
                    module N : sig module M : sig  end end
                    module A : sig module M : sig  end end
                    module B : sig module M : sig  end end
                  end
              end
            module A :
              sig
                module M :
                  sig
                    module N : sig module M : sig  end end
                    module A : sig module M : sig  end end
                    module B : sig module M : sig  end end
                  end
              end
            module B :
              sig
                module M :
                  sig
                    module N : sig module M : sig  end end
                    module A : sig module M : sig  end end
                    module B : sig module M : sig  end end
                  end
              end
          end
      end
  end

including some which can cause the compiler real difficulty:

# module type S = sig module M : sig end module N = M end;;
module type S = sig module M : sig  end module N = M end
# module rec M : S with module M := M = M;;
module rec M : sig module N = M end
# module type T = module type of M;;
Fatal error: exception Stack overflow

Process ocaml-toplevel exited abnormally with code 2

Such module types would normally be rejected, but they are not caught when constructed using with module because approx_modtype ignores the right-hand side of with module. This patch adjusts this behavior to check that the right-hand sides of with modules are not recursive modules.

@Drup

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

The way you check for this is slightly disconcerting. I suppose you intend to provide a real error message instead?

In general, wouldn't be better to extend approx_modtype to be aware of constraints? The test case you removed is safe (even if a bit contrived).

@lpw25

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

I suppose you intend to provide a real error message instead?

I'm not 100% what you mean. If you're asking if I'm going to change the code to improve the error message, then no probably not. It's the same message you get for any similar error, and fixing the terrible error messages in this part of the compiler is orthogonal to fixing this bug.

In general, wouldn't be better to extend approx_modtype to be aware of constraints

You could possibly make it aware of module constraints, although I'm not totally sure that wouldn't introduce spurious errors.

You can't make it aware of type constraints because you cannot yet check the type expressions.

@damiendoligez damiendoligez added the bug label May 18, 2018

@lpw25 lpw25 force-pushed the lpw25:no-recursive-with branch from e091bd9 to aadfff6 Jul 25, 2018

@lpw25

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

I've rebased and added a Changes entry. This still needs review/approval. @Drup or @garrigue I think one of you is probably called for.

@garrigue
Copy link
Contributor

left a comment

I don't see any test (only a removed one).
Or is there a test somewhere else?

I also do not understand why just checking for the presence of a module by that name helps. Couldn't there be an unrelated one in env?

@lpw25 lpw25 force-pushed the lpw25:no-recursive-with branch from aadfff6 to 2c30528 Aug 1, 2018

@lpw25

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2018

Thanks for the review. I've added a new test, and a comment to the code since it is a bit opaque.

We're just doing the same environment lookup that will be done later when creating the real (non-approximate) module type. When approximating module types the environment contains dummy bindings for all recursive modules, which will cause an error if you attempt to use them. This is the same mechanism which causes:

# module rec M : sig module N = M end = M;;
Characters 30-31:
  module rec M : sig module N = M end = M;;
                                ^
Error: Illegal recursive module reference

which I think is completely analogous to this case, and is why I am checking it this way.

@lpw25 lpw25 merged commit a421da0 into ocaml:trunk Aug 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.