Repository: tweag/nickel
Pull request #632 (2022-03-02 21:00:17)

Remove -$ and $[ .. ] syntax and use standard %xxx% primop instead
The following syntax is currently used for field removal and field insertion:

```nickel
{foo = "foo", bar = "bar} -$ "foo" // Gives {bar = "bar"}
{foo = "foo"} $[ "baz" = 0 ] // Gives {foo = "foo", baz = 0}
```

This syntax dates back to the very first prototype of Nickel, is quite idiosyncratic, takes syntax real estate, and now that we have the reverse application operator `|>`, isn't really helpful even for chaining several operations.

This PR removes the original idiosyncratic operators `-$` and `$[ ]` for field removal and field insertion and replaces them with standard primop syntax %record_remove% and %record_insert%. It adds corresponding wrappers in the record stdlib, as well as an `update` fonction, that is the composition of the two (removal then insertion, or just insertion if the field wasn't there).

On a side note, I also made the type of `record.has_field` slightly more precise, from `Str -> Dyn -> Bool` to `forall a. Str -> {_ : a} -> Bool` to make `remove` typecheck. We could probably do the same for `record.fields` and `record.values`, but this is out of scope for this PR.


Patch:
diff --git a/benches/records/countLetters.ncl b/benches/records/countLetters.ncl
index 44dd220f..86833106 100644
--- a/benches/records/countLetters.ncl
+++ b/benches/records/countLetters.ncl
@@ -2,8 +2,8 @@
   run = fun s =>
     let update_dict = fun dict char =>
       if record.has_field char dict then
-        dict -$ char & {"%{char}" = dict."%{char}" + 1}
+        record.update char (dict."%{char}" + 1) dict
       else
-        dict & {"%{char}" = 1} in
+        record.insert "%{char}" 1 dict in
     array.foldl update_dict {} (string.chars s)
 }
diff --git a/doc/manual/typing.md b/doc/manual/typing.md
index 7b2474b7..84027d2e 100644
--- a/doc/manual/typing.md
+++ b/doc/manual/typing.md
@@ -383,19 +383,6 @@ could still write `addTotal {total = 1, foo = 1} {total = 2, foo = 2}` but not
 What comes before the tail may include several fields, is in e.g. `forall a.
 {total: Num, subtotal: Num | a} -> Num`.
 
-<!-- TODO: should we keep this example? To revisit before release. Extension
-operator is a tad ugly, and ideally we would only use merge. But the type of
-merge can't be currently express in the type system. -->
-Row types can appear in the result of the function as well. The following
-example returns a new version of the input where fields `a` and `b` have been
-summed, without modifying the rest:
-
-```nickel
-let sum : forall r. {a : Num, b : Num | r} -> {a : Num, b : Num, sum : Num | r}
-        = fun x => x $[ "sum" = x.a + x.b]
-in sum {a = 1, b = 2, c = 3} # {a=1, b=2, sum=3, c=3}
-```
-
 Note that row polymorphism also works with enums, with the same intuition of a
 tail that can be substituted for something else. For example:
 
diff --git a/src/eval/mod.rs b/src/eval/mod.rs
index 092b2a52..8a872d84 100644
--- a/src/eval/mod.rs
+++ b/src/eval/mod.rs
@@ -510,7 +510,7 @@ where
 
                 // Transform the static part `{stat1 = val1, ..., statn = valn}` and the dynamic
                 // part `{exp1 = dyn_val1, ..., expm = dyn_valm}` to a sequence of extensions
-                // `{stat1 = val1, ..., statn = valn} $[ exp1 = dyn_val1] ... $[ expn = dyn_valn ]`
+                // `%record_insert% exp1 (... (%record_insert% expn {stat1 = val1, ..., statn = valn} dyn_valn)) dyn_val1`
                 // The `dyn_val` are given access to the recursive environment, but the recursive
                 // environment only contains the static fields, and not the dynamic fields.
                 let extended = dyn_fields
diff --git a/src/grammar.lalrpop b/src/grammar.lalrpop
index 5f14aba2..548b4815 100644
--- a/src/grammar.lalrpop
+++ b/src/grammar.lalrpop
@@ -264,9 +264,6 @@ RecordOperand: UniTerm = {
 RecordOperationChain: RichTerm = {
     <t: AsTerm<RecordOperand>> "." <id: Ident> => mk_term::op1(UnaryOp::StaticAccess(id), t),
     <t: AsTerm<RecordOperand>> "." <t_id: WithPos<StrChunks>> => mk_access(t_id, t),
-    <t: AsTerm<RecordOperand>> "-$" <t_id: AsTerm<Atom>> => mk_term::op2(BinaryOp::DynRemove(), t_id, t),
-    <r: AsTerm<RecordOperand>> "$[" <id: Term> "=" <t: Term> "]" =>
-        mk_app!(mk_term::op2(BinaryOp::DynExtend(), id, r), t),
 };
 
 RowTail: Types = {
@@ -723,6 +720,8 @@ BOpPre: BinaryOp = {
     "str_contains" => BinaryOp::StrContains(),
     "str_match" => BinaryOp::StrMatch(),
     "str_is_match" => BinaryOp::StrIsMatch(),
+    "record_insert" => BinaryOp::DynExtend(),
+    "record_remove" => BinaryOp::DynRemove(),
 }
 
 NOpPre<ArgRule>: UniTerm = {
@@ -797,10 +796,8 @@ extern {
         "!=" => Token::Normal(NormalToken::NotEquals),
         "&" => Token::Normal(NormalToken::Ampersand),
         "." => Token::Normal(NormalToken::Dot),
-        "$[" => Token::Normal(NormalToken::DollarBracket),
         "%{" => Token::Str(StringToken::Interpolation),
         "multstr %{" => Token::MultiStr(MultiStringToken::Interpolation),
-        "-$" => Token::Normal(NormalToken::MinusDollar),
 
         "+" => Token::Normal(NormalToken::Plus),
         "-" => Token::Normal(NormalToken::Minus),
@@ -852,6 +849,8 @@ extern {
         "unwrap" => Token::Normal(NormalToken::Unwrap),
         "embed" => Token::Normal(NormalToken::Embed),
         "record_map" => Token::Normal(NormalToken::RecordMap),
+        "record_insert" => Token::Normal(NormalToken::RecordInsert),
+        "record_remove" => Token::Normal(NormalToken::RecordRemove),
         "seq" => Token::Normal(NormalToken::Seq),
         "deep_seq" => Token::Normal(NormalToken::DeepSeq),
         "head" => Token::Normal(NormalToken::Head),
diff --git a/src/parser/lexer.rs b/src/parser/lexer.rs
index 746bf077..fe8c1332 100644
--- a/src/parser/lexer.rs
+++ b/src/parser/lexer.rs
@@ -96,12 +96,8 @@ pub enum NormalToken<'input> {
     Ampersand,
     #[token(".")]
     Dot,
-    #[token("$[")]
-    DollarBracket,
     #[token("\"")]
     DoubleQuote,
-    #[token("-$")]
-    MinusDollar,
 
     #[token("+")]
     Plus,
@@ -187,6 +183,10 @@ pub enum NormalToken<'input> {
     Embed,
     #[token("%record_map%")]
     RecordMap,
+    #[token("%record_insert%")]
+    RecordInsert,
+    #[token("%record_remove%")]
+    RecordRemove,
     #[token("%seq%")]
     Seq,
     #[token("%deep_seq%")]
diff --git a/src/typecheck/operation.rs b/src/typecheck/operation.rs
index 01cef877..0aa9d917 100644
--- a/src/typecheck/operation.rs
+++ b/src/typecheck/operation.rs
@@ -259,19 +259,21 @@ pub fn get_bop_type(
         // forall a. Str -> { _ : a } -> { _ : a}
         BinaryOp::DynRemove() => {
             let res = TypeWrapper::Ptr(state.table.fresh_var());
-
             (
                 mk_typewrapper::str(),
                 mk_typewrapper::dyn_record(res.clone()),
                 mk_typewrapper::dyn_record(res),
             )
         }
-        // Str -> Dyn -> Bool
-        BinaryOp::HasField() => (
-            mk_typewrapper::str(),
-            mk_typewrapper::dynamic(),
-            mk_typewrapper::bool(),
-        ),
+        // forall a. Str -> {_: a} -> Bool
+        BinaryOp::HasField() => {
+            let ty_elt = TypeWrapper::Ptr(state.table.fresh_var());
+            (
+                mk_typewrapper::str(),
+                mk_typewrapper::dyn_record(ty_elt),
+                mk_typewrapper::bool(),
+            )
+        }
         // forall a. Array a -> Array a -> Array a
         BinaryOp::ArrayConcat() => {
             let ty_elt = TypeWrapper::Ptr(state.table.fresh_var());
diff --git a/stdlib/contract.ncl b/stdlib/contract.ncl
index caffc6b0..01e8863a 100644
--- a/stdlib/contract.ncl
+++ b/stdlib/contract.ncl
@@ -47,8 +47,11 @@
 
   "$record_extend" = fun field contr cont acc l t =>
       if %has_field% field t then
-          let acc = acc$[field = %assume% contr (%go_field% field l) (t."%{field}")] in
-          let t = t -$ field in
+          let acc = %record_insert%
+            field
+            acc
+            (%assume% contr (%go_field% field l) (t."%{field}")) in
+          let t = %record_remove% field t in
           cont acc l t
       else
           %blame% (%tag% "missing field `%{field}`" l),
@@ -57,7 +60,7 @@
       let magic_fld = "_%wrapped" in
       if pol == (%polarity% l) then
           if %has_field% magic_fld t then
-              let rest = (t -$ magic_fld) in
+              let rest = %record_remove% magic_fld t in
               if rest == {} then
                   let fail = %blame% (%tag% "polymorphic tail mismatch" l) in
                   let inner = %unwrap% sy (t."%{magic_fld}") fail in
@@ -67,7 +70,7 @@
           else
               %blame% (%tag% "missing polymorphic part" l)
       else
-          acc$[magic_fld = %wrap% sy t],
+          %record_insert% magic_fld acc (%wrap% sy t),
 
   "$dyn_tail" = fun acc l t => acc & t,
 
@@ -166,7 +169,7 @@
         records. Moreover, the interpreter performs additional book-keeping for
         error reporting when applying a contract in an expression `value |
         Contract`. You should not use standard function application to apply a
-        contract, but this function instead. 
+        contract, but this function instead.
 
         For example:
         ```
diff --git a/stdlib/record.ncl b/stdlib/record.ncl
index 1adf436d..ed9e9adc 100644
--- a/stdlib/record.ncl
+++ b/stdlib/record.ncl
@@ -37,7 +37,7 @@
       "%m
     = fun r => %values% r,
 
-    has_field : Str -> Dyn -> Bool
+    has_field : forall a. Str -> {_ : a} -> Bool
     | doc m%"
       Given the name of a field and a record, checks if the record contains the given field.
 
@@ -46,7 +46,66 @@
           false
         has_field "one" { one = 1, two = 2 } =>
           true
+      ```
       "%m
     = fun field r => %has_field% field r,
+
+    insert : forall a. Str -> a -> {_: a} -> {_: a}
+      | doc m%%"
+        Insert a new field in a record. `insert` doesn't mutate the original
+        record but returns a new one instead.
+
+        ```nickel
+        insert "foo" foo { bar = "bar" } =>
+          { foo = "foo", bar = "bar }  
+
+        {}
+        |> insert "file.%{ext}" "data/text"
+        |> insert "length" 10*1000 =>
+          {"file.txt" = "data/text", "length" = 10000}
+        ```
+      "%%m
+      = fun field content r => %record_insert% field r content,
+
+    remove : forall a. Str -> {_: a} -> {_: a}
+      | doc m%"
+        Remove a field from a record. `remove` doesn't mutate the original
+        record but returns a new one instead.
+
+        ```nickel
+        remove "foo" foo { foo = "foo", bar = "bar" } =>
+          { bar = "bar }  
+        ```
+      "%m
+      = fun field r => %record_remove% field r,
+
+    update | forall a. Str -> a -> { ; a} -> { ; a}
+      | doc m%"
+        Update a field of a record with a new value. `update` doesn't mutate the
+        original record but returns a new one instead. If the field to update is absent
+        from the given record, `update` simply adds it.
+
+        ```nickel
+        remove "foo" foo { foo = "foo", bar = "bar" } =>
+          { bar = "bar" }  
+        ```
+
+        As opposed to overriding a value with the merge operator `&`, `update`
+        will only change the specified field and won't automatically update the other
+        fields which depend on it: 
+
+        ```nickel
+        { foo = bar + 1, bar | default = 0 } & { bar = 1 } =>
+          { foo = 2, bar = 1 }
+        update "bar" 1 {foo = bar + 1, bar | default = 0 } =>
+          { foo = 1, bar = 1 } 
+        ```
+      "%m
+      = fun field content r => 
+        let r = if %has_field% field r then
+          %record_remove% field r
+        else
+          r in
+        %record_insert% field r,
   }
 }


Test patch:
diff --git a/tests/contracts_fail.rs b/tests/contracts_fail.rs
index 82d6e32c..453606bf 100644
--- a/tests/contracts_fail.rs
+++ b/tests/contracts_fail.rs
@@ -98,13 +98,16 @@ fn records_contracts_poly() {
     // but this is not yet the case.
     // eval_string(&format!("{} {{foo = 2}}", extd)).unwrap_err();
 
-    let remv = "let f | forall a. {foo: Num ; a} -> { ; a} = fun x => x -$ \"foo\" in f";
+    let remv =
+        "let f | forall a. {foo: Num ; a} -> { ; a} = fun x => %record_remove% \"foo\" x in f";
     assert_raise_blame!(&format!("{} {{}}", remv));
 
     let bad_cst = "let f | forall a. { ; a} -> { ; a} = fun x => {a=1} in f";
     let bad_acc = "let f | forall a. { ; a} -> { ; a} = fun x => %seq% x.a x in f";
-    let bad_extd = "let f | forall a. { ; a} -> {foo: Num ; a} = fun x => x -$ \"foo\" in f";
-    let bad_rmv = "let f | forall a. {foo: Num ; a} -> { ; a} = fun x => x$[\"foo\"=1] in f";
+    let bad_extd =
+        "let f | forall a. { ; a} -> {foo: Num ; a} = fun x => %record_remove% \"foo\" x in f";
+    let bad_rmv =
+        "let f | forall a. {foo: Num ; a} -> { ; a} = fun x => %record_insert% \"foo\" x 1 in f";
 
     assert_raise_blame!(&format!("{} {{}}", bad_cst));
     // The polymorphic parts is protected by hiding it. Thus, trying to access the protected field
@@ -124,7 +127,7 @@ fn records_contracts_poly() {
                 (forall b. {a: Num, b: Num ; b} -> { a: Num ; b})
                 -> {a: Num ; a}
                 -> { ; a}
-            = fun f rec => (f rec) -$ \"a\" -$ \"b\" in
+            = fun f rec => %record_remove% \"b\" (%record_remove% \"a\" (f rec)) in
         f (fun x => x) {a = 1, b = true, c = 3}"
     );
 }
diff --git a/tests/pass/contracts.ncl b/tests/pass/contracts.ncl
index 2d847081..baa79b12 100644
--- a/tests/pass/contracts.ncl
+++ b/tests/pass/contracts.ncl
@@ -64,8 +64,7 @@ let Assert = fun l x => x || %blame% l in
   (let id | forall a. { ; a} -> { ; a} = fun x => x in
     let extend | forall a. { ; a} -> {foo: Num ; a} = fun x =>
       x & {foo = 1} in
-    let remove | forall a. {foo: Num ; a} -> { ; a} = fun x =>
-      x -$ "foo" in
+    let remove | forall a. {foo: Num ; a} -> { ; a} = record.remove "foo" in
 
     (id {} == {} | Assert) &&
     (id {a = 1, b = false} == {a = 1, b = false} | Assert) &&
diff --git a/tests/pass/eq.ncl b/tests/pass/eq.ncl
index 036a5a1e..648306cb 100644
--- a/tests/pass/eq.ncl
+++ b/tests/pass/eq.ncl
@@ -35,7 +35,9 @@ let Assert = fun l x => x || %blame% l in
 
   # records
   {} == {},
-  {}$["a" = 1]$["b" = true]
+  ({}
+  |> record.insert "a" 1
+  |> record.insert "b" true)
     == {a = 1, b = true},
   {a = 1 + 0, b = 1 + 1, c = 1 + 1 + 1}
     == { a = 1, b = 2, c = 3 },
@@ -45,7 +47,7 @@ let Assert = fun l x => x || %blame% l in
       baz = if true then true else false
     } == {foo = 1, bar = "ab", baz = true},
 
-  {}$["a" = { a = { a = {} } }]
+  record.insert "a" {a.a = {}} {}
     == { a = { a = { a = {} } } },
   {
       foo = {bar = 2 + (-1)},
diff --git a/tests/pass/records.ncl b/tests/pass/records.ncl
index 735cd57c..67785eb1 100644
--- a/tests/pass/records.ncl
+++ b/tests/pass/records.ncl
@@ -10,15 +10,19 @@ let Assert = fun l x => x || %blame% l in
   {"%{if true then "foo" else "bar"}" = false, bar = true}."%{"foo"}"
    == false,
 
-  ({bar = 3}$["foo" = true]).foo == true,
+  (record.insert "foo" true {bar = 3}).foo == true,
 
   # primitive_ops
   record.has_field "foo" {foo = 1, bar = 2},
   record.has_field "fop" {foo = 1, bar = 2} == false,
-  record.has_field "foo" ({foo = 2, bar = 3} -$ "foo")
+  ({foo = 2, bar = 3}
+  |> record.remove "foo"
+  |> record.has_field "foo")
     == false,
 
-  record.has_field "foo" ({bar = 3}$["foo" = 1]),
+  {bar = 3}
+  |> record.insert "foo" 1
+  |> record.has_field "foo",
 
   # lazyness of map
   (record.map (fun x y => y + 1) {foo = 1, bar = "it's lazy"}).foo