Repository: tweag/nickel (rust)

Issue:
Multiple contracts on a record not working as intended
**Describe the bug**
Typically, the following code would apply multiple contracts on a value and if value is a negative number it would fail on the second part of the contract.

```nickel
-123 | Number | std.number.PosNat 
```

However, this is not the case when applying multiple record contracts and both have the same set of fields. The snippet below should have failed when evaluating `container` but it doesn't.  Based on my testing, this only happens when both inner and outer have the same of fields and adding an additional field to either contracts would give the expected behavior.

```nickel
let outer = {
  spec
    | {
      hostAliases | Number | optional,
      containers | Number | optional,
      ..
    }
    | optional,
  ..
}
in
let inner = {

  spec
    | {
      hostAliases
        | optional,
      containers
        | Array {
          ..
        },
      ..
    }
    | optional,
  ..
}
in
{
  spec = {
    hostAliases = "this should have failed",
    containers = [
      {
        name = "test",
        image = "nginx",
        ports = [
          {
            containerPort = 80,
            name = "http"
          }
        ],
      }
    ]
  }
} | inner | outer 
```

**Expected behavior**
`{} | contract1 |  contract2` should work when both contracts run checks against the same set of fields. 

**Environment**
 - OS name + version: NixOS 24..05
 - Version of the code: 1.8.0


Pull request description:
Fix unsound record contract deduplication
Closes #2041.

As hinted in the original issue, the culprit is indeed the contract deduplication optimization, and more specifically the contract equality checker. The checker only looks at the fields' values to establish equality without looking at the metadata, which is obviously wrong, as it would equate most contracts with the same structures such as `{foo | Number}` and `{foo | String}`, since the distinction is in the metadata.

This commit fixes the issue by comparing the field metadata as well.

Patch:
diff --git a/core/src/typecheck/eq.rs b/core/src/typecheck/eq.rs
index 0cceb9e3..e9835b28 100644
--- a/core/src/typecheck/eq.rs
+++ b/core/src/typecheck/eq.rs
@@ -548,6 +548,11 @@ fn erows_as_map<E: TermEnvironment>(
 
 /// Check for contract equality between record fields. Fields are equal if they are both without a
 /// definition, or are both defined and their values are equal.
+///
+/// The attached metadata must be equal as well: most record contracts are written as field with
+/// metadata but without definition. For example, take `{ foo | {bar | Number}}` and `{foo | {bar |
+/// String}}`. Those two record contracts are obviously not equal, but to know that, we have to
+/// look at the contracts of each bar field.
 fn contract_eq_fields<E: TermEnvironment>(
     state: &mut State,
     field1: &Field,
@@ -555,13 +560,57 @@ fn contract_eq_fields<E: TermEnvironment>(
     field2: &Field,
     env2: &E,
 ) -> bool {
-    match (&field1.value, &field2.value) {
+    // Check that the pending contracts are equal.
+    //
+    // [^contract-eq-ignore-label]: We mostly ignore the label here, which doesn't impact the fact
+    // that a contract blame or not. Different labels might lead to different error messages,
+    // though. Note that there is one important exception: the field `type_environment` does impact
+    // the evaluation of the contract. Fortunately, it's a simple datastructure that is easy to
+    // compare, so we do check for equality here.
+    //
+    // Otherwise, comparing the rest of the labels seem rather clumsy (as labels store a wide
+    // variety of static and runtime data) and not very meaningful.
+    let pending_contracts_eq = field1
+        .pending_contracts
+        .iter()
+        .zip(field2.pending_contracts.iter())
+        .all(|(c1, c2)| {
+            c1.label.type_environment == c2.label.type_environment
+                && contract_eq_bounded(state, &c1.contract, env1, &c2.contract, env2)
+        });
+
+    // Check that the type and contrat annotations are equal. [^contract-eq-ignore-label] applies
+    // here as well.
+    let annotations_eq = field1
+        .metadata
+        .annotation
+        .iter()
+        .zip(field2.metadata.annotation.iter())
+        .all(|(t1, t2)| {
+            t1.label.type_environment == t2.label.type_environment
+                && type_eq_bounded(
+                    state,
+                    &GenericUnifType::from_type(t1.typ.clone(), env1),
+                    env1,
+                    &GenericUnifType::from_type(t2.typ.clone(), env2),
+                    env2,
+                )
+        });
+
+    // Check that "scalar" metadata (simple values) are equals
+    let scalar_metadata_eq = field1.metadata.opt == field2.metadata.opt
+        && field1.metadata.not_exported == field2.metadata.not_exported
+        && field1.metadata.priority == field2.metadata.priority;
+
+    let value_eq = match (&field1.value, &field2.value) {
         (Some(ref value1), Some(ref value2)) => {
             contract_eq_bounded(state, value1, env1, value2, env2)
         }
         (None, None) => true,
         _ => false,
-    }
+    };
+
+    pending_contracts_eq && annotations_eq && scalar_metadata_eq && value_eq
 }
 
 /// Perform the type equality comparison on types. Structurally recurse into type constructors and


Test patch:
diff --git a/core/tests/integration/inputs/contracts/unsound_dedup_record_contract.ncl b/core/tests/integration/inputs/contracts/unsound_dedup_record_contract.ncl
new file mode 100644
index 00000000..beb669e0
--- /dev/null
+++ b/core/tests/integration/inputs/contracts/unsound_dedup_record_contract.ncl
@@ -0,0 +1,49 @@
+# test.type = 'error'
+# eval = 'full'
+#
+# [test.metadata]
+# error = 'EvalError::BlameError'
+
+# Regression test for https://github.com/tweag/nickel/issues/1700
+let outer = {
+  spec
+    | {
+      hostAliases | Number,
+      containers | Number,
+    },
+}
+in
+
+let inner = {
+  spec
+    | {
+      hostAliases,
+      containers
+        | Array {
+          ..
+        },
+    },
+}
+in
+
+std.deep_seq
+  (
+    {
+      spec = {
+        hostAliases = "this should fail",
+        containers = [
+          {
+            name = "test",
+            image = "nginx",
+            ports = [
+              {
+                containerPort = 80,
+                name = "http"
+              }
+            ],
+          }
+        ]
+      }
+    } | inner | outer
+  )
+  true