Refactor detailed diff v2 to short circuit on nulls and unknowns (#2496)

This PR changes the logic in the detailed diff v2 calculation to
short-circuit on encountering nils and unknowns.

Previously, when we encountered a null or unknown in either news or olds
we would still recurse down and compare the non-nil values versus a nil
value in order to get any replacements which might be happening. We
would then simplify the diff later to trim these extra diffs but would
propagate the replacement to the right level.

We would now instead short-circuit on encountering a null or unknown and
walk the non-null value to find any properties which can trigger a
replacement. This makes it much easier to handle sets in
#2451 as recursing
into sets is not necessary, as they are only compared by hashes.


This change is not meant to change any behaviour and is tested in
#2405

Stacked on #2515
and #2516

Author: VenelinMartinov

pkg/tfbridge/detailed_diff.go

@@ -12,6 +12,7 @@ import (
 	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge/info"
 	shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
 	"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/walk"
+	"github.com/pulumi/pulumi-terraform-bridge/v3/unstable/propertyvalue"
 )
 
 func isPresent(val resource.PropertyValue) bool {
@@ -41,6 +42,87 @@ func sortedMergedKeys[K cmp.Ordered, V any, M ~map[K]V](a, b M) []K {
 	return keysSlice
 }
 
+func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType) bool {
+	contract.Assertf(!val.IsSecret() || val.IsOutput(), "secrets and outputs are not handled")
+	if val.IsComputed() {
+		return false
+	}
+	if !isPresent(val) {
+		return false
+	}
+	switch propType {
+	case shim.TypeList:
+		return !val.IsArray()
+	case shim.TypeSet:
+		return !val.IsArray()
+	case shim.TypeMap:
+		return !val.IsObject()
+	default:
+		return false
+	}
+}
+
+func lookupSchemas(
+	path propertyPath, tfs shim.SchemaMap, ps map[string]*info.Schema,
+) (shim.Schema, *info.Schema, error) {
+	schemaPath := PropertyPathToSchemaPath(resource.PropertyPath(path), tfs, ps)
+	return LookupSchemas(schemaPath, tfs, ps)
+}
+
+func propertyPathTriggersReplacement(
+	path propertyPath, rootTFSchema shim.SchemaMap, rootPulumiSchema map[string]*info.Schema,
+) bool {
+	// A change on a property might trigger a replacement if:
+	// - The property itself is marked as ForceNew
+	// - The direct parent property is a collection (list, set, map) and is marked as ForceNew
+	// See pkg/cross-tests/diff_cross_test.go
+	// TestAttributeCollectionForceNew, TestBlockCollectionForceNew, TestBlockCollectionElementForceNew
+	// for a full case study of replacements in TF
+	tfs, ps, err := lookupSchemas(path, rootTFSchema, rootPulumiSchema)
+	if err != nil {
+		return false
+	}
+	if isForceNew(tfs, ps) {
+		return true
+	}
+
+	if len(path) == 1 {
+		return false
+	}
+
+	parent := path[:len(path)-1]
+	tfs, ps, err = lookupSchemas(parent, rootTFSchema, rootPulumiSchema)
+	if err != nil {
+		return false
+	}
+	// Note this is mimicking the TF behaviour, so the effective type is not considered here.
+	if tfs.Type() != shim.TypeList && tfs.Type() != shim.TypeSet && tfs.Type() != shim.TypeMap {
+		return false
+	}
+	return isForceNew(tfs, ps)
+}
+
+func propertyValueTriggersReplacement(
+	path propertyPath, value resource.PropertyValue, tfs shim.SchemaMap, ps map[string]*info.Schema,
+) bool {
+	replacement := false
+	visitor := func(subpath resource.PropertyPath, val resource.PropertyValue) (resource.PropertyValue, error) {
+		if propertyPathTriggersReplacement(propertyPath(subpath), tfs, ps) {
+			replacement = true
+		}
+		return val, nil
+	}
+
+	_, err := propertyvalue.TransformPropertyValue(
+		resource.PropertyPath(path),
+		visitor,
+		value,
+	)
+	contract.AssertNoErrorf(err, "TransformPropertyValue should not return an error")
+
+	return replacement
+}
+
 func promoteToReplace(diff *pulumirpc.PropertyDiff) *pulumirpc.PropertyDiff {
 	if diff == nil {
 		return nil
@@ -143,17 +225,6 @@ func (k propertyPath) IsReservedKey() bool {
 	return leaf == "__meta" || leaf == "__defaults"
 }
 
-func mapHasReplacements(m map[detailedDiffKey]*pulumirpc.PropertyDiff) bool {
-	for _, diff := range m {
-		if diff.GetKind() == pulumirpc.PropertyDiff_ADD_REPLACE ||
-			diff.GetKind() == pulumirpc.PropertyDiff_DELETE_REPLACE ||
-			diff.GetKind() == pulumirpc.PropertyDiff_UPDATE_REPLACE {
-			return true
-		}
-	}
-	return false
-}
-
 type detailedDiffer struct {
 	tfs shim.SchemaMap
 	ps  map[string]*SchemaInfo
@@ -182,79 +253,21 @@ func (differ detailedDiffer) getEffectiveType(path walk.SchemaPath) shim.ValueTy
 	return tfs.Type()
 }
 
-func (differ detailedDiffer) lookupSchemas(path propertyPath) (shim.Schema, *info.Schema, error) {
-	schemaPath := PropertyPathToSchemaPath(resource.PropertyPath(path), differ.tfs, differ.ps)
-	return LookupSchemas(schemaPath, differ.tfs, differ.ps)
-}
-
-func (differ detailedDiffer) isForceNew(pair propertyPath) bool {
-	// A change on a property might trigger a replacement if:
-	// - The property itself is marked as ForceNew
-	// - The direct parent property is a collection (list, set, map) and is marked as ForceNew
-	// See pkg/cross-tests/diff_cross_test.go
-	// TestAttributeCollectionForceNew, TestBlockCollectionForceNew, TestBlockCollectionElementForceNew
-	// for a full case study of replacements in TF
-	tfs, ps, err := differ.lookupSchemas(pair)
-	if err != nil {
-		return false
-	}
-	if isForceNew(tfs, ps) {
-		return true
-	}
-
-	if len(pair) == 1 {
-		return false
-	}
-
-	parent := pair[:len(pair)-1]
-	tfs, ps, err = differ.lookupSchemas(parent)
-	if err != nil {
-		return false
-	}
-	// Note this is mimicking the TF behaviour, so the effective type is not considered here.
-	if tfs.Type() != shim.TypeList && tfs.Type() != shim.TypeSet && tfs.Type() != shim.TypeMap {
-		return false
-	}
-	return isForceNew(tfs, ps)
-}
-
-// We do not short-circuit detailed diffs when comparing non-nil properties against nil ones. The reason for that is
-// that a replace might be triggered by a ForceNew inside a nested property of a non-ForceNew property. We instead
-// always walk the full tree even when comparing against a nil property. We then later do a simplification step for
-// the detailed diff in simplifyDiff in order to reduce the diff to what the user expects to see.
-// See [pulumi/pulumi-terraform-bridge#2405] for more details.
-func (differ detailedDiffer) simplifyDiff(
-	diff map[detailedDiffKey]*pulumirpc.PropertyDiff, path propertyPath, old, new resource.PropertyValue,
-) (map[detailedDiffKey]*pulumirpc.PropertyDiff, bool) {
-	baseDiff := makeBaseDiff(old, new)
-	if baseDiff == undecidedDiff {
-		return nil, false
-	}
-	propDiff := baseDiff.ToPropertyDiff()
-	if propDiff == nil {
-		return nil, true
-	}
-	if differ.isForceNew(path) || mapHasReplacements(diff) {
-		propDiff = promoteToReplace(propDiff)
-	}
-	return map[detailedDiffKey]*pulumirpc.PropertyDiff{path.Key(): propDiff}, true
-}
-
 // makePlainPropDiff is used for plain properties and ones with an unknown schema.
 // It does not access the TF schema, so it does not know about the type of the property.
 func (differ detailedDiffer) makePlainPropDiff(
 	path propertyPath, old, new resource.PropertyValue,
 ) map[detailedDiffKey]*pulumirpc.PropertyDiff {
 	baseDiff := makeBaseDiff(old, new)
-	isForceNew := differ.isForceNew(path)
+	isReplacement := propertyPathTriggersReplacement(path, differ.tfs, differ.ps)
 	var propDiff *pulumirpc.PropertyDiff
 	if baseDiff != undecidedDiff {
 		propDiff = baseDiff.ToPropertyDiff()
 	} else if !old.DeepEquals(new) {
 		propDiff = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE}
 	}
 
-	if isForceNew {
+	if isReplacement {
 		propDiff = promoteToReplace(propDiff)
 	}
 
@@ -264,6 +277,32 @@ func (differ detailedDiffer) makePlainPropDiff(
 	return nil
 }
 
+// makeShortCircuitDiff is used for properties that are nil or computed in either the old or new state.
+// It makes sure to check recursively if the property will trigger a replacement.
+func (differ detailedDiffer) makeShortCircuitDiff(
+	path propertyPath, old, new resource.PropertyValue,
+) map[detailedDiffKey]*pulumirpc.PropertyDiff {
+	contract.Assertf(old.IsNull() || new.IsNull() || new.IsComputed(),
+		"short-circuit diff should only be used for nil properties")
+	if old.IsNull() && new.IsNull() {
+		return nil
+	}
+
+	baseDiff := makeBaseDiff(old, new)
+	contract.Assertf(baseDiff != undecidedDiff, "short-circuit diff could not determine diff kind")
+
+	propDiff := baseDiff.ToPropertyDiff()
+	if new.IsComputed() && propertyPathTriggersReplacement(path, differ.tfs, differ.ps) {
+		propDiff = promoteToReplace(propDiff)
+	} else if !new.IsNull() && !new.IsComputed() && propertyValueTriggersReplacement(path, new, differ.tfs, differ.ps) {
+		propDiff = promoteToReplace(propDiff)
+	} else if !old.IsNull() && propertyValueTriggersReplacement(path, old, differ.tfs, differ.ps) {
+		propDiff = promoteToReplace(propDiff)
+	}
+
+	return map[detailedDiffKey]*pulumirpc.PropertyDiff{path.Key(): propDiff}
+}
+
 func (differ detailedDiffer) makePropDiff(
 	path propertyPath, old, new resource.PropertyValue,
 ) map[detailedDiffKey]*pulumirpc.PropertyDiff {
@@ -272,6 +311,20 @@ func (differ detailedDiffer) makePropDiff(
 	}
 	propType := differ.getEffectiveType(differ.propertyPathToSchemaPath(path))
 
+	if isTypeShapeMismatched(old, propType) || isTypeShapeMismatched(new, propType) {
+		return differ.makePlainPropDiff(path, old, new)
+	}
+
+	if !isPresent(old) {
+		old = resource.NewNullProperty()
+	}
+	if !new.IsComputed() && !isPresent(new) {
+		new = resource.NewNullProperty()
+	}
+	if old.IsNull() || new.IsNull() || new.IsComputed() {
+		return differ.makeShortCircuitDiff(path, old, new)
+	}
+
 	switch propType {
 	case shim.TypeList:
 		return differ.makeListDiff(path, old, new)
@@ -290,14 +343,8 @@ func (differ detailedDiffer) makeListDiff(
 	path propertyPath, old, new resource.PropertyValue,
 ) map[detailedDiffKey]*pulumirpc.PropertyDiff {
 	diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff)
-	oldList := []resource.PropertyValue{}
-	newList := []resource.PropertyValue{}
-	if isPresent(old) && old.IsArray() {
-		oldList = old.ArrayValue()
-	}
-	if isPresent(new) && new.IsArray() {
-		newList = new.ArrayValue()
-	}
+	oldList := old.ArrayValue()
+	newList := new.ArrayValue()
 
 	// naive diffing of lists
 	// TODO[pulumi/pulumi-terraform-bridge#2295]: implement a more sophisticated diffing algorithm
@@ -318,27 +365,15 @@ func (differ detailedDiffer) makeListDiff(
 		}
 	}
 
-	simplerDiff, isSimplified := differ.simplifyDiff(diff, path, old, new)
-	if isSimplified {
-		return simplerDiff
-	}
-
 	return diff
 }
 
 func (differ detailedDiffer) makeMapDiff(
 	path propertyPath, old, new resource.PropertyValue,
 ) map[detailedDiffKey]*pulumirpc.PropertyDiff {
+	oldMap := old.ObjectValue()
+	newMap := new.ObjectValue()
 	diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff)
-	oldMap := resource.PropertyMap{}
-	newMap := resource.PropertyMap{}
-	if isPresent(old) && old.IsObject() {
-		oldMap = old.ObjectValue()
-	}
-	if isPresent(new) && new.IsObject() {
-		newMap = new.ObjectValue()
-	}
-
 	for _, k := range sortedMergedKeys(oldMap, newMap) {
 		subindex := path.Subpath(string(k))
 		oldVal := oldMap[k]
@@ -351,11 +386,6 @@ func (differ detailedDiffer) makeMapDiff(
 		}
 	}
 
-	simplerDiff, isSimplified := differ.simplifyDiff(diff, path, old, new)
-	if isSimplified {
-		return simplerDiff
-	}
-
 	return diff
 }