Hint on type error on int operators#2307
Merged
gasche merged 5 commits intoocaml:trunkfrom Mar 11, 2019
Merged
Conversation
gasche
reviewed
Mar 10, 2019
Octachron
reviewed
Mar 10, 2019
| \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: |
Member
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
This is what I got with make promote. I updated the test.
gasche
approved these changes
Mar 11, 2019
Member
gasche
left a comment
There was a problem hiding this 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:
- explanations for the expected type is passed as an argument with value built by
report_type_expected_expanation - explanations derived from the expression shape are turned in a separate call that is performed after
report_unification_error, thanks toreport_expr_type_clash_hints. - your new hint is passed as
~subparameter built byreport_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.
Contributor
Author
|
Thanks for the review ! |
d47375e to
7cdc669
Compare
Contributor
Author
|
I've fixed the bug with |
Octachron
reviewed
Mar 11, 2019
Octachron
reviewed
Mar 11, 2019
List printer fields explicitly to avoid the same problem from happening again
Member
|
Back in May we had a development meeting where this specific feature was discussed, following the crticism expressed in #2319 (comment) (that the compiler should not hardcode logic that detects identifiers from the standard library; a more general library-level mechanism should be used instead). Personally I think that the current PR is worth having in the compiler codebase, but there was a majority of other maintainers that preferred to wait for a more principled solution, so we decided to revert this PR -- before 4.09, which I still have to do. I have tried to build a proposal for what a more general mechanism would look like, but my attempts at getting something reasonably nice have not reached a satisfying point for now, so I have nothing to propose. (The difficulty is to find a formulation that is reasonably independent of the specific details of the implementation of type inference in the OCaml compiler.) |
Octachron
added a commit
that referenced
this pull request
Aug 12, 2019
Revert #2307 ( Hint on type error on numeric operators )
OlivierNicole
pushed a commit
to OlivierNicole/ocaml
that referenced
this pull request
Feb 28, 2025
* Start of implicit-source-positions Expand [%src_pos] to hard-coded record (ocaml#1565) * Update codeowners * Revert "Update codeowners" This reverts commit 313cb2d1415a34971ea6e3cbd02d008e255ca888. * Start * backup * Hacky checkpoint with hard-coded record * Rename test * Shadowing works as intended * Simpler test case * Fix label layouts * Change name to lexing_position, fix record layout, add to shadowing test * Add to test * Formatting * Tests, fix predef lexing_position layout, add CR for untypeast shadowing * Fix test * Fix untracked file * Revert "Fix untracked file" This reverts commit 2a18a6aef66322e4e897aef29ace14eda25b7785. * reformat * Bootstrap, with predef lexing_position --------- Co-authored-by: Richard Eisenberg <reisenberg@janestreet.com> Separate `arg_label`s for Parse and Typed trees (ocaml#1589) * src_pos in fn decls and defns * Revert is_optional modifications for now * src_pos shouldn't work in arbitrary places * Tidy code * Whitespace * Add test * Comment on documentation * Whitespace * more whitespace * Amend test to use Position * Correctly print [%src_pos] rather than lexing_position and fix corresponding test * Store hard-coded string in variable * Separate parsed and typed labels * Delete fn_decl_and_defn.ml * Delete invalid_usages.ml * Minimize diffs in PR * Minimize diffs in PR * Update named_function.ml for less diffs * Clean up * Put arg_label in Outcome tree * Revert "Put arg_label in Outcome tree" This reverts commit cd5ace530c602975bac5f259e7d078a52dfabdaa. * is_optional for Parsetree arg_labels * Restore Asttypes.arg_label, re-export arg_label from Types * Document distinction between arg_labels * Fix whitespace * Add todo comment Add `arg_label` to `Otyp_arrow` (ocaml#1644) * Put arg_label in Outcome tree * Add comment for Outcometree.arg_label `[%src_pos]` in fn decls and defns (ocaml#1579) * src_pos in fn decls and defns * Revert is_optional modifications for now * src_pos shouldn't work in arbitrary places * Tidy code * Whitespace * Add test * Comment on documentation * Whitespace * more whitespace * Amend test to use Position * Correctly print [%src_pos] rather than lexing_position and fix corresponding test * Store hard-coded string in variable * Hacky outcometree printing without creating nontrivial node * Update documentation for Position labels * Update tests * Fix erroneously printing lexing_position for Position arguments * Clean up code * Reconstruct constraint for Position arguments in Untypeast * Update tests * Move comments around * Slightly prettify * Use Location.none in Untypeast reconstructed extensions * Add Ttyp_src_pos to Typedtree * Add CR * Comments * Clean up code * Update comment in printtyp.ml * Add documentation for Ttyp_src_pos, remove extraneous comments * *synonyms.ml * src_pos in fn decls and defns * Revert is_optional modifications for now * src_pos shouldn't work in arbitrary places * Tidy code * Whitespace * Add test * Comment on documentation * Whitespace * Correctly print [%src_pos] rather than lexing_position and fix corresponding test * Store hard-coded string in variable * Hacky outcometree printing without creating nontrivial node * Update documentation for Position labels * Update tests * Fix erroneously printing lexing_position for Position arguments * Clean up code * Reconstruct constraint for Position arguments in Untypeast * Update tests * Move comments around * Slightly prettify * Use Location.none in Untypeast reconstructed extensions * Add Ttyp_src_pos to Typedtree * Add CR * Comments * Clean up code * Add documentation for Ttyp_src_pos, remove extraneous comments * *synonyms.ml * Merge cleanup * Add label to Octy_arrow * Rename function * transl_label checks for Position * Pass None to transl_label for classes * Add comment * Delete comment * Consider Position when approximating type * Add tests for application, recursion * Error messages mention Position * Prettify * Comments * Rename function * Add comment * Remove extraneous calls to label translation * Test type-based disambiguation * Add comment for fn app labels * Add commuting tests * Remove duplicated src_pos match logic * Reduce instances of src_pos string * src_pos in fn decls and defns * src_pos shouldn't work in arbitrary places * Whitespace * Correctly print [%src_pos] rather than lexing_position and fix corresponding test * Store hard-coded string in variable * Hacky outcometree printing without creating nontrivial node * Clean up code * Slightly prettify * Add CR * Clean up code * Update comment in printtyp.ml * Merge cleanup * Add label to Octy_arrow * Rename function * transl_label checks for Position * Pass None to transl_label for classes * Add comment * Delete comment * Consider Position when approximating type * Add tests for application, recursion * Error messages mention Position * Prettify * Comments * Rename function * Add comment * Remove extraneous calls to label translation * Test type-based disambiguation * Add comment for fn app labels * Add commuting tests * Remove duplicated src_pos match logic * Reduce instances of src_pos string * Make things is_optional again * Add commuting tests * Add transl_label_from_pat * Whitespace * Parenthesize src_pos in error message * Fix test Create `Lexing.position` values when `[%src_pos]` in expressions (ocaml#1661) * Everything * checkpoint Construct lambda for src_pos directly * Revert now unneeded changes to predef * Clean up comments, whitespace Implicitly supply source position arguments (ocaml#1671) * Everything * Apply position arguments when expected type is nothing, refactor creating src_pos exprs * Add warning instead of modifying existing one * Fix test * Move test * Resolved comments Clearer classic mode label equivalence checks (ocaml#1692) * Everything * Apply position arguments when expected type is nothing, refactor creating src_pos exprs * Add warning instead of modifying existing one * Fix test * Move test * Resolved comments * Classic mode equivalence check * Refactor * Implicit source position merge conflicts (ocaml#2275) * Implicit Source Positions Conflict resolution This feature solely fixes merge conflicts that the implicit source positions project had after not having been rebased since the summer. Sadly, I mistakenly committed a new test! I meant to do it on a different branch, but I accidentally committed some more changes after that making it trickier to split... Please let me know if I should only keep this feature do "merge conflict resolution". Thanks! Testing ------- - I ran `make test`, and it passes. - `make install` works, I migrated bonsai to start using this and it works! - Added test on let operators. - Sadly it doesn't quite work, but I think it would be cool if it did work as it would allow codebases that use let+ and let* to get source code locations! I am unsure about its sound-ness though and left CR src_pos'es/question CR's. Please let me know if there is other additional testing/ci I should perform. Thanks! I also performed additional cleanup in this feature from self-questions I had during the merge. Please let me know if there are any changes I should perform. Thanks! * More cleanup after self-review * Made CI on Github pass - CI seems to run `make ci` instead of just `make test`. I've fixed more `make ci` changes, although not all, pushing to let ci run in case the current "compiler-libs.common" being missing is due to a misconfiguration on my environment/if it also fails on the github ci being missing is due to a misconfiguration on my environment/if it also fails on the github ci. - Performed a bootstrap - Fixed chamelon - Additionally turned my questions on let operator support into CR src_pos: * Removed empty file added accidentally * Updated let_operator support comment * Widened question on chamelon compatibility * Implicit source positions object system support (ocaml#2307) * Implicit Source Positions Conflict resolution This feature solely fixes merge conflicts that the implicit source positions project had after not having been rebased since the summer. Sadly, I mistakenly committed a new test! I meant to do it on a different branch, but I accidentally committed some more changes after that making it trickier to split... Please let me know if I should only keep this feature do "merge conflict resolution". Thanks! Testing ------- - I ran `make test`, and it passes. - `make install` works, I migrated bonsai to start using this and it works! - Added test on let operators. - Sadly it doesn't quite work, but I think it would be cool if it did work as it would allow codebases that use let+ and let* to get source code locations! I am unsure about its sound-ness though and left CR src_pos'es/question CR's. Please let me know if there is other additional testing/ci I should perform. Thanks! I also performed additional cleanup in this feature from self-questions I had during the merge. Please let me know if there are any changes I should perform. Thanks! * Moves changes from original class-type support branch into a rebased branch * Removed lingering merge conflict markers * More tests + manually moved a commit from the original class type branch - For some reason I originally missed a commit that typed the argument on classes from the original branch. This feature also grabs the tests. Some of my questions revolving `Principal` are no longer needed as they seem to have disappeared! I suspect that `make ci` now passing in the parent feature is partly/transitively responsible somehow for `Principal` now no longer showing up. * Fixed incorrect merging of invalid_usages.ml * Removes weird whitespace observed after self-review * More tests! Found out that application on an inheritance call is unhandled! * Explicit passing positional argument in a pcl_apply works, erasure still does not. * Added Pcl_apply support * More cleanup + removed a question cr from the parent feature. * More tests. Found another bug! Class type arrows seeem to be left untranslated... * Fixed type annotation bug + added more tests * Removed duplicated test * Added more tests + fixed weird whitespace * Added question on the two class system environments (val_env vs. met_env) * Deduplicated more code after self-review * minor whitespace change * Removed resolved question CR and addressed new CRs * Implicit source positions directory locations (ocaml#2346) * Implicit Source Positions Conflict resolution This feature solely fixes merge conflicts that the implicit source positions project had after not having been rebased since the summer. Sadly, I mistakenly committed a new test! I meant to do it on a different branch, but I accidentally committed some more changes after that making it trickier to split... Please let me know if I should only keep this feature do "merge conflict resolution". Thanks! Testing ------- - I ran `make test`, and it passes. - `make install` works, I migrated bonsai to start using this and it works! - Added test on let operators. - Sadly it doesn't quite work, but I think it would be cool if it did work as it would allow codebases that use let+ and let* to get source code locations! I am unsure about its sound-ness though and left CR src_pos'es/question CR's. Please let me know if there is other additional testing/ci I should perform. Thanks! I also performed additional cleanup in this feature from self-questions I had during the merge. Please let me know if there are any changes I should perform. Thanks! * More cleanup after self-review * Made CI on Github pass - CI seems to run `make ci` instead of just `make test`. I've fixed more `make ci` changes, although not all, pushing to let ci run in case the current "compiler-libs.common" being missing is due to a misconfiguration on my environment/if it also fails on the github ci being missing is due to a misconfiguration on my environment/if it also fails on the github ci. - Performed a bootstrap - Fixed chamelon - Additionally turned my questions on let operator support into CR src_pos: * Removed empty file added accidentally * Updated let_operator support comment * Widened question on chamelon compatibility * Moves changes from original class-type support branch into a rebased branch * Created branch with working changes for directory positions. * Added a test sanity checking being able to pass in the flag. * Changed test to properly test that the right basename is supplied * Removed merge conflict markers upon self review. * Removed lingering merge artifacts upon self review * Renamed flag from -dir to -directory * Fix typo * Removed directory flag from ocamldoc options * [Implicit Source Positions] - Better Error Messages (ocaml#2364) * Rename src_pos -> call_pos Also left some self notes regarding the remaining CR src_pos * Improved error messages for %call_pos * Addressed a CR src_pos and removed an already addressed CR src_pos * Cleanup after self-review * Updated missing rename after self-review * Removed addressed c r questions * Fixed merge conflicts. `make ci` passes locally * bootstrap --------- Co-authored-by: jose r <45022810+Enoumy@users.noreply.github.com> Co-authored-by: enoumy <enoumy@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add an hint when using
int's operators with other number types.Example:
Will report:
The hint is only enabled on
+,-,*,/andmodoperators.I made a small refactor of the
report_errorfunction so it returns aLocation.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'?