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

Better error message for private constructors of extensible types #8579

Merged
merged 1 commit into from Apr 4, 2019

Conversation

Gbury
Copy link
Contributor

@Gbury Gbury commented Apr 3, 2019

Currently:

# type exn += private Foobar
let _ = raise Foobar;;
Error: Cannot create values of the private type exn

Which is surprising given that the exn type is not private.

From what I understand, there is a difference between a regular sum type begin private, which means that all its constructors are private and therefore, no value of this type can be constructed (which corresponds to the current error message), and an extensible variant type, where each constructor can be private or not, and where the type being private means that no constructors can be added to the type (but not that all constructors are private, contrary to the regular variant case).

This PR adapts the error message in cases of private constructors of extensible variant types to be:
Cannot use private constructor Foobar to create values of the extensible variant type exn which seems more correct.

Copy link
Member

@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.

I agree this is a good idea in principle and the implementation looks good overall. See minor comments.

typing/typecore.ml Outdated Show resolved Hide resolved
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.

This seems like a fine change. But I'd like to see the toplevel tests be replaced by expect tests.

testsuite/tests/typing-extensions/open_types.ml Outdated Show resolved Hide resolved
@trefis
Copy link
Contributor

trefis commented Apr 3, 2019

Oh no! @gasche was faster.

Also:
oh no

typing/typecore.mli Outdated Show resolved Hide resolved
@Gbury
Copy link
Contributor Author

Gbury commented Apr 3, 2019

All review taken into account and Changes added.

@Gbury
Copy link
Contributor Author

Gbury commented Apr 3, 2019

Rebase and squashed.

Copy link
Member

@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.

Good to go if CI passes. Thanks!

(For the record, I'm not fond of "many fine eyes" as a cop-out to writing actual reviewer's names, except in the case where the list would get unreasonably long as it was used in stdlib PRs. If you start contributing more to the compiler, which would be very nice, don't pull that one too often.)

@trefis
Copy link
Contributor

trefis commented Apr 3, 2019

If you start contributing more to the compiler, which would be very nice, don't pull that one too often.

I was the one to suggest it :)

@Gbury
Copy link
Contributor Author

Gbury commented Apr 4, 2019

Travis fails (on i386) on test tests/tool-ocamldep-modalias/'main.ml' though, since it also happens on trunk currently, I suppose it's not this PR's fault.

@trefis
Copy link
Contributor

trefis commented Apr 4, 2019

Yes, that's correct.
I'll merge as is.

@trefis trefis merged commit aec35c5 into ocaml:trunk Apr 4, 2019
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.

None yet

5 participants