Fix the order of argument in (.) (#1752)

* Fix order of arguments for curried dot operator

* Add std.record.get to stdlib

* Update release note with breaking changes for (.)

Co-authored-by: jneem <joeneeman@gmail.com>

* Remove snapshot test for std.record.get

Unfortunately, the output of the error message shows a closure address
(in practice, a memory address), which makes it non-determinstic and
thus unfit for a snpashot test.

Instead, it's been replaced with an integration test, which
Unfortunately won't allow to detect a deterioriation in error reporting,
but at least ensure that the contract of `std.record.get` works as
expected.

---------

Co-authored-by: jneem <joeneeman@gmail.com>

RELEASES.md

@@ -1,3 +1,26 @@
+Next version (planned as 1.4)
+=============================
+
+Breaking changes
+----------------
+
+* The curried dot operator added in Nickel 1.3 was implemented the wrong way:
+    the arguments were flipped, meaning that `(.) foo bar` was `bar."%{foo}"`
+    instead of the expected `foo."%{bar}"`. While the initial flipped
+    implementation seems more useful for piping operations using the reverse
+    application operator `|>`, it's inconsistent with all other curried
+    operators, where `(<operator>)` is always defined as `fun x y => x
+    <operator> y`. To ensure consistency, and because the initial behavior was
+    an oversight and not a conscious design decision, we decided to change the
+    definition of `(.)` to match the other curried operator by flipping its
+    arguments.
+
+    To fill the gap, Nickel 1.4 introduces `std.record.get` with the same
+    definition as the `(.)` introduced in Nickel 1.3. To migrate from 1.3 to
+    1.4, you can either flip the arguments of the curried dot as a function
+    `(.)` whenever possible, or you can just replace it with the new
+    `std.record.get`.
+
 Version 1.3
 ===========
 

core/src/parser/grammar.lalrpop

@@ -814,15 +814,28 @@ CurriedOp: RichTerm = {
             mk_term::op1(
                 UnaryOp::BoolNot(),
                 Term::Op2(BinaryOp::Eq(),
-                    mk_term::var("x2"),
-                    mk_term::var("x1")
+                    mk_term::var("x1"),
+                    mk_term::var("x2")
                 )
             )
             .with_pos(mk_pos(src_id, l, r))
         ),
+    //`foo.bar` is a static
+    // record access, but when used in a curried form, it's a dynamic record
+    // access (that is, `(.) foo bar` is `foo."%{bar}"`). It turns out a dynamic
+    // record access takes the record as the last argument, in the style of the
+    // stdlib. If we want `(.) foo bar` to be `foo."%{bar}"`, we thus have to
+    // flip the arguments.
     <l: @L> "." <r: @R> =>
-        InfixOp::from(BinaryOp::DynAccess())
-            .eta_expand(mk_pos(src_id, l, r)),
+        mk_fun!(
+            "x1",
+            "x2",
+            mk_term::op2(
+                BinaryOp::DynAccess(),
+                mk_term::var("x2"),
+                mk_term::var("x1"),
+            ).with_pos(mk_pos(src_id, l, r))
+        ),
     //<l: @L> "->" <r: @R> =>?
     //    UniTerm::from(
     //        mk_fun!("x1", "x2",

core/stdlib/std.ncl

@@ -1416,7 +1416,57 @@
                       fun end_index =>
                         ArraySliceArray end_index -> Dyn
                     )
-              )
+              ),
+
+        HasField
+          | doc m%%"
+              **Warning**: this is an unstable item. It might be renamed,
+              modified or deleted in any subsequent minor Nickel version.
+
+              A function contract for 2-ary functions which checks that second
+              argument is a record that contains the first argument as a field.
+              This contract is parametrized by the return type of the function.
+
+              This contract blames only if the arguments are of the right type
+              but don't satisfy the tested condition (the first is a string and
+              the second is a record). The type of arguments is assumed to be
+              checked by an associated static type annotation. For example, if
+              the first argument is a number, this contract silently passes.
+
+              ## Example
+
+              ```nickel
+              let f | HasField Dyn = fun field record => record."%{field}" in
+              (f "foo" { foo = 1, bar = 2 })
+              + (f "baz" { foo = 1, bar = 2 })
+              ```
+
+              In this example, the first call to `f` won't blame, but the second
+              will, as `baz` isn't a field of the record argument.
+            "%%
+          =
+            # capture the `contract.label` module to avoid name collisions with
+            # local contract argument `label`
+            let label_module = label in
+            let attach_message = label_module.with_message "missing field" in
+
+            fun Codomain =>
+              let HasFieldSecond = fun field label value =>
+                if %typeof% field == 'String
+                && %typeof% value == 'Record
+                && !(%has_field% field value) then
+                  label
+                  |> attach_message
+                  |> label_module.append_note
+                    "The record given as an argument lacks the required field `%{field}`."
+                  |> blame
+                else
+                  value
+              in
+
+              DependentFun
+                Dyn
+                (fun field => HasFieldSecond field -> Codomain),
       }
   },
 
@@ -1893,6 +1943,37 @@
       "%
       = fun field r => %has_field% field r,
 
+    get
+      : forall a. String -> { _ : a } -> a
+      | std.contract.unstable.HasField Dyn
+      | doc m%%"
+        Returns the field of a record with the given name.
+
+        `std.record.get field record` is just `record."%{field}"`. The latter
+        form is more idiomatic and should be generally preferred.
+
+        However, `std.record.get` can come in handy when a proper function is
+        expected, typically in a sequence of operations chained with the reverse
+        application operator `|>`.
+
+        Trying to extract a field which doesn't exist will result in a contract
+        error.
+
+        # Examples
+
+        ```nickel
+        std.record.get "one" { one = 1, two = 2 } =>
+          1
+        { one = 1, two = 2, string = "three"}
+        |> std.record.to_array
+        |> std.array.filter (fun { field, value } => std.is_number value)
+        |> std.record.from_array
+        |> std.record.get "two"
+          => 2
+        ```
+      "%%
+      = fun field r => r."%{field}",
+
     insert
       : forall a. String -> a -> { _ : a } -> { _ : a }
       | doc m%%"

core/tests/integration/fail/stdlib/record_get_missing_field.ncl

@@ -0,0 +1,15 @@
+# test.type = 'error'
+# eval = 'full'
+#
+# [test.metadata]
+# error = 'EvalError::BlameError'
+let record = {
+  foo = 1,
+  bar = "two",
+  baz = false,
+}
+in
+
+record
+|> std.record.remove "foo"
+|> std.record.get "foo"

core/tests/integration/pass/core/curried_dot.ncl

@@ -5,8 +5,8 @@ let record = {foo = 1, bar = { baz = 2 }} in
 let field = "bar" in
 let part = "ba" in
 [
-  (.) "foo" record == 1,
-  (.) field record == { baz = 2 },
-  let res = (.) field record == {baz = 2} in res,
-  (.) "%{part}r" record == { baz = 2 },
+  (.) record "foo" == 1,
+  (.) record field == { baz = 2 },
+  let res = (.) record field == {baz = 2} in res,
+  (.) record "%{part}r" == { baz = 2 },
 ] |> check

doc/manual/merging.md

@@ -621,9 +621,9 @@ error: missing definition for `required_field2`
    8 │     & { foo.required_field1 = "here" }
      │             ------------------------ in this record
      │
-     ┌─ <stdlib/std.ncl>:2868:18
+     ┌─ <stdlib/std.ncl>:2949:18
      │
-2868 │     = fun x y => %deep_seq% x y,
+2949 │     = fun x y => %deep_seq% x y,
      │                  ------------ accessed here
 ```