Repository: tweag/nickel
Pull request #550 (2022-01-06 16:48:33)

Allow piecewise signatures in records definition
Implementation of #496.
Merged `TypeAnnot` into `MetaAnnot` (renamed now as `Annot`) inside `grammar.lalrpop`.
This allows that kind of Nickel code inside records:
```
{
   foo : Num,
   bar = 3,
   foo = 5
}
```
It could also let these partial type-annotations to be used when destructuring a record like so: `let {foo : Num, bar} = x`
However the semantic of this specific case needs to be precised.

> Wether or not to allow undefined type-annotated fields are yet to be decided, and are allowed for now as this is a pre-relase.
> Wether or not to allow code in the middle of a field's type annotation and definition is yet to be decided also.
> Both these points should be debated in a separated issue before modifying this feature.

litchipi: ```
{
  foo : Num,
  foo = 1,
  bar : Num = foo,
}
```
This new test doesn't pass as it tries to unify `Dyn` with `Num`.
In the internals, the record is considered as a `RecRecord` and that makes the evaluation use `Dyn` everywhere.
I suppose it's not a normal behavior, it should be considered as a special form of static record, what do you think about it ?
yannham: It shouldn't be related to the type of record. The type of each field is dictated by the content: here, we want the content to be a metavalue with a `type` field set to `Num` in both places. If that's not the case there's something fishy going on with respect to `flatten` or the way `build_record` operate.
yannham: What is possible is that until now, `build_record` just merged everything dynamically without care. If that's the case, we probably rather want to gather all the values first and statically flatten the outer metavalues.
litchipi: It seams that this code:
```
{
  foo : Num,
  foo = 1,
  bar : Num = foo,
}
```
makes Nickel use an `Op2::Merge` operation over `foo` to create a single term, no problem from here.
However in `get_bop_type()`, this operation returns `(Dyn, Dyn, Dyn)` types which enters in conflict with the statically typed `bar : Num`, and causes the bug.

Is the way to go is simply to avoid using `Op2::Merge` ? Perform the merge much sooner, in the parser ?
yannham: > Is the way to go is simply to avoid using Op2::Merge ? Perform the merge much sooner, in the parser ?

Yes, precisely. At least for the *outer metavalues*. I guess it's fine to rely on merge for the inner values. So, for example, if you have a vector of definitions for field `foo`, you would morally split them as `(Option<MetaData>, RichTerm)`, flatten the metadata, merge the terms, and reconstruct a `MetaValue` with the merged terms as inner term, or directly the merged terms if no definition had metavalues (it's just draft steps, not saying you should necessarily do exactly like that).
litchipi: I got a problem creating the `test/merge_fail.rs` file. I created a macro:
```
macro_rules! assert_merge_fails {
    ($term:expr) => {
        assert_matches!(
            eval($term),
            Err(Error::EvalError(EvalError::MergeIncompatibleArgs(..)))
        )
    };
}
```
(inspired from `src/eval/test.rs` and `test/typecheck_fail.rs`)
And this happends:
```
// Works
assert_merge_fails!("({a=1} & {a=2}).a");  
// Doesn't work
assert_merge_fails!("{ foo = (fun x => x) (1 | default), foo = (fun x => x) (1 | default) } & {foo | default = 2 }");   
```
Even tho when I write the exact same test in a `a.ncl` file and `cargo run` it, it fails like it should.
Is there anything different from using the commandline and using the `eval` function (from the `utilities` crate) ?
yannham: >Even tho when I write the exact same test in a a.ncl file and cargo run it, it fails like it should.
Is there anything different from using the commandline and using the eval function (from the utilities crate) ?

Yes, we recently made the default `nickel` command and the `repl` to deeply evaluate the argument. This it triggers the contract check. On the other hand, you are just using the standard `eval`, that doesn't evaluate further than the first record. You need to either `eval_full`, or you can also access the field `foo`:

```
({ foo = (fun x => x) (1 | default), foo = (fun x => x) (1 | default) } & {foo | default = 2 }).foo
```

to trigger the error.
litchipi: Yep indeed, should have found that ^^'
Pushed the last version, cleaned and ready to be merged if it's OK for you

Patch:
diff --git a/src/grammar.lalrpop b/src/grammar.lalrpop
index 0e6ff6d8..845cb263 100644
--- a/src/grammar.lalrpop
+++ b/src/grammar.lalrpop
@@ -23,15 +23,7 @@ grammar<'input, 'err>(src_id: FileId, errors: &'err mut Vec<ErrorRecovery<usize,
 WithPos<Rule>: RichTerm = <l: @L> <t: Rule> <r: @R> => t.with_pos(mk_pos(src_id, l, r));
 CheckUnbound<Rule>: Types = <l: @L> <t: Rule> <r: @R> =>? check_unbound(&t, mk_span(src_id, l, r)).map_err(|e| lalrpop_util::ParseError::User{error: e}).and(Ok(t));
 
-TypeAnnot: MetaValue = ":" <l: @L> <ty_res: CheckUnbound<Types>> <r: @R> => MetaValue {
-    doc: None,
-    types: Some(Contract {types: ty_res.clone(), label: mk_label(ty_res, src_id, l, r)}),
-    contracts: Vec::new(),
-    priority: Default::default(),
-    value: None,
-};
-
-MetaAnnotAtom: MetaValue = {
+AnnotAtom: MetaValue = {
     "|" <l: @L> <ty_res: CheckUnbound<Types>> <r: @R> => MetaValue {
         doc: None,
         types: None,
@@ -53,6 +45,13 @@ MetaAnnotAtom: MetaValue = {
         priority: Default::default(),
         value: None,
     },
+    ":" <l: @L> <ty_res: CheckUnbound<Types>> <r: @R> => MetaValue {
+        doc: None,
+        types: Some(Contract {types: ty_res.clone(), label: mk_label(ty_res, src_id, l, r)}),
+        contracts: Vec::new(),
+        priority: Default::default(),
+        value: None,
+    },
 };
 
 DefaultAnnot: MetaValue = "?" <t: Term> => MetaValue {
@@ -61,19 +60,7 @@ DefaultAnnot: MetaValue = "?" <t: Term> => MetaValue {
     ..Default::default()
 };
 
-MetaAnnot: MetaValue = <anns: MetaAnnotAtom+> => anns.into_iter().fold(MetaValue::new(), MetaValue::flatten);
-
-Annot: MetaValue = {
-    <TypeAnnot>,
-    <ty_ann: TypeAnnot?> <meta: MetaAnnot> => {
-        if let Some(ty_meta) = ty_ann {
-            MetaValue::flatten(ty_meta, meta)
-        }
-        else {
-            meta
-        }
-    }
-};
+Annot: MetaValue = <anns: AnnotAtom+> => anns.into_iter().fold(MetaValue::new(), MetaValue::flatten);
 
 pub Term: RichTerm = WithPos<RootTerm>;
 
@@ -208,34 +195,21 @@ Atom: RichTerm = {
 };
 
 RecordField: (FieldPathElem, RichTerm) = {
-    <path: FieldPath> <ty_ann: TypeAnnot?> "=" <t: Term> => {
-        let t = if let Some(mut meta) = ty_ann {
-            let pos = t.pos;
-            meta.value = Some(t);
-            RichTerm::new(Term::MetaValue(meta), pos)
-        }
-        else {
-            t
-        };
-
-        elaborate_field_path(path, t)
-    },
-    <path: FieldPath> <ty_ann: TypeAnnot> <meta: MetaAnnot> "=" <t: Term> => {
-        let pos = t.pos;
-        let mut meta = MetaValue::flatten(ty_ann, meta);
-        meta.value = Some(t);
-        let t = RichTerm::new(Term::MetaValue(meta), pos);
-
-        elaborate_field_path(path, t)
-    },
-    <l: @L> <path: FieldPath> <meta: MetaAnnot> <r: @R> <t: ("=" <Term>)?> => {
-        let mut meta = meta;
+    <l: @L> <path: FieldPath> <ann: Annot?> <r: @R> <t: ("=" <Term>)?> => {
         let pos = t.as_ref()
             .map(|t| t.pos.clone())
             .unwrap_or(mk_pos(src_id, l, r));
-        meta.value = t;
-        let t = RichTerm::new(Term::MetaValue(meta), pos);
-        elaborate_field_path(path, t)
+        let term = if let Some(mut meta) = ann {
+            meta.value = t;
+            RichTerm::new(Term::MetaValue(meta), pos)
+        } else {
+            if let Some(deft) = t {
+                deft
+            } else {
+                RichTerm::new(Term::Null, pos)
+            }
+        };
+        elaborate_field_path(path, term)
     }
 }
 
@@ -278,7 +252,7 @@ Destruct: Destruct = {
 };
 
 Match: Match = {
-    <left:Ident> <anns: MetaAnnot?> <default: DefaultAnnot?> "=" <right: Pattern> => {
+    <left:Ident> <anns: Annot?> <default: DefaultAnnot?> "=" <right: Pattern> => {
 	let meta = match (default, anns) {
 	    (Some(d), Some(m)) => MetaValue::flatten(d,m),
 	    (Some(m),_) | (_,Some(m)) => m,
@@ -286,7 +260,7 @@ Match: Match = {
 	};
 	Match::Assign(left, meta, right)
     },
-    <id:Ident> <anns: MetaAnnot?> <default: DefaultAnnot?> => {
+    <id:Ident> <anns: Annot?> <default: DefaultAnnot?> => {
 	let meta = match (default, anns) {
 	    (Some(d), Some(m)) => MetaValue::flatten(d,m),
 	    (Some(m),_) | (_,Some(m)) => m,
diff --git a/src/parser/utils.rs b/src/parser/utils.rs
index 986627bf..8c194f25 100644
--- a/src/parser/utils.rs
+++ b/src/parser/utils.rs
@@ -12,7 +12,7 @@ use crate::{
     mk_app, mk_fun,
     parser::error::ParseError,
     position::{RawSpan, TermPos},
-    term::{make as mk_term, BinaryOp, RecordAttrs, RichTerm, StrChunk, Term, UnaryOp},
+    term::{make as mk_term, BinaryOp, MetaValue, RecordAttrs, RichTerm, StrChunk, Term, UnaryOp},
     types::{AbsType, Types},
 };
 
@@ -162,7 +162,9 @@ where
             Entry::Occupied(mut occpd) => {
                 // temporary putting null in the entry to take the previous value.
                 let prev = occpd.insert(Term::Null.into());
-                occpd.insert(mk_term::op2(BinaryOp::Merge(), prev, t));
+
+                // A field of a record without metadata AND without value is impossible
+                occpd.insert(merge_field(prev, t).unwrap());
             }
             Entry::Vacant(vac) => {
                 vac.insert(t);
@@ -219,6 +221,41 @@ where
     Term::RecRecord(static_map, dynamic_fields, attrs)
 }
 
+/// Merge two fields by performing the merge of both their value and MetaValue if any.
+fn merge_field(rterm1: RichTerm, rterm2: RichTerm) -> Option<RichTerm> {
+    let term1 = if let Term::MetaValue(meta) = &*rterm1.term {
+        (Some(meta.clone()), meta.value.clone())
+    } else {
+        (None, Some(rterm1))
+    };
+
+    let term2 = if let Term::MetaValue(meta) = &*rterm2.term {
+        (Some(meta.clone()), meta.value.clone())
+    } else {
+        (None, Some(rterm2))
+    };
+
+    let new_value = match (term1.1, term2.1) {
+        (Some(t1), Some(t2)) => Some(mk_term::op2(BinaryOp::Merge(), t1, t2)),
+        (Some(t), None) | (None, Some(t)) => Some(t),
+        (None, None) => None,
+    };
+
+    match (term1.0, term2.0) {
+        (Some(meta1), Some(meta2)) => {
+            let mut new_meta = MetaValue::flatten(meta1, meta2);
+            new_meta.value = new_value;
+            Some(RichTerm::from(Term::MetaValue(new_meta)))
+        }
+        (Some(meta), None) | (None, Some(meta)) => {
+            let mut new_meta = meta.clone();
+            new_meta.value = new_value;
+            Some(RichTerm::from(Term::MetaValue(new_meta)))
+        }
+        (None, None) => new_value,
+    }
+}
+
 /// Make a span from parser byte offsets.
 pub fn mk_span(src_id: FileId, l: usize, r: usize) -> RawSpan {
     RawSpan {


Test patch:
diff --git a/tests/merge_fail.rs b/tests/merge_fail.rs
new file mode 100644
index 00000000..633718a9
--- /dev/null
+++ b/tests/merge_fail.rs
@@ -0,0 +1,32 @@
+use assert_matches::assert_matches;
+use nickel::error::{Error, EvalError};
+use nickel::position::TermPos;
+use nickel::program::Program;
+use nickel::term::RichTerm;
+use std::io::Cursor;
+
+fn eval_full(s: &str) -> Result<RichTerm, Error> {
+    let src = Cursor::new(s);
+
+    let mut p = Program::new_from_source(src, "<test>").map_err(|io_err| {
+        Error::EvalError(EvalError::Other(
+            format!("IO error: {}", io_err),
+            TermPos::None,
+        ))
+    })?;
+    p.eval_full()
+}
+
+macro_rules! assert_merge_fails {
+    ($term:expr) => {
+        assert_matches!(
+            eval_full($term),
+            Err(Error::EvalError(EvalError::MergeIncompatibleArgs(..)))
+        )
+    };
+}
+
+#[test]
+fn merge_conflict_inside_metavalue() {
+    assert_merge_fails!("{ foo = (fun x => x) (1 | default), foo = (fun x => x) (1 | default) } & {foo | default = 2 }");
+}
diff --git a/tests/pass/records.ncl b/tests/pass/records.ncl
index 203b8e2f..d43da72b 100644
--- a/tests/pass/records.ncl
+++ b/tests/pass/records.ncl
@@ -80,5 +80,18 @@ let Assert = fun l x => x || %blame% l in
       g = fun y => f (y + (-1))
     }.f 10 in
     with_res "done" == "done",
+
+    // piecewise signatures
+    {
+        foo : Num,
+        bar = 3,
+        foo = 5
+    }.foo == 5,
+    {
+        foo : Num,
+        foo = 1,
+        bar : Num = foo,
+    }.bar == 1,
+    let {foo : Num} = {foo = 1} in foo == 1,
 ]
 |> lists.foldl (fun x y => (x | #Assert) && y) true
diff --git a/tests/typecheck_fail.rs b/tests/typecheck_fail.rs
index e62b3b02..0468c9e8 100644
--- a/tests/typecheck_fail.rs
+++ b/tests/typecheck_fail.rs
@@ -222,3 +222,11 @@ fn dynamic_record_field() {
         Err(TypecheckError::TypeMismatch(..))
     );
 }
+
+#[test]
+fn piecewise_signature() {
+    assert_matches!(
+        type_check_expr("{foo : Num, foo = \"bar\"}"),
+        Err(TypecheckError::TypeMismatch(..))
+    );
+}