From 9e66c4133702aaee4ce329c1babfb043085e3830 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Fri, 16 Feb 2024 20:01:11 +0100 Subject: [PATCH 1/4] Apply pattern contracts in match expressions Match expressions have been extended to accept the full range of patterns (that was reserved to destructuring before). Patterns allow, in particular, to annotate fields with metadata, such as contract annotations or default values. Those were, until now, simply ignored by the new match expressions. This commit modifies the pattern compilation scheme to handle contracts and default values properly. --- core/src/eval/operation.rs | 2 + core/src/term/mod.rs | 15 ++- core/src/term/pattern/compile.rs | 194 +++++++++++++++++++++++-------- core/src/term/record.rs | 8 ++ 4 files changed, 169 insertions(+), 50 deletions(-) diff --git a/core/src/eval/operation.rs b/core/src/eval/operation.rs index edc7cb61c6..1ba6052cef 100644 --- a/core/src/eval/operation.rs +++ b/core/src/eval/operation.rs @@ -1169,6 +1169,8 @@ impl VirtualMachine { match_sharedterm!(match (t) { Term::Record(data) => { for (id, field) in data.fields { + debug_assert!(field.metadata.is_empty()); + if let Some(value) = field.value { match_sharedterm!(match (value.term) { Term::Closure(idx) => { diff --git a/core/src/term/mod.rs b/core/src/term/mod.rs index c3455201df..b575b94017 100644 --- a/core/src/term/mod.rs +++ b/core/src/term/mod.rs @@ -712,13 +712,20 @@ impl TypeAnnotation { } /// Convert all the contracts of this annotation, including the potential type annotation as - /// the first element, to a runtime representation. + /// the first element, to a runtime representation. Apply contract optimizations to the static + /// type annotation. pub fn all_contracts(&self) -> Result, UnboundTypeVariableError> { self.typ - .iter() - .chain(self.contracts.iter()) + .as_ref() .cloned() - .map(RuntimeContract::try_from) + .map(RuntimeContract::from_static_type) + .into_iter() + .chain( + self.contracts + .iter() + .cloned() + .map(RuntimeContract::try_from), + ) .collect::, _>>() } diff --git a/core/src/term/pattern/compile.rs b/core/src/term/pattern/compile.rs index bc2cb383d3..a2feaa5584 100644 --- a/core/src/term/pattern/compile.rs +++ b/core/src/term/pattern/compile.rs @@ -165,48 +165,68 @@ impl CompilePart for RecordPattern { // // if %typeof% value_id == 'Record // let final_bindings_id = + // " bindings_id value_id` + // > + // # if this field pattern has some extra annotations (contract, default value, etc.) + // # we let merging take care of it by assembling a one-field record and merging it + // # with the matched value + // # + // # If there is a default value, we must merge it BEFORE the %field_is_defined%, + // # because the default acts as if the original matched value always have this field + // # defined + // + // let value_id = value_id & { "" = } in + // // - // " bindings_id value_id` - // > - // if %field_is_defined% field value_id then - // let local_bindings_id = cont in + // if %field_is_defined% field value_id then + // # However, if there are extra annotations without a default value, we must first + // # check that the field is defined before merging the extra annotations. + // + // let value_id = value_id & { "" = } in + // // - // if local_bindings_id == null then - // null - // else - // let local_value_id = %static_access(field)% (%static_access(REST_FIELD)% local_bindings_id) - // let local_bindings_id = in - // - // else - // null - // + // let local_bindings_id = cont in // + // if local_bindings_id == null then + // null + // else + // let local_value_id = %static_access(field)% (%static_access(REST_FIELD)% local_bindings_id) + // let local_bindings_id = in + // + // else + // null + // // in // - // if final_bindings_id == null then - // null - // else - // - // # if tail is empty, check that the value doesn't contain extra fields - // if (%static_access% final_bindings_id) != {} then - // null - // else - // %record_remove% "" final_bindings_id - // - // # move the rest from REST_FIELD to rest, and remove REST_FIELD - // %record_remove% "" - // (%record_insert% - // final_bindings_id - // (%static_access% final_bindings_id) - // ) - // - // %record_remove% "" final_bindings_id - // + // if final_bindings_id == null then + // null + // else + // + // # if tail is empty, check that the value doesn't contain extra fields + // if (%static_access% final_bindings_id) != {} then + // null + // else + // %record_remove% "" final_bindings_id + // + // # move the rest from REST_FIELD to rest, and remove REST_FIELD + // %record_remove% "" + // (%record_insert% + // final_bindings_id + // (%static_access% final_bindings_id) + // ) + // + // %record_remove% "" final_bindings_id + // // else // null fn compile_part(&self, value_id: LocIdent, bindings_id: LocIdent) -> RichTerm { + use crate::{ + label::{MergeKind, MergeLabel}, + term::IndexMap, + }; + let rest_field = LocIdent::fresh(); // `%record_insert% "" bindings_id value_id` @@ -226,14 +246,30 @@ impl CompilePart for RecordPattern { // - initial accumulator is `%record_insert% "" bindings_id value_id` // > // + // # if this field pattern has some extra annotations (contract, default value, etc.) + // # we let merging take care of it by assembling a one-field record and merging it + // # with the matched value + // # + // # If there is a default value, we must merge it BEFORE the %field_is_defined%, + // # because the default acts like the original matched value always have this field + // # defined + // + // let value_id = value_id & { "" = } in + // // // if %field_is_defined% field value_id then + // # However, if there are extra annotations without a default value, we must first + // # check that the field is defined before merging the extra annotations. + // + // let value_id = value_id & { "" = } in + // + // // let local_bindings_id = cont in // // if local_bindings_id == null then // null // else - // let local_value_id = %static_access(field)% (%static_access(REST_FIELD)% local_bindings_id) + // let local_value_id = %static_access(field)% (%static_access(REST_FIELD)% local_bindings_id) in // let local_bindings_id = in // let fold_block: RichTerm = self.patterns.iter().fold(init_bindings, |cont, field_pat| { @@ -251,20 +287,19 @@ impl CompilePart for RecordPattern { .compile_part(local_value_id, local_bindings_id), ); - // let value_id = %static_access(field)% (%static_access(REST_FIELD)% local_bindings_id) - // in - let inner_else_block = make::let_in( - local_value_id, + // %static_access(field)% (%static_access(REST_FIELD)% local_bindings_id) + let extracted_value = make::op1( + UnaryOp::StaticAccess(field), make::op1( - UnaryOp::StaticAccess(field), - make::op1( - UnaryOp::StaticAccess(rest_field), - Term::Var(local_bindings_id), - ), + UnaryOp::StaticAccess(rest_field), + Term::Var(local_bindings_id), ), - updated_bindings_let, ); + // let local_value_id = in + let inner_else_block = + make::let_in(local_value_id, extracted_value, updated_bindings_let); + // The innermost if: // // if local_bindings_id == null then @@ -280,6 +315,63 @@ impl CompilePart for RecordPattern { // let local_bindings_id = cont in let binding_cont_let = make::let_in(local_bindings_id, cont, inner_if); + // let value_id = value_id & { "" = } in + let mk_merge = |id: LocIdent, field: &Field, merge_cont: RichTerm| { + let singleton = Term::Record(RecordData { + fields: IndexMap::from([(id, field.clone())]), + ..Default::default() + }); + // Right now, patterns are compiled on-the-fly during evaluation. We thus need to + // perform the gen_pending_contract transformation manually, or the contracts will + // just be ignored. One step suffices, as we create a singleton record that doesn't + // contain other non-transformed records (the default value, if any, has been + // transformed normally). + // + // unwrap(): typechecking ensures that there are no unbound variables at this point + let singleton = + crate::transform::gen_pending_contracts::transform_one(singleton.into()) + .unwrap(); + + let span = field + .metadata + .annotation + .iter() + .map(|labeled_ty| labeled_ty.label.span) + .chain( + field + .value + .as_ref() + .and_then(|v| v.pos.into_opt()) + .into_iter(), + ) + // We fuse all the definite spans together. + // unwrap(): all span should come from the same file + // unwrap(): we hope that at least one position is defined + .reduce(|span1, span2| crate::position::RawSpan::fuse(span1, span2).unwrap()) + .unwrap(); + + let merge_label = MergeLabel { + span, + kind: MergeKind::Standard, + }; + + make::let_in( + value_id, + make::op2(BinaryOp::Merge(merge_label), Term::Var(value_id), singleton), + merge_cont, + ) + }; + + // + // + // + let optional_merge = + if !field_pat.extra.metadata.is_empty() && field_pat.extra.value.is_none() { + mk_merge(field, &field_pat.extra, binding_cont_let) + } else { + binding_cont_let + }; + // %field_is_defined% field value_id let has_field = make::op2( BinaryOp::FieldIsDefined(RecordOpKind::ConsiderAllFields), @@ -287,7 +379,17 @@ impl CompilePart for RecordPattern { Term::Var(value_id), ); - make::if_then_else(has_field, binding_cont_let, Term::Null) + // if then else null + let enclosing_if = make::if_then_else(has_field, optional_merge, Term::Null); + + // + // + // + if field_pat.extra.value.is_some() { + mk_merge(field, &field_pat.extra, enclosing_if) + } else { + enclosing_if + } }); // %typeof% value_id == 'Record diff --git a/core/src/term/record.rs b/core/src/term/record.rs index 93b88836c2..145563160b 100644 --- a/core/src/term/record.rs +++ b/core/src/term/record.rs @@ -119,6 +119,14 @@ impl FieldMetadata { pub fn new() -> Self { Default::default() } + + pub fn is_empty(&self) -> bool { + self.doc.is_none() + && self.annotation.is_empty() + && !self.opt + && !self.not_exported + && matches!(self.priority, MergePriority::Neutral) + } } /// A record field with its metadata. From 3db6609f7a01265b0edbae97e3ad9f9753b83868 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Fri, 16 Feb 2024 20:37:28 +0100 Subject: [PATCH 2/4] Add tests for contracts in match patterns --- .../inputs/pattern-matching/contract_blame.ncl | 7 +++++++ .../inputs/pattern-matching/contract_merge_conflict.ncl | 7 +++++++ .../pattern-matching/contract_merge_conflict_array.ncl | 7 +++++++ .../integration/inputs/pattern-matching/contracts.ncl | 9 +++++++++ 4 files changed, 30 insertions(+) create mode 100644 core/tests/integration/inputs/pattern-matching/contract_blame.ncl create mode 100644 core/tests/integration/inputs/pattern-matching/contract_merge_conflict.ncl create mode 100644 core/tests/integration/inputs/pattern-matching/contract_merge_conflict_array.ncl create mode 100644 core/tests/integration/inputs/pattern-matching/contracts.ncl diff --git a/core/tests/integration/inputs/pattern-matching/contract_blame.ncl b/core/tests/integration/inputs/pattern-matching/contract_blame.ncl new file mode 100644 index 0000000000..f532c4d762 --- /dev/null +++ b/core/tests/integration/inputs/pattern-matching/contract_blame.ncl @@ -0,0 +1,7 @@ +# test.type = 'error' +# +# [test.metadata] +# error = 'EvalError::BlameError' +{foo.bar = 5} |> match { + {foo={bar | String}} => bar, +} diff --git a/core/tests/integration/inputs/pattern-matching/contract_merge_conflict.ncl b/core/tests/integration/inputs/pattern-matching/contract_merge_conflict.ncl new file mode 100644 index 0000000000..4704c341b7 --- /dev/null +++ b/core/tests/integration/inputs/pattern-matching/contract_merge_conflict.ncl @@ -0,0 +1,7 @@ +# test.type = 'error' +# +# [test.metadata] +# error = 'EvalError::NonMergeableTerms' +{foo.bar | default = 5} |> match { + {foo={bar ? 6}} => bar, +} diff --git a/core/tests/integration/inputs/pattern-matching/contract_merge_conflict_array.ncl b/core/tests/integration/inputs/pattern-matching/contract_merge_conflict_array.ncl new file mode 100644 index 0000000000..045b619588 --- /dev/null +++ b/core/tests/integration/inputs/pattern-matching/contract_merge_conflict_array.ncl @@ -0,0 +1,7 @@ +# test.type = 'error' +# +# [test.metadata] +# error = 'EvalError::BlameError' +{foo.bar | default = []} |> match { + {foo={bar ? [1]}} => bar, +} diff --git a/core/tests/integration/inputs/pattern-matching/contracts.ncl b/core/tests/integration/inputs/pattern-matching/contracts.ncl new file mode 100644 index 0000000000..d31afef755 --- /dev/null +++ b/core/tests/integration/inputs/pattern-matching/contracts.ncl @@ -0,0 +1,9 @@ +# test.type = 'pass' + +let {check, ..} = import "../lib/assert.ncl" in + +[ + {foo = {}} |> match { {foo = {bar ? 5}} => true}, + {foo = {}} |> match { {foo = {bar | String }} => false, {foo} => true}, +] +|> check From 46a5a57a8b029452dc31682ac2c1bdd68007f83a Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Fri, 16 Feb 2024 22:16:29 +0100 Subject: [PATCH 3/4] Fix clippy warning --- core/src/term/pattern/compile.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/core/src/term/pattern/compile.rs b/core/src/term/pattern/compile.rs index a2feaa5584..063bc7c165 100644 --- a/core/src/term/pattern/compile.rs +++ b/core/src/term/pattern/compile.rs @@ -337,13 +337,7 @@ impl CompilePart for RecordPattern { .annotation .iter() .map(|labeled_ty| labeled_ty.label.span) - .chain( - field - .value - .as_ref() - .and_then(|v| v.pos.into_opt()) - .into_iter(), - ) + .chain(field.value.as_ref().and_then(|v| v.pos.into_opt())) // We fuse all the definite spans together. // unwrap(): all span should come from the same file // unwrap(): we hope that at least one position is defined From 31226e3f94fec7bb1012d04018446f431063d98b Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Fri, 16 Feb 2024 22:16:48 +0100 Subject: [PATCH 4/4] Renaming a couple test files --- ...onflict_array.ncl => default_value_array_merge_conflict.ncl} | 0 ...ract_merge_conflict.ncl => default_value_merge_conflict.ncl} | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename core/tests/integration/inputs/pattern-matching/{contract_merge_conflict_array.ncl => default_value_array_merge_conflict.ncl} (100%) rename core/tests/integration/inputs/pattern-matching/{contract_merge_conflict.ncl => default_value_merge_conflict.ncl} (70%) diff --git a/core/tests/integration/inputs/pattern-matching/contract_merge_conflict_array.ncl b/core/tests/integration/inputs/pattern-matching/default_value_array_merge_conflict.ncl similarity index 100% rename from core/tests/integration/inputs/pattern-matching/contract_merge_conflict_array.ncl rename to core/tests/integration/inputs/pattern-matching/default_value_array_merge_conflict.ncl diff --git a/core/tests/integration/inputs/pattern-matching/contract_merge_conflict.ncl b/core/tests/integration/inputs/pattern-matching/default_value_merge_conflict.ncl similarity index 70% rename from core/tests/integration/inputs/pattern-matching/contract_merge_conflict.ncl rename to core/tests/integration/inputs/pattern-matching/default_value_merge_conflict.ncl index 4704c341b7..d454a418b2 100644 --- a/core/tests/integration/inputs/pattern-matching/contract_merge_conflict.ncl +++ b/core/tests/integration/inputs/pattern-matching/default_value_merge_conflict.ncl @@ -1,7 +1,7 @@ # test.type = 'error' # # [test.metadata] -# error = 'EvalError::NonMergeableTerms' +# error = 'EvalError::MergeIncompatibleArgs' {foo.bar | default = 5} |> match { {foo={bar ? 6}} => bar, }