Fix bugs when appending selects

The biggest issue here is that Configurable objects needed to be
cloned before appended to one another, otherwise some Configurables
that were used in defaults could be edited after being applied to one
module and apply a different value to another module.

Also fix an issue where a select without a defined appendWrapper
always evaluated to nil.

I plan to make a followup refactor cl to make these things clearer,
but start with the bugfix.

Bug: 323382414
Test: m nothing --no-skip-soong-tests (see other cl in topic for tests)
Change-Id: Icf68d0ee1779c76bfb3d68db43b35d7e09bc0dd9

3 files changed, 74 insertions, 18 deletions

diff --git a/proptools/configurable.go b/proptools/configurable.go
index 9fe79c3..2a094a3 100644
--- a/proptools/configurable.go
+++ b/proptools/configurable.go
@@ -335,9 +335,12 @@ type appendWrapper[T ConfigurableElements] struct {
 // Get returns the final value for the configurable property.
 // A configurable property may be unset, in which case Get will return nil.
 func (c *Configurable[T]) Get(evaluator ConfigurableEvaluator) *T {
-	if c == nil || c.appendWrapper == nil {
+	if c == nil {
 		return nil
 	}
+	if c.appendWrapper == nil {
+		return c.evaluateNonTransitive(evaluator)
+	}
 	if c.appendWrapper.replace {
 		return replaceConfiguredValues(
 			c.evaluateNonTransitive(evaluator),
@@ -488,19 +491,52 @@ func (c *Configurable[T]) initialize(propertyName string, conditions []Configura
 }
 
 func (c Configurable[T]) setAppend(append any, replace bool) {
+	// TODO(b/323382414) Update the propertyName of appended selects
 	if c.appendWrapper.append.isEmpty() {
-		c.appendWrapper.append = append.(Configurable[T])
+		x := append.(Configurable[T])
+		c.appendWrapper.append = *(&x).clone()
 		c.appendWrapper.replace = replace
 	} else {
-		c.appendWrapper.append.setAppend(append, replace)
+		// If we're replacing with something that always has a value set,
+		// we can optimize the code by replacing our entire append chain here.
+		if replace && append.(Configurable[T]).alwaysHasValue() {
+			x := append.(Configurable[T])
+			c.appendWrapper.append = *(&x).clone()
+			c.appendWrapper.replace = replace
+		} else {
+			c.appendWrapper.append.setAppend(append, replace)
+		}
 	}
 }
 
 func (c Configurable[T]) isEmpty() bool {
-	if c.appendWrapper != nil && !c.appendWrapper.append.isEmpty() {
+	if len(c.cases) > 1 {
+		return false
+	}
+	if len(c.cases) == 1 && c.cases[0].value != nil {
+		return false
+	}
+	if c.appendWrapper != nil {
+		return c.appendWrapper.append.isEmpty()
+	}
+	return true
+}
+
+func (c Configurable[T]) alwaysHasValue() bool {
+	if len(c.cases) == 0 {
 		return false
 	}
-	return len(c.cases) == 0
+	for _, c := range c.cases {
+		if c.value == nil {
+			return false
+		}
+	}
+
+	if c.appendWrapper != nil && !c.appendWrapper.append.isEmpty() {
+		return c.appendWrapper.append.alwaysHasValue()
+	}
+
+	return true
 }
 
 func (c Configurable[T]) configuredType() reflect.Type {
@@ -524,12 +560,18 @@ func (c *Configurable[T]) clone() *Configurable[T] {
 		}
 	}
 
-	conditionsCopy := make([]ConfigurableCondition, len(c.conditions))
-	copy(conditionsCopy, c.conditions)
+	var conditionsCopy []ConfigurableCondition
+	if c.conditions != nil {
+		conditionsCopy = make([]ConfigurableCondition, len(c.conditions))
+		copy(conditionsCopy, c.conditions)
+	}
 
-	casesCopy := make([]ConfigurableCase[T], len(c.cases))
-	for i, case_ := range c.cases {
-		casesCopy[i] = case_.Clone()
+	var casesCopy []ConfigurableCase[T]
+	if c.cases != nil {
+		casesCopy = make([]ConfigurableCase[T], len(c.cases))
+		for i, case_ := range c.cases {
+			casesCopy[i] = case_.Clone()
+		}
 	}
 
 	return &Configurable[T]{
diff --git a/proptools/extend.go b/proptools/extend.go
index 68c04a3..dc5a145 100644
--- a/proptools/extend.go
+++ b/proptools/extend.go
@@ -507,18 +507,24 @@ func ExtendBasicType(dstFieldValue, srcFieldValue reflect.Value, order Order) {
 		if !isConfigurable(srcFieldValue.Type()) {
 			panic("Should be unreachable")
 		}
-		if dstFieldValue.Interface().(configurableReflection).isEmpty() {
-			dstFieldValue.Set(srcFieldValue)
+		unpackedSrc := srcFieldValue.Interface().(configurableReflection)
+		unpackedDst := dstFieldValue.Interface().(configurableReflection)
+		if unpackedSrc.isEmpty() {
+			// Do nothing
+		} else if unpackedDst.isEmpty() {
+			dstFieldValue.Set(unpackedSrc.cloneToReflectValuePtr().Elem())
 		} else if order == Prepend {
-			srcFieldValue.Interface().(configurableReflection).setAppend(dstFieldValue.Interface(), false)
-			dstFieldValue.Set(srcFieldValue)
+			clonedSrc := unpackedSrc.cloneToReflectValuePtr().Elem()
+			clonedSrc.Interface().(configurableReflection).setAppend(dstFieldValue.Interface(), false)
+			dstFieldValue.Set(clonedSrc)
 		} else if order == Append {
-			dstFieldValue.Interface().(configurableReflection).setAppend(srcFieldValue.Interface(), false)
+			unpackedDst.setAppend(srcFieldValue.Interface(), false)
 		} else if order == Replace {
-			dstFieldValue.Interface().(configurableReflection).setAppend(srcFieldValue.Interface(), true)
+			unpackedDst.setAppend(srcFieldValue.Interface(), true)
 		} else if order == Prepend_replace {
-			srcFieldValue.Interface().(configurableReflection).setAppend(dstFieldValue.Interface(), true)
-			dstFieldValue.Set(srcFieldValue)
+			clonedSrc := unpackedSrc.cloneToReflectValuePtr().Elem()
+			clonedSrc.Interface().(configurableReflection).setAppend(dstFieldValue.Interface(), true)
+			dstFieldValue.Set(clonedSrc)
 		} else {
 			panic(fmt.Sprintf("Unexpected order: %d", order))
 		}
diff --git a/proptools/typeequal.go b/proptools/typeequal.go
index d9b3c18..3fadae4 100644
--- a/proptools/typeequal.go
+++ b/proptools/typeequal.go
@@ -62,6 +62,10 @@ func typeEqual(v1, v2 reflect.Value) bool {
 		return true
 	}
 
+	if isConfigurable(v1.Type()) {
+		return true
+	}
+
 	for i := 0; i < v1.NumField(); i++ {
 		v1 := v1.Field(i)
 		v2 := v2.Field(i)
@@ -94,6 +98,10 @@ func concreteType(v reflect.Value) bool {
 		return true
 	}
 
+	if isConfigurable(v.Type()) {
+		return true
+	}
+
 	for i := 0; i < v.NumField(); i++ {
 		v := v.Field(i)