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

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

left a comment

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

left a comment

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

@trefis

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

Oh no! @gasche was faster.

Also:
oh no

typing/typecore.mli Outdated Show resolved Hide resolved
@Gbury

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

All review taken into account and Changes added.

@Gbury Gbury force-pushed the Gbury:private_constr branch from bd8d163 to ad0451d Apr 3, 2019
@Gbury

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

Rebase and squashed.

@gasche
gasche approved these changes Apr 3, 2019
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

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

@trefis trefis merged commit aec35c5 into ocaml:trunk Apr 4, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.