Return dyn.InvalidValue instead of dyn.NilValue when errors happen (

#1514)

## Changes
With #1507 and
#1511 we are clarifying the
semantics associated with `dyn.InvalidValue` and `dyn.NilValue`. An
invalid value is the default zero value and is used to signals the
complete absence of the value.

A nil value, on the other hand, is a valid value for a piece of
configuration and signals explicitly setting a key to nil in the
configuration tree. In keeping with that theme, this PR returns
`dyn.InvalidValue` instead of `dyn.NilValue` at error sites. This change
is not expected to have a material change in behaviour and is being done
to set the right convention since we have well-defined semantics
associated with both `NilValue` and `InvalidValue`.

## Tests
Unit tests and integration tests pass. Also manually scanned the changes
and the associated call sites to verify the `NilValue` value itself was
not being relied upon.

bundle/config/generate/job.go

@@ -17,7 +17,7 @@ func ConvertJobToValue(job *jobs.Job) (dyn.Value, error) {
 		for _, task := range job.Settings.Tasks {
 			v, err := convertTaskToValue(task, taskOrder)
 			if err != nil {
-				return dyn.NilValue, err
+				return dyn.InvalidValue, err
 			}
 			tasks = append(tasks, v)
 		}

bundle/config/mutator/rewrite_sync_paths.go

@@ -35,7 +35,7 @@ func (m *rewriteSyncPaths) makeRelativeTo(root string) dyn.MapFunc {
 		dir := filepath.Dir(v.Location().File)
 		rel, err := filepath.Rel(root, dir)
 		if err != nil {
-			return dyn.NilValue, err
+			return dyn.InvalidValue, err
 		}
 
 		return dyn.NewValue(filepath.Join(rel, v.MustString()), v.Location()), nil
@@ -47,11 +47,11 @@ func (m *rewriteSyncPaths) Apply(ctx context.Context, b *bundle.Bundle) diag.Dia
 		return dyn.Map(v, "sync", func(_ dyn.Path, v dyn.Value) (nv dyn.Value, err error) {
 			v, err = dyn.Map(v, "include", dyn.Foreach(m.makeRelativeTo(b.RootPath)))
 			if err != nil {
-				return dyn.NilValue, err
+				return dyn.InvalidValue, err
 			}
 			v, err = dyn.Map(v, "exclude", dyn.Foreach(m.makeRelativeTo(b.RootPath)))
 			if err != nil {
-				return dyn.NilValue, err
+				return dyn.InvalidValue, err
 			}
 			return v, nil
 		})

bundle/deploy/terraform/tfdyn/rename_keys.go

@@ -28,7 +28,7 @@ func renameKeys(v dyn.Value, rename map[string]string) (dyn.Value, error) {
 			p[0] = dyn.Key(newKey)
 			acc, err = dyn.SetByPath(acc, p, v)
 			if err != nil {
-				return dyn.NilValue, err
+				return dyn.InvalidValue, err
 			}
 			return dyn.InvalidValue, dyn.ErrDrop
 		}

libs/dyn/merge/merge.go

@@ -34,17 +34,17 @@ func merge(a, b dyn.Value) (dyn.Value, error) {
 	switch ak {
 	case dyn.KindMap:
 		if bk != dyn.KindMap {
-			return dyn.NilValue, fmt.Errorf("cannot merge map with %s", bk)
+			return dyn.InvalidValue, fmt.Errorf("cannot merge map with %s", bk)
 		}
 		return mergeMap(a, b)
 	case dyn.KindSequence:
 		if bk != dyn.KindSequence {
-			return dyn.NilValue, fmt.Errorf("cannot merge sequence with %s", bk)
+			return dyn.InvalidValue, fmt.Errorf("cannot merge sequence with %s", bk)
 		}
 		return mergeSequence(a, b)
 	default:
 		if ak != bk {
-			return dyn.NilValue, fmt.Errorf("cannot merge %s with %s", ak, bk)
+			return dyn.InvalidValue, fmt.Errorf("cannot merge %s with %s", ak, bk)
 		}
 		return mergePrimitive(a, b)
 	}
@@ -66,7 +66,7 @@ func mergeMap(a, b dyn.Value) (dyn.Value, error) {
 			// If the key already exists, merge the values.
 			merged, err := merge(ov, pv)
 			if err != nil {
-				return dyn.NilValue, err
+				return dyn.InvalidValue, err
 			}
 			out.Set(pk, merged)
 		} else {

libs/dyn/merge/merge_test.go

@@ -76,7 +76,7 @@ func TestMergeMapsError(t *testing.T) {
 	{
 		out, err := Merge(v, other)
 		assert.EqualError(t, err, "cannot merge map with string")
-		assert.Equal(t, dyn.NilValue, out)
+		assert.Equal(t, dyn.InvalidValue, out)
 	}
 }
 
@@ -151,7 +151,7 @@ func TestMergeSequencesError(t *testing.T) {
 	{
 		out, err := Merge(v, other)
 		assert.EqualError(t, err, "cannot merge sequence with string")
-		assert.Equal(t, dyn.NilValue, out)
+		assert.Equal(t, dyn.InvalidValue, out)
 	}
 }
 
@@ -202,6 +202,6 @@ func TestMergePrimitivesError(t *testing.T) {
 	{
 		out, err := Merge(v, other)
 		assert.EqualError(t, err, "cannot merge string with map")
-		assert.Equal(t, dyn.NilValue, out)
+		assert.Equal(t, dyn.InvalidValue, out)
 	}
 }

libs/dyn/yamlloader/loader.go

@@ -55,7 +55,7 @@ func (d *loader) load(node *yaml.Node) (dyn.Value, error) {
 	case yaml.AliasNode:
 		value, err = d.loadAlias(node, loc)
 	default:
-		return dyn.NilValue, errorf(loc, "unknown node kind: %v", node.Kind)
+		return dyn.InvalidValue, errorf(loc, "unknown node kind: %v", node.Kind)
 	}
 
 	if err != nil {
@@ -80,7 +80,7 @@ func (d *loader) loadSequence(node *yaml.Node, loc dyn.Location) (dyn.Value, err
 	for i, n := range node.Content {
 		v, err := d.load(n)
 		if err != nil {
-			return dyn.NilValue, err
+			return dyn.InvalidValue, err
 		}
 
 		acc[i] = v
@@ -99,7 +99,7 @@ func (d *loader) loadMapping(node *yaml.Node, loc dyn.Location) (dyn.Value, erro
 
 		// Assert that keys are strings
 		if key.Kind != yaml.ScalarNode {
-			return dyn.NilValue, errorf(loc, "key is not a scalar")
+			return dyn.InvalidValue, errorf(loc, "key is not a scalar")
 		}
 
 		st := key.ShortTag()
@@ -113,17 +113,17 @@ func (d *loader) loadMapping(node *yaml.Node, loc dyn.Location) (dyn.Value, erro
 			merge = val
 			continue
 		default:
-			return dyn.NilValue, errorf(loc, "invalid key tag: %v", st)
+			return dyn.InvalidValue, errorf(loc, "invalid key tag: %v", st)
 		}
 
 		k, err := d.load(key)
 		if err != nil {
-			return dyn.NilValue, err
+			return dyn.InvalidValue, err
 		}
 
 		v, err := d.load(val)
 		if err != nil {
-			return dyn.NilValue, err
+			return dyn.InvalidValue, err
 		}
 
 		acc.Set(k, v)
@@ -155,7 +155,7 @@ func (d *loader) loadMapping(node *yaml.Node, loc dyn.Location) (dyn.Value, erro
 	for _, n := range mnodes {
 		v, err := d.load(n)
 		if err != nil {
-			return dyn.NilValue, err
+			return dyn.InvalidValue, err
 		}
 		m, ok := v.AsMap()
 		if !ok {
@@ -186,12 +186,12 @@ func (d *loader) loadScalar(node *yaml.Node, loc dyn.Location) (dyn.Value, error
 		case "false":
 			return dyn.NewValue(false, loc), nil
 		default:
-			return dyn.NilValue, errorf(loc, "invalid bool value: %v", node.Value)
+			return dyn.InvalidValue, errorf(loc, "invalid bool value: %v", node.Value)
 		}
 	case "!!int":
 		i64, err := strconv.ParseInt(node.Value, 10, 64)
 		if err != nil {
-			return dyn.NilValue, errorf(loc, "invalid int value: %v", node.Value)
+			return dyn.InvalidValue, errorf(loc, "invalid int value: %v", node.Value)
 		}
 		// Use regular int type instead of int64 if possible.
 		if i64 >= math.MinInt32 && i64 <= math.MaxInt32 {
@@ -201,7 +201,7 @@ func (d *loader) loadScalar(node *yaml.Node, loc dyn.Location) (dyn.Value, error
 	case "!!float":
 		f64, err := strconv.ParseFloat(node.Value, 64)
 		if err != nil {
-			return dyn.NilValue, errorf(loc, "invalid float value: %v", node.Value)
+			return dyn.InvalidValue, errorf(loc, "invalid float value: %v", node.Value)
 		}
 		return dyn.NewValue(f64, loc), nil
 	case "!!null":
@@ -219,9 +219,9 @@ func (d *loader) loadScalar(node *yaml.Node, loc dyn.Location) (dyn.Value, error
 				return dyn.NewValue(t, loc), nil
 			}
 		}
-		return dyn.NilValue, errorf(loc, "invalid timestamp value: %v", node.Value)
+		return dyn.InvalidValue, errorf(loc, "invalid timestamp value: %v", node.Value)
 	default:
-		return dyn.NilValue, errorf(loc, "unknown tag: %v", st)
+		return dyn.InvalidValue, errorf(loc, "unknown tag: %v", st)
 	}
 }
 

libs/dyn/yamlloader/yaml.go

@@ -15,7 +15,7 @@ func LoadYAML(path string, r io.Reader) (dyn.Value, error) {
 		if err == io.EOF {
 			return dyn.NilValue, nil
 		}
-		return dyn.NilValue, err
+		return dyn.InvalidValue, err
 	}
 
 	return newLoader(path).load(&node)

libs/dyn/yamlsaver/utils.go

@@ -15,7 +15,7 @@ func ConvertToMapValue(strct any, order *Order, skipFields []string, dst map[str
 	ref := dyn.NilValue
 	mv, err := convert.FromTyped(strct, ref)
 	if err != nil {
-		return dyn.NilValue, err
+		return dyn.InvalidValue, err
 	}
 
 	if mv.Kind() != dyn.KindMap {