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

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
 	}
-	return len(c.cases) == 0
+	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
+	}
+	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]{

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))
 		}

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)