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

Rewording for "constructor has no type" error #2094

Merged
merged 3 commits into from Oct 12, 2018

Conversation

Projects
None yet
5 participants
@Octachron
Copy link
Contributor

commented Oct 5, 2018

This small PR proposes to reword the error messages for
mismatched upper and lower bounds in polymorphic variant type:

type t = [< `A | `B > `B `C ]

Error: The present constructor C has no type

by

Error: The constructor `C is missing from the upper bound of this
polymorphic variant type. Upper and lower bounds are mismatched.

The current error message refers to an implementation detail and may be interpreted as a suggestion to add

type t = [< `A | `B > `B (`C: [`C]) ]

or maybe

type t = [< `A | `B > `B `C of int]

which are both syntax error, whereas the probable fix is to add the missing `C to the upper bound:

type t = [< `A | `B |`C > `B `C ]

That is why I propose to replace this error message by

Error: The constructor `C is missing from the upper bound of this
polymorphic variant type. Upper and lower bounds are mismatched.

Another option that I considered was

Error: The constructor `C is present but not accepted in this
polymorphic variant type.

but this error message has the disadvantage that the notion of present or accepted is quite removed from the surface syntax compared to the notion of upper and lower bound
(in other words, there is a < and a > in the syntax).

@Drup

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2018

I like the new wording. I'm not sure how upper and lower bounds are easy to understand for average OCamlers, but it's certainly better than the original one. I don't like present/absent, exactly for the reason you gave.

@garrigue
Copy link
Contributor

left a comment

The suggested message is fine.
The old message seems to reflect a very old version of ocaml, where one could give a type to a constructor even it was neither present nor explicitly accepted: [types > tags].

It would still be a good idea to write something in the change log (but you may not need to have separate entries for all your nice error message fixes).

@pmetzger

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

I'm not sure how upper and lower bounds are easy to understand for average OCamlers, but it's certainly better than the original one.

I agree the new message is much better, but I also agree it will still be cryptic, especially to newcomers. I don't have a good suggestion for something even better, though, so I'd say this should get committed until someone has a brilliant idea.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2018

I have collated all similar changes entry into one with few sub-entries.
Concerning the issue with "upper and lower bounds" , one possibility might be to be even more graphical:

Error: The constructor `C is missing from the left-hand side of ">" in this
polymorphic variant type. The left-hand and right-hand sides are mismatched.
@pmetzger

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

@Octachron I like that. It's more explicit.

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 7, 2018

Maybe you could even give both a semantic and a syntactic indication.

Error: In the lower bound of this polymorphic variant type (on the left,
between ">" and "<") the constructor `C is missing, but it is present
in the upper bound (on the right, after "<").
If you want `C to be present in all instances, it should be added
on the left, otherwise it should be removed on the right.
Changes Outdated
- more context for unbound type parameter error
- full explanation for unsafe cycles in recursive module definitions
(suggestion by Ivan Gotovchits)
- rewording for "constructor has no type" error

This comment has been minimized.

Copy link
@gasche

gasche Oct 7, 2018

Member

The grouping is nice (I hope people don't get the impression that those improvements were minor, recursive cycles in particular were a carefully crafted change).

I think you could point out that those are "typing error message improvements" -- as opposed to runtime errors or parsing/lexing errors.

I would still list the PR associated to each individual change (in addition to having them at the top), so that one can more easily get context/discussion on a specific change.

This comment has been minimized.

Copy link
@Octachron

Octachron Oct 8, 2018

Author Contributor

Good points, I will apply those changes once the error message is fixed.

@pmetzger

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

Maybe you could even give both a semantic and a syntactic indication.

This is nicer still!

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

@gasche, this sounds like the right direction. I would amend it to

Error: The constructor `C is missing from the upper bound  (between "<" and ">") 
of this polymorphic variant but is present in its lower bound (after ">").
Hint: Either add `C in the upper bound or remove it from the lower bound.

under the principles that

  • the missing constructor should appear as soon as possible
  • hints for possible fixes belong to hint and not the main body of the messages.
@pmetzger

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

@gasche and @Octachron someday, someone is not going to know to thank you for this because they're going to have so much less trouble fixing a problem that they won't be aware of the effort you went through to come up with the clearest possible message. Therefore, I'll thank you on their behalf now.

@Octachron Octachron force-pushed the Octachron:no_type_rewording branch from 0cd139d to 1055533 Oct 12, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

I have updated the error message with my last proposition.

@gasche gasche merged commit f47e4a6 into ocaml:trunk Oct 12, 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.