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
[compiler-v2] Finishing refactoring of abilities + type checker overhaul #12675
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12675 +/- ##
===========================================
- Coverage 69.8% 62.6% -7.2%
===========================================
Files 2293 819 -1474
Lines 436088 183454 -252634
===========================================
- Hits 304431 114978 -189453
+ Misses 131657 68476 -63181 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor comments. Looks great!
SomeReceiverFunction(Symbol, Option<Vec<Type>>, Vec<(Loc, Type)>, Type), | ||
/// The type variable must be instantiated with a type which has the given ability | ||
HasAbility(Ability), | ||
SomeReceiverFunction( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the notes added.
), | ||
/// The type must not be reference because it is used as the type of some field or | ||
/// as a type argument. | ||
NoReference, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Using NotReference
, NotTuple
, etc., may be a bit clearer. But because there is a clear comment, it is up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this would be clearer? I thought NoReference
is the right English here, compared to NotReference
which is not a valid word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constraint is that it is "not a reference" => NotReference
.
Both NoReference
and NotReference
are not valid words by themselves.
Anyway, I have marked this as a nit because the comment makes the constraint clear to me.
@@ -309,13 +619,29 @@ impl Constraint { | |||
/// Represents an error resulting from type unification. | |||
#[derive(Debug)] | |||
pub enum TypeUnificationError { | |||
/// The two types mismatch: `TypeMatch(actual, expected)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The two types mismatch: `TypeMatch(actual, expected)` | |
/// The two types mismatch: `TypeMismatch(actual, expected)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
/// Lifts a type mismatch error to a pair of context type | ||
/// Lifts a type unification error from the critical pair to the given context type. | ||
/// A critical pair in type unification is the sub-term in which two type terms disagree, | ||
/// e.g for `S<t> != S<t'>`, `(t, t`)` is the critical pair. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// e.g for `S<t> != S<t'>`, `(t, t`)` is the critical pair. | |
/// e.g for `S<t> != S<t'>`, `(t, t')` is the critical pair. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
}; | ||
// Providing instantiation context for number constraints is | ||
// confusing for users. Those constraints are used in | ||
// operators like `*` and so on but not visible to the use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// operators like `*` and so on but not visible to the use. | |
// operators like `*` and so on but not visible to the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -1,48 +1,48 @@ | |||
|
|||
Diagnostics: | |||
error: operand of cast must be a number | |||
error: expected `u8|u16|u32|u64|u128|u256|num` but found a value of type `S` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: looks like we use integer
in some error message contexts (eg, in binary_gt_invalid.exp
), and u8|....|num
here. Maybe we could use integer
here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I haven't finished readng ty.rs.
@@ -374,34 +387,18 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo | |||
&mut self, | |||
node_id: NodeId, | |||
ty: &Type, | |||
reported_types: &mut BTreeSet<Type>, | |||
reported_vars: &mut BTreeSet<u32>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be useful to state in the comment above that
/// Free type variables found are added to `reported_vars`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still reading test outputs. Here's one, though.
...y/move/move-compiler-v2/tests/checking/abilities/v1/ability_constraint_generic_in_field.move
Show resolved
Hide resolved
/// given a struct with declared abilities. | ||
pub fn for_field(struct_abilities: AbilitySet, field_ty: &Type) -> Vec<Constraint> { | ||
let mut result = vec![Constraint::NoPhantom, Constraint::NoTuple]; | ||
// Only if the field is not a type parameter, requires abilities. That reflects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need NoReference(true)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fields can be references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good but we have a number of test regressions, so if you don't fix them here file an issue to make sure we don't miss them.
Notably:
- we don't seem to type-check structs until they are used
- maybe also inline functions
- we don't seem to check for reference fields of structs (Zekun noted in the code)
- we could do better with types like
u8|u16|..|num
andu16|u32|...|num
- when we infer types for variables it would be a bonus if we could show the reason when we complain about an assignment of a different type
- other items I may have forgotten.
Fix field reference types and I think we can record the test regressions and move on.
I checked all the tests. I hope you will forgive any mistakes I made, since Github makes it really hard to review all my comments before sending them.
TypeParameter(_) => true, | ||
Tuple(ts) => ts.iter().any(|t| t.depends_from_type_parameter()), | ||
Vector(t) => t.depends_from_type_parameter(), | ||
Struct(_, _, ts) => ts.iter().any(|t| t.depends_from_type_parameter()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think you could use
Tuple(ts) | Struct(_, _, ts) | ResourceDomain(_, _, Some(ts) => ts.iter().any(|t| t.depends_from_type_parameter()),
Vector(t) | Reference(_, t) | TypeDomain(t) => t.depends_from_type_parameter(),
Fun(t, r) => t.depends_from_type_parameter() || r.depends_from_type_parameter()
to be a little more concise and easier to see how each is handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -0,0 +1,112 @@ | |||
|
|||
Diagnostics: | |||
error: type `R` is missing required ability `drop` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: before the move, this file had errors at lines 11, 12, 14, 19, 21, 24, 26, 28, 33, 35, 38, 40, 43, 45. You may want to compare with the original test output.
Do we perhaps not check struct field constraints until the struct is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this bug is now fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still missing errors, compared to main:
v1 had: 11, 12, 13, 14, 18, 19, 20, 21, 23, 24, 25, 26, 32, 33, 34, 35, 37, 38, 39, 40, 42, 43, 44, 45
v2 had: 11, 12, 13, 14, 19, 21, 24, 26, 28, 33, 35, 38, 40, 43, 45
now have: 11, 12, 13, 14, 32, 33, 35, 38, 39, 40, 42, 43, 44, 45
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, there was another bug which lead to that types of parameters were not checked. Fixed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I see you're still missing some errors that V1 had, but these seem to all be redundant (same type instantiation as a previous error), so I'll assume that's why we don't have them.
third_party/move/move-compiler-v2/tests/checking/abilities/invalid_struct_def.exp
Show resolved
Hide resolved
third_party/move/move-compiler-v2/tests/checking/abilities/missing_key.exp
Show resolved
Hide resolved
...y/move/move-compiler-v2/tests/checking/abilities/v1/ability_constraint_generic_in_field.move
Show resolved
Hide resolved
third_party/move/move-compiler-v2/tests/checking/typing/expected_actual_assign.exp
Show resolved
Hide resolved
third_party/move/move-compiler-v2/tests/checking/typing/recursive_structs_malformed.exp
Show resolved
Hide resolved
...rty/move/move-compiler-v2/tests/checking/typing/type_variable_join_threaded_pack_invalid.exp
Show resolved
Hide resolved
third_party/move/move-compiler-v2/tests/checking/typing/vector_with_non_base_type.exp
Outdated
Show resolved
Hide resolved
Thanks for the reviews, all issues addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some errors missing.
@@ -0,0 +1,112 @@ | |||
|
|||
Diagnostics: | |||
error: type `R` is missing required ability `drop` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still missing errors, compared to main:
v1 had: 11, 12, 13, 14, 18, 19, 20, 21, 23, 24, 25, 26, 32, 33, 34, 35, 37, 38, 39, 40, 42, 43, 44, 45
v2 had: 11, 12, 13, 14, 19, 21, 24, 26, 28, 33, 35, 38, 40, 43, 45
now have: 11, 12, 13, 14, 32, 33, 35, 38, 39, 40, 42, 43, 44, 45
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more I think.
@@ -0,0 +1,40 @@ | |||
|
|||
Diagnostics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't see it. Perhaps you forgot to push?
┌─ tests/checking/typing/conditional_global_operations.move:15:20 | ||
│ | ||
15 │ move_to(s, Box<R> { f: R {} }); | ||
│ ^^^^^^^^^^^^^^^^^^ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors are now all there, but they used to complain about ability 'key' and now they complain about ability 'store'. Perhaps we could mention multiple abilities if multiple are required and missing?
third_party/move/move-compiler-v2/tests/checking/typing/expected_actual_assign.exp
Show resolved
Hide resolved
third_party/move/move-compiler-v2/tests/checking/typing/constant_inference.exp
Show resolved
Hide resolved
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.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, comments addressed. I also spend some time to generally get error positions better. Now type parameter constraint errors are actually reported at the type argument, and constraints for inferred types at the function/struct which introduces the type parameters.
There seem to be still some errors missing compared to v1 but this stems from removing duplicate errors. If in a function a particular type has been identified to not satisfy e.g. an ability constraint, this will be only reported once in this function. v2 reports it multiple times.
@@ -0,0 +1,40 @@ | |||
|
|||
Diagnostics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AH, now I see what you mean. Those drop errors on code level are later handled on bytecode level in AbilityProcessor. There is a test in ability-check/assign.rs
.
┌─ tests/checking/typing/conditional_global_operations.move:15:20 | ||
│ | ||
15 │ move_to(s, Box<R> { f: R {} }); | ||
│ ^^^^^^^^^^^^^^^^^^ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error before was imprecise. For Box to have key
, R
needs to have store
. I improved the error positions a bit (this is actually a requirement on the inferred type of move_to
), as well as add now the constraints from type parameters to the notes, so you can see it was key
which was originally requested.
error: type `R` is missing required ability `store` (type was inferred)
┌─ tests/checking/typing/conditional_global_operations.move:15:9
│
15 │ move_to(s, Box<R> { f: R {} });
│ ^^^^^^^
│
= required by instantiating type parameter `T` of struct `Box`
= required by instantiating type parameter `T:key` of function `move_to`
@@ -0,0 +1,112 @@ | |||
|
|||
Diagnostics: | |||
error: type `R` is missing required ability `drop` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, there was another bug which lead to that types of parameters were not checked. Fixed now.
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.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So close. I think there's a case missing involving phantom type parameters.
@@ -0,0 +1,112 @@ | |||
|
|||
Diagnostics: | |||
error: type `R` is missing required ability `drop` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I see you're still missing some errors that V1 had, but these seem to all be redundant (same type instantiation as a previous error), so I'll assume that's why we don't have them.
third_party/move/move-compiler-v2/tests/checking/abilities/missing_key.exp
Show resolved
Hide resolved
11 │ fun cds<T: copy + drop + store>() {} | ||
│ - declaration of type parameter `T` | ||
· | ||
29 │ cds<vector<Cup<u8>>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V1 had errors on 31(c), 32(c), 33(k), 34(k), 35(k), 36(k), 37(cs), 38(c), 39(cds). These are phantom parameters, maybe that's the issue?
(c=copy, k=key, d=drop, s=store)
@@ -0,0 +1,114 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing errors for lines 29+, these relate to phantom parameters.
@@ -0,0 +1,114 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 29 on previously (v1) have errors involving phantom type parameters that are missing now.
… of duplicate removal
The reason for the remaining missing errors is duplicate removal. I extended one of those tests to demonstrate this. Basically, once we reported say that |
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.
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
|
Description
This concludes moving all ability checking into the type inference algorithm by representing checks as type constraints. It also introduces some new constraints to deal with some other checks which have done previously ad-hoc (whether types are allowed to be references, tuples, phantom).
The PR turned out to be a larger refactoring of the checker as expected. On the good side, a lot of redundant ad-hoc code could be removed and replaced by more generic code.
Closes #12437 and closes #12674
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
New and existing tests
Key Areas to Review
Baseline files