[move-compiler-v2] fix #14633: fix error messages for lambdas, identify change points for more complete lambda implementation #14742

brmataptos · 2024-09-24T20:57:54Z

Description

Catch all errors in move-compiler-v2/tests/checking/typing/lambda.move in Compiler V2,
after splitting and gradually commenting out code as lambda[2-5].move,
to avoid shadowing by other errors. Lay groundwork for support of lambdas in various capacities.

Fixes #14633.

add checking for function-typed function results in function_checker, and functions in parameters and results of function types in both args and results to functions there.
Change internal_error to error in bytecode_generator and module_generator
to properly show "error" rather than "bug" if lambdas are mistakenly used as values today.
tag some code to show where changes are needed to support lambda: // TODO(LAMBDA)
fix unused_params_checker and recursive_struct_checker to tolerate lambda types
add experiments to control enabling of various aspects of lambda support:
- LAMBDA_IN_PARAMS, LAMBDA_IN_RETURNS to allow lambdas as general function params or results
- LAMBDA_FIELDS - support lambdas in struct fields
- LAMBDA_VALUES - support lambdas in general-purpose values
- may add LAMBDA_TYPE_PARAMS later
update lambda-lifting test config to use those flags rather than disabling checks
add a result_type_loc field to move-model's FunctionData (and model-builder's FunEntry)
to more precisely locate errors in function result types.
Fix GlobalEnv::internal_dump_env to use function- and struct-specific type contexts
so that types are shown with correct type parameter names.
Check that struct fields are not functions.

How Has This Been Tested?

Running the usual tests.

Key Areas to Review

Note that a few errors are caught quite late, in bytecode-generator,
and the order in which we catch errors may be a bit surprising to
users (errors in exp_builder come first, then errors caught by
function_checker, then (much later) bytecode_generator.).
Actually supporting particular subsets of the "experiments" mentioned
above would be difficult, but it seems useful to have the labels to
help understand just what features are supported by the implementation
as we go.

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-09-24T20:57:57Z

⏱️ 2h 7m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
rust-move-unit-coverage	17m	🟩
rust-move-unit-coverage	11m	🟩
rust-move-unit-coverage	11m	🟩
rust-move-tests	10m	🟩
rust-move-tests	10m	🟩
rust-move-tests	10m	🟩
rust-move-tests	10m	🟩
rust-move-unit-coverage	10m	🟩
rust-move-unit-coverage	9m	⬜
rust-move-tests	9m	⬜
rust-cargo-deny	8m	🟩 🟩 🟩 🟩 🟩
check-dynamic-deps	5m	🟩 🟩 🟩 🟩 🟩
general-lints	4m	🟩 🟩 🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩
file_change_determinator	54s	🟩 🟩 🟩 🟩 🟩

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

brmataptos · 2024-09-24T20:58:10Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @brmataptos and the rest of your teammates on Graphite

codecov · 2024-09-24T21:09:33Z

Codecov Report

Attention: Patch coverage is 85.32819% with 38 lines in your changes missing coverage. Please review.

Project coverage is 60.1%. Comparing base (cbb422d) to head (f72846f).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...e-compiler-v2/src/env_pipeline/function_checker.rs	88.6%	14 Missing ⚠️
...r-v2/src/file_format_generator/module_generator.rs	0.0%	10 Missing ⚠️
...ty/move/move-compiler-v2/src/bytecode_generator.rs	55.5%	8 Missing ⚠️
...piler-v2/src/env_pipeline/unused_params_checker.rs	33.3%	4 Missing ⚠️
third_party/move/move-model/src/ty.rs	93.5%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #14742    +/-   ##
========================================
  Coverage    60.1%    60.1%            
========================================
  Files         856      856            
  Lines      210640   210845   +205     
========================================
+ Hits       126615   126742   +127     
- Misses      84025    84103    +78

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vineethk

Looking good, ~~can approve once the comment in unused_params_checker.rs is addressed.~~

vineethk · 2024-09-29T20:56:52Z

third_party/move/move-compiler-v2/src/env_pipeline/function_checker.rs


 type QualifiedFunId = QualifiedId<FunId>;

-/// check that non-inline function parameters do not have function type.
+fn type_has_function(ty: &Type) -> bool {


Might be useful to move this into impl Type, alongside is_function (and we can call is just has_function).

vineethk · 2024-09-29T21:01:16Z

third_party/move/move-compiler-v2/src/env_pipeline/function_checker.rs

+}
+
+// Takes a list of function types, returns those which have a function type in their argument type
+fn identify_function_types_with_functions_in_args(func_returns: Vec<Type>) -> Vec<Type> {


Because this function a list of function types, could we call the param func_types or even just types? Not clear from the local context why this param is called func_returns.

third_party/move/move-compiler-v2/src/env_pipeline/unused_params_checker.rs

…r more complete lambda implementation

…eeth.

github-actions · 2024-10-10T04:50:34Z

✅ Forge suite `compat` success on `46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775` ==> `f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b`

Compatibility test results for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b (PR)
1. Check liveness of validators at old version: 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775
compatibility::simple-validator-upgrade::liveness-check : committed: 12777.46 txn/s, latency: 2509.36 ms, (p50: 2100 ms, p70: 2200, p90: 4100 ms, p99: 10300 ms), latency samples: 493540
2. Upgrading first Validator to new version: f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6970.03 txn/s, latency: 4016.74 ms, (p50: 4500 ms, p70: 4900, p90: 5000 ms, p99: 5200 ms), latency samples: 127520
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6748.52 txn/s, latency: 4819.96 ms, (p50: 5300 ms, p70: 5400, p90: 5500 ms, p99: 5600 ms), latency samples: 223360
3. Upgrading rest of first batch to new version: f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7115.39 txn/s, latency: 3961.75 ms, (p50: 4500 ms, p70: 4800, p90: 4900 ms, p99: 5100 ms), latency samples: 130240
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6628.36 txn/s, latency: 4812.93 ms, (p50: 4800 ms, p70: 4900, p90: 7400 ms, p99: 7700 ms), latency samples: 234680
4. upgrading second batch to new version: f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 7043.81 txn/s, latency: 4026.71 ms, (p50: 2800 ms, p70: 5800, p90: 7400 ms, p99: 10200 ms), latency samples: 128400
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 9836.98 txn/s, latency: 2895.33 ms, (p50: 2600 ms, p70: 3200, p90: 3600 ms, p99: 8200 ms), latency samples: 381160
5. check swarm health
Compatibility test for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b passed
Test Ok

github-actions · 2024-10-10T04:51:07Z

✅ Forge suite `realistic_env_max_load` success on `f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b`

two traffics test: inner traffic : committed: 13755.02 txn/s, latency: 2894.11 ms, (p50: 2700 ms, p70: 3000, p90: 3000 ms, p99: 6900 ms), latency samples: 5229940
two traffics test : committed: 100.09 txn/s, latency: 2500.46 ms, (p50: 2400 ms, p70: 2500, p90: 2800 ms, p99: 4000 ms), latency samples: 1720
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.235, avg: 0.218", "QsPosToProposal: max: 0.289, avg: 0.247", "ConsensusProposalToOrdered: max: 0.334, avg: 0.298", "ConsensusOrderedToCommit: max: 0.493, avg: 0.458", "ConsensusProposalToCommit: max: 0.791, avg: 0.757"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.21s no progress at version 2780885 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.29s no progress at version 2780883 (avg 8.29s) [limit 15].
Test Ok

github-actions · 2024-10-10T04:52:14Z

✅ Forge suite `framework_upgrade` success on `46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775` ==> `f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b`

Compatibility test results for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b (PR)
Upgrade the nodes to version: f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1399.35 txn/s, submitted: 1402.17 txn/s, failed submission: 2.82 txn/s, expired: 2.82 txn/s, latency: 2313.21 ms, (p50: 2100 ms, p70: 2400, p90: 3600 ms, p99: 4800 ms), latency samples: 119120
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1279.93 txn/s, submitted: 1281.52 txn/s, failed submission: 1.59 txn/s, expired: 1.59 txn/s, latency: 2384.74 ms, (p50: 2100 ms, p70: 2400, p90: 3900 ms, p99: 5100 ms), latency samples: 113000
5. check swarm health
Compatibility test for 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b passed
Upgrade the remaining nodes to version: f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1259.74 txn/s, submitted: 1262.41 txn/s, failed submission: 2.67 txn/s, expired: 2.67 txn/s, latency: 2473.87 ms, (p50: 2400 ms, p70: 2700, p90: 3900 ms, p99: 5100 ms), latency samples: 113260
Test Ok

brmataptos requested review from fEst1ck, rahxephon89, vineethk and wrwg September 24, 2024 20:59

brmataptos marked this pull request as ready for review September 24, 2024 20:59

brmataptos changed the title ~~fix #14633: fix error messages for lambdas, identify change points for more complete lambda implementation~~ [move-compiler-v2] fix #14633: fix error messages for lambdas, identify change points for more complete lambda implementation Sep 24, 2024

brmataptos force-pushed the 09-24-fix_14633_fix_error_messages_for_lambdas_identify_change_points_for_more_complete_lambda_implementation branch from cac57d5 to 61459eb Compare September 28, 2024 04:23

brmataptos mentioned this pull request Sep 28, 2024

[compiler-v2] add test cases for lambda #14791

Merged

22 tasks

brmataptos force-pushed the 09-24-fix_14633_fix_error_messages_for_lambdas_identify_change_points_for_more_complete_lambda_implementation branch from 61459eb to 0f3224a Compare September 28, 2024 04:32

brmataptos mentioned this pull request Sep 28, 2024

[move-compiler-v2] add parser code for lambda types #14792

Open

22 tasks

vineethk reviewed Sep 29, 2024

View reviewed changes

vineethk approved these changes Sep 30, 2024

View reviewed changes

rahxephon89 approved these changes Oct 2, 2024

View reviewed changes

brmataptos force-pushed the 09-24-fix_14633_fix_error_messages_for_lambdas_identify_change_points_for_more_complete_lambda_implementation branch 3 times, most recently from a050828 to 8a317b8 Compare October 3, 2024 17:01

brmataptos added 6 commits October 9, 2024 21:08

fix #14633: fix error messages for lambdas, identify change points fo…

7c8f264

…r more complete lambda implementation

Response to Vineeth's review

b64a50c

Final response to Vineeth's review

c427bf6

fix lint

f8612d6

move type_has_function into Type as has_function, as suggested by Vin…

fa2ff8b

…eeth.

add type param list for struct in dump

f72846f

brmataptos force-pushed the 09-24-fix_14633_fix_error_messages_for_lambdas_identify_change_points_for_more_complete_lambda_implementation branch from 8a317b8 to f72846f Compare October 10, 2024 04:17

brmataptos enabled auto-merge (squash) October 10, 2024 04:21

brmataptos disabled auto-merge October 10, 2024 04:22

brmataptos enabled auto-merge (squash) October 10, 2024 04:24

This comment has been minimized.

Sign in to view

brmataptos merged commit 918b643 into main Oct 10, 2024
121 of 141 checks passed

brmataptos deleted the 09-24-fix_14633_fix_error_messages_for_lambdas_identify_change_points_for_more_complete_lambda_implementation branch October 10, 2024 04:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move-compiler-v2] fix #14633: fix error messages for lambdas, identify change points for more complete lambda implementation #14742

[move-compiler-v2] fix #14633: fix error messages for lambdas, identify change points for more complete lambda implementation #14742

brmataptos commented Sep 24, 2024 •

edited

Loading

trunk-io bot commented Sep 24, 2024 •

edited

Loading

brmataptos commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

vineethk left a comment •

edited

Loading

vineethk Sep 29, 2024

vineethk Sep 29, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

[move-compiler-v2] fix #14633: fix error messages for lambdas, identify change points for more complete lambda implementation #14742

[move-compiler-v2] fix #14633: fix error messages for lambdas, identify change points for more complete lambda implementation #14742

Conversation

brmataptos commented Sep 24, 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 Sep 24, 2024 • edited Loading

brmataptos commented Sep 24, 2024 • edited Loading

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

vineethk left a comment • edited Loading

Choose a reason for hiding this comment

vineethk Sep 29, 2024

Choose a reason for hiding this comment

vineethk Sep 29, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Oct 10, 2024

✅ Forge suite compat success on 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b

github-actions bot commented Oct 10, 2024

✅ Forge suite realistic_env_max_load success on f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b

github-actions bot commented Oct 10, 2024

✅ Forge suite framework_upgrade success on 46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775 ==> f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b

brmataptos commented Sep 24, 2024 •

edited

Loading

trunk-io bot commented Sep 24, 2024 •

edited

Loading

brmataptos commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

vineethk left a comment •

edited

Loading

✅ Forge suite `compat` success on `46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775` ==> `f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b`

✅ Forge suite `realistic_env_max_load` success on `f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b`

✅ Forge suite `framework_upgrade` success on `46bf19eb4f132b9d8fc19eff3f3334cdf9aa1775` ==> `f72846fa63678a0cfb7ea0c682b1aa9c4fa2b48b`