command/jsonplan: Don't panic with mixtures of known/unknown/empty

The omitUnknowns and unknownAsBool functions were previously trying hard
to preserve the same collection types in the output as they had in the
input, by attempting to keep everything matched up so that the results
would be valid.

Unfortunately, this turns out to be a harder problem than we originally
thought: it was possible for a collection value going in to produce
inconsistent element types out (and thus a panic) in the following
situations:
- when a collection with mixed known and unknown values was passed in
  to omitUnknowns.
- when a collection of collections where the inner collections are a
  mixture of empty and not empty in unknownAsNull.

The results of these functions are only used to marshal to JSON anyway,
and JSON serialization can't distinguish between the three sequence types
or the two mapping types, so in practice we can just standardize on
converting all sequences to tuple and all mappings to object here and not
change the resulting output at all, and then we don't have to worry about
making sure all of the inner types get preserved exactly.

A nice consequence of that relaxation is that we can now do what we
originally wanted to do with unknownAsBool, and omit map keys and
object attributes altogether if their values would've been false,
producing a much more compact result. This is easiest to do now when
there's only one known user of this JSON plan output, and we know that
user will treat both false and omitted as the same here.

command/jsonplan/plan.go

@@ -351,22 +351,21 @@ func (p *plan) marshalPlannedValues(changes *plans.Changes, schemas *terraform.S
 
 // omitUnknowns recursively walks the src cty.Value and returns a new cty.Value,
 // omitting any unknowns.
+//
+// The result also normalizes some types: all sequence types are turned into
+// tuple types and all mapping types are converted to object types, since we
+// assume the result of this is just going to be serialized as JSON (and thus
+// lose those distinctions) anyway.
 func omitUnknowns(val cty.Value) cty.Value {
-	if val.IsWhollyKnown() {
-		return val
-	}
-
 	ty := val.Type()
 	switch {
 	case val.IsNull():
 		return val
 	case !val.IsKnown():
 		return cty.NilVal
+	case ty.IsPrimitiveType():
+		return val
 	case ty.IsListType() || ty.IsTupleType() || ty.IsSetType():
-		if val.LengthInt() == 0 {
-			return val
-		}
-
 		var vals []cty.Value
 		it := val.ElementIterator()
 		for it.Next() {
@@ -379,29 +378,12 @@ func omitUnknowns(val cty.Value) cty.Value {
 				vals = append(vals, cty.NullVal(v.Type()))
 			}
 		}
-		if len(vals) == 0 {
-			return cty.NilVal
-		}
-		switch {
-		case ty.IsListType():
-			return cty.ListVal(vals)
-		case ty.IsTupleType():
-			return cty.TupleVal(vals)
-		default:
-			return cty.SetVal(vals)
-		}
+		// We use tuple types always here, because the work we did above
+		// may have caused the individual elements to have different types,
+		// and we're doing this work to produce JSON anyway and JSON marshalling
+		// represents all of these sequence types as an array.
+		return cty.TupleVal(vals)
 	case ty.IsMapType() || ty.IsObjectType():
-		var length int
-		switch {
-		case ty.IsMapType():
-			length = val.LengthInt()
-		default:
-			length = len(val.Type().AttributeTypes())
-		}
-		if length == 0 {
-			// If there are no elements then we can't have unknowns
-			return val
-		}
 		vals := make(map[string]cty.Value)
 		it := val.ElementIterator()
 		for it.Next() {
@@ -411,29 +393,24 @@ func omitUnknowns(val cty.Value) cty.Value {
 				vals[k.AsString()] = newVal
 			}
 		}
-
-		if len(vals) == 0 {
-			return cty.NilVal
-		}
-
-		switch {
-		case ty.IsMapType():
-			return cty.MapVal(vals)
-		default:
-			return cty.ObjectVal(vals)
-		}
+		// We use object types always here, because the work we did above
+		// may have caused the individual elements to have different types,
+		// and we're doing this work to produce JSON anyway and JSON marshalling
+		// represents both of these mapping types as an object.
+		return cty.ObjectVal(vals)
+	default:
+		// Should never happen, since the above should cover all types
+		panic(fmt.Sprintf("omitUnknowns cannot handle %#v", val))
 	}
-
-	return val
 }
 
-// recursively iterate through a cty.Value, replacing known values (including
-// null) with cty.True and unknown values with cty.False.
+// recursively iterate through a cty.Value, replacing unknown values (including
+// null) with cty.True and known values with cty.False.
 //
-// TODO:
-// In the future, we may choose to only return unknown values. At that point,
-// this will need to convert lists/sets into tuples and maps into objects, so
-// that the result will have a valid type.
+// The result also normalizes some types: all sequence types are turned into
+// tuple types and all mapping types are converted to object types, since we
+// assume the result of this is just going to be serialized as JSON (and thus
+// lose those distinctions) anyway.
 func unknownAsBool(val cty.Value) cty.Value {
 	ty := val.Type()
 	switch {
@@ -450,7 +427,7 @@ func unknownAsBool(val cty.Value) cty.Value {
 		length := val.LengthInt()
 		if length == 0 {
 			// If there are no elements then we can't have unknowns
-			return cty.False
+			return cty.EmptyTupleVal
 		}
 		vals := make([]cty.Value, 0, length)
 		it := val.ElementIterator()
@@ -458,14 +435,12 @@ func unknownAsBool(val cty.Value) cty.Value {
 			_, v := it.Element()
 			vals = append(vals, unknownAsBool(v))
 		}
-		switch {
-		case ty.IsListType():
-			return cty.ListVal(vals)
-		case ty.IsTupleType():
-			return cty.TupleVal(vals)
-		default:
-			return cty.SetVal(vals)
-		}
+		// The above transform may have changed the types of some of the
+		// elements, so we'll always use a tuple here in case we've now made
+		// different elements have different types. Our ultimate goal is to
+		// marshal to JSON anyway, and all of these sequence types are
+		// indistinguishable in JSON.
+		return cty.TupleVal(vals)
 	case ty.IsMapType() || ty.IsObjectType():
 		var length int
 		switch {
@@ -476,23 +451,27 @@ func unknownAsBool(val cty.Value) cty.Value {
 		}
 		if length == 0 {
 			// If there are no elements then we can't have unknowns
-			return cty.False
+			return cty.EmptyObjectVal
 		}
 		vals := make(map[string]cty.Value)
 		it := val.ElementIterator()
 		for it.Next() {
 			k, v := it.Element()
-			vals[k.AsString()] = unknownAsBool(v)
-		}
-		switch {
-		case ty.IsMapType():
-			return cty.MapVal(vals)
-		default:
-			return cty.ObjectVal(vals)
+			vAsBool := unknownAsBool(v)
+			if !vAsBool.RawEquals(cty.False) { // all of the "false"s for known values for more compact serialization
+				vals[k.AsString()] = unknownAsBool(v)
+			}
 		}
+		// The above transform may have changed the types of some of the
+		// elements, so we'll always use an object here in case we've now made
+		// different elements have different types. Our ultimate goal is to
+		// marshal to JSON anyway, and all of these mapping types are
+		// indistinguishable in JSON.
+		return cty.ObjectVal(vals)
+	default:
+		// Should never happen, since the above should cover all types
+		panic(fmt.Sprintf("unknownAsBool cannot handle %#v", val))
 	}
-
-	return val
 }
 
 func actionString(action string) []string {

command/jsonplan/plan_test.go

@@ -26,30 +26,30 @@ func TestOmitUnknowns(t *testing.T) {
 		},
 		{
 			cty.ListValEmpty(cty.String),
-			cty.ListValEmpty(cty.String),
+			cty.EmptyTupleVal,
 		},
 		{
 			cty.ListVal([]cty.Value{cty.StringVal("hello")}),
-			cty.ListVal([]cty.Value{cty.StringVal("hello")}),
+			cty.TupleVal([]cty.Value{cty.StringVal("hello")}),
 		},
 		{
 			cty.ListVal([]cty.Value{cty.NullVal(cty.String)}),
-			cty.ListVal([]cty.Value{cty.NullVal(cty.String)}),
+			cty.TupleVal([]cty.Value{cty.NullVal(cty.String)}),
 		},
 		{
 			cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)}),
-			cty.ListVal([]cty.Value{cty.NullVal(cty.String)}),
+			cty.TupleVal([]cty.Value{cty.NullVal(cty.String)}),
 		},
 		{
 			cty.ListVal([]cty.Value{cty.StringVal("hello")}),
-			cty.ListVal([]cty.Value{cty.StringVal("hello")}),
+			cty.TupleVal([]cty.Value{cty.StringVal("hello")}),
 		},
 		//
 		{
 			cty.ListVal([]cty.Value{
 				cty.StringVal("hello"),
 				cty.UnknownVal(cty.String)}),
-			cty.ListVal([]cty.Value{
+			cty.TupleVal([]cty.Value{
 				cty.StringVal("hello"),
 				cty.NullVal(cty.String),
 			}),
@@ -59,7 +59,7 @@ func TestOmitUnknowns(t *testing.T) {
 				"hello": cty.True,
 				"world": cty.UnknownVal(cty.Bool),
 			}),
-			cty.MapVal(map[string]cty.Value{
+			cty.ObjectVal(map[string]cty.Value{
 				"hello": cty.True,
 			}),
 		},
@@ -70,12 +70,28 @@ func TestOmitUnknowns(t *testing.T) {
 				cty.StringVal("stg"),
 				cty.UnknownVal(cty.String),
 			}),
-			cty.SetVal([]cty.Value{
+			cty.TupleVal([]cty.Value{
 				cty.StringVal("dev"),
 				cty.StringVal("foo"),
 				cty.StringVal("stg"),
 			}),
 		},
+		{
+			cty.SetVal([]cty.Value{
+				cty.ObjectVal(map[string]cty.Value{
+					"a": cty.UnknownVal(cty.String),
+				}),
+				cty.ObjectVal(map[string]cty.Value{
+					"a": cty.StringVal("known"),
+				}),
+			}),
+			cty.TupleVal([]cty.Value{
+				cty.ObjectVal(map[string]cty.Value{
+					"a": cty.StringVal("known"),
+				}),
+				cty.EmptyObjectVal,
+			}),
+		},
 	}
 
 	for _, test := range tests {
@@ -122,39 +138,39 @@ func TestUnknownAsBool(t *testing.T) {
 
 		{
 			cty.ListValEmpty(cty.String),
-			cty.False,
+			cty.EmptyTupleVal,
 		},
 		{
 			cty.ListVal([]cty.Value{cty.StringVal("hello")}),
-			cty.ListVal([]cty.Value{cty.False}),
+			cty.TupleVal([]cty.Value{cty.False}),
 		},
 		{
 			cty.ListVal([]cty.Value{cty.NullVal(cty.String)}),
-			cty.ListVal([]cty.Value{cty.False}),
+			cty.TupleVal([]cty.Value{cty.False}),
 		},
 		{
 			cty.ListVal([]cty.Value{cty.UnknownVal(cty.String)}),
-			cty.ListVal([]cty.Value{cty.True}),
+			cty.TupleVal([]cty.Value{cty.True}),
 		},
 		{
 			cty.SetValEmpty(cty.String),
-			cty.False,
+			cty.EmptyTupleVal,
 		},
 		{
 			cty.SetVal([]cty.Value{cty.StringVal("hello")}),
-			cty.SetVal([]cty.Value{cty.False}),
+			cty.TupleVal([]cty.Value{cty.False}),
 		},
 		{
 			cty.SetVal([]cty.Value{cty.NullVal(cty.String)}),
-			cty.SetVal([]cty.Value{cty.False}),
+			cty.TupleVal([]cty.Value{cty.False}),
 		},
 		{
 			cty.SetVal([]cty.Value{cty.UnknownVal(cty.String)}),
-			cty.SetVal([]cty.Value{cty.True}),
+			cty.TupleVal([]cty.Value{cty.True}),
 		},
 		{
 			cty.EmptyTupleVal,
-			cty.False,
+			cty.EmptyTupleVal,
 		},
 		{
 			cty.TupleVal([]cty.Value{cty.StringVal("hello")}),
@@ -170,36 +186,70 @@ func TestUnknownAsBool(t *testing.T) {
 		},
 		{
 			cty.MapValEmpty(cty.String),
-			cty.False,
+			cty.EmptyObjectVal,
 		},
 		{
 			cty.MapVal(map[string]cty.Value{"greeting": cty.StringVal("hello")}),
-			cty.MapVal(map[string]cty.Value{"greeting": cty.False}),
+			cty.EmptyObjectVal,
 		},
 		{
 			cty.MapVal(map[string]cty.Value{"greeting": cty.NullVal(cty.String)}),
-			cty.MapVal(map[string]cty.Value{"greeting": cty.False}),
+			cty.EmptyObjectVal,
 		},
 		{
 			cty.MapVal(map[string]cty.Value{"greeting": cty.UnknownVal(cty.String)}),
-			cty.MapVal(map[string]cty.Value{"greeting": cty.True}),
+			cty.ObjectVal(map[string]cty.Value{"greeting": cty.True}),
 		},
 		{
 			cty.EmptyObjectVal,
-			cty.False,
+			cty.EmptyObjectVal,
 		},
 		{
 			cty.ObjectVal(map[string]cty.Value{"greeting": cty.StringVal("hello")}),
-			cty.ObjectVal(map[string]cty.Value{"greeting": cty.False}),
+			cty.EmptyObjectVal,
 		},
 		{
 			cty.ObjectVal(map[string]cty.Value{"greeting": cty.NullVal(cty.String)}),
-			cty.ObjectVal(map[string]cty.Value{"greeting": cty.False}),
+			cty.EmptyObjectVal,
 		},
 		{
 			cty.ObjectVal(map[string]cty.Value{"greeting": cty.UnknownVal(cty.String)}),
 			cty.ObjectVal(map[string]cty.Value{"greeting": cty.True}),
 		},
+		{
+			cty.SetVal([]cty.Value{
+				cty.ObjectVal(map[string]cty.Value{
+					"a": cty.UnknownVal(cty.String),
+				}),
+				cty.ObjectVal(map[string]cty.Value{
+					"a": cty.StringVal("known"),
+				}),
+			}),
+			cty.TupleVal([]cty.Value{
+				cty.EmptyObjectVal,
+				cty.ObjectVal(map[string]cty.Value{
+					"a": cty.True,
+				}),
+			}),
+		},
+		{
+			cty.SetVal([]cty.Value{
+				cty.MapValEmpty(cty.String),
+				cty.MapVal(map[string]cty.Value{
+					"a": cty.StringVal("known"),
+				}),
+				cty.MapVal(map[string]cty.Value{
+					"a": cty.UnknownVal(cty.String),
+				}),
+			}),
+			cty.TupleVal([]cty.Value{
+				cty.EmptyObjectVal,
+				cty.ObjectVal(map[string]cty.Value{
+					"a": cty.True,
+				}),
+				cty.EmptyObjectVal,
+			}),
+		},
 	}
 
 	for _, test := range tests {

command/jsonplan/values_test.go

@@ -166,14 +166,14 @@ func TestMarshalPlannedOutputs(t *testing.T) {
 }
 
 func TestMarshalPlanResources(t *testing.T) {
-	tests := []struct {
+	tests := map[string]struct {
 		Action plans.Action
 		Before cty.Value
 		After  cty.Value
 		Want   []resource
 		Err    bool
 	}{
-		{
+		"create with unkonwns": {
 			Action: plans.Create,
 			Before: cty.NullVal(cty.EmptyObject),
 			After: cty.ObjectVal(map[string]cty.Value{
@@ -188,18 +188,18 @@ func TestMarshalPlanResources(t *testing.T) {
 				Index:           addrs.InstanceKey(nil),
 				ProviderName:    "test",
 				SchemaVersion:   1,
-				AttributeValues: attributeValues(nil),
+				AttributeValues: attributeValues{},
 			}},
 			Err: false,
 		},
-		{
+		"delete": {
 			Action: plans.Delete,
 			Before: cty.NullVal(cty.EmptyObject),
 			After:  cty.NilVal,
 			Want:   nil,
 			Err:    false,
 		},
-		{
+		"update without unknowns": {
 			Action: plans.Update,
 			Before: cty.ObjectVal(map[string]cty.Value{
 				"woozles": cty.StringVal("foo"),
@@ -227,50 +227,52 @@ func TestMarshalPlanResources(t *testing.T) {
 		},
 	}
 
-	for _, test := range tests {
-		before, err := plans.NewDynamicValue(test.Before, test.Before.Type())
-		if err != nil {
-			t.Fatal(err)
-		}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			before, err := plans.NewDynamicValue(test.Before, test.Before.Type())
+			if err != nil {
+				t.Fatal(err)
+			}
 
-		after, err := plans.NewDynamicValue(test.After, test.After.Type())
-		if err != nil {
-			t.Fatal(err)
-		}
-		testChange := &plans.Changes{
-			Resources: []*plans.ResourceInstanceChangeSrc{
-				{
-					Addr: addrs.Resource{
-						Mode: addrs.ManagedResourceMode,
-						Type: "test_thing",
-						Name: "example",
-					}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
-					ProviderAddr: addrs.ProviderConfig{Type: "test"}.Absolute(addrs.RootModuleInstance),
-					ChangeSrc: plans.ChangeSrc{
-						Action: test.Action,
-						Before: before,
-						After:  after,
+			after, err := plans.NewDynamicValue(test.After, test.After.Type())
+			if err != nil {
+				t.Fatal(err)
+			}
+			testChange := &plans.Changes{
+				Resources: []*plans.ResourceInstanceChangeSrc{
+					{
+						Addr: addrs.Resource{
+							Mode: addrs.ManagedResourceMode,
+							Type: "test_thing",
+							Name: "example",
+						}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
+						ProviderAddr: addrs.ProviderConfig{Type: "test"}.Absolute(addrs.RootModuleInstance),
+						ChangeSrc: plans.ChangeSrc{
+							Action: test.Action,
+							Before: before,
+							After:  after,
+						},
 					},
 				},
-			},
-		}
-
-		ris := testResourceAddrs()
-
-		got, err := marshalPlanResources(testChange, ris, testSchemas())
-		if test.Err {
-			if err == nil {
-				t.Fatal("succeeded; want error")
 			}
-			return
-		} else if err != nil {
-			t.Fatalf("unexpected error: %s", err)
-		}
 
-		eq := reflect.DeepEqual(got, test.Want)
-		if !eq {
-			t.Fatalf("wrong result:\nGot: %#v\nWant: %#v\n", got, test.Want)
-		}
+			ris := testResourceAddrs()
+
+			got, err := marshalPlanResources(testChange, ris, testSchemas())
+			if test.Err {
+				if err == nil {
+					t.Fatal("succeeded; want error")
+				}
+				return
+			} else if err != nil {
+				t.Fatalf("unexpected error: %s", err)
+			}
+
+			eq := reflect.DeepEqual(got, test.Want)
+			if !eq {
+				t.Fatalf("wrong result:\nGot: %#v\nWant: %#v\n", got, test.Want)
+			}
+		})
 	}
 }
 

command/test-fixtures/show-json/basic-create/output.json

@@ -67,7 +67,6 @@
                 ],
                 "before": null,
                 "after_unknown": {
-                    "ami": false,
                     "id": true
                 },
                 "after": {
@@ -88,7 +87,6 @@
                 ],
                 "before": null,
                 "after_unknown": {
-                    "ami": false,
                     "id": true
                 },
                 "after": {
@@ -109,7 +107,6 @@
                 ],
                 "before": null,
                 "after_unknown": {
-                    "ami": false,
                     "id": true
                 },
                 "after": {

command/test-fixtures/show-json/modules/output.json

@@ -86,7 +86,6 @@
                     "ami": "bar-var"
                 },
                 "after_unknown": {
-                    "ami": false,
                     "id": true
                 }
             }
@@ -108,7 +107,6 @@
                     "ami": "baz"
                 },
                 "after_unknown": {
-                    "ami": false,
                     "id": true
                 }
             }
@@ -130,7 +128,6 @@
                     "ami": "baz"
                 },
                 "after_unknown": {
-                    "ami": false,
                     "id": true
                 }
             }
@@ -152,7 +149,6 @@
                     "ami": "baz"
                 },
                 "after_unknown": {
-                    "ami": false,
                     "id": true
                 }
             }