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

…r more complete lambda implementation
aptos-labs · Sep 24, 2024 · 0cd189b · 0cd189b
1 parent 8689b9a
commit 0cd189b
Show file tree

Hide file tree

Showing 65 changed files with 1,972 additions and 126 deletions.
diff --git a/third_party/move/move-compiler-v2/src/bytecode_generator.rs b/third_party/move/move-compiler-v2/src/bytecode_generator.rs
@@ -406,9 +406,12 @@ impl<'env> Generator<'env> {
                         ),
                     );
                 }
-                self.emit_call(*id, targets, BytecodeOperation::WriteRef, vec![
-                    lhs_temp, rhs_temp,
-                ])
+                self.emit_call(
+                    *id,
+                    targets,
+                    BytecodeOperation::WriteRef,
+                    vec![lhs_temp, rhs_temp],
+                )
             },
             ExpData::Assign(id, lhs, rhs) => self.gen_assign(*id, lhs, rhs, None),
             ExpData::Return(id, exp) => {
@@ -476,9 +479,16 @@ impl<'env> Generator<'env> {
                     .rewrite_spec_descent(&SpecBlockTarget::Inline, spec);
                 self.emit_with(*id, |attr| Bytecode::SpecBlock(attr, spec));
             },
-            ExpData::Invoke(id, _, _) | ExpData::Lambda(id, _, _) => {
-                self.internal_error(*id, format!("not yet implemented: {:?}", exp))
-            },
+            // TODO(LAMBDA)
+            ExpData::Lambda(id, _, _) => self.error(
+                *id,
+                "Function-typed values not yet supported except as parameters to calls to inline functions",
+            ),
+            // TODO(LAMBDA)
+            ExpData::Invoke(_, exp, _) => self.error(
+                exp.as_ref().node_id(),
+                "Calls to function values other than inline function parameters not yet supported",
+            ),
             ExpData::Quant(id, _, _, _, _, _) => {
                 self.internal_error(*id, "unsupported specification construct")
             },
@@ -803,7 +813,11 @@ impl<'env> Generator<'env> {
 
             Operation::NoOp => {}, // do nothing
 
-            Operation::Closure(..) => self.internal_error(id, "closure not yet implemented"),
+            // TODO(LAMBDA)
+            Operation::Closure(..) => self.error(
+                id,
+                "Function-typed values not yet supported except as parameters to calls to inline functions",
+            ),
 
             // Non-supported specification related operations
             Operation::Exists(Some(_))

diff --git a/third_party/move/move-compiler-v2/src/env_pipeline/function_checker.rs b/third_party/move/move-compiler-v2/src/env_pipeline/function_checker.rs
@@ -3,7 +3,7 @@
 
 //! Do a few checks of functions and function calls.
 
-use crate::Options;
+use crate::{experiments::Experiment, Options};
 use codespan_reporting::diagnostic::Severity;
 use move_binary_format::file_format::Visibility;
 use move_model::{
@@ -17,26 +17,54 @@ type QualifiedFunId = QualifiedId<FunId>;
 
 /// check that non-inline function parameters do not have function type.
 pub fn check_for_function_typed_parameters(env: &mut GlobalEnv) {
+    let options = env
+        .get_extension::<Options>()
+        .expect("Options is available");
+    let lambda_params_ok = options.experiment_on(Experiment::LAMBDA_PARAMS);
+    let lambda_return_ok = options.experiment_on(Experiment::LAMBDA_RESULTS);
+    if lambda_params_ok && lambda_return_ok {
+        return;
+    }
+
     for caller_module in env.get_modules() {
         if caller_module.is_primary_target() {
             for caller_func in caller_module.get_functions() {
+                // Check that no functions have function return type
+                if !lambda_return_ok {
+                    let caller_name = caller_func.get_full_name_str();
+                    let return_type = caller_func.get_result_type();
+                    let has_function_value =
+                        return_type.clone().flatten().iter().any(Type::is_function);
+                    if has_function_value {
+                        let type_display_ctx = caller_func.get_type_display_ctx();
+                        env.diag(
+                            Severity::Error,
+                            &caller_func.get_id_loc(),
+                            &format!("Functions may not return function-typed values, but function `{}` return type is `{}`:",
+                                     caller_name,
+                                     return_type.display(&type_display_ctx)),
+                        );
+                    }
+                }
                 // Check that non-inline function parameters don't have function type
-                if !caller_func.is_inline() {
+                if !caller_func.is_inline() && !lambda_params_ok {
                     let parameters = caller_func.get_parameters();
                     let bad_params: Vec<_> = parameters
                         .iter()
                         .filter(|param| matches!(param.1, Type::Fun(_, _)))
                         .collect();
                     if !bad_params.is_empty() {
+                        let type_display_ctx = caller_func.get_type_display_ctx();
                         let caller_name = caller_func.get_full_name_str();
                         let reasons: Vec<(Loc, String)> = bad_params
                             .iter()
                             .map(|param| {
                                 (
                                     param.2.clone(),
                                     format!(
-                                        "Parameter `{}` has a function type.",
+                                        "Parameter `{}` has function-valued type `{}`.",
                                         param.0.display(env.symbol_pool()),
+                                        param.1.display(&type_display_ctx)
                                     ),
                                 )
                             })

diff --git a/third_party/move/move-compiler-v2/src/env_pipeline/lambda_lifter.rs b/third_party/move/move-compiler-v2/src/env_pipeline/lambda_lifter.rs
@@ -309,7 +309,7 @@ impl<'a> ExpRewriterFunctions for LambdaLifter<'a> {
                 env.error(
                     &loc,
                     &format!(
-                        "captured variable `{}` cannot be modified inside of a lambda",
+                        "captured variable `{}` cannot be modified inside of a lambda", // TODO(LAMBDA)
                         name.display(env.symbol_pool())
                     ),
                 );
@@ -327,7 +327,7 @@ impl<'a> ExpRewriterFunctions for LambdaLifter<'a> {
                 env.error(
                     &loc,
                     &format!(
-                        "captured variable `{}` cannot be modified inside of a lambda",
+                        "captured variable `{}` cannot be modified inside of a lambda", // TODO(LAMBDA)
                         name.display(env.symbol_pool())
                     ),
                 );

diff --git a/third_party/move/move-compiler-v2/src/env_pipeline/recursive_struct_checker.rs b/third_party/move/move-compiler-v2/src/env_pipeline/recursive_struct_checker.rs
@@ -101,7 +101,7 @@ impl<'a> RecursiveStructChecker<'a> {
                         self.report_invalid_field(&struct_env, &field_env);
                     }
                 },
-                Type::Primitive(_) | Type::TypeParameter(_) => {},
+                Type::Primitive(_) | Type::TypeParameter(_) | Type::Fun(..) => {},
                 _ => unreachable!("invalid field type"),
             }
             path.pop();

diff --git a/third_party/move/move-compiler-v2/src/env_pipeline/unused_params_checker.rs b/third_party/move/move-compiler-v2/src/env_pipeline/unused_params_checker.rs
@@ -55,12 +55,16 @@ fn used_type_parameters_in_fields(struct_env: &StructEnv) -> BTreeSet<u16> {
 fn used_type_parameters_in_ty(ty: &Type) -> BTreeSet<u16> {
     match ty {
         Type::Primitive(_) => BTreeSet::new(),
-        Type::Struct(_, _, tys) => tys.iter().flat_map(used_type_parameters_in_ty).collect(),
+        Type::Tuple(tys) | Type::Struct(_, _, tys) => {
+            tys.iter().flat_map(used_type_parameters_in_ty).collect()
+        },
         Type::TypeParameter(i) => BTreeSet::from([*i]),
         Type::Vector(ty) => used_type_parameters_in_ty(ty),
+        Type::Fun(t1, t2) => [t1, t2]
+            .iter()
+            .flat_map(|t| used_type_parameters_in_ty(t))
+            .collect(),
         Type::Reference(..)
-        | Type::Fun(..)
-        | Type::Tuple(..)
         | Type::TypeDomain(..)
         | Type::ResourceDomain(..)
         | Type::Error

diff --git a/third_party/move/move-compiler-v2/src/experiments.rs b/third_party/move/move-compiler-v2/src/experiments.rs
@@ -111,11 +111,32 @@ pub static EXPERIMENTS: Lazy<BTreeMap<String, Experiment>> = Lazy::new(|| {
             description: "Turns on or off specification rewriting".to_string(),
             default: Given(false),
         },
+        Experiment {
+            name: Experiment::LAMBDA_FIELDS.to_string(),
+            description: "Turns on or off function values in struct fields".to_string(),
+            default: Given(false),
+        },
         Experiment {
             name: Experiment::LAMBDA_LIFTING.to_string(),
             description: "Turns on or off lambda lifting".to_string(),
             default: Given(false),
         },
+        Experiment {
+            name: Experiment::LAMBDA_PARAMS.to_string(),
+            description: "Turns on or off function values as parameters to non-inline functions"
+                .to_string(),
+            default: Given(false),
+        },
+        Experiment {
+            name: Experiment::LAMBDA_RESULTS.to_string(),
+            description: "Turns on or off function values in function results".to_string(),
+            default: Given(false),
+        },
+        Experiment {
+            name: Experiment::LAMBDA_VALUES.to_string(),
+            description: "Turns on or off first-class function values".to_string(),
+            default: Given(false),
+        },
         Experiment {
             name: Experiment::RECURSIVE_TYPE_CHECK.to_string(),
             description: "Turns on or off checking of recursive structs and type instantiations"
@@ -275,7 +296,11 @@ impl Experiment {
     pub const INLINING: &'static str = "inlining";
     pub const KEEP_INLINE_FUNS: &'static str = "keep-inline-funs";
     pub const KEEP_UNINIT_ANNOTATIONS: &'static str = "keep-uninit-annotations";
+    pub const LAMBDA_FIELDS: &'static str = "lambda-fields";
     pub const LAMBDA_LIFTING: &'static str = "lambda-lifting";
+    pub const LAMBDA_PARAMS: &'static str = "lambda-params";
+    pub const LAMBDA_RESULTS: &'static str = "lambda-results";
+    pub const LAMBDA_VALUES: &'static str = "lambda-values";
     pub const LINT_CHECKS: &'static str = "lint-checks";
     pub const OPTIMIZE: &'static str = "optimize";
     pub const OPTIMIZE_EXTRA: &'static str = "optimize-extra";

diff --git a/third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs b/third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs
@@ -365,7 +365,20 @@ impl ModuleGenerator {
                     ReferenceKind::Mutable => FF::SignatureToken::MutableReference(target_ty),
                 }
             },
-            Fun(_, _) | TypeDomain(_) | ResourceDomain(_, _, _) | Error | Var(_) => {
+            Fun(_param_ty, _result_ty) => {
+                // let _param_ty = Box::new(self.signature_token(ctx, loc, param_ty));
+                // let _result_ty = Box::new(self.signature_token(ctx, loc, result_ty));
+                // TODO(LAMBDA)
+                ctx.error(
+                    loc,
+                    format!(
+                        "Unexpected type: {}",
+                        ty.display(&ctx.env.get_type_display_ctx())
+                    ),
+                );
+                FF::SignatureToken::Bool
+            },
+            TypeDomain(_) | ResourceDomain(_, _, _) | Error | Var(_) => {
                 ctx.internal_error(
                     loc,
                     format!(

diff --git a/third_party/move/move-compiler-v2/tests/bytecode-generator/bug_14471_receiver_inference.exp b/third_party/move/move-compiler-v2/tests/bytecode-generator/bug_14471_receiver_inference.exp
@@ -4,8 +4,8 @@ module 0x815::m {
         table: m::Table<address, m::ValueWrap>,
     }
     struct Table {
-        x: #0,
-        y: #1,
+        x: T1,
+        y: T2,
     }
     struct ValueWrap {
         val: u64,

diff --git a/third_party/move/move-compiler-v2/tests/bytecode-generator/fields.exp b/third_party/move/move-compiler-v2/tests/bytecode-generator/fields.exp
@@ -4,7 +4,7 @@ module 0x42::fields {
         h: u64,
     }
     struct G {
-        f: #0,
+        f: X,
     }
     struct S {
         f: u64,

diff --git a/third_party/move/move-compiler-v2/tests/bytecode-generator/matching_ok.exp b/third_party/move/move-compiler-v2/tests/bytecode-generator/matching_ok.exp
@@ -42,7 +42,7 @@ module 0xc0ffee::m {
     enum Option {
         None,
         Some {
-            value: #0,
+            value: A,
         }
     }
     enum Outer {

diff --git a/third_party/move/move-compiler-v2/tests/checking/abilities/v1/ability_constraints.exp b/third_party/move/move-compiler-v2/tests/checking/abilities/v1/ability_constraints.exp
@@ -1,11 +1,11 @@
 // -- Model dump before bytecode pipeline
 module 0x42::M {
     struct Box {
-        f: #0,
+        f: T,
     }
     struct Pair {
-        f1: #0,
-        f2: #1,
+        f1: T1,
+        f2: T2,
     }
     struct R {
         dummy_field: bool,

diff --git a/third_party/move/move-compiler-v2/tests/checking/abilities/v1/phantom_param_op_abilities.exp b/third_party/move/move-compiler-v2/tests/checking/abilities/v1/phantom_param_op_abilities.exp
@@ -1,22 +1,22 @@
 // -- Model dump before bytecode pipeline
 module 0x42::M {
     struct HasCopy {
-        a: #1,
+        a: T2,
     }
     struct HasDrop {
-        a: #1,
+        a: T2,
     }
     struct HasKey {
-        a: #1,
+        a: T2,
     }
     struct HasStore {
-        a: #1,
+        a: T2,
     }
     struct NoAbilities {
         dummy_field: bool,
     }
     struct RequireStore {
-        a: #0,
+        a: T,
     }
     private fun f1(ref: &mut M::HasDrop<M::NoAbilities, u64>) {
         ref = pack M::HasDrop<M::NoAbilities, u64>(1);

diff --git a/...move/move-compiler-v2/tests/checking/abilities/v1/phantom_params_constraint_abilities.exp b/...move/move-compiler-v2/tests/checking/abilities/v1/phantom_params_constraint_abilities.exp
@@ -1,34 +1,34 @@
 // -- Model dump before bytecode pipeline
 module 0x42::M {
     struct HasAbilities {
-        a: #1,
+        a: T2,
     }
     struct HasCopy {
-        a: #1,
+        a: T2,
     }
     struct HasDrop {
-        a: #1,
+        a: T2,
     }
     struct HasKey {
-        a: #1,
+        a: T2,
     }
     struct HasStore {
-        a: #1,
+        a: T2,
     }
     struct NoAbilities {
         a: bool,
     }
     struct S1 {
-        a: #0,
+        a: T,
     }
     struct S2 {
         a: M::S1<M::HasAbilities<M::NoAbilities, u64>>,
     }
     struct S3 {
-        a: #0,
-        b: #1,
-        c: #2,
-        d: #3,
+        a: T1,
+        b: T2,
+        c: T3,
+        d: T4,
     }
     struct S4 {
         a: M::S3<M::HasDrop<M::NoAbilities, u64>, M::HasCopy<M::NoAbilities, u64>, M::HasStore<M::NoAbilities, u64>, M::HasKey<M::NoAbilities, u64>>,

diff --git a/...arty/move/move-compiler-v2/tests/checking/abilities/v1/phantom_params_field_abilities.exp b/...arty/move/move-compiler-v2/tests/checking/abilities/v1/phantom_params_field_abilities.exp
@@ -1,16 +1,16 @@
 // -- Model dump before bytecode pipeline
 module 0x42::M {
     struct HasCopy {
-        a: #1,
+        a: T2,
     }
     struct HasDrop {
-        a: #1,
+        a: T2,
     }
     struct HasKey {
-        a: #1,
+        a: T2,
     }
     struct HasStore {
-        a: #1,
+        a: T2,
     }
     struct NoAbilities {
         dummy_field: bool,

diff --git a/third_party/move/move-compiler-v2/tests/checking/dotdot/dotdot_valid.exp b/third_party/move/move-compiler-v2/tests/checking/dotdot/dotdot_valid.exp
@@ -28,16 +28,16 @@ module 0x42::test {
         y: u8,
     }
     struct S4 {
-        x: #0,
+        x: T,
         y: test::S3,
     }
     struct S5 {
-        0: #0,
-        1: #1,
+        0: T,
+        1: U,
     }
     struct S6 {
-        x: #0,
-        y: #1,
+        x: T,
+        y: U,
     }
     struct S7 {
         0: u8,