Repository: tweag/nickel
Pull request #688 (2022-04-28 15:50:40)

Fix data being serialized as a Switch
We encountered the following "funny" bug: we were importing some YAML files, to which we applied a Nickel contract, and had the evaluation panicking about a `Switch` construct missing an argument. But we could find a switch nowhere in the code of the example, nor in any of the elaborated code nor in the stdlib.

When importing and re-exporting the YAML files immediately, without any other action, parts where strangely missing. It turns out serde, the serialization library, is being too smart for our own good. Because we don't want to serialize or deserialize anything that is not data (i.e. a value in the lambda-calculus sense, additionally excluding functions), we use the `#[serde(skip)]` annotation for most of the computational constructs. However, the `Switch` was missing this precious annotation. Here is the layout the Switch variant:

```rust
Switch(
  RichTerm, /* the matched expression */
  HashMap<Ident, RichTerm>, /* cases and their associated body */
  Option<RichTerm> /* default case */
)
```

Because serde tries to be smart, it doesn't complain that it can't serialize the switch. Instead, it produces a switch for something that matches this layout, which means in JSON: a list (the tuple) of size exactly 3, with the second element being a record, and the others being arbitrary. That is, the following example in JSON:

```json
{
  "3_tuple_of_records": [ 
    { "foo": 1 },
    { "bar": "bar" },
    { "baz": false }
  ]
}
```

is serialized as 

```nickel
{
  "3_uple_of_records" =
    switch {
      `bar => "bar",
       _ => { baz = false },
    } {foo = 1}
}
```

Because the matched expression `{foo = 1}` is not an enum (and nothing is ever serialized as an enum, so no matter what the first element is), the default case is picked, which is the last element of the list:

```console
$ nickel export <<< 'import "test-repro.ncl"'
{
  "3_tuple_of_records": {
    "baz": false
  }
}
```

This PR fixes the issue by adding the missing field and add a regression test for variants with up to 4 elements. The original panic is a separate issue that is not solved by this PR.


Patch:
diff --git a/src/term.rs b/src/term.rs
index e3ff97f7..3708aa1b 100644
--- a/src/term.rs
+++ b/src/term.rs
@@ -97,6 +97,7 @@ pub enum Term {
     ),
     /// A switch construct. The evaluation is done by the corresponding unary operator, but we
     /// still need this one for typechecking.
+    #[serde(skip)]
     Switch(
         RichTerm,                 /* tested expression */
         HashMap<Ident, RichTerm>, /* cases */


Test patch:
diff --git a/tests/pass/serialize.ncl b/tests/pass/serialize.ncl
index 1c09f9ef..ead99f81 100644
--- a/tests/pass/serialize.ncl
+++ b/tests/pass/serialize.ncl
@@ -21,6 +21,34 @@ let assertDeserInv = fun x =>
   assertSerInv {val = ["a", 3, []]},
   assertSerInv {a.foo.bar = "2", b = false, c = [{d = "e"}, {d = "f"}]},
 
+  # regression test for a previously missing `#[serde(skip)]` on the `Switch`
+  # variant of `term::Term`. That was causing a list with the right size to be
+  # serialized as a `Switch` instead of an array. This test checks that lists of
+  # various sizes don't get misinterpreted again as a term construct that misses a
+  # `#[serde(skip)]` annotations
+  {
+    one = [
+      {foo = 1},
+    ],
+    two = [
+      {foo = 1},
+      {bar = "bar"},
+    ],
+    three = [
+      {foo = 1},
+      {bar = "bar"},
+      {baz = false},
+    ],
+    four = [
+      {foo = 1},
+      {bar = "bar"},
+      {baz = false},
+      {last = 4},
+    ]
+  }
+  |> assertSerInv,
+
+
   assertDeserInv {a = 1, b = 4, c = 3},
   assertDeserInv {a.b.c = "richtig"},
   assertDeserInv {