Retain location metadata for values in `convert.FromTyped` (#1523)

## Changes

There are four different treatments location metadata can receive in the
`convert.FromTyped` method.

1. Location metadata is **retained** for maps, structs and slices if the
value is **not nil**
2. Location metadata is **lost** for maps, structs and slices if the
value is **is nil**
3. Location metadata is **retained** if a scalar type (eg. bool, string
etc) does not change.
4. Location metadata is **lost** if the value for a scalar type changes.

This PR ensures that location metadata is not lost in any case; that is,
it's always preserved.

For (2), this serves as a bug fix so that location information is not
lost on conversion to and from typed for nil values of complex types
(struct, slices, and maps).

For (4) this is a change in semantics. For primitive values modified in
a `typed` mutator, any references to `.Location()` for computed
primitive fields will now return associated YAML location metadata (if
any) instead of an empty location.

While arguable, these semantics are OK since:
1. Situations like these will be rare.
2. Knowing the YAML location (if any) is better than not knowing the
location at all. These locations are typically visible to the user in
errors and warnings.

## Tests

Unit tests

libs/dyn/convert/from_typed.go

@@ -42,7 +42,7 @@ func fromTyped(src any, ref dyn.Value, options ...fromTypedOptions) (dyn.Value,
 	// Dereference pointer if necessary
 	for srcv.Kind() == reflect.Pointer {
 		if srcv.IsNil() {
-			return dyn.NilValue, nil
+			return dyn.NilValue.WithLocation(ref.Location()), nil
 		}
 		srcv = srcv.Elem()
 
@@ -55,27 +55,35 @@ func fromTyped(src any, ref dyn.Value, options ...fromTypedOptions) (dyn.Value,
 		}
 	}
 
+	var v dyn.Value
+	var err error
 	switch srcv.Kind() {
 	case reflect.Struct:
-		return fromTypedStruct(srcv, ref, options...)
+		v, err = fromTypedStruct(srcv, ref, options...)
 	case reflect.Map:
-		return fromTypedMap(srcv, ref)
+		v, err = fromTypedMap(srcv, ref)
 	case reflect.Slice:
-		return fromTypedSlice(srcv, ref)
+		v, err = fromTypedSlice(srcv, ref)
 	case reflect.String:
-		return fromTypedString(srcv, ref, options...)
+		v, err = fromTypedString(srcv, ref, options...)
 	case reflect.Bool:
-		return fromTypedBool(srcv, ref, options...)
+		v, err = fromTypedBool(srcv, ref, options...)
 	case reflect.Int, reflect.Int32, reflect.Int64:
-		return fromTypedInt(srcv, ref, options...)
+		v, err = fromTypedInt(srcv, ref, options...)
 	case reflect.Float32, reflect.Float64:
-		return fromTypedFloat(srcv, ref, options...)
+		v, err = fromTypedFloat(srcv, ref, options...)
 	case reflect.Invalid:
 		// If the value is untyped and not set (e.g. any type with nil value), we return nil.
-		return dyn.NilValue, nil
+		v, err = dyn.NilValue, nil
+	default:
+		return dyn.InvalidValue, fmt.Errorf("unsupported type: %s", srcv.Kind())
 	}
 
-	return dyn.InvalidValue, fmt.Errorf("unsupported type: %s", srcv.Kind())
+	// Ensure the location metadata is retained.
+	if err != nil {
+		return dyn.InvalidValue, err
+	}
+	return v.WithLocation(ref.Location()), err
 }
 
 func fromTypedStruct(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) {
@@ -117,7 +125,7 @@ func fromTypedStruct(src reflect.Value, ref dyn.Value, options ...fromTypedOptio
 	// 2. The reference is a map (i.e. the struct was and still is empty).
 	// 3. The "includeZeroValues" option is set (i.e. the struct is a non-nil pointer).
 	if out.Len() > 0 || ref.Kind() == dyn.KindMap || slices.Contains(options, includeZeroValues) {
-		return dyn.NewValue(out, ref.Location()), nil
+		return dyn.V(out), nil
 	}
 
 	// Otherwise, return nil.
@@ -164,7 +172,7 @@ func fromTypedMap(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
 		out.Set(refk, nv)
 	}
 
-	return dyn.NewValue(out, ref.Location()), nil
+	return dyn.V(out), nil
 }
 
 func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
@@ -199,7 +207,7 @@ func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
 		out[i] = nv
 	}
 
-	return dyn.NewValue(out, ref.Location()), nil
+	return dyn.V(out), nil
 }
 
 func fromTypedString(src reflect.Value, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) {

libs/dyn/convert/from_typed_test.go

@@ -49,7 +49,7 @@ func TestFromTypedStructPointerZeroFields(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, dyn.NilValue, nv)
 
-	// For an initialized pointer with a nil reference we expect a nil.
+	// For an initialized pointer with a nil reference we expect an empty map.
 	src = &Tmp{}
 	nv, err = FromTyped(src, dyn.NilValue)
 	require.NoError(t, err)
@@ -103,7 +103,7 @@ func TestFromTypedStructSetFields(t *testing.T) {
 	}), nv)
 }
 
-func TestFromTypedStructSetFieldsRetainLocationIfUnchanged(t *testing.T) {
+func TestFromTypedStructSetFieldsRetainLocation(t *testing.T) {
 	type Tmp struct {
 		Foo string `json:"foo"`
 		Bar string `json:"bar"`
@@ -122,11 +122,9 @@ func TestFromTypedStructSetFieldsRetainLocationIfUnchanged(t *testing.T) {
 	nv, err := FromTyped(src, ref)
 	require.NoError(t, err)
 
-	// Assert foo has retained its location.
+	// Assert foo and bar have retained their location.
 	assert.Equal(t, dyn.NewValue("bar", dyn.Location{File: "foo"}), nv.Get("foo"))
-
+	assert.Equal(t, dyn.NewValue("qux", dyn.Location{File: "bar"}), nv.Get("bar"))
-	// Assert bar lost its location (because it was overwritten).
-	assert.Equal(t, dyn.NewValue("qux", dyn.Location{}), nv.Get("bar"))
 }
 
 func TestFromTypedStringMapWithZeroValue(t *testing.T) {
@@ -354,7 +352,7 @@ func TestFromTypedMapNonEmpty(t *testing.T) {
 	}), nv)
 }
 
-func TestFromTypedMapNonEmptyRetainLocationIfUnchanged(t *testing.T) {
+func TestFromTypedMapNonEmptyRetainLocation(t *testing.T) {
 	var src = map[string]string{
 		"foo": "bar",
 		"bar": "qux",
@@ -368,11 +366,9 @@ func TestFromTypedMapNonEmptyRetainLocationIfUnchanged(t *testing.T) {
 	nv, err := FromTyped(src, ref)
 	require.NoError(t, err)
 
-	// Assert foo has retained its location.
+	// Assert foo and bar have retained their locations.
 	assert.Equal(t, dyn.NewValue("bar", dyn.Location{File: "foo"}), nv.Get("foo"))
-
+	assert.Equal(t, dyn.NewValue("qux", dyn.Location{File: "bar"}), nv.Get("bar"))
-	// Assert bar lost its location (because it was overwritten).
-	assert.Equal(t, dyn.NewValue("qux", dyn.Location{}), nv.Get("bar"))
 }
 
 func TestFromTypedMapFieldWithZeroValue(t *testing.T) {
@@ -429,7 +425,7 @@ func TestFromTypedSliceNonEmpty(t *testing.T) {
 	}), nv)
 }
 
-func TestFromTypedSliceNonEmptyRetainLocationIfUnchanged(t *testing.T) {
+func TestFromTypedSliceNonEmptyRetainLocation(t *testing.T) {
 	var src = []string{
 		"foo",
 		"bar",
@@ -437,17 +433,15 @@ func TestFromTypedSliceNonEmptyRetainLocationIfUnchanged(t *testing.T) {
 
 	ref := dyn.V([]dyn.Value{
 		dyn.NewValue("foo", dyn.Location{File: "foo"}),
-		dyn.NewValue("baz", dyn.Location{File: "baz"}),
+		dyn.NewValue("bar", dyn.Location{File: "bar"}),
 	})
 
 	nv, err := FromTyped(src, ref)
 	require.NoError(t, err)
 
-	// Assert foo has retained its location.
+	// Assert foo and bar have retained their locations.
 	assert.Equal(t, dyn.NewValue("foo", dyn.Location{File: "foo"}), nv.Index(0))
-
+	assert.Equal(t, dyn.NewValue("bar", dyn.Location{File: "bar"}), nv.Index(1))
-	// Assert bar lost its location (because it was overwritten).
-	assert.Equal(t, dyn.NewValue("bar", dyn.Location{}), nv.Index(1))
 }
 
 func TestFromTypedStringEmpty(t *testing.T) {
@@ -482,12 +476,20 @@ func TestFromTypedStringNonEmptyOverwrite(t *testing.T) {
 	assert.Equal(t, dyn.V("new"), nv)
 }
 
-func TestFromTypedStringRetainsLocationsIfUnchanged(t *testing.T) {
+func TestFromTypedStringRetainsLocations(t *testing.T) {
-	var src string = "foo"
 	var ref = dyn.NewValue("foo", dyn.Location{File: "foo"})
+
+	// case: value has not been changed
+	var src string = "foo"
 	nv, err := FromTyped(src, ref)
 	require.NoError(t, err)
 	assert.Equal(t, dyn.NewValue("foo", dyn.Location{File: "foo"}), nv)
+
+	// case: value has been changed
+	src = "bar"
+	nv, err = FromTyped(src, ref)
+	require.NoError(t, err)
+	assert.Equal(t, dyn.NewValue("bar", dyn.Location{File: "foo"}), nv)
 }
 
 func TestFromTypedStringTypeError(t *testing.T) {
@@ -529,12 +531,20 @@ func TestFromTypedBoolNonEmptyOverwrite(t *testing.T) {
 	assert.Equal(t, dyn.V(true), nv)
 }
 
-func TestFromTypedBoolRetainsLocationsIfUnchanged(t *testing.T) {
+func TestFromTypedBoolRetainsLocations(t *testing.T) {
-	var src bool = true
 	var ref = dyn.NewValue(true, dyn.Location{File: "foo"})
+
+	// case: value has not been changed
+	var src bool = true
 	nv, err := FromTyped(src, ref)
 	require.NoError(t, err)
 	assert.Equal(t, dyn.NewValue(true, dyn.Location{File: "foo"}), nv)
+
+	// case: value has been changed
+	src = false
+	nv, err = FromTyped(src, ref)
+	require.NoError(t, err)
+	assert.Equal(t, dyn.NewValue(false, dyn.Location{File: "foo"}), nv)
 }
 
 func TestFromTypedBoolVariableReference(t *testing.T) {
@@ -584,12 +594,20 @@ func TestFromTypedIntNonEmptyOverwrite(t *testing.T) {
 	assert.Equal(t, dyn.V(int64(1234)), nv)
 }
 
-func TestFromTypedIntRetainsLocationsIfUnchanged(t *testing.T) {
+func TestFromTypedIntRetainsLocations(t *testing.T) {
-	var src int = 1234
 	var ref = dyn.NewValue(1234, dyn.Location{File: "foo"})
+
+	// case: value has not been changed
+	var src int = 1234
 	nv, err := FromTyped(src, ref)
 	require.NoError(t, err)
 	assert.Equal(t, dyn.NewValue(1234, dyn.Location{File: "foo"}), nv)
+
+	// case: value has been changed
+	src = 1235
+	nv, err = FromTyped(src, ref)
+	require.NoError(t, err)
+	assert.Equal(t, dyn.NewValue(int64(1235), dyn.Location{File: "foo"}), nv)
 }
 
 func TestFromTypedIntVariableReference(t *testing.T) {
@@ -639,12 +657,21 @@ func TestFromTypedFloatNonEmptyOverwrite(t *testing.T) {
 	assert.Equal(t, dyn.V(1.23), nv)
 }
 
-func TestFromTypedFloatRetainsLocationsIfUnchanged(t *testing.T) {
+func TestFromTypedFloatRetainsLocations(t *testing.T) {
-	var src float64 = 1.23
+	var src float64
 	var ref = dyn.NewValue(1.23, dyn.Location{File: "foo"})
+
+	// case: value has not been changed
+	src = 1.23
 	nv, err := FromTyped(src, ref)
 	require.NoError(t, err)
 	assert.Equal(t, dyn.NewValue(1.23, dyn.Location{File: "foo"}), nv)
+
+	// case: value has been changed
+	src = 1.24
+	nv, err = FromTyped(src, ref)
+	require.NoError(t, err)
+	assert.Equal(t, dyn.NewValue(1.24, dyn.Location{File: "foo"}), nv)
 }
 
 func TestFromTypedFloatVariableReference(t *testing.T) {
@@ -669,3 +696,35 @@ func TestFromTypedAnyNil(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, dyn.NilValue, nv)
 }
+
+func TestFromTypedNilPointerRetainsLocations(t *testing.T) {
+	type Tmp struct {
+		Foo string `json:"foo"`
+		Bar string `json:"bar"`
+	}
+
+	var src *Tmp
+	ref := dyn.NewValue(nil, dyn.Location{File: "foobar"})
+
+	nv, err := FromTyped(src, ref)
+	require.NoError(t, err)
+	assert.Equal(t, dyn.NewValue(nil, dyn.Location{File: "foobar"}), nv)
+}
+
+func TestFromTypedNilMapRetainsLocation(t *testing.T) {
+	var src map[string]string
+	ref := dyn.NewValue(nil, dyn.Location{File: "foobar"})
+
+	nv, err := FromTyped(src, ref)
+	require.NoError(t, err)
+	assert.Equal(t, dyn.NewValue(nil, dyn.Location{File: "foobar"}), nv)
+}
+
+func TestFromTypedNilSliceRetainsLocation(t *testing.T) {
+	var src []string
+	ref := dyn.NewValue(nil, dyn.Location{File: "foobar"})
+
+	nv, err := FromTyped(src, ref)
+	require.NoError(t, err)
+	assert.Equal(t, dyn.NewValue(nil, dyn.Location{File: "foobar"}), nv)
+}