[move-compiler-v2] fix issue 6922 by allowing type annotations for lambda parameters #14254
Conversation
⏱️ 1h 12m total CI duration on this PR
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14254 +/- ##
===========================================
- Coverage 71.8% 59.8% -12.0%
===========================================
Files 2395 851 -1544
Lines 481973 207415 -274558
===========================================
- Hits 346399 124184 -222215
+ Misses 135574 83231 -52343 ☔ View full report in Codecov by Sentry. |
| - ┌─ TEMPFILE:7:22 | ||
| - │ | ||
| - 7 │ assert!(foo(|x: u64, _: u64| x, |_: u64, y: u64| y, 10, 100) == 110, 0); | ||
| - │ ^^^^^^ Typed parameter to lambda is only allowed in Move 2 and beyond |
There was a problem hiding this comment.
nit: could we reword this to - "Explicit type annotations for lambda parameters are only allowed ..."
Because the lambda parameters are indeed typed even in Move 1, just that explicit type annotations are not allowed.
| @@ -421,6 +421,12 @@ pub type LValue = Spanned<LValue_>; | |||
| pub type LValueList_ = Vec<LValue>; | |||
| pub type LValueList = Spanned<LValueList_>; | |||
|
|
|||
| #[derive(Debug, Clone, PartialEq)] | |||
| pub struct TypedLValue_(pub LValue, pub Option<Type>); | |||
There was a problem hiding this comment.
Perhaps add a brief comment here saying these are used to represent explicitly typed (by the developer) lvalues.
| Some(sp(loc, E::TypedLValue_(b, ot))) | ||
| } | ||
|
|
||
| // Some(E::TypedLValue_(sp(loc, b_)); |
There was a problem hiding this comment.
Looks like some leftover code that should be removed.
| @@ -0,0 +1,13 @@ | |||
| module 0x42::m { | |||
|
|
|||
There was a problem hiding this comment.
The referenced issue #6922 says "type annotations are required to help the inference algorithm in some occasions". Do we have an example/test case where the type inference was not able to infer the types when lambda params were not explicitly typed, but the addition of the feature in this PR allows the user to progress in the natural way by adding explicit types (instead of the hack mentioned in the referenced issue for explicit typing)?
There was a problem hiding this comment.
I'm still trying to find such a case.
| } | ||
|
|
||
| fun g() { | ||
| foo(|v: &u64| { |
There was a problem hiding this comment.
Once this PR is in, we should fix the text in the move book: https://aptos.dev/en/build/smart-contracts/book/functions#current-restrictions, where we currently say this is not allowed.
There was a problem hiding this comment.
@brmataptos Just double checking this comment was seen.
There was a problem hiding this comment.
Added a note to #14343.
| expected_type, | ||
| context, | ||
| ); | ||
| self.translate_lvalue(lv, &bound_type, expected_order, match_locals, context) |
There was a problem hiding this comment.
Why are we using the expected_order here? Shouldn't we check tha lvalues have the bound_type exactly, similar to how wo check annotated expressions?
EA::Exp_::Annotate(exp, typ) => {
let ty = self.translate_type(typ);
let exp =
self.translate_exp_in_context(exp, &ty, &ErrorMessageContext::TypeAnnotation);
self.check_type(&loc, &ty, expected_type, context);
exp
},
There was a problem hiding this comment.
I don't think so. The type of the LValue is the type of a container, not a value. It may be wider than the value put in it. I've added a test lambda_typed_widen.move which tries to use the "subtyping" relationship between &mut u64 and & u64 to test what happens to lambda params with declared types versus those with undeclared types. Not sure it's a strong enough test, though.
brmataptos
changed the title
brmataptos
force-pushed
the
brm-issue-6922
branch
from
6898b5e
to
72937b1
Compare
| } | ||
|
|
||
| fun g() { | ||
| foo(|v: &u64| { |
There was a problem hiding this comment.
@brmataptos Just double checking this comment was seen.
| } | ||
|
|
||
| fun g() { | ||
| foo(|v: &u64| { |
There was a problem hiding this comment.
Added a note to #14343.
brmataptos
force-pushed
the
brm-issue-6922
branch
from
72937b1
to
c8d8ce9
Compare
| module 0x8675309::M { | ||
| use std::vector; | ||
|
|
||
| public inline fun use_mut_ref<T>(v: &mut vector<T>, action: |&mut T|T): T { |
There was a problem hiding this comment.
Could you write a test case where the return value of the lambda function is immutable/mutable reference?
There was a problem hiding this comment.
I added some, but it doesn't really show anything exciting.
There was a problem hiding this comment.
The new tests do reveal some awkward error messages, which can be elicited without this PR, please see #14681.
brmataptos
force-pushed
the
brm-issue-6922
branch
from
c8d8ce9
to
bc10e43
Compare
brmataptos
force-pushed
the
brm-issue-6922
branch
from
bc10e43
to
ecd92af
Compare
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
|
✅ Forge suite
|
Description
Allow type annotations on lambda parameters in Move 2.
Fixes #6922.
How Has This Been Tested?
Added type annotations to lambda params on a lot of existing tests, renamed as
*_typed.move.Did a diff of outputs to make sure outputs are similar to the previous, without annotations, where appropriate.
Later tests of more interesting Lambda uses will test more subtyping cases before release.
Key Areas to Review
This includes some cases that should involve integer/u64 subtyping,
but maybe there could be more interesting subtyping cases checked.
Feel free to suggest a feasible test case.
Someone more familiar with the type widening directions than I should take a look at the spot where
I have a comment "is this arg ordering right?" in exp_builder.rs.
Type of Change
Which Components or Systems Does This Change Impact?
Checklist