Repository: tweag/nickel
Pull request #251 (2020-12-14 14:56:28)

Eliminate the inner terms of primitive operators 4: DynExtend
Depend on #250. Follow-up of #247, #248, #249, and #250. Last PR of this series. Use the stack to store the non-strict argument of `DynExtend`. `DynExtend` is the dynamic record extension operator, currently written as `someRecord $[foo=bar]` which extends `someRecord` with the field `field = bar` if the `foo` expression evaluates to the string `"field"`, where `bar` is said non-strict argument. This terminates the elimination of the dependency of `UnaryOp` and `BinaryOp` on `RichTerm`.

dpulls[bot]: :tada: All dependencies have been resolved !
edolstra: @yannham There is a merge conflict, can you resolve it please?

Patch:
diff --git a/src/eval.rs b/src/eval.rs
index 38ab5ea5..1c596042 100644
--- a/src/eval.rs
+++ b/src/eval.rs
@@ -295,11 +295,6 @@ where
                 Closure { body: t, env }
             }
             Term::Op2(op, fst, snd) => {
-                let op = op.map(|t| Closure {
-                    body: t,
-                    env: env.clone(),
-                });
-
                 let prev_strict = enriched_strict;
                 enriched_strict = op.is_strict();
                 stack.push_op_cont(
diff --git a/src/grammar.lalrpop b/src/grammar.lalrpop
index f087a95e..b6a26eb4 100644
--- a/src/grammar.lalrpop
+++ b/src/grammar.lalrpop
@@ -106,7 +106,7 @@ RecordOperationChain: RichTerm = {
     <t: SpTerm<RecordOperand>> ".$" <t_id: SpTerm<Atom>> => mk_term::op2(BinaryOp::DynAccess(), t_id, t),
     <t: SpTerm<RecordOperand>> "-$" <t_id: SpTerm<Atom>> => mk_term::op2(BinaryOp::DynRemove(), t_id, t),
     <r: SpTerm<RecordOperand>> "$[" <id: Term> "=" <t: Term> "]" =>
-        mk_term::op2(BinaryOp::DynExtend(t), id, r),
+        mk_app!(mk_term::op2(BinaryOp::DynExtend(), id, r), t),
 };
 
 Atom: RichTerm = {
@@ -179,7 +179,7 @@ Atom: RichTerm = {
 
         dynamic_fields.into_iter().fold(static_rec, |rec, field| {
             let (id_t, t) = field;
-            RichTerm::from(Term::Op2(BinaryOp::DynExtend(t), id_t, rec))
+            mk_app!(mk_term::op2(BinaryOp::DynExtend(), id_t, rec), t)
         })
     },
     "[" <terms: (SpTerm<Atom> ",")*> <last: Term?> "]" => {
@@ -303,7 +303,6 @@ UOp: UnaryOp = {
     "polarity" => UnaryOp::Pol(),
     "goDom" => UnaryOp::GoDom(),
     "goCodom" => UnaryOp::GoCodom(),
-    "tag" <s: Str> => UnaryOp::Tag(s),
     "wrap" => UnaryOp::Wrap(),
     "embed" <Ident> => UnaryOp::Embed(<>),
     "map"  => UnaryOp::ListMap(),
@@ -333,7 +332,7 @@ PrefixExpr1: RichTerm = {
         mk_term::op2(BinaryOp::Sub(), Term::Num(0.0), t),
 }
 
-BinOp2: BinaryOp<RichTerm> = {
+BinOp2: BinaryOp = {
     "++" => BinaryOp::PlusStr(),
     "@" => BinaryOp::ListConcat(),
 }
@@ -343,7 +342,7 @@ InfixExpr2: RichTerm = {
     LeftOp<BinOp2, InfixExpr2, PrefixExpr1> => <>,
 }
 
-BinOp3: BinaryOp<RichTerm> = {
+BinOp3: BinaryOp = {
     "*" => BinaryOp::Mult(),
     "/" => BinaryOp::Div(),
     "%" => BinaryOp::Modulo(),
@@ -354,7 +353,7 @@ InfixExpr3: RichTerm = {
     LeftOp<BinOp3, InfixExpr3, InfixExpr2> => <>,
 }
 
-BinOp4: BinaryOp<RichTerm> = {
+BinOp4: BinaryOp = {
     "+" => BinaryOp::Plus(),
     "-" => BinaryOp::Sub(),
 }
@@ -369,7 +368,7 @@ PrefixExpr5: RichTerm = {
     "!" <PrefixExpr5> => mk_term::op1(UnaryOp::BoolNot(), <>),
 }
 
-BinOp6: BinaryOp<RichTerm> = {
+BinOp6: BinaryOp = {
     "&" => BinaryOp::Merge(),
 }
 
@@ -378,7 +377,7 @@ PrefixExpr6: RichTerm = {
     LeftOp<BinOp6, PrefixExpr6, PrefixExpr5> => <>,
 }
 
-BinOp7: BinaryOp<RichTerm> = {
+BinOp7: BinaryOp = {
     "<" => BinaryOp::LessThan(),
     "<=" => BinaryOp::LessOrEq(),
     ">" => BinaryOp::GreaterThan(),
@@ -390,7 +389,7 @@ InfixExpr7: RichTerm = {
     LeftOp<BinOp7, InfixExpr7, PrefixExpr6> => <>,
 }
 
-BinOp8: BinaryOp<RichTerm> = {
+BinOp8: BinaryOp = {
     "==" => BinaryOp::Eq(),
 }
 
@@ -423,11 +422,12 @@ InfixExpr: RichTerm = {
     InfixExpr10,
 }
 
-BOpPre: BinaryOp<RichTerm> = {
+BOpPre: BinaryOp = {
     "unwrap" => BinaryOp::Unwrap(),
     "goField" => BinaryOp::GoField(),
     "hasField" => BinaryOp::HasField(),
     "elemAt" => BinaryOp::ListElemAt(),
+    "tag" => BinaryOp::Tag(),
 }
 
 Types: Types = {
diff --git a/src/operation.rs b/src/operation.rs
index c163ca61..113d8f7a 100644
--- a/src/operation.rs
+++ b/src/operation.rs
@@ -49,13 +49,13 @@ pub enum OperationCont {
     ),
     // The last parameter saves the strictness mode before the evaluation of the operator
     Op2First(
-        /* the binary operation */ BinaryOp<Closure>,
+        /* the binary operation */ BinaryOp,
         /* second argument, to evaluate next */ Closure,
         /* original position of the first argument */ Option<RawSpan>,
         /* previous value of enriched_strict */ bool,
     ),
     Op2Second(
-        /* binary operation */ BinaryOp<Closure>,
+        /* binary operation */ BinaryOp,
         /* first argument, evaluated */ Closure,
         /* original position of the first argument before evaluation */ Option<RawSpan>,
         /* original position of the second argument before evaluation */ Option<RawSpan>,
@@ -367,19 +367,6 @@ fn process_unary_operation(
                 ))
             }
         }
-        UnaryOp::Tag(s) => {
-            if let Term::Lbl(mut l) = *t {
-                l.tag = String::from(&s);
-                Ok(Closure::atomic_closure(Term::Lbl(l).into()))
-            } else {
-                Err(EvalError::TypeError(
-                    String::from("Label"),
-                    String::from("tag"),
-                    arg_pos,
-                    RichTerm { term: t, pos },
-                ))
-            }
-        }
         UnaryOp::Wrap() => {
             if let Term::Sym(s) = *t {
                 Ok(Closure::atomic_closure(mk_fun!(
@@ -660,7 +647,7 @@ fn process_unary_operation(
 /// Both arguments are expected to be evaluated (in WHNF). `pos_op` corresponds to the whole
 /// operation position, that may be needed for error reporting.
 fn process_binary_operation(
-    b_op: BinaryOp<Closure>,
+    b_op: BinaryOp,
     fst_clos: Closure,
     fst_pos: Option<RawSpan>,
     clos: Closure,
@@ -879,6 +866,34 @@ fn process_binary_operation(
                 ))
             }
         }
+        BinaryOp::Tag() => {
+            if let Term::Str(s) = *t1 {
+                if let Term::Lbl(mut l) = *t2 {
+                    l.tag = s;
+                    Ok(Closure::atomic_closure(Term::Lbl(l).into()))
+                } else {
+                    Err(EvalError::TypeError(
+                        String::from("Label"),
+                        String::from("tag, 2nd argument"),
+                        snd_pos,
+                        RichTerm {
+                            term: t2,
+                            pos: pos2,
+                        },
+                    ))
+                }
+            } else {
+                Err(EvalError::TypeError(
+                    String::from("Str"),
+                    String::from("tag, 1st argument"),
+                    fst_pos,
+                    RichTerm {
+                        term: t1,
+                        pos: pos1,
+                    },
+                ))
+            }
+        }
         BinaryOp::Eq() => {
             let mut env = Environment::new();
 
@@ -1098,7 +1113,11 @@ fn process_binary_operation(
                 ))
             }
         }
-        BinaryOp::DynExtend(clos) => {
+        BinaryOp::DynExtend() => {
+            let (clos, _) = stack.pop_arg().ok_or_else(|| {
+                EvalError::NotEnoughArgs(3, String::from("$[ .. ]"), pos_op.clone())
+            })?;
+
             if let Term::Str(id) = *t1 {
                 if let Term::Record(mut static_map) = *t2 {
                     let as_var = clos.body.closurize(&mut env2, clos.env);
diff --git a/src/term.rs b/src/term.rs
index 6e695bd6..87a9edf3 100644
--- a/src/term.rs
+++ b/src/term.rs
@@ -90,7 +90,7 @@ pub enum Term {
     Op1(UnaryOp, RichTerm),
     /// A primitive binary operator.
     #[serde(skip)]
-    Op2(BinaryOp<RichTerm>, RichTerm, RichTerm),
+    Op2(BinaryOp, RichTerm, RichTerm),
 
     /// A promise.
     ///
@@ -219,11 +219,7 @@ impl Term {
                     func(t);
                 });
             }
-            Op2(BinaryOp::DynExtend(ref mut t), ref mut t1, ref mut t2) => {
-                func(t);
-                func(t1);
-                func(t2)
-            }
+
             Bool(_) | Num(_) | Str(_) | Lbl(_) | Var(_) | Sym(_) | Enum(_) | Import(_)
             | ResolvedImport(_) => {}
             Fun(_, ref mut t)
@@ -534,8 +530,6 @@ pub enum UnaryOp {
     ///
     /// See `GoDom`.
     GoCodom(),
-    /// Append text to the tag of a label.
-    Tag(String),
 
     /// Wrap a term with a type tag (see `Wrapped` in [`Term`](enum.Term.html)).
     Wrap(),
@@ -567,7 +561,7 @@ pub enum UnaryOp {
 
 /// Primitive binary operators
 #[derive(Clone, Debug, PartialEq)]
-pub enum BinaryOp<CapturedTerm> {
+pub enum BinaryOp {
     /// Addition of numerals.
     Plus(),
     /// Substraction of numerals.
@@ -598,12 +592,15 @@ pub enum BinaryOp<CapturedTerm> {
     ///
     /// See `GoDom`.
     GoField(),
+    /// Set the tag text of a blame label.
+    Tag(),
     /// Extend a record with a dynamic field.
     ///
     /// Dynamic means that the field name may be an expression instead of a statically known
-    /// string.  `DynExtend` tries to evaluate this name to a string, and in case of success, add a
-    /// field with this name to the given record with the `CapturedTerm` as content.
-    DynExtend(CapturedTerm),
+    /// string. `DynExtend` tries to evaluate this name to a string, and in case of success, add a
+    /// field with this name to the given record with the expression on top of the stack as
+    /// content.
+    DynExtend(),
     /// Remove a field from a record. The field name is given as an arbitrary Nickel expression.
     DynRemove(),
     /// Access the field of record. The field name is given as an arbitrary Nickel expression.
@@ -618,34 +615,7 @@ pub enum BinaryOp<CapturedTerm> {
     Merge(),
 }
 
-impl<Ty> BinaryOp<Ty> {
-    pub fn map<To, F: Fn(Ty) -> To>(self, f: F) -> BinaryOp<To> {
-        use BinaryOp::*;
-
-        match self {
-            DynExtend(t) => DynExtend(f(t)),
-            Plus() => Plus(),
-            Sub() => Sub(),
-            Div() => Div(),
-            Mult() => Mult(),
-            Modulo() => Modulo(),
-            PlusStr() => PlusStr(),
-            Eq() => Eq(),
-            LessThan() => LessThan(),
-            LessOrEq() => LessOrEq(),
-            GreaterThan() => GreaterThan(),
-            GreaterOrEq() => GreaterOrEq(),
-            Unwrap() => Unwrap(),
-            GoField() => GoField(),
-            DynRemove() => DynRemove(),
-            DynAccess() => DynAccess(),
-            HasField() => HasField(),
-            ListConcat() => ListConcat(),
-            ListElemAt() => ListElemAt(),
-            Merge() => Merge(),
-        }
-    }
-
+impl BinaryOp {
     pub fn is_strict(&self) -> bool {
         match self {
             BinaryOp::Merge() => false,
@@ -1034,7 +1004,7 @@ pub mod make {
         Term::Op1(op, t.into()).into()
     }
 
-    pub fn op2<T1, T2>(op: BinaryOp<RichTerm>, t1: T1, t2: T2) -> RichTerm
+    pub fn op2<T1, T2>(op: BinaryOp, t1: T1, t2: T2) -> RichTerm
     where
         T1: Into<RichTerm>,
         T2: Into<RichTerm>,
diff --git a/src/typecheck.rs b/src/typecheck.rs
index 9c0badfc..6cc7b09b 100644
--- a/src/typecheck.rs
+++ b/src/typecheck.rs
@@ -622,7 +622,7 @@ fn type_check_(
             }
         }
         Term::Op1(op, t) => {
-            let ty_op = get_uop_type(state, envs.clone(), strict, op)?;
+            let ty_op = get_uop_type(state, op)?;
 
             let src = TypeWrapper::Ptr(new_var(state.table));
             let arr = mk_tyw_arrow!(src.clone(), ty);
@@ -631,7 +631,7 @@ fn type_check_(
             type_check_(state, envs.clone(), strict, t, src)
         }
         Term::Op2(op, e, t) => {
-            let ty_op = get_bop_type(state, envs.clone(), strict, op)?;
+            let ty_op = get_bop_type(state, op)?;
 
             let src1 = TypeWrapper::Ptr(new_var(state.table));
             let src2 = TypeWrapper::Ptr(new_var(state.table));
@@ -1355,12 +1355,7 @@ fn instantiate_foralls(state: &mut State, mut ty: TypeWrapper, inst: ForallInst)
 }
 
 /// Type of unary operations.
-pub fn get_uop_type(
-    state: &mut State,
-    _envs: Envs,
-    _strict: bool,
-    op: &UnaryOp,
-) -> Result<TypeWrapper, TypecheckError> {
+pub fn get_uop_type(state: &mut State, op: &UnaryOp) -> Result<TypeWrapper, TypecheckError> {
     Ok(match op {
         // forall a. bool -> a -> a -> a
         UnaryOp::Ite() => {
@@ -1408,7 +1403,7 @@ pub fn get_uop_type(
         // This should not happen, as Switch() is only produced during evaluation.
         UnaryOp::Switch(_) => panic!("cannot typecheck Switch()"),
         // Dyn -> Dyn
-        UnaryOp::ChangePolarity() | UnaryOp::GoDom() | UnaryOp::GoCodom() | UnaryOp::Tag(_) => {
+        UnaryOp::ChangePolarity() | UnaryOp::GoDom() | UnaryOp::GoCodom() => {
             mk_tyw_arrow!(AbsType::Dyn(), AbsType::Dyn())
         }
         // Sym -> Dyn -> Dyn
@@ -1469,12 +1464,7 @@ pub fn get_uop_type(
 }
 
 /// Type of a binary operation.
-pub fn get_bop_type(
-    state: &mut State,
-    envs: Envs,
-    strict: bool,
-    op: &BinaryOp<RichTerm>,
-) -> Result<TypeWrapper, TypecheckError> {
+pub fn get_bop_type(state: &mut State, op: &BinaryOp) -> Result<TypeWrapper, TypecheckError> {
     Ok(match op {
         // Num -> Num -> Num
         BinaryOp::Plus()
@@ -1491,6 +1481,8 @@ pub fn get_bop_type(
             AbsType::Dyn(),
             AbsType::Dyn()
         ),
+        // Str -> Dyn -> Dyn
+        BinaryOp::Tag() => mk_tyw_arrow!(AbsType::Str(), AbsType::Dyn(), AbsType::Dyn()),
         // forall a b. a -> b -> Bool
         BinaryOp::Eq() => mk_tyw_arrow!(
             TypeWrapper::Ptr(new_var(state.table)),
@@ -1510,16 +1502,14 @@ pub fn get_bop_type(
 
             mk_tyw_arrow!(AbsType::Str(), mk_typewrapper::dyn_record(res.clone()), res)
         }
-        // Str -> { _ : a } -> { _ : a }
-        // Unify t with a.
-        BinaryOp::DynExtend(t) => {
+        // forall a. Str -> { _ : a } -> a -> { _ : a }
+        BinaryOp::DynExtend() => {
             let res = TypeWrapper::Ptr(new_var(state.table));
 
-            type_check_(state, envs.clone(), strict, t, res.clone())?;
-
             mk_tyw_arrow!(
                 AbsType::Str(),
                 mk_typewrapper::dyn_record(res.clone()),
+                res.clone(),
                 mk_typewrapper::dyn_record(res)
             )
         }
diff --git a/stdlib/contracts.ncl b/stdlib/contracts.ncl
index bde17055..76801b16 100644
--- a/stdlib/contracts.ncl
+++ b/stdlib/contracts.ncl
@@ -48,18 +48,19 @@
           let t = t -$ field in
           cont acc l t
       else
-          %blame% (%tag% "missing field" l);
+          %blame% (%tag% "missing field `#{field}`" l);
 
   forall_tail = fun sy pol acc l t =>
       let magic_fld = "_%wrapped" in
       if pol == (%polarity% l) then
           if %hasField% magic_fld t then
-              if (t -$ magic_fld) == {} then
+              let rest = (t -$ magic_fld) in
+              if rest == {} then
                   let fail = %blame% (%tag% "polymorphic tail mismatch" l) in
                   let inner = %unwrap% sy (t.$magic_fld) fail in
                   acc & inner
               else
-                  %blame% (%tag% "extra field" l)
+                  %blame% (%tag% "extra field `#{%head% (%fieldsOf% rest)}`" l)
           else
               %blame% (%tag% "missing polymorphic part" l)
       else
@@ -69,5 +70,5 @@
 
   empty_tail = fun acc l t =>
       if t == {} then acc
-      else %blame% (%tag% "extra field" l);
+      else %blame% (%tag% "extra field `#{%head% (%fieldsOf% t)}`" l);
 }


Test patch:
diff --git a/src/parser/tests.rs b/src/parser/tests.rs
index 90b11188..7297ddee 100644
--- a/src/parser/tests.rs
+++ b/src/parser/tests.rs
@@ -177,21 +177,20 @@ fn record_terms() {
 
     assert_eq!(
         parse_without_pos("{ a = 1; $123 = (if 4 then 5 else 6); d = 42;}"),
-        mk_term::op2(
-            BinaryOp::DynExtend(mk_app!(
-                mk_term::op1(UnaryOp::Ite(), Num(4.)),
-                Num(5.),
-                Num(6.)
-            )),
-            Num(123.),
-            RecRecord(
-                vec![
-                    (Ident::from("a"), Num(1.).into()),
-                    (Ident::from("d"), Num(42.).into()),
-                ]
-                .into_iter()
-                .collect()
-            )
+        mk_app!(
+            mk_term::op2(
+                BinaryOp::DynExtend(),
+                Num(123.),
+                RecRecord(
+                    vec![
+                        (Ident::from("a"), Num(1.).into()),
+                        (Ident::from("d"), Num(42.).into()),
+                    ]
+                    .into_iter()
+                    .collect()
+                )
+            ),
+            mk_app!(mk_term::op1(UnaryOp::Ite(), Num(4.)), Num(5.), Num(6.))
         )
     );
 }