[move-compiler-v2] Generate suitable error messages for current misuse of function-typed values, lay groundwork for implementing and testing more general lambdas. #14732

brmataptos · 2024-09-24T02:30:19Z

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
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_PARAMS, LAMBDA_RESULTS 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

… values, lay groundwork for implementing and testing more general lambdas. Fixes #14633. Catch all errors in test/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. - add checking for function-typed function results in `function_checker` - 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_PARAMS, LAMBDA_RESULTS 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.

trunk-io · 2024-09-24T02:30:22Z

⏱️ 29m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-unit-coverage	15m	🟩
rust-move-tests	9m	🟩
general-lints	2m	🟩
rust-cargo-deny	2m	🟩
check-dynamic-deps	37s	🟩
semgrep/ci	22s	🟩
file_change_determinator	10s	🟩
file_change_determinator	10s	🟩
permission-check	3s	🟩
permission-check	2s	🟩
permission-check	2s	🟩
permission-check	2s	🟩

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

codecov · 2024-09-24T02:46:30Z

Codecov Report

Attention: Patch coverage is 76.19048% with 30 lines in your changes missing coverage. Please review.

Project coverage is 59.8%. Comparing base (e87224f) to head (e1c6588).
Report is 313 commits behind head on main.

Files with missing lines	Patch %	Lines
...r-v2/src/file_format_generator/module_generator.rs	0.0%	12 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/model.rs	70.0%	3 Missing ⚠️
third_party/move/move-model/src/ty.rs	92.0%	2 Missing ⚠️
...e-compiler-v2/src/env_pipeline/function_checker.rs	96.4%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (e87224f) and HEAD (e1c6588). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (e87224f) HEAD (e1c6588)

2 1

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #14732       +/-   ##
===========================================
- Coverage    71.9%    59.8%    -12.1%     
===========================================
  Files        2395      852     -1543     
  Lines      482134   207572   -274562     
===========================================
- Hits       346822   124307   -222515     
+ Misses     135312    83265    -52047

Flag	Coverage Δ
	`59.8% <76.1%> (-12.1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

vineethk · 2024-09-24T20:47:24Z

third_party/move/move-model/src/ty.rs

@@ -552,6 +567,7 @@ impl Constraint {
            Constraint::NoPhantom,
            Constraint::NoReference,
            Constraint::NoTuple,
+            Constraint::NoFunction,


Will we not support a vector of lambdas (of the same function type)? Or is this a short-term limitation that might be lifted later (if so, a todo might be good)?

I think this is fairly self-evident. Note that someday we may support references or tuples in type parameters and/or vectors, but there are no TODOs there. But I'm adding a comment.

vineethk · 2024-09-24T21:01:00Z

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

@@ -3977,6 +3979,9 @@ pub struct FunctionData {
    /// Location of the function identifier, suitable for error messages alluding to the function.
    pub(crate) id_loc: Loc,

+    /// Location of the function result type, suitable for error messages alluding to the result type.
+    pub(crate) result_type_loc: Loc,


nit: since we have 3 fields to track various locations pertaining to a function, may be it should be its own struct (in case we want to further extend it)?

Kind of nitty. But I''m doing that.

vineethk · 2024-09-24T21:04:01Z

third_party/move/move-compiler/tests/move_check/typing/lambda2.move

+
+    public fun fun_arg_lambda_not_allowed(x: |u64|) {} // expected lambda not allowed
+
+    public inline fun macro_result_lambda_not_allowed(): |u64| {  // expected lambda not allowed


Suggested change

public inline fun macro_result_lambda_not_allowed(): |u64| { // expected lambda not allowed

public inline fun inline_result_lambda_not_allowed(): |u64| { // expected lambda not allowed

I think we only call assert! a macro currently.

This test case came from existing test lambda.move.

vineethk · 2024-09-24T21:05:40Z

third_party/move/move-compiler/tests/move_check/typing/lambda2.move

+module 0x8675309::M {
+    // use 0x1::XVector;
+
+    // public inline fun foreach<T>(v: &vector<T>, action: |&T|) { // expected to be not implemented


We seem to have a lot of commented code in this test. Should it be removed? If not, would be good to leave some notes as to why the commented out code should be left around.

I'm adding a comment at the top to try to clarify what we're doing:

// *** NOTE: THIS TEST FILE IS DERIVED FROM lambda.move by commenting out code which has errors // successfully flagged by move-compiler-v2, so that we can check for errors on other lines // which may be shadowed by those errors. // // We keep the commented code so that the error line numbers line up. // // // ... code removed here to allow above message ... //

Hopefully that is sufficient.

vineethk · 2024-09-24T21:06:51Z

third_party/move/move-compiler/tests/move_check/typing/lambda2.exp

+   ┌─ tests/move_check/typing/lambda2.move:77:18
+   │
+77 │         let _x = |i| i + 1; // expected lambda not allowed
+   │                  ^^^^^^^^^ function type only allowed for inline function arguments


This is a function value and not function type?

Changed to "function-typed values" here.

vineethk · 2024-09-24T21:44:08Z

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

+        let _x = |i| i + 1; // expected lambda not allowed
+    }
+
+    // struct FieldFunNotAllowed {


Why is this commented out? Don't we want some of this to be checked here?

vineethk · 2024-09-24T21:44:22Z

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

+module 0x8675309::M {
+    // use 0x1::XVector;
+
+    // public inline fun foreach<T>(v: &vector<T>, action: |&T|) { // expected to be not implemented


Uncomment or remove.

I've added a comment at the top to explain what we're doing here, commenting out lines to suppress errors that were shown for lambda2.move, so that shadowed errors can be revealed while keeping line numbers identical to facilitate output matching with V1.

vineethk · 2024-09-24T21:44:41Z

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

+
+    // public fun fun_arg_lambda_not_allowed(x: |u64|) {} // expected lambda not allowed
+
+    public inline fun macro_result_lambda_not_allowed(): |u64| {  // expected lambda not allowed


Suggested change

public inline fun macro_result_lambda_not_allowed(): |u64| { // expected lambda not allowed

public inline fun inline_result_lambda_not_allowed(): |u64| { // expected lambda not allowed

vineethk · 2024-09-24T21:45:01Z

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

+module 0x8675309::M {
+    // use 0x1::XVector;
+
+    // public inline fun foreach<T>(v: &vector<T>, action: |&T|) { // expected to be not implemented


Not sure I understand the logic behind having multiple test files with a bunch of code commented out and some code uncommented. Maybe these could be split into multiple test files without duplication?

See responses elsewhere.

vineethk · 2024-09-24T22:11:55Z

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

+module 0x8675309::M {
+    // use 0x1::XVector;
+
+    // public inline fun foreach<T>(v: &vector<T>, action: |&T|) { // expected to be not implemented


Bunch of commented out code.

The idea of these heavily commented-out tests is that lambda.move is the original test, with no comments. Some errors don't show up, so we comment out the code generating errors on lambda.move, and call that lambda2.move. That test shows certain other errors, so we comment out the relevant lines there, giving lambda3.move, etc. I left the commented-out lines to keep the line numbers the same. Since there is an empty line, I'll add a comment on that line to indicate what we are doing.

github-actions · 2024-11-12T01:59:56Z

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

brmataptos requested review from wrwg, vineethk, rahxephon89 and fEst1ck September 24, 2024 02:30

brmataptos marked this pull request as draft September 24, 2024 21:00

brmataptos removed request for vineethk, rahxephon89, wrwg and fEst1ck September 24, 2024 21:01

vineethk reviewed Sep 24, 2024

View reviewed changes

github-actions bot added Stale and removed Stale labels Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move-compiler-v2] Generate suitable error messages for current misuse of function-typed values, lay groundwork for implementing and testing more general lambdas. #14732

[move-compiler-v2] Generate suitable error messages for current misuse of function-typed values, lay groundwork for implementing and testing more general lambdas. #14732

brmataptos commented Sep 24, 2024 •

edited

Loading

trunk-io bot commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

vineethk Sep 24, 2024

brmataptos Sep 25, 2024

vineethk Sep 24, 2024

brmataptos Sep 25, 2024

vineethk Sep 24, 2024

vineethk Sep 24, 2024

brmataptos Sep 25, 2024

vineethk Sep 24, 2024

brmataptos Sep 25, 2024

vineethk Sep 24, 2024

brmataptos Sep 25, 2024

vineethk Sep 24, 2024

vineethk Sep 24, 2024

brmataptos Sep 27, 2024

vineethk Sep 24, 2024

vineethk Sep 24, 2024

brmataptos Sep 27, 2024

vineethk Sep 24, 2024

brmataptos Sep 25, 2024

github-actions bot commented Nov 12, 2024


		public fun fun_arg_lambda_not_allowed(x: \|u64\|) {} // expected lambda not allowed

		public inline fun macro_result_lambda_not_allowed(): \|u64\| { // expected lambda not allowed

	public inline fun macro_result_lambda_not_allowed(): \|u64\| { // expected lambda not allowed
	public inline fun inline_result_lambda_not_allowed(): \|u64\| { // expected lambda not allowed


		// public fun fun_arg_lambda_not_allowed(x: \|u64\|) {} // expected lambda not allowed

		public inline fun macro_result_lambda_not_allowed(): \|u64\| { // expected lambda not allowed

[move-compiler-v2] Generate suitable error messages for current misuse of function-typed values, lay groundwork for implementing and testing more general lambdas. #14732

Are you sure you want to change the base?

[move-compiler-v2] Generate suitable error messages for current misuse of function-typed values, lay groundwork for implementing and testing more general lambdas. #14732

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

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

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

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

github-actions bot commented Nov 12, 2024

brmataptos commented Sep 24, 2024 •

edited

Loading

trunk-io bot commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading