From 8255c9d9fbb7dc320672869befcd64197c5929e8 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 19 Mar 2024 10:49:26 +0100 Subject: [PATCH] Make `Append` function to `dyn.Path` return independent slice (#1295) ## Changes While working on #1273, I found that calls to `Append` on a `dyn.Pattern` were mutating the original slice. This is expected because appending to a slice will mutate in place if the capacity of the original slice is large enough. This change updates the `Append` call on the `dyn.Path` as well to return a newly allocated slice to avoid inadvertently mutating the originals. We have existing call sites in the `dyn` package that mutate a `dyn.Path` (e.g. walk or visit) and these are modified to continue to do this with a direct call to `append`. Callbacks that use the `dyn.Path` argument outside of the callback need to make a copy to ensure it isn't mutated (this is no different from existing semantics). The `Join` function wasn't used and is removed as part of this change. ## Tests Unit tests. --- libs/dyn/path.go | 14 +++++--------- libs/dyn/path_test.go | 16 +++++++--------- libs/dyn/pattern.go | 4 ++-- libs/dyn/pattern_test.go | 15 +++++++++++---- libs/dyn/visit.go | 2 +- libs/dyn/visit_map.go | 4 ++-- libs/dyn/visit_set.go | 2 +- libs/dyn/walk.go | 4 ++-- 8 files changed, 31 insertions(+), 30 deletions(-) diff --git a/libs/dyn/path.go b/libs/dyn/path.go index 91893f92..76377e2d 100644 --- a/libs/dyn/path.go +++ b/libs/dyn/path.go @@ -49,17 +49,13 @@ func NewPath(cs ...pathComponent) Path { return cs } -// Join joins the given paths. -func (p Path) Join(qs ...Path) Path { - for _, q := range qs { - p = p.Append(q...) - } - return p -} - // Append appends the given components to the path. +// Mutations to the returned path do not affect the original path. func (p Path) Append(cs ...pathComponent) Path { - return append(p, cs...) + out := make(Path, len(p)+len(cs)) + copy(out, p) + copy(out[len(p):], cs) + return out } // Equal returns true if the paths are equal. diff --git a/libs/dyn/path_test.go b/libs/dyn/path_test.go index c4ea26c4..1152a060 100644 --- a/libs/dyn/path_test.go +++ b/libs/dyn/path_test.go @@ -19,16 +19,14 @@ func TestPathAppend(t *testing.T) { assert.True(t, p2.Equal(dyn.NewPath(dyn.Key("foo"), dyn.Key("bar"), dyn.Index(1)))) } -func TestPathJoin(t *testing.T) { - p := dyn.NewPath(dyn.Key("foo")) +func TestPathAppendAlwaysNew(t *testing.T) { + p := make(dyn.Path, 0, 2) + p = append(p, dyn.Key("foo")) - // Single arg. - p1 := p.Join(dyn.NewPath(dyn.Key("bar"))) - assert.True(t, p1.Equal(dyn.NewPath(dyn.Key("foo"), dyn.Key("bar")))) - - // Multiple args. - p2 := p.Join(dyn.NewPath(dyn.Key("bar")), dyn.NewPath(dyn.Index(1))) - assert.True(t, p2.Equal(dyn.NewPath(dyn.Key("foo"), dyn.Key("bar"), dyn.Index(1)))) + // There is room for a second element in the slice. + p1 := p.Append(dyn.Index(1)) + p2 := p.Append(dyn.Index(2)) + assert.NotEqual(t, p1, p2) } func TestPathEqualEmpty(t *testing.T) { diff --git a/libs/dyn/pattern.go b/libs/dyn/pattern.go index 665ba0c5..960a50d5 100644 --- a/libs/dyn/pattern.go +++ b/libs/dyn/pattern.go @@ -58,7 +58,7 @@ func (c anyKeyComponent) visit(v Value, prefix Path, suffix Pattern, opts visitO m = maps.Clone(m) for key, value := range m { var err error - nv, err := visit(value, prefix.Append(Key(key)), suffix, opts) + nv, err := visit(value, append(prefix, Key(key)), suffix, opts) if err != nil { // Leave the value intact if the suffix pattern didn't match any value. if IsNoSuchKeyError(err) || IsIndexOutOfBoundsError(err) { @@ -89,7 +89,7 @@ func (c anyIndexComponent) visit(v Value, prefix Path, suffix Pattern, opts visi s = slices.Clone(s) for i, value := range s { var err error - nv, err := visit(value, prefix.Append(Index(i)), suffix, opts) + nv, err := visit(value, append(prefix, Index(i)), suffix, opts) if err != nil { // Leave the value intact if the suffix pattern didn't match any value. if IsNoSuchKeyError(err) || IsIndexOutOfBoundsError(err) { diff --git a/libs/dyn/pattern_test.go b/libs/dyn/pattern_test.go index b968f113..372fe746 100644 --- a/libs/dyn/pattern_test.go +++ b/libs/dyn/pattern_test.go @@ -28,13 +28,20 @@ func TestNewPatternFromPath(t *testing.T) { } func TestPatternAppend(t *testing.T) { - p1 := dyn.NewPattern(dyn.Key("foo"), dyn.Index(1)) - p2 := dyn.NewPattern(dyn.Key("foo")).Append(dyn.Index(1)) - assert.Equal(t, p1, p2) + p := dyn.NewPattern(dyn.Key("foo")) + + // Single arg. + p1 := p.Append(dyn.Key("bar")) + assert.Equal(t, dyn.NewPattern(dyn.Key("foo"), dyn.Key("bar")), p1) + + // Multiple args. + p2 := p.Append(dyn.Key("bar"), dyn.Index(1)) + assert.Equal(t, dyn.NewPattern(dyn.Key("foo"), dyn.Key("bar"), dyn.Index(1)), p2) } func TestPatternAppendAlwaysNew(t *testing.T) { - p := make(dyn.Pattern, 0, 2).Append(dyn.Key("foo")) + p := make(dyn.Pattern, 0, 2) + p = append(p, dyn.Key("foo")) // There is room for a second element in the slice. p1 := p.Append(dyn.Index(1)) diff --git a/libs/dyn/visit.go b/libs/dyn/visit.go index ffd8323d..376dcc22 100644 --- a/libs/dyn/visit.go +++ b/libs/dyn/visit.go @@ -66,7 +66,7 @@ func visit(v Value, prefix Path, suffix Pattern, opts visitOptions) (Value, erro } func (component pathComponent) visit(v Value, prefix Path, suffix Pattern, opts visitOptions) (Value, error) { - path := prefix.Append(component) + path := append(prefix, component) switch { case component.isKey(): diff --git a/libs/dyn/visit_map.go b/libs/dyn/visit_map.go index 05d17c73..18fc668e 100644 --- a/libs/dyn/visit_map.go +++ b/libs/dyn/visit_map.go @@ -18,7 +18,7 @@ func Foreach(fn MapFunc) MapFunc { m := maps.Clone(v.MustMap()) for key, value := range m { var err error - m[key], err = fn(p.Append(Key(key)), value) + m[key], err = fn(append(p, Key(key)), value) if err != nil { return InvalidValue, err } @@ -28,7 +28,7 @@ func Foreach(fn MapFunc) MapFunc { s := slices.Clone(v.MustSequence()) for i, value := range s { var err error - s[i], err = fn(p.Append(Index(i)), value) + s[i], err = fn(append(p, Index(i)), value) if err != nil { return InvalidValue, err } diff --git a/libs/dyn/visit_set.go b/libs/dyn/visit_set.go index b22c3da4..edcd9bb7 100644 --- a/libs/dyn/visit_set.go +++ b/libs/dyn/visit_set.go @@ -30,7 +30,7 @@ func SetByPath(v Value, p Path, nv Value) (Value, error) { return visit(v, EmptyPath, NewPatternFromPath(p), visitOptions{ fn: func(prefix Path, v Value) (Value, error) { - path := prefix.Append(component) + path := append(prefix, component) switch { case component.isKey(): diff --git a/libs/dyn/walk.go b/libs/dyn/walk.go index 138816be..26ddfc11 100644 --- a/libs/dyn/walk.go +++ b/libs/dyn/walk.go @@ -36,7 +36,7 @@ func walk(v Value, p Path, fn func(p Path, v Value) (Value, error)) (Value, erro m := v.MustMap() out := make(map[string]Value, len(m)) for k := range m { - nv, err := walk(m[k], p.Append(Key(k)), fn) + nv, err := walk(m[k], append(p, Key(k)), fn) if err == ErrDrop { continue } @@ -50,7 +50,7 @@ func walk(v Value, p Path, fn func(p Path, v Value) (Value, error)) (Value, erro s := v.MustSequence() out := make([]Value, 0, len(s)) for i := range s { - nv, err := walk(s[i], p.Append(Index(i)), fn) + nv, err := walk(s[i], append(p, Index(i)), fn) if err == ErrDrop { continue }