[compiler v2][type checker] Make arithmetic operators generic with constraints #9792
Conversation
This file contains 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
wrwg
force-pushed
the
wg-ints
branch
3 times, most recently
from
d9b0fb9 to
3678009
Compare
wrwg
requested review from
davidiw,
rahxephon89,
georgemitenkov,
brmataptos,
runtian-zhou and
vgao1996
brmataptos
reviewed
Aug 28, 2023
third_party/move/move-compiler-v2/tests/checking/typing/binary_add_invalid.exp
Show resolved
Hide resolved
brmataptos
requested changes
Aug 30, 2023
third_party/move/move-compiler-v2/tests/checking/typing/bind_wrong_type.exp
Outdated
Show resolved
Hide resolved
third_party/move/move-compiler-v2/tests/checking/typing/constant_inference.exp
Outdated
Show resolved
Hide resolved
rahxephon89
reviewed
Aug 30, 2023
|
I attempted to fix some of the orientation errors (where it appears 'expected' is wrong wr.t. 'found'), and in course of this removed some heuristics which appeared to be broken. Those heuristics have been added in the last review round on pressure of reviewers. Please note: error messages are not perfect and this is not the objective at this point. Specifically, in a contra-variance scenario (when assigning values), the expected vs found might be now a bit confusing again, but at least I hope it is correct. |
|
I filed a bug to follow up on the clarity of some of these error messages. |
This was referenced Aug 31, 2023
brmataptos
approved these changes
Aug 31, 2023
wrwg
force-pushed
the
wg-ints
branch
2 times, most recently
from
be9262c to
1594409
Compare
…nstraints Until now, we had represented arithmetic operators via overloads. For instance, we had `fun _+_(u8,u8):u8, fun _+_(u16,u16):u16` and so on for all integer types. The problem with this approach is that it does not work for constants, as described in #9330. Move allows to write `1+2` and this expression gets its type inferred from the context. In this PR, arithmetic ops are represented not longer as overloads but as generic functions with constraints. Basically: ``` fun _+_<A>(x: A, y: A): A where A:u8|u16|u32|..|u256; fun _-_<A>(...)... ``` This has the following effects: - Expressions over constants inherit their generic result type (that is, `1 + 2` does not fix the operator to a specific integer type. Instead the context may do this.) - Error messages get better. The messages where we just printed non-matching overloads were not very good, no we see more precise error messages. Technically, this needed some refinements of the type constraint system. Also, some error reporting has been fixed which was before only working in very specific contexts.
…or messages, removing some older broken heuristics and trying to fix the problem of orientation of 'expected' vs 'found' at the core. Some context annotation 'from assignment context' not tries to clarify the error message if the typing error is for an lvalue, where the expected type is a _lower bound_ instead of an upper bound, which may leads to confusion for end users. The messages are now more systematic but perhaps a bit harder to understand, yet this is not trivial to fix and we have to iterate on this.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Until now, we had represented arithmetic operators via overloads. For instance, we had
fun _+_(u8,u8):u8, fun _+_(u16,u16):u16and so on for all integer types. The problem with this approach is that it does not work for constants, as described in #9330. Move allows to write1+2and this expression behaves from the typing perspective like3, which has a type which is automatically adapted to the usage context.In this PR, arithmetic ops are represented not longer as overloads but as generic functions with constraints. For this, the already existing
SomeNumber(..)type constraint is used. Basically:This has the following effects:
1 + 2does not fix the operator to a specific integer type. Instead the context may do this.)Technically, this needed some refinements of the type constraint system, which brings us even closer to a type checker supporting traits/type classes. Also, some error reporting has been fixed which was before only working in very specific contexts.
Test Plan
Updates baselines files, adds a new test for constant expression type inference