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

Hint when the expected type is wrapped in a ref #2319

Open
wants to merge 6 commits into
base: trunk
from

Conversation

Projects
None yet
3 participants
@Julow
Copy link
Contributor

commented Mar 13, 2019

This PR add an hint suggesting to use the (!) operator

This example,

let a = ref 0
let b = a + 1

gives:

Line 2, characters 8-9:
2 | let b = a + 1
            ^
Error: This expression has type int ref
       but an expression was expected of type int
  Hint: This is a `ref', did you mean `!a'?

@Armael tried to implement this before.
I hope I handled all the corner cases and this can be accepted.

This PR is based on top of #2313

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

A meta-comment: in your testsuite entries, it would be nice if you could include a comment in each case describing what you are trying and what is the behavior you expect. Otherwise it's hard to understand why, in a certain case, there is no hint (is this a bug? a not-implemented-yet-feature? a known limitation of the approach? a case where it's good to not hint?).

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

A meta-meta-comment:

References are entirely defined in the standard library. It feels wrong to special-case them in the typechecker, from an architectural standpoint. The same can be said about #2307 (int and float operators are not "known" to the typechecker) but it was hastily merged before I could make that remark.

I think this approach is misguided. This kind of error explanations should be declared in the standard library, perhaps as annotations, using some generic mechanism for explanations to be implemented in the typechecker. Not unlike the way Menhir handles user-defined error messages for specific parsing errors.

Can we please have a discussion and vote on caml-devel?

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Sure, let's have a discussion. Personally I think that, to a common
user, there is no intuitive different between the "languaginess"
status of while and for and int32 versus (+) and (+.) and
(!). Having a generic mechanism sounds even better, and we should
discuss that -- there was some work on the GHC side for
library-defined error annotations I think -- but I would be sorry if
we delayed notable ergonomics improvement for beginners in the wait
for a non-existing ultimate solution.

(For the record, I think that the Did you mean 32l point (which if
I understand correctly does not fall by your criticism, as those types
are compiler-defined) is a nice-to-have, that the later "Did you mean
+." is actually going to make a big difference for beginners, and
that the present PR is a bit more suspect but still corresponds to
a big pain point. These two latter things were explicitly pointed as
hurdles by Arthur @charguer Charguéraud's feedback on common type
mistakes when teaching OCaml.)

@Julow

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

@xavierleroy I don't think it is a problem on the architecture side. This is part of the error-reporting code, which could be moved to its own module (I'll be happy to do it).

@Julow Julow referenced this pull request Apr 10, 2019

Merged

Clean up int literal hint #2313

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.