Skip to content

Commit

Permalink
Generate suitable error messages for current misuse of function-typed…
Browse files Browse the repository at this point in the history
… 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.
  • Loading branch information
brmataptos committed Sep 24, 2024
1 parent ac60da1 commit e1c6588
Show file tree
Hide file tree
Showing 65 changed files with 1,972 additions and 126 deletions.
28 changes: 21 additions & 7 deletions third_party/move/move-compiler-v2/src/bytecode_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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",
),

Check warning on line 491 in third_party/move/move-compiler-v2/src/bytecode_generator.rs

View check run for this annotation

Codecov / codecov/patch

third_party/move/move-compiler-v2/src/bytecode_generator.rs#L488-L491

Added lines #L488 - L491 were not covered by tests
ExpData::Quant(id, _, _, _, _, _) => {
self.internal_error(*id, "unsupported specification construct")
},
Expand Down Expand Up @@ -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",
),

Check warning on line 820 in third_party/move/move-compiler-v2/src/bytecode_generator.rs

View check run for this annotation

Codecov / codecov/patch

third_party/move/move-compiler-v2/src/bytecode_generator.rs#L817-L820

Added lines #L817 - L820 were not covered by tests

// Non-supported specification related operations
Operation::Exists(Some(_))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand All @@ -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 warning on line 48 in third_party/move/move-compiler-v2/src/env_pipeline/function_checker.rs

View check run for this annotation

Codecov / codecov/patch

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

Added line #L48 was not covered by tests
// 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)
),
)
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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())
),
);
Expand All @@ -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())
),
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(),

Check warning on line 66 in third_party/move/move-compiler-v2/src/env_pipeline/unused_params_checker.rs

View check run for this annotation

Codecov / codecov/patch

third_party/move/move-compiler-v2/src/env_pipeline/unused_params_checker.rs#L63-L66

Added lines #L63 - L66 were not covered by tests
Type::Reference(..)
| Type::Fun(..)
| Type::Tuple(..)
| Type::TypeDomain(..)
| Type::ResourceDomain(..)
| Type::Error
Expand Down
25 changes: 25 additions & 0 deletions third_party/move/move-compiler-v2/src/experiments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Check warning on line 379 in third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs

View check run for this annotation

Codecov / codecov/patch

third_party/move/move-compiler-v2/src/file_format_generator/module_generator.rs#L368-L379

Added lines #L368 - L379 were not covered by tests
},
TypeDomain(_) | ResourceDomain(_, _, _) | Error | Var(_) => {
ctx.internal_error(
loc,
format!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module 0x42::fields {
h: u64,
}
struct G {
f: #0,
f: X,
}
struct S {
f: u64,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ module 0xc0ffee::m {
enum Option {
None,
Some {
value: #0,
value: A,
}
}
enum Outer {
Expand Down
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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>>,
Expand Down
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit e1c6588

Please sign in to comment.