fix: objectRemoveKey exclusions leaking across + inheritance boundaries (#659)

objectRemoveKey's excludedKeys mechanism suppressed re-added keys in
both directions of object inheritance via +.

Two fixes in Val.Obj:

1. gatherKeys: replaced global exclusion set with incremental processing
during root-first iteration. Each object's excludedKeys removes keys
from lower levels, but higher-level objects can re-introduce them.
Fixes: removeKey(obj, k) + {k: v}

2. addSuper: when the RHS has excludedKeys and the LHS provides those
keys as visible, inject synthetic members that delegate to the LHS and
clear the exclusion. Fixes: {k: v} + removeKey(obj, k)

Author: stephenamar-db

sjsonnet/src/sjsonnet/Val.scala

@@ -425,12 +425,39 @@ object Val {
       var i = chain.length - 1
       while (i >= 0) {
         val s = chain(i)
-        val filteredExcludedKeys = if (s.excludedKeys != null) {
+        var members = s.getValue0
+        var filteredExcludedKeys = if (s.excludedKeys != null) {
           Util.intersect(s.excludedKeys, keysInThisChain)
         } else null
+
+        // If this object has excluded keys that the LHS provides as visible,
+        // re-introduce them with synthetic members that delegate to the LHS.
+        // This ensures `lhs + removeKey(rhs, k)` preserves lhs's visible key `k`.
+        if (filteredExcludedKeys != null) {
+          val iter = filteredExcludedKeys.iterator()
+          while (iter.hasNext) {
+            val key = iter.next()
+            if (lhs.containsVisibleKey(key)) {
+              if (members eq s.getValue0) {
+                members = new util.LinkedHashMap[String, Obj.Member](s.getValue0)
+              }
+              val capturedKey = key
+              members.put(
+                key,
+                new Obj.Member(false, Visibility.Normal, deprecatedSkipAsserts = true) {
+                  def invoke(self: Val.Obj, sup: Val.Obj, fs: FileScope, ev: EvalScope): Val =
+                    lhs.value(capturedKey, pos, self)(ev)
+                }
+              )
+              iter.remove()
+            }
+          }
+          if (filteredExcludedKeys.isEmpty) filteredExcludedKeys = null
+        }
+
         current = new Val.Obj(
           s.pos,
-          s.getValue0,
+          members,
           false,
           s.triggerAsserts,
           current,
@@ -498,30 +525,17 @@ object Val {
       val chain = builder.result()
       val chainLength = chain.length
 
-      // Collect all excluded keys, reusing the set directly when only one source has exclusions
-      var exclusionSet: java.util.Set[String] = null
-      var multipleExclusions = false
-      for (s <- chain) {
-        val keys = s.excludedKeys
-        if (Util.isNotEmpty(keys)) {
-          if (exclusionSet == null) {
-            exclusionSet = keys
-          } else {
-            if (!multipleExclusions) {
-              val merged = new util.HashSet[String](exclusionSet.size + keys.size)
-              merged.addAll(exclusionSet)
-              exclusionSet = merged
-              multipleExclusions = true
-            }
-            exclusionSet.asInstanceOf[util.HashSet[String]].addAll(keys)
-          }
-        }
-      }
-
-      // Iterate root-first (reverse of collection order) and populate the mapping
+      // Iterate root-first (reverse of collection order) and populate the mapping.
+      // Each object's excludedKeys removes keys gathered from lower levels (closer to root),
+      // but objects above it in the chain can re-introduce those keys via their own members.
       var i = chainLength - 1
       while (i >= 0) {
-        gatherKeysForSingle(chain(i), exclusionSet, mapping)
+        val s = chain(i)
+        if (Util.isNotEmpty(s.excludedKeys)) {
+          val iter = s.excludedKeys.iterator()
+          while (iter.hasNext) mapping.remove(iter.next())
+        }
+        gatherKeysForSingle(s, null, mapping)
         i -= 1
       }
     }

sjsonnet/test/src/sjsonnet/StdObjectRemoveKeyTests.scala

@@ -0,0 +1,75 @@
+package sjsonnet
+
+import utest._
+import TestUtils.eval
+
+object StdObjectRemoveKeyTests extends TestSuite {
+
+  def tests: Tests = Tests {
+
+    test("basic removal") {
+      eval("std.objectRemoveKey({a: 1, b: 2}, 'a')") ==> ujson.Obj("b" -> 2)
+      eval("std.objectRemoveKey({a: 1, b: 2}, 'b')") ==> ujson.Obj("a" -> 1)
+      eval("std.objectRemoveKey({a: 1}, 'a')") ==> ujson.Obj()
+    }
+
+    test("removing non-existent key is a no-op") {
+      eval("std.objectRemoveKey({a: 1}, 'b')") ==> ujson.Obj("a" -> 1)
+    }
+
+    test("re-adding removed key via object inheritance") {
+      eval("""std.objectRemoveKey({foo: "bar"}, "foo") + {foo: "bar2"}""") ==>
+        ujson.Obj("foo" -> "bar2")
+
+      eval("""std.objectRemoveKey({a: 1, b: 2}, "a") + {a: 3}""") ==>
+        ujson.Obj("b" -> 2, "a" -> 3)
+
+      eval("""std.objectRemoveKey({a: 1, b: 2, c: 3}, "b") + {b: 99, d: 4}""") ==>
+        ujson.Obj("a" -> 1, "c" -> 3, "b" -> 99, "d" -> 4)
+    }
+
+    test("double removal then re-add") {
+      eval(
+        """std.objectRemoveKey(std.objectRemoveKey({a: 1, b: 2, c: 3}, "a"), "b") + {a: 10}"""
+      ) ==> ujson.Obj("c" -> 3, "a" -> 10)
+    }
+
+    test("remove then re-add then remove again") {
+      eval(
+        """std.objectRemoveKey(std.objectRemoveKey({foo: 1, bar: 2}, "foo") + {foo: 3}, "foo")"""
+      ) ==> ujson.Obj("bar" -> 2)
+    }
+
+    test("internal super chain preserved after removal") {
+      eval("std.objectRemoveKey({a: 1} + {b: super.a}, 'a')") ==> ujson.Obj("b" -> 1)
+    }
+
+    test("external inheritance preserved after removal") {
+      eval("{a: 1} + std.objectRemoveKey({b: super.a}, 'a')") ==>
+        ujson.Obj("a" -> 1, "b" -> 1)
+    }
+
+    test("LHS key preserved when RHS removes it") {
+      eval("""{a: 1} + std.objectRemoveKey({a: 2}, "a")""") ==>
+        ujson.Obj("a" -> 1)
+
+      eval("""{a: 1} + std.objectRemoveKey({a: 2, b: 3}, "a")""") ==>
+        ujson.Obj("a" -> 1, "b" -> 3)
+
+      eval("""{a: 10} + std.objectRemoveKey({a: 1} + {b: super.a}, "a")""") ==>
+        ujson.Obj("a" -> 10, "b" -> 1)
+    }
+
+    test("containsKey and objectFields reflect re-added key") {
+      eval(
+        """local r = std.objectRemoveKey({a: 1}, "a") + {a: 2};
+          |std.objectHas(r, "a")""".stripMargin
+      ) ==> ujson.True
+
+      eval(
+        """local r = std.objectRemoveKey({a: 1}, "a") + {a: 2};
+          |std.objectFields(r)""".stripMargin
+      ) ==> ujson.Arr("a")
+    }
+  }
+}