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

Private extensible variants #1253

Merged
merged 2 commits into from Sep 11, 2017
Merged

Conversation

lpw25
Copy link
Contributor

@lpw25 lpw25 commented Jul 20, 2017

Currently, in signatures, extensible variant constructors can be declared for types of abstract kind:

module type S = sig
  type t
  type t += Foo
end

This is a useful feature allowing constructors to be created by functors without exposing the underlying extensiblity. See the msg.ml example in the testsuite for a nice example of this. However, it also allows nonsensical signatures to be created:

module type S = sig
  type int += A_new_number
end

This doesn't cause any soundness issues but it does cause difficulties for the compiler. For example:

# module F (X : sig type int += A_new_number end) = struct
    let f = function
      | X.A_new_number | 0 -> true
      | _ -> false
  end;;
Fatal error: exception File "typing/parmatch.ml", line 109, characters 6-12: Assertion failed

It also makes changes like #792 unnecessarily difficult. In general it is quite awkward to support having constructors available for arbitrary types.

This PR adds a notion of "private" extensible variant type:

module type S = sig
  type t = private ..
  type t += Foo
end

which are visibly extensible but cannot actually be extended. This allows us to make it illegal to extend a type which is not known to be extensible:

module type S = sig
  type t
  type t += Foo
end;;
      Characters 31-44:
    type t += Foo
    ^^^^^^^^^^^^^
Error: Type definition t is not extensible

and so avoid the various problems described above whilst still allowing the use cases previously supported with extensions to abstract types.

This change is not backwards compatible, however extending abstract types is a little used and undocumented feature so I doubt that there will be much disruption.

Copy link
Contributor

@trefis trefis left a comment

Choose a reason for hiding this comment

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

Missing a Changes entry but apart from that looks good to me.

Copy link
Contributor

@alainfrisch alainfrisch left a comment

Choose a reason for hiding this comment

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

This is a very good move. I think it was confusing to allow adding constructor to abstract types in signatures. Moreover, the proposed extension reuse an existing "impossible" form in the Parsetree, i.e. removes one invariant for a Parsetree to represent a parsable program.

I've reviewed the code and it looks ok.

Please add the new form to testsuite/tests/parsetree/source.ml (to check roundtrip between concrete and abstract syntax), and a Changelog entry.

@gasche
Copy link
Member

gasche commented Aug 21, 2017

This work needs to be merged. @lpw25, would you include a Changes entry?

@lpw25
Copy link
Contributor Author

lpw25 commented Sep 11, 2017

Rebased and added changes entry. Good to merge once tests pass.

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

Successfully merging this pull request may close these issues.

None yet

4 participants