Use `dyn.InvalidValue` to indicate absence (#1507)

## Changes

Previously, the functions `Get` and `Index` returned `dyn.NilValue` to
indicate that a map key or sequence index wasn't found. This is a valid
value, so we need to differentiate between actual absence and a real
`dyn.NilValue`. We do this with the zero value of a `dyn.Value` (also
captured in the constant `dyn.InvalidValue`).

## Tests

* Unit tests.
* Renamed `Get` and `Index` to find and update all call sites.

bundle/config/mutator/environments_compat.go

@@ -32,18 +32,18 @@ func (m *environmentsToTargets) Apply(ctx context.Context, b *bundle.Bundle) dia
 		targets := v.Get("targets")
 
 		// Return an error if both "environments" and "targets" are set.
-		if environments != dyn.NilValue && targets != dyn.NilValue {
+		if environments != dyn.InvalidValue && targets != dyn.InvalidValue {
-			return dyn.NilValue, fmt.Errorf(
+			return dyn.InvalidValue, fmt.Errorf(
 				"both 'environments' and 'targets' are specified; only 'targets' should be used: %s",
 				environments.Location().String(),
 			)
 		}
 
 		// Rewrite "environments" to "targets".
-		if environments != dyn.NilValue && targets == dyn.NilValue {
+		if environments != dyn.InvalidValue && targets == dyn.InvalidValue {
 			nv, err := dyn.Set(v, "targets", environments)
 			if err != nil {
-				return dyn.NilValue, err
+				return dyn.InvalidValue, err
 			}
 			// Drop the "environments" key.
 			return dyn.Walk(nv, func(p dyn.Path, v dyn.Value) (dyn.Value, error) {

bundle/config/mutator/merge_job_clusters.go

@@ -21,7 +21,7 @@ func (m *mergeJobClusters) Name() string {
 
 func (m *mergeJobClusters) jobClusterKey(v dyn.Value) string {
 	switch v.Kind() {
-	case dyn.KindNil:
+	case dyn.KindInvalid, dyn.KindNil:
 		return ""
 	case dyn.KindString:
 		return v.MustString()

bundle/config/mutator/merge_job_tasks.go

@@ -21,7 +21,7 @@ func (m *mergeJobTasks) Name() string {
 
 func (m *mergeJobTasks) taskKeyString(v dyn.Value) string {
 	switch v.Kind() {
-	case dyn.KindNil:
+	case dyn.KindInvalid, dyn.KindNil:
 		return ""
 	case dyn.KindString:
 		return v.MustString()

bundle/config/mutator/merge_pipeline_clusters.go

@@ -22,7 +22,7 @@ func (m *mergePipelineClusters) Name() string {
 
 func (m *mergePipelineClusters) clusterLabel(v dyn.Value) string {
 	switch v.Kind() {
-	case dyn.KindNil:
+	case dyn.KindInvalid, dyn.KindNil:
 		// Note: the cluster label is optional and defaults to 'default'.
 		// We therefore ALSO merge all clusters without a label.
 		return "default"

bundle/config/root.go

@@ -337,7 +337,7 @@ func (r *Root) MergeTargetOverrides(name string) error {
 	}
 
 	// Merge `run_as`. This field must be overwritten if set, not merged.
-	if v := target.Get("run_as"); v != dyn.NilValue {
+	if v := target.Get("run_as"); v != dyn.InvalidValue {
 		root, err = dyn.Set(root, "run_as", v)
 		if err != nil {
 			return err
@@ -345,7 +345,7 @@ func (r *Root) MergeTargetOverrides(name string) error {
 	}
 
 	// Below, we're setting fields on the bundle key, so make sure it exists.
-	if root.Get("bundle") == dyn.NilValue {
+	if root.Get("bundle") == dyn.InvalidValue {
 		root, err = dyn.Set(root, "bundle", dyn.NewValue(map[string]dyn.Value{}, dyn.Location{}))
 		if err != nil {
 			return err
@@ -353,7 +353,7 @@ func (r *Root) MergeTargetOverrides(name string) error {
 	}
 
 	// Merge `mode`. This field must be overwritten if set, not merged.
-	if v := target.Get("mode"); v != dyn.NilValue {
+	if v := target.Get("mode"); v != dyn.InvalidValue {
 		root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("mode")), v)
 		if err != nil {
 			return err
@@ -361,7 +361,7 @@ func (r *Root) MergeTargetOverrides(name string) error {
 	}
 
 	// Merge `compute_id`. This field must be overwritten if set, not merged.
-	if v := target.Get("compute_id"); v != dyn.NilValue {
+	if v := target.Get("compute_id"); v != dyn.InvalidValue {
 		root, err = dyn.SetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("compute_id")), v)
 		if err != nil {
 			return err
@@ -369,7 +369,7 @@ func (r *Root) MergeTargetOverrides(name string) error {
 	}
 
 	// Merge `git`.
-	if v := target.Get("git"); v != dyn.NilValue {
+	if v := target.Get("git"); v != dyn.InvalidValue {
 		ref, err := dyn.GetByPath(root, dyn.NewPath(dyn.Key("bundle"), dyn.Key("git")))
 		if err != nil {
 			ref = dyn.NewValue(map[string]dyn.Value{}, dyn.Location{})
@@ -382,7 +382,7 @@ func (r *Root) MergeTargetOverrides(name string) error {
 		}
 
 		// If the branch was overridden, we need to clear the inferred flag.
-		if branch := v.Get("branch"); branch != dyn.NilValue {
+		if branch := v.Get("branch"); branch != dyn.InvalidValue {
 			out, err = dyn.SetByPath(out, dyn.NewPath(dyn.Key("inferred")), dyn.NewValue(false, dyn.Location{}))
 			if err != nil {
 				return err
@@ -410,7 +410,7 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) {
 	// For each target, rewrite the variables block.
 	return dyn.Map(v, "targets", dyn.Foreach(func(_ dyn.Path, target dyn.Value) (dyn.Value, error) {
 		// Confirm it has a variables block.
-		if target.Get("variables") == dyn.NilValue {
+		if target.Get("variables") == dyn.InvalidValue {
 			return target, nil
 		}
 
@@ -440,15 +440,19 @@ func validateVariableOverrides(root, target dyn.Value) (err error) {
 	var tv map[string]variable.Variable
 
 	// Collect variables from the root.
-	err = convert.ToTyped(&rv, root.Get("variables"))
+	if v := root.Get("variables"); v != dyn.InvalidValue {
-	if err != nil {
+		err = convert.ToTyped(&rv, v)
-		return fmt.Errorf("unable to collect variables from root: %w", err)
+		if err != nil {
+			return fmt.Errorf("unable to collect variables from root: %w", err)
+		}
 	}
 
 	// Collect variables from the target.
-	err = convert.ToTyped(&tv, target.Get("variables"))
+	if v := target.Get("variables"); v != dyn.InvalidValue {
-	if err != nil {
+		err = convert.ToTyped(&tv, v)
-		return fmt.Errorf("unable to collect variables from target: %w", err)
+		if err != nil {
+			return fmt.Errorf("unable to collect variables from target: %w", err)
+		}
 	}
 
 	// Check that all variables in the target exist in the root.

libs/dyn/convert/from_typed.go

@@ -172,9 +172,15 @@ func fromTypedSlice(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
 	out := make([]dyn.Value, src.Len())
 	for i := 0; i < src.Len(); i++ {
 		v := src.Index(i)
+		refv := ref.Index(i)
+
+		// Use nil reference if there is no reference for this index.
+		if refv == dyn.InvalidValue {
+			refv = dyn.NilValue
+		}
 
 		// Convert entry taking into account the reference value (may be equal to dyn.NilValue).
-		nv, err := fromTyped(v.Interface(), ref.Index(i), includeZeroValuedScalars)
+		nv, err := fromTyped(v.Interface(), refv, includeZeroValuedScalars)
 		if err != nil {
 			return dyn.InvalidValue, err
 		}

libs/dyn/value.go

@@ -110,12 +110,12 @@ func (v Value) AsAny() any {
 func (v Value) Get(key string) Value {
 	m, ok := v.AsMap()
 	if !ok {
-		return NilValue
+		return InvalidValue
 	}
 
 	vv, ok := m.GetByString(key)
 	if !ok {
-		return NilValue
+		return InvalidValue
 	}
 
 	return vv
@@ -124,11 +124,11 @@ func (v Value) Get(key string) Value {
 func (v Value) Index(i int) Value {
 	s, ok := v.v.([]Value)
 	if !ok {
-		return NilValue
+		return InvalidValue
 	}
 
 	if i < 0 || i >= len(s) {
-		return NilValue
+		return InvalidValue
 	}
 
 	return s[i]

libs/dyn/value_underlying_test.go

@@ -18,15 +18,15 @@ func TestValueUnderlyingMap(t *testing.T) {
 	vv1, ok := v.AsMap()
 	assert.True(t, ok)
 
-	_, ok = dyn.NilValue.AsMap()
+	_, ok = dyn.InvalidValue.AsMap()
 	assert.False(t, ok)
 
 	vv2 := v.MustMap()
 	assert.Equal(t, vv1, vv2)
 
 	// Test panic.
-	assert.PanicsWithValue(t, "expected kind map, got nil", func() {
+	assert.PanicsWithValue(t, "expected kind map, got invalid", func() {
-		dyn.NilValue.MustMap()
+		dyn.InvalidValue.MustMap()
 	})
 }
 
@@ -40,15 +40,15 @@ func TestValueUnderlyingSequence(t *testing.T) {
 	vv1, ok := v.AsSequence()
 	assert.True(t, ok)
 
-	_, ok = dyn.NilValue.AsSequence()
+	_, ok = dyn.InvalidValue.AsSequence()
 	assert.False(t, ok)
 
 	vv2 := v.MustSequence()
 	assert.Equal(t, vv1, vv2)
 
 	// Test panic.
-	assert.PanicsWithValue(t, "expected kind sequence, got nil", func() {
+	assert.PanicsWithValue(t, "expected kind sequence, got invalid", func() {
-		dyn.NilValue.MustSequence()
+		dyn.InvalidValue.MustSequence()
 	})
 }
 
@@ -58,15 +58,15 @@ func TestValueUnderlyingString(t *testing.T) {
 	vv1, ok := v.AsString()
 	assert.True(t, ok)
 
-	_, ok = dyn.NilValue.AsString()
+	_, ok = dyn.InvalidValue.AsString()
 	assert.False(t, ok)
 
 	vv2 := v.MustString()
 	assert.Equal(t, vv1, vv2)
 
 	// Test panic.
-	assert.PanicsWithValue(t, "expected kind string, got nil", func() {
+	assert.PanicsWithValue(t, "expected kind string, got invalid", func() {
-		dyn.NilValue.MustString()
+		dyn.InvalidValue.MustString()
 	})
 }
 
@@ -76,15 +76,15 @@ func TestValueUnderlyingBool(t *testing.T) {
 	vv1, ok := v.AsBool()
 	assert.True(t, ok)
 
-	_, ok = dyn.NilValue.AsBool()
+	_, ok = dyn.InvalidValue.AsBool()
 	assert.False(t, ok)
 
 	vv2 := v.MustBool()
 	assert.Equal(t, vv1, vv2)
 
 	// Test panic.
-	assert.PanicsWithValue(t, "expected kind bool, got nil", func() {
+	assert.PanicsWithValue(t, "expected kind bool, got invalid", func() {
-		dyn.NilValue.MustBool()
+		dyn.InvalidValue.MustBool()
 	})
 }
 
@@ -94,15 +94,15 @@ func TestValueUnderlyingInt(t *testing.T) {
 	vv1, ok := v.AsInt()
 	assert.True(t, ok)
 
-	_, ok = dyn.NilValue.AsInt()
+	_, ok = dyn.InvalidValue.AsInt()
 	assert.False(t, ok)
 
 	vv2 := v.MustInt()
 	assert.Equal(t, vv1, vv2)
 
 	// Test panic.
-	assert.PanicsWithValue(t, "expected kind int, got nil", func() {
+	assert.PanicsWithValue(t, "expected kind int, got invalid", func() {
-		dyn.NilValue.MustInt()
+		dyn.InvalidValue.MustInt()
 	})
 
 	// Test int32 type specifically.
@@ -124,15 +124,15 @@ func TestValueUnderlyingFloat(t *testing.T) {
 	vv1, ok := v.AsFloat()
 	assert.True(t, ok)
 
-	_, ok = dyn.NilValue.AsFloat()
+	_, ok = dyn.InvalidValue.AsFloat()
 	assert.False(t, ok)
 
 	vv2 := v.MustFloat()
 	assert.Equal(t, vv1, vv2)
 
 	// Test panic.
-	assert.PanicsWithValue(t, "expected kind float, got nil", func() {
+	assert.PanicsWithValue(t, "expected kind float, got invalid", func() {
-		dyn.NilValue.MustFloat()
+		dyn.InvalidValue.MustFloat()
 	})
 
 	// Test float64 type specifically.
@@ -148,14 +148,14 @@ func TestValueUnderlyingTime(t *testing.T) {
 	vv1, ok := v.AsTime()
 	assert.True(t, ok)
 
-	_, ok = dyn.NilValue.AsTime()
+	_, ok = dyn.InvalidValue.AsTime()
 	assert.False(t, ok)
 
 	vv2 := v.MustTime()
 	assert.Equal(t, vv1, vv2)
 
 	// Test panic.
-	assert.PanicsWithValue(t, "expected kind time, got nil", func() {
+	assert.PanicsWithValue(t, "expected kind time, got invalid", func() {
-		dyn.NilValue.MustTime()
+		dyn.InvalidValue.MustTime()
 	})
 }

libs/dyn/walk.go

@@ -28,7 +28,7 @@ func walk(v Value, p Path, fn func(p Path, v Value) (Value, error)) (Value, erro
 		if err == ErrSkip {
 			return v, nil
 		}
-		return NilValue, err
+		return InvalidValue, err
 	}
 
 	switch v.Kind() {
@@ -43,7 +43,7 @@ func walk(v Value, p Path, fn func(p Path, v Value) (Value, error)) (Value, erro
 				continue
 			}
 			if err != nil {
-				return NilValue, err
+				return InvalidValue, err
 			}
 			out.Set(pk, nv)
 		}
@@ -57,7 +57,7 @@ func walk(v Value, p Path, fn func(p Path, v Value) (Value, error)) (Value, erro
 				continue
 			}
 			if err != nil {
-				return NilValue, err
+				return InvalidValue, err
 			}
 			out = append(out, nv)
 		}

libs/dyn/walk_test.go

@@ -39,7 +39,7 @@ func (w *walkCallTracker) returnSkip(path string) {
 }
 
 func (w *walkCallTracker) returnDrop(path string) {
-	w.on(path, func(v Value) Value { return NilValue }, ErrDrop)
+	w.on(path, func(v Value) Value { return InvalidValue }, ErrDrop)
 }
 
 func (w *walkCallTracker) track(p Path, v Value) (Value, error) {
@@ -148,7 +148,7 @@ func TestWalkMapError(t *testing.T) {
 	})
 	out, err := Walk(value, tracker.track)
 	assert.Equal(t, cerr, err)
-	assert.Equal(t, NilValue, out)
+	assert.Equal(t, InvalidValue, out)
 
 	// The callback should have been called twice.
 	assert.Len(t, tracker.calls, 2)
@@ -239,7 +239,7 @@ func TestWalkSequenceError(t *testing.T) {
 	})
 	out, err := Walk(value, tracker.track)
 	assert.Equal(t, cerr, err)
-	assert.Equal(t, NilValue, out)
+	assert.Equal(t, InvalidValue, out)
 
 	// The callback should have been called three times.
 	assert.Len(t, tracker.calls, 3)