[move-compiler-v2] fix issue 6922 by allowing type annotations for lambda parameters #14254

brmataptos · 2024-08-13T05:49:07Z

Description

Allow type annotations on lambda parameters in Move 2.

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

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-08-13T05:49:10Z

⏱️ 1h 12m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-unit-coverage	12m	🟩
rust-move-unit-coverage	9m	🟩
rust-move-unit-coverage	9m	🟩
rust-move-tests	9m	🟩
rust-move-tests	8m	🟩
rust-move-tests	8m	🟩
general-lints	5m	🟩 🟩 🟩
rust-cargo-deny	5m	🟩 🟩 🟩
check-dynamic-deps	3m	🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩
file_change_determinator	35s	🟩 🟩 🟩
file_change_determinator	27s	🟩 🟩 🟩
permission-check	9s	🟩 🟩 🟩
permission-check	9s	🟩 🟩 🟩
permission-check	8s	🟩 🟩 🟩
permission-check	8s	🟩 🟩 🟩

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

codecov · 2024-08-13T06:00:04Z

Codecov Report

Attention: Patch coverage is 81.60920% with 32 lines in your changes missing coverage. Please review.

Project coverage is 59.8%. Comparing base (25499ae) to head (9a3d746).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
third_party/move/move-compiler/src/parser/ast.rs	0.0%	15 Missing ⚠️
...hird_party/move/move-compiler/src/expansion/ast.rs	0.0%	11 Missing ⚠️
...d_party/move/move-model/src/builder/exp_builder.rs	92.2%	6 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

vineethk

Looking pretty good so far. Have a few comments.

vineethk · 2024-08-13T22:17:47Z

third_party/move/move-compiler-v2/transactional-tests/tests/inlining/bug_10991_typed.exp

+- ┌─ 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


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.

vineethk · 2024-08-13T22:19:59Z

third_party/move/move-compiler/src/expansion/ast.rs

@@ -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>);


Perhaps add a brief comment here saying these are used to represent explicitly typed (by the developer) lvalues.

vineethk · 2024-08-13T22:20:38Z

third_party/move/move-compiler/src/expansion/translate.rs

+ Some(sp(loc, E::TypedLValue_(b, ot)))
+}
+
+// Some(E::TypedLValue_(sp(loc, b_));


Looks like some leftover code that should be removed.

third_party/move/move-compiler/src/parser/ast.rs

vineethk · 2024-08-13T22:32:04Z

third_party/move/move-compiler-v2/tests/checking/typing/eq_inline_typed.move

@@ -0,0 +1,13 @@
+module 0x42::m {
+


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)?

I'm still trying to find such a case.

third_party/move/move-model/src/builder/exp_builder.rs

vineethk · 2024-08-13T22:42:36Z

third_party/move/move-compiler-v2/tests/checking-lang-v1/eq_inline_typed.move

+ }
+
+ fun g() {
+ foo(|v: &u64| {


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.

@brmataptos Just double checking this comment was seen.

Added a note to #14343.

fEst1ck · 2024-08-14T19:38:30Z

third_party/move/move-model/src/builder/exp_builder.rs

+ expected_type,
+ context,
+ );
+ self.translate_lvalue(lv, &bound_type, expected_order, match_locals, context)


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 },

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.

third_party/move/move-compiler/src/parser/ast.rs

vineethk · 2024-08-20T01:29:08Z

third_party/move/move-compiler-v2/tests/checking-lang-v1/eq_inline_typed.move

+ }
+
+ fun g() {
+ foo(|v: &u64| {


@brmataptos Just double checking this comment was seen.

third_party/move/move-compiler/src/parser/ast.rs

third_party/move/move-model/src/builder/exp_builder.rs

brmataptos

I think I found all of your comments, Vineeth. Please let me know if not.

brmataptos · 2024-08-20T01:55:23Z

third_party/move/move-compiler-v2/tests/checking-lang-v1/eq_inline_typed.move

+ }
+
+ fun g() {
+ foo(|v: &u64| {


Added a note to #14343.

third_party/move/move-compiler/src/parser/ast.rs

rahxephon89 · 2024-08-20T06:23:31Z

third_party/move/move-compiler-v2/tests/checking/typing/lambda_typed_widen.move

+module 0x8675309::M {
+ use std::vector;
+
+ public inline fun use_mut_ref<T>(v: &mut vector<T>, action: |&mut T|T): T {


Could you write a test case where the return value of the lambda function is immutable/mutable reference?

I added some, but it doesn't really show anything exciting.

The new tests do reveal some awkward error messages, which can be elicited without this PR, please see #14681.

github-actions · 2024-09-20T06:03:54Z

✅ Forge suite `framework_upgrade` success on `25a081116546670e62ca927ba90478de78557056` ==> `9a3d746c9ffb3eb77132aebc5fb207141a3e2c78`

Compatibility test results for 25a081116546670e62ca927ba90478de78557056 ==> 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78 (PR)
Upgrade the nodes to version: 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1287.97 txn/s, submitted: 1290.19 txn/s, failed submission: 2.22 txn/s, expired: 2.22 txn/s, latency: 2564.68 ms, (p50: 2100 ms, p70: 2700, p90: 4500 ms, p99: 6600 ms), latency samples: 115860
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1182.78 txn/s, submitted: 1185.91 txn/s, failed submission: 3.13 txn/s, expired: 3.13 txn/s, latency: 2516.43 ms, (p50: 2400 ms, p70: 2800, p90: 3900 ms, p99: 6000 ms), latency samples: 105780
5. check swarm health
Compatibility test for 25a081116546670e62ca927ba90478de78557056 ==> 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78 passed
Upgrade the remaining nodes to version: 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1326.80 txn/s, submitted: 1329.27 txn/s, failed submission: 2.47 txn/s, expired: 2.47 txn/s, latency: 2528.69 ms, (p50: 2400 ms, p70: 2700, p90: 3600 ms, p99: 5800 ms), latency samples: 107300
Test Ok

github-actions · 2024-09-20T06:04:07Z

✅ Forge suite `realistic_env_max_load` success on `9a3d746c9ffb3eb77132aebc5fb207141a3e2c78`

two traffics test: inner traffic : committed: 14492.15 txn/s, latency: 2739.52 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3200 ms), latency samples: 5510220
two traffics test : committed: 100.03 txn/s, latency: 1366.64 ms, (p50: 1400 ms, p70: 1500, p90: 1600 ms, p99: 1700 ms), latency samples: 2400
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.238, avg: 0.215", "QsPosToProposal: max: 1.108, avg: 0.942", "ConsensusProposalToOrdered: max: 0.356, avg: 0.302", "ConsensusOrderedToCommit: max: 0.412, avg: 0.353", "ConsensusProposalToCommit: max: 0.705, avg: 0.655"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.17s no progress at version 8531 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.64s no progress at version 2889485 (avg 4.84s) [limit 15].
Test Ok

github-actions · 2024-09-20T06:05:15Z

✅ Forge suite `compat` success on `25a081116546670e62ca927ba90478de78557056` ==> `9a3d746c9ffb3eb77132aebc5fb207141a3e2c78`

Compatibility test results for 25a081116546670e62ca927ba90478de78557056 ==> 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78 (PR)
1. Check liveness of validators at old version: 25a081116546670e62ca927ba90478de78557056
compatibility::simple-validator-upgrade::liveness-check : committed: 13864.64 txn/s, latency: 2428.99 ms, (p50: 2000 ms, p70: 2100, p90: 4200 ms, p99: 8800 ms), latency samples: 449380
2. Upgrading first Validator to new version: 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7847.53 txn/s, latency: 3621.19 ms, (p50: 4100 ms, p70: 4200, p90: 4300 ms, p99: 4400 ms), latency samples: 146180
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7216.21 txn/s, latency: 4445.72 ms, (p50: 4400 ms, p70: 4500, p90: 6800 ms, p99: 7000 ms), latency samples: 254160
3. Upgrading rest of first batch to new version: 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7720.19 txn/s, latency: 3581.83 ms, (p50: 4000 ms, p70: 4300, p90: 4600 ms, p99: 4800 ms), latency samples: 137800
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7594.76 txn/s, latency: 4208.07 ms, (p50: 4400 ms, p70: 4500, p90: 6100 ms, p99: 6300 ms), latency samples: 253960
4. upgrading second batch to new version: 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11814.49 txn/s, latency: 2334.96 ms, (p50: 2500 ms, p70: 2700, p90: 2800 ms, p99: 3000 ms), latency samples: 205820
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10345.85 txn/s, latency: 3005.07 ms, (p50: 2700 ms, p70: 2800, p90: 5600 ms, p99: 8000 ms), latency samples: 379800
5. check swarm health
Compatibility test for 25a081116546670e62ca927ba90478de78557056 ==> 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78 passed
Test Ok

brmataptos requested review from vineethk, rahxephon89 and wrwg August 13, 2024 05:50

brmataptos requested a review from fEst1ck August 13, 2024 16:53

vineethk reviewed Aug 13, 2024

View reviewed changes

fEst1ck reviewed Aug 14, 2024

View reviewed changes

brmataptos changed the title ~~fix issue 6922 by allowing type annotations for lambda parameters~~ [move-compiler-v2] fix issue 6922 by allowing type annotations for lambda parameters Aug 19, 2024

brmataptos force-pushed the brm-issue-6922 branch from 6898b5e to 72937b1 Compare August 19, 2024 22:35

brmataptos requested review from vineethk and fEst1ck August 19, 2024 22:39

vineethk approved these changes Aug 20, 2024

View reviewed changes

brmataptos mentioned this pull request Aug 20, 2024

[move-compiler-v2] Update Move Book with Move 2 features #14343

Closed

brmataptos commented Aug 20, 2024

View reviewed changes

brmataptos force-pushed the brm-issue-6922 branch from 72937b1 to c8d8ce9 Compare August 20, 2024 02:03

rahxephon89 reviewed Aug 20, 2024

View reviewed changes

brmataptos force-pushed the brm-issue-6922 branch from c8d8ce9 to bc10e43 Compare September 18, 2024 21:01

brmataptos mentioned this pull request Sep 18, 2024

[Bug][move-compiler-v2] Function-valued argument type error message is confusing #14681

Open

brmataptos requested a review from rahxephon89 September 18, 2024 21:21

rahxephon89 approved these changes Sep 19, 2024

View reviewed changes

brmataptos added 4 commits September 19, 2024 22:07

fix issue 6922 by allowing type annotations for lambda parameters

2b5c1a2

address Vineeth's comments, also add attempt at widening test

fae9bdb

update parser/ast.rs comments with typed bindlist for lambda

44cde8e

add a test that tries to catch result widening

ecd92af

brmataptos force-pushed the brm-issue-6922 branch from bc10e43 to ecd92af Compare September 20, 2024 05:11

brmataptos enabled auto-merge (squash) September 20, 2024 05:12

This comment has been minimized.

Sign in to view

fix lint

9a3d746

This comment has been minimized.

Sign in to view

brmataptos merged commit f5134a5 into main Sep 20, 2024
51 checks passed

brmataptos deleted the brm-issue-6922 branch September 20, 2024 06:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move-compiler-v2] fix issue 6922 by allowing type annotations for lambda parameters #14254

[move-compiler-v2] fix issue 6922 by allowing type annotations for lambda parameters #14254

brmataptos commented Aug 13, 2024 •

edited

Loading

trunk-io bot commented Aug 13, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading

vineethk left a comment

vineethk Aug 13, 2024

brmataptos Aug 19, 2024

vineethk Aug 13, 2024

brmataptos Aug 19, 2024

vineethk Aug 13, 2024

brmataptos Aug 19, 2024

vineethk Aug 13, 2024

brmataptos Aug 19, 2024

vineethk Aug 13, 2024

vineethk Aug 20, 2024

brmataptos Aug 20, 2024

fEst1ck Aug 14, 2024

brmataptos Aug 19, 2024

vineethk Aug 20, 2024

brmataptos left a comment

brmataptos Aug 20, 2024

rahxephon89 Aug 20, 2024

brmataptos Sep 18, 2024

brmataptos Sep 18, 2024

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.

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

[move-compiler-v2] fix issue 6922 by allowing type annotations for lambda parameters #14254

[move-compiler-v2] fix issue 6922 by allowing type annotations for lambda parameters #14254

Conversation

brmataptos commented Aug 13, 2024 • edited Loading

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Aug 13, 2024 • edited Loading

codecov bot commented Aug 13, 2024 • edited Loading

Codecov Report

vineethk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brmataptos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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.

github-actions bot commented Sep 20, 2024

✅ Forge suite framework_upgrade success on 25a081116546670e62ca927ba90478de78557056 ==> 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78

github-actions bot commented Sep 20, 2024

✅ Forge suite realistic_env_max_load success on 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78

github-actions bot commented Sep 20, 2024

✅ Forge suite compat success on 25a081116546670e62ca927ba90478de78557056 ==> 9a3d746c9ffb3eb77132aebc5fb207141a3e2c78

brmataptos commented Aug 13, 2024 •

edited

Loading

trunk-io bot commented Aug 13, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading

✅ Forge suite `framework_upgrade` success on `25a081116546670e62ca927ba90478de78557056` ==> `9a3d746c9ffb3eb77132aebc5fb207141a3e2c78`

✅ Forge suite `realistic_env_max_load` success on `9a3d746c9ffb3eb77132aebc5fb207141a3e2c78`

✅ Forge suite `compat` success on `25a081116546670e62ca927ba90478de78557056` ==> `9a3d746c9ffb3eb77132aebc5fb207141a3e2c78`