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 operators #2307

Merged
merged 5 commits into from Mar 11, 2019

Conversation

Projects
None yet
3 participants
@Julow
Copy link
Contributor

Julow commented Mar 10, 2019

Add an hint when using int's operators with other number types.

Example:

let x = 42.
let _ = x + 1.

Will report:

Line 8, characters 8-9:
8 | let _ = x + 1.
            ^
Error: This expression has type float but an expression was expected of type
         int
Line 8, characters 10-11:
8 | let _ = x + 1.
              ^
  Hint: Did you mean to use `+.'?

The hint is only enabled on +, -, *, / and mod operators.

I made a small refactor of the report_error function so it returns a Location.report.

Currently, the printing of qualifed functions is not nice. How can I improve this?
eg. Hint: Did you mean to use `Stdlib__int32.add'?

Show resolved Hide resolved typing/typecore.ml Outdated
@@ -21,6 +21,10 @@
\caml\camlinput\?1 + \<2.\> ;;
\endcamlinput\camlerror\:Error: This expression has type float but an expression was expected of type
\: int
\:File "/home/jules/ocaml/testsuite/tests/tool-caml-tex/redirections.ml", line 1, characters 2-3:

This comment has been minimized.

Copy link
@Octachron

Octachron Mar 10, 2019

Contributor

There is no way that the test can pass with this location information here. You should modify the test to avoid this suberror for now, and I will have a look at caml-tex handling of suberrors.

This comment has been minimized.

Copy link
@Julow

Julow Mar 10, 2019

Author Contributor

This is what I got with make promote. I updated the test.

@Julow Julow force-pushed the Julow:hint_num_infix branch from 708238e to 769e304 Mar 10, 2019

@gasche

gasche approved these changes Mar 11, 2019

Copy link
Member

gasche left a comment

I believe that the code is correct, and the feature is a user-improvement. The patch was a bit painful to review (the refactorings mixed with the actual feature makes it a bit unpleasant; in particular, 717ad28 could be split in changes to parameter-passing and the actual feature), but the tests give me a fair amount of confidence.

I spotted a few readability improvements that you could make.

One thing that I think is not optimal is that the three error-hint logics are all performed in different ways at the point we call report_unification_error:

  1. explanations for the expected type is passed as an argument with value built by report_type_expected_expanation
  2. explanations derived from the expression shape are turned in a separate call that is performed after report_unification_error, thanks to report_expr_type_clash_hints.
  3. your new hint is passed as ~sub parameter built by report_application_clash_hints

It's a shame that we have these three different passing protocol. I think a nicer API would be something like Printtyp.unification_error, which itself calls error_of_printer and arranges the various kind of sub-explanations in the way that it wants, but from the Typecore point of view they are all passed as parameters in the same way.
This is not a suggestion for the current PR, but for a future refactoring, to be discussed with @Armael.

Show resolved Hide resolved typing/typecore.ml Outdated
Show resolved Hide resolved typing/typecore.ml Outdated
Show resolved Hide resolved typing/typecore.ml Outdated
Show resolved Hide resolved typing/typecore.ml Outdated

@Julow Julow changed the title Hint on type error on int's operators Hint on type error on int operators Mar 11, 2019

@Julow Julow force-pushed the Julow:hint_num_infix branch from 021ec67 to 33ed767 Mar 11, 2019

@Julow

This comment has been minimized.

Copy link
Contributor Author

Julow commented Mar 11, 2019

Thanks for the review !
I applied most of your suggestions and rewritten the history (sorry for that).
I agree that this code does not fit elegantly with the previous PR. I'll try to make this a little more coherent.

@gasche gasche force-pushed the Julow:hint_num_infix branch from 33ed767 to e61156e Mar 11, 2019

@Julow Julow force-pushed the Julow:hint_num_infix branch 3 times, most recently from d47375e to 7cdc669 Mar 11, 2019

@Julow

This comment has been minimized.

Copy link
Contributor Author

Julow commented Mar 11, 2019

I've fixed the bug with tools/caml-tex.

Show resolved Hide resolved tools/caml_tex.ml Outdated

@Julow Julow force-pushed the Julow:hint_num_infix branch from 7cdc669 to 32eafc7 Mar 11, 2019

Julow added some commits Mar 10, 2019

Fix tools/caml-tex printing of sub message locations
List printer fields explicitly to avoid the same problem from happening again

@Julow Julow force-pushed the Julow:hint_num_infix branch from 32eafc7 to 0eefeae Mar 11, 2019

@gasche gasche merged commit f9e6490 into ocaml:trunk Mar 11, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Julow Julow referenced this pull request Mar 12, 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.