From efabb30a8d8f47c9cad571e3e8833a0f5906a07a Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Tue, 9 Jun 2026 23:11:46 +0200 Subject: [PATCH 1/2] Fix dead map identity comparison in dyn.Value.eq The KindMap branch compared addresses of the receiver's local copies, which is never true, so visit rebuilt ancestor maps on every visited path even when nothing changed. Compare the mapping's backing pairs slice instead, mirroring the KindSequence branch. Co-authored-by: Isaac --- libs/dyn/value.go | 19 ++++++++-- libs/dyn/value_eq_test.go | 76 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 2 deletions(-) create mode 100644 libs/dyn/value_eq_test.go diff --git a/libs/dyn/value.go b/libs/dyn/value.go index e4d72a0b203..42e955c190d 100644 --- a/libs/dyn/value.go +++ b/libs/dyn/value.go @@ -206,9 +206,24 @@ func (v Value) eq(w Value) bool { switch v.k { case KindMap: - // Compare pointers to the underlying map. + // Note: comparing &v.v to &w.v is always false because the method + // has value receivers, so we compare the mapping's backing storage. + vm := v.v.(Mapping) + wm := w.v.(Mapping) + lv := vm.Len() + lw := wm.Len() + // If both maps are empty, they are equal. + if lv == 0 && lw == 0 { + return true + } + // If they have different lengths, they are not equal. + if lv != lw { + return false + } + // They are both non-empty and have the same length. + // Compare pointers to the underlying pairs slice. // This is safe because we don't allow maps to be mutated. - return &v.v == &w.v + return &vm.pairs[0] == &wm.pairs[0] case KindSequence: vs := v.v.([]Value) ws := w.v.([]Value) diff --git a/libs/dyn/value_eq_test.go b/libs/dyn/value_eq_test.go new file mode 100644 index 00000000000..83c255edd54 --- /dev/null +++ b/libs/dyn/value_eq_test.go @@ -0,0 +1,76 @@ +package dyn + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValueEqMap(t *testing.T) { + loc := []Location{{File: "file", Line: 1, Column: 2}} + v := NewValue(map[string]Value{"key": V("value")}, loc) + + tests := []struct { + name string + a, b Value + want bool + }{ + { + name: "same underlying mapping", + a: v, + b: v, + want: true, + }, + { + name: "cloned mapping with equal contents", + a: v, + b: Value{v: v.v.(Mapping).Clone(), k: KindMap, l: v.l}, + want: false, + }, + { + name: "different lengths", + a: v, + b: NewValue(map[string]Value{"key": V("value"), "other": V("value")}, loc), + want: false, + }, + { + name: "different locations", + a: v, + b: v.WithLocations([]Location{{File: "other", Line: 1, Column: 2}}), + want: false, + }, + { + name: "empty mappings", + a: NewValue(map[string]Value{}, loc), + b: NewValue(map[string]Value{}, loc), + want: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, tc.a.eq(tc.b)) + }) + } +} + +func TestValueEqMapVisitDoesNotRebuildAncestors(t *testing.T) { + vin := V(map[string]Value{ + "a": V(map[string]Value{ + "b": V("value"), + }), + }) + + vout, err := Map(vin, "a.b", func(_ Path, v Value) (Value, error) { + return v, nil + }) + require.NoError(t, err) + + // The identity transform must return the original value without + // cloning the ancestor maps. + vm := vin.v.(Mapping) + wm := vout.v.(Mapping) + require.Equal(t, vm.Len(), wm.Len()) + assert.Same(t, &vm.pairs[0], &wm.pairs[0]) +} From c372ab79d2ae9b2dbecd572e4d4dfa6e720a3cc7 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Wed, 10 Jun 2026 07:30:56 +0200 Subject: [PATCH 2/2] Remove redundant comments Co-authored-by: Isaac --- libs/dyn/value.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/libs/dyn/value.go b/libs/dyn/value.go index 42e955c190d..72803511b80 100644 --- a/libs/dyn/value.go +++ b/libs/dyn/value.go @@ -206,21 +206,17 @@ func (v Value) eq(w Value) bool { switch v.k { case KindMap: - // Note: comparing &v.v to &w.v is always false because the method - // has value receivers, so we compare the mapping's backing storage. + // Comparing &v.v to &w.v is always false (value receivers), so compare the Mapping's backing storage. vm := v.v.(Mapping) wm := w.v.(Mapping) lv := vm.Len() lw := wm.Len() - // If both maps are empty, they are equal. if lv == 0 && lw == 0 { return true } - // If they have different lengths, they are not equal. if lv != lw { return false } - // They are both non-empty and have the same length. // Compare pointers to the underlying pairs slice. // This is safe because we don't allow maps to be mutated. return &vm.pairs[0] == &wm.pairs[0]