From 83c5ab318b62223b316d28318b9e4ddec32c1af2 Mon Sep 17 00:00:00 2001 From: Joe Groocock Date: Tue, 4 May 2021 13:09:32 +0100 Subject: [PATCH] lang: types: Clear map/list types during Into() Map and list types are now unconditionally initialised during an Into() call to ensure that the only data within them after the operation is that added by the Into() function. Prior to this change, map/list types would likely not be cleared prior to the data being inserted into them with a few exceptions. Nil pointers or maps/lists that were not sufficient in capacity would be reinitialised and used to replace the existing backing data store. In some cases this wouldn't occur meaning any residual data existing in the container before the Into() call could persist after the data copy completes. This behaviour is wildly inconsistent and not ideal in the vast majority of cases. It should be assumed that the Into() call will preserve nothing and always produce a consistent and deterministic output. Signed-off-by: Joe Groocock --- lang/types/value.go | 28 ++++++++++++----- lang/types/value_test.go | 68 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 86 insertions(+), 10 deletions(-) diff --git a/lang/types/value.go b/lang/types/value.go index 4cdf888b..f428d164 100644 --- a/lang/types/value.go +++ b/lang/types/value.go @@ -238,6 +238,12 @@ func ValueOf(v reflect.Value) (Value, error) { } // Into mutates the given reflect.Value with the data represented by the Value. +// +// Container types like map/list (and to a certain extent structs) will be +// cleared before adding the contained data such that the existing data doesn't +// affect the outcome, and the output reflect.Value directly maps to the input +// Value. +// // In almost every case, it is likely that the reflect.Value will be modified, // instantiating nil pointers and even potentially partially filling data before // returning an error. It should be assumed that if this returns an error, the @@ -323,16 +329,24 @@ func Into(v Value, rv reflect.Value) error { return nil case *ListValue: - if err := mustInto(reflect.Slice, reflect.Array); err != nil { - return err - } - count := len(v.V) - if count > rv.Cap() { + + switch kind { + case reflect.Slice: pow := nextPowerOfTwo(uint32(count)) nval := reflect.MakeSlice(rv.Type(), count, int(pow)) rv.Set(nval) + + case reflect.Array: + if count > rv.Len() { + return fmt.Errorf("%+v is too small for %+v", typ, v) + } + rv.Set(reflect.New(typ).Elem()) + + default: + return mustInto() // nothing, always returns err } + for i, x := range v.V { f := rv.Index(i) el := reflect.New(f.Type()).Elem() @@ -348,9 +362,7 @@ func Into(v Value, rv reflect.Value) error { return err } - if rv.IsNil() { - rv.Set(reflect.MakeMapWithSize(typ, len(v.V))) - } + rv.Set(reflect.MakeMapWithSize(typ, len(v.V))) // convert both key and value, then set them in the map for mk, mv := range v.V { diff --git a/lang/types/value_test.go b/lang/types/value_test.go index 2a2a59f2..d793324d 100644 --- a/lang/types/value_test.go +++ b/lang/types/value_test.go @@ -781,7 +781,7 @@ func TestValueInto0(t *testing.T) { var l []string var ll [][]string var lptrlptr []*[]*string - var arr [10]string + var arr [3]string var m map[string]int @@ -860,7 +860,7 @@ func TestValueInto0(t *testing.T) { { // arrays are pretty much the same as slices container: &arr, value: mustValue([]string{"1", "2", "3"}), - compare: [10]string{"1", "2", "3"}, + compare: [3]string{"1", "2", "3"}, }, { container: &ll, @@ -894,6 +894,11 @@ func TestValueInto0(t *testing.T) { value: mustValue(map[int]int{1: 2, 3: 4}), shouldErr: true, }, + { // [4]string into [3]string + container: &arr, + value: mustValue([4]string{"1", "2", "3", "4"}), + shouldErr: true, + }, // Pointer and pointer-to-pointer tests { @@ -1008,6 +1013,65 @@ func TestValueInto0(t *testing.T) { }) } } + +func TestValueInto1(t *testing.T) { + testCases := []struct { + container interface{} + compare interface{} + data Value + }{ + { + container: &[]string{"", "", "three"}, + compare: []string{"one", "two"}, + data: &ListValue{ + T: NewType("[]str"), + V: []Value{&StrValue{V: "one"}, &StrValue{V: "two"}}, + }, + }, + { + container: &[3]string{"", "", "three"}, + compare: [3]string{"one", "two", ""}, + data: &ListValue{ + T: NewType("[]str"), + V: []Value{&StrValue{V: "one"}, &StrValue{V: "two"}}, + }, + }, + { + container: &map[string]string{"3": "three"}, + compare: map[string]string{"1": "one", "2": "two"}, + data: &MapValue{ + T: NewType("map{str: str}"), + V: map[Value]Value{ + &StrValue{V: "1"}: &StrValue{V: "one"}, + &StrValue{V: "2"}: &StrValue{V: "two"}, + }, + }, + }, + } + + for index, tc := range testCases { + name := fmt.Sprintf("test Into() %s #%d", reflect.TypeOf(tc.container).Elem(), index) + + tc := tc + t.Run(name, func(t *testing.T) { + ctrVal := reflect.ValueOf(tc.container) + + // ensure Into() clears existing elements out of the container + if err := Into(tc.data, ctrVal); err != nil { + t.Errorf("func Into(%+v, %+v) failed: %s", tc.data, tc.container, err) + return + } + + // follow the container pointer: (*tc.container).(interface{}) + ctrPtr := ctrVal.Elem().Interface() + if !reflect.DeepEqual(ctrPtr, tc.compare) { + t.Errorf("func Into(%+v, %+v) did not clear existing values from the list", tc.data, ctrPtr) + return + } + }) + } +} + func TestValueIntoStructNameMapping(t *testing.T) { st := NewStruct(NewType("struct{word str; magic int}")) if err := st.Set("word", &StrValue{V: "zing"}); err != nil {