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 on type error on int literal #2301

Merged
merged 4 commits into from Mar 8, 2019

Conversation

@Julow
Copy link
Contributor

commented Mar 7, 2019

Add an hint on type error on int literal where expecting int32, int64 or nativeint.

Example:

let a = 2l
let _ = Int32.add a 3

Will give:

Line 2, characters 20-21:
2 | let _ = Int32.add a 3
                        ^
Error: This expression has type int but an expression was expected of type
         int32
       Hint: Did you mean `3l'?
@nojb

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Nice! What about float?

@Julow

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

float is a nice idea ! I added it.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Do I understand correctly that if I typed 3L instead of 3 then the hint wouldn't trigger?

typing/typecore.ml Outdated Show resolved Hide resolved
typing/typecore.mli Outdated Show resolved Hide resolved
@Julow

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Yes. Because 3L instead of 3 seems intentional where 3 instead of 3L may not.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

I think it would better if the hints were symmetric.

typing/typecore.ml Outdated Show resolved Hide resolved
typing/typecore.ml Outdated Show resolved Hide resolved
typing/ctype.ml Outdated Show resolved Hide resolved
typing/ctype.mli Outdated Show resolved Hide resolved
@Julow

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

I implemented symmetry, except in two cases: to int and from float.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

I agree that for the floatinteger direction, there is no clear hint to give in general. I am not so convinced for the {int32, int64, nativeint} → int one. For instance, I could have been refactoring code from int32 to int and just missed a l at some stage. In this case, the hint would be useful.

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

@Octachron: yes, but think about fun n -> pred n = 0l. With the symmetry, you would get an advice to downgrade 0l into 0, while it's likely that you meant Int32.pred instead.

@Julow

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

@Octachron: yes, but think about fun n -> pred n = 0l. With the symmetry, you would get an advice to downgrade 0l into 0, while it's likely that you meant Int32.pred instead.

I had the simpler 2l + 2l in mind.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

In other words, we are expecting the uncommon literal to be more likely to carry genuine intent compared to the context in this case. This sounds like a reasonable heuristic. It could be nice to document this design choice in a comment (along with the float* case ).

@@ -70,6 +70,11 @@ module Unification_trace: sig
(** Switch [expected] and [got] *)
val swap: t -> t

(** [explain f trace] calls [f] on trace elements starting from the end

This comment has been minimized.

Copy link
@Octachron

Octachron Mar 7, 2019

Contributor

[explain f trace][explain trace f]

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

I had the simpler 2l + 2l in mind.

For the record, in that case @Julow, @Armael and myself discussed extending the "type forcing context" to remember "under a binary operator + at this location"; when we find a "precision downgrade" error (a float or int constant downgraded into int), we can look in the context if we find an operator, and say instead: "this + should be a +. (Int32.add, etc.)". We also suggested doing this in a later PR if there is interest, not this one -- I think the current feature support (just literals in patterns or expressions) is a good, well-defined scope.

@gasche
gasche approved these changes Mar 8, 2019
Copy link
Member

left a comment

The current state is fine. Please fix the small issue noticed by @Octachron ( [explain f trace] → [explain trace f]) and add a Changes entry, and this should be good to merge.

@Julow

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Done.

@Julow Julow force-pushed the Julow:warning_int_literal branch from 261ea2a to 2ceeaf5 Mar 8, 2019
@Julow

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

I've rewritten the history.

@gasche

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

@Julow: check-typo reports the following errors (see the Travis logs), you need to fix them before this can be merged:

./typing/ctype.mli:76.81: [long-line] line is over 80 columns
./typing/typecore.ml:60.81: [long-line] line is over 80 columns
./typing/typecore.ml:2289.81: [long-line] line is over 80 columns
./typing/typecore.ml:4785.81: [long-line] line is over 80 columns
./typing/typecore.ml:4877.81: [long-line] line is over 80 columns
./typing/typecore.mli:122.81: [long-line] line is over 80 columns
./typing/typecore.mli:127.81: [long-line] line is over 80 columns
@Julow

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

It's fixed.

@Julow Julow force-pushed the Julow:warning_int_literal branch from 2ceeaf5 to 241f90a Mar 8, 2019
@gasche gasche merged commit 0c9b121 into ocaml:trunk Mar 8, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
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
Projects
None yet
5 participants
You can’t perform that action at this time.