[move-compiler-v2] clean up a few remaining issues in lambda parser/front-end code #15365

brmataptos · 2024-11-22T01:38:51Z

Description

Various fixes:

Remove Constraint::NoFunction as it's too broad.
Make sure there are still errors for all planned unsupported cases
Fix LambdaLifter case for bound free variable without copy.
Distinguish between "not supported" and "not implemented" for lambda features.
Refine lambda test cases to make _ok versions that compile up to "not implemented" errors.
Add a test case to illustrate the function name aliasing issue ([Bug][move-compiler-v2] visible function names shadow local variables with same name in function calls #15360), but changed other tests here to avoid it.

How Has This Been Tested?

Usual tests run, showing unchanged behavior on existing code. Tests under /lambda/ without .lambda. config show proper errors generated if to many functions are used. _ok tests now show code working up until "not implemented" errors.

Key Areas to Review

Most complex stuff is in previous PR, but:

Am I missing some ability cases (e.g., filtering for store and copy properties in captured free variables?
Are closures with references getting through?
Do /lambda/storage/*_ok.move tests look like reasonable code to expect from users?

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-11-22T01:38:54Z

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

Job	Cumulative Duration	Recent Runs
rust-move-tests	12m	🟩
rust-move-tests	12m	🟥
rust-move-tests	12m	🟥
rust-move-tests	12m	🟥
rust-move-tests	12m	🟥
rust-cargo-deny	9m	🟩 🟩 🟩 🟩 🟩
check-dynamic-deps	4m	🟩 🟩 🟩 🟩 🟩
general-lints	2m	🟩 🟩 🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩
file_change_determinator	57s	🟩 🟩 🟩 🟩 🟩
permission-check	21s	🟩 🟩 🟩 🟩 🟩
permission-check	14s	🟩 🟩 🟩 🟩 🟩
check-branch-prefix	1s	🟩

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

brmataptos · 2024-11-22T01:39:06Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

rahxephon89 · 2024-11-23T06:22:03Z

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

+            env.error(
+                &loc,
+                // TODO(LAMBDA)
+                "Lambdas expressions with `store` ability currently may only be a simple call to an existing `public` function.  This lambda expression requires defining a `public` helper function, which might affect module upgradeability and is not yet supported."


This error message looks a little bit confusing for users (e.g what is a helper function and how it may affect upgradeability). Maybe just remove requires defining a public helper function, which might affect module upgradeability` and just saying it is not supported yet?

rahxephon89 · 2024-11-23T06:25:48Z

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

+                env.error(
+                    &loc,
+                    &format!(
+                        "captured variable `{}` must have a value with `copy` ability", // TODO(LAMBDA)


Is there a test case for this error?

added closure_args.move, see closure_args.lambda.exp.

This exercises a lot of cases.

vineethk · 2024-11-26T22:21:49Z

third_party/move/move-compiler-v2/src/bytecode_generator.rs

-            self.emit_call(id, vec![target], BytecodeOperation::ReadRef, vec![
-                borrow_dest,
-            ])
+            self.emit_call(


Looks like formatting only change? When I run the formatter, it goes back to the original.

vineethk · 2024-11-26T22:22:22Z

third_party/move/move-compiler-v2/src/bytecode_generator.rs

-            probing_vars: Some(_),
-            ..
-        })
+        matches!(


Similar as above, when I run the formatter, this goes back to original.

should be fixed,

vineethk · 2024-11-26T22:27:33Z

third_party/move/move-compiler-v2/tests/checking/inlining/function_name_shadowing.move

+    }
+
+    public inline fun quux(f:|u64, u64|u64, a: u64, b: u64): u64 {
+        f(a, b)


Could we also add test cases where we have:

Use statement importing (at the module level and at the function level) a function foo, where foo is also a function parameter to an inline function like here.

Similar as above, but without explicit use statement and instead explicit qualification like other_module::foo.

This is rather tangential to this PR, but I've added use statements. The explicit qualification case is not interesting, as we know what it will do.

What is happening is very clear if you look at move-compiler/src/expander/translation.rs and .../aliases.rs, which tracks nesting of use of symbols from other modules. All that is tracked are those imported symbols, not any local variables. A name in function call position (and a few other places) is turned into an explicit ModuleIdent::Name pair. A name in other positions may be left unevaluated until later.

vineethk · 2024-11-26T22:30:19Z

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

-                node_id,
-                modified: true,
-            });
+            self.free_locals.insert(


A number of changes here seem to be only whitespace changes that go away when I format. Any idea how these changes came by?

Merge conflicts? Also, Emacs sometimes indents differently than the Rust usual.

vineethk · 2024-11-26T22:31:24Z

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

+        let Some((mut params, mut closure_args, param_index_mapping)) =
+            self.get_params_for_freevars()
+        else {
+            return None;


I think the ? operator fits well here.

Done, though I personally find it a little too subtle.

brmataptos mentioned this pull request Nov 22, 2024

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

Open

22 tasks

This was referenced Nov 22, 2024

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

Merged

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

Merged

brmataptos changed the title ~~lambda-compiler-completion~~ [move-compiler-v2] clean up a few remaining issues in lambda parser/front-end code Nov 22, 2024

brmataptos marked this pull request as ready for review November 22, 2024 01:45

brmataptos force-pushed the 11-21-lambda-compiler-completion branch from 3e3563e to dac085c Compare November 22, 2024 21:39

brmataptos requested review from fEst1ck, rahxephon89, vineethk and wrwg November 22, 2024 21:41

rahxephon89 reviewed Nov 23, 2024

View reviewed changes

brmataptos force-pushed the 09-27-add_parser_code_for_lambda_types branch from 9395a8f to cead217 Compare November 25, 2024 04:53

brmataptos force-pushed the 11-21-lambda-compiler-completion branch from dac085c to 42421fe Compare November 25, 2024 04:54

brmataptos mentioned this pull request Nov 25, 2024

Lambda support in VM: Wolfgang's PR plus some simplifications (removing mask, clarifying ops) #15387

Open

vineethk reviewed Nov 26, 2024

View reviewed changes

brmataptos force-pushed the 09-27-add_parser_code_for_lambda_types branch from cead217 to 44baaf0 Compare November 27, 2024 10:24

brmataptos force-pushed the 11-21-lambda-compiler-completion branch from 42421fe to 92ae56c Compare November 27, 2024 10:24

vineethk approved these changes Nov 27, 2024

View reviewed changes

brmataptos requested a review from rahxephon89 November 27, 2024 17:49

brmataptos added 2 commits November 27, 2024 14:41

lambda-compiler-completion

c9dfd1b

address PR 15365 comments as of 11/26

3c02ba1

brmataptos force-pushed the 11-21-lambda-compiler-completion branch from 92ae56c to 3c02ba1 Compare November 27, 2024 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move-compiler-v2] clean up a few remaining issues in lambda parser/front-end code #15365

[move-compiler-v2] clean up a few remaining issues in lambda parser/front-end code #15365

brmataptos commented Nov 22, 2024 •

edited

Loading

trunk-io bot commented Nov 22, 2024 •

edited

Loading

brmataptos commented Nov 22, 2024 •

edited

Loading

rahxephon89 Nov 23, 2024

brmataptos Nov 27, 2024

rahxephon89 Nov 23, 2024

brmataptos Nov 27, 2024

vineethk Nov 26, 2024

brmataptos Nov 27, 2024

vineethk Nov 26, 2024

brmataptos Nov 27, 2024

vineethk Nov 26, 2024

brmataptos Nov 27, 2024

vineethk Nov 26, 2024

brmataptos Nov 27, 2024

vineethk Nov 26, 2024

brmataptos Nov 27, 2024

[move-compiler-v2] clean up a few remaining issues in lambda parser/front-end code #15365

Are you sure you want to change the base?

[move-compiler-v2] clean up a few remaining issues in lambda parser/front-end code #15365

Conversation

brmataptos commented Nov 22, 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 Nov 22, 2024 • edited Loading

brmataptos commented Nov 22, 2024 • edited Loading

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 commented Nov 22, 2024 •

edited

Loading

trunk-io bot commented Nov 22, 2024 •

edited

Loading

brmataptos commented Nov 22, 2024 •

edited

Loading