Replace unpack's replace semantics with append

Blueprint was using "replace" semantics when unpacking properties
into property structs, meaning if a module factory pre-set property
values they would be overwritten by whatever was in the Blueprint
file.  This is different than what would happen if the same property
was updated using the Append*Properties functions in proptools, which
would use "append" semantics, which append strings and lists,
logically ORs booleans and replaces pointers to strings and booleans.
Replace unpack's semantics with append semantics for consistency.
Any previous users of pre-set properties can move to using a pointer
to a string or boolean if they want the old behavior.

Test: unpack_test.go
Test: extend_test.go
Change-Id: I02eebe80916e578938142f8e76889bd985223afc

proptools/extend.go

@@ -238,7 +238,7 @@ func extendMatchingProperties(dst []interface{}, src interface{}, filter ExtendP
 
 func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value,
 	prefix string, filter ExtendPropertyFilterFunc, sameTypes bool,
-	order ExtendPropertyOrderFunc) error {
+	orderFunc ExtendPropertyOrderFunc) error {
 
 	srcType := srcValue.Type()
 	for i, srcField := range typeFields(srcType) {
@@ -373,9 +373,10 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
 				}
 			}
 
-			prepend := false
-			if order != nil {
-				b, err := order(propertyName, dstField, srcField,
+			order := Append
+			if orderFunc != nil {
+				var err error
+				order, err = orderFunc(propertyName, dstField, srcField,
 					dstFieldInterface, srcFieldInterface)
 				if err != nil {
 					return &ExtendPropertyError{
@@ -383,69 +384,14 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
 						Err:      err,
 					}
 				}
-				prepend = b == Prepend
 			}
 
-			switch srcFieldValue.Kind() {
-			case reflect.Bool:
-				// Boolean OR
-				dstFieldValue.Set(reflect.ValueOf(srcFieldValue.Bool() || dstFieldValue.Bool()))
-			case reflect.String:
-				// Append the extension string.
-				if prepend {
-					dstFieldValue.SetString(srcFieldValue.String() +
-						dstFieldValue.String())
-				} else {
-					dstFieldValue.SetString(dstFieldValue.String() +
-						srcFieldValue.String())
-				}
-			case reflect.Slice:
-				if srcFieldValue.IsNil() {
-					break
-				}
-
-				newSlice := reflect.MakeSlice(srcFieldValue.Type(), 0,
-					dstFieldValue.Len()+srcFieldValue.Len())
-				if prepend {
-					newSlice = reflect.AppendSlice(newSlice, srcFieldValue)
-					newSlice = reflect.AppendSlice(newSlice, dstFieldValue)
-				} else {
-					newSlice = reflect.AppendSlice(newSlice, dstFieldValue)
-					newSlice = reflect.AppendSlice(newSlice, srcFieldValue)
-				}
-				dstFieldValue.Set(newSlice)
-			case reflect.Ptr:
-				if srcFieldValue.IsNil() {
-					break
-				}
-
-				switch ptrKind := srcFieldValue.Type().Elem().Kind(); ptrKind {
-				case reflect.Bool:
-					if prepend {
-						if dstFieldValue.IsNil() {
-							dstFieldValue.Set(reflect.ValueOf(BoolPtr(srcFieldValue.Elem().Bool())))
-						}
-					} else {
-						// For append, replace the original value.
-						dstFieldValue.Set(reflect.ValueOf(BoolPtr(srcFieldValue.Elem().Bool())))
-					}
-				case reflect.String:
-					if prepend {
-						if dstFieldValue.IsNil() {
-							dstFieldValue.Set(reflect.ValueOf(StringPtr(srcFieldValue.Elem().String())))
-						}
-					} else {
-						// For append, replace the original value.
-						dstFieldValue.Set(reflect.ValueOf(StringPtr(srcFieldValue.Elem().String())))
-					}
-				default:
-					panic(fmt.Errorf("unexpected pointer kind %s", ptrKind))
-				}
-			}
+			ExtendBasicType(dstFieldValue, srcFieldValue, order)
 		}
+
 		if len(recurse) > 0 {
 			err := extendPropertiesRecursive(recurse, srcFieldValue,
-				propertyName+".", filter, sameTypes, order)
+				propertyName+".", filter, sameTypes, orderFunc)
 			if err != nil {
 				return err
 			}
@@ -457,6 +403,66 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
 	return nil
 }
 
+func ExtendBasicType(dstFieldValue, srcFieldValue reflect.Value, order Order) {
+	prepend := order == Prepend
+
+	switch srcFieldValue.Kind() {
+	case reflect.Bool:
+		// Boolean OR
+		dstFieldValue.Set(reflect.ValueOf(srcFieldValue.Bool() || dstFieldValue.Bool()))
+	case reflect.String:
+		if prepend {
+			dstFieldValue.SetString(srcFieldValue.String() +
+				dstFieldValue.String())
+		} else {
+			dstFieldValue.SetString(dstFieldValue.String() +
+				srcFieldValue.String())
+		}
+	case reflect.Slice:
+		if srcFieldValue.IsNil() {
+			break
+		}
+
+		newSlice := reflect.MakeSlice(srcFieldValue.Type(), 0,
+			dstFieldValue.Len()+srcFieldValue.Len())
+		if prepend {
+			newSlice = reflect.AppendSlice(newSlice, srcFieldValue)
+			newSlice = reflect.AppendSlice(newSlice, dstFieldValue)
+		} else {
+			newSlice = reflect.AppendSlice(newSlice, dstFieldValue)
+			newSlice = reflect.AppendSlice(newSlice, srcFieldValue)
+		}
+		dstFieldValue.Set(newSlice)
+	case reflect.Ptr:
+		if srcFieldValue.IsNil() {
+			break
+		}
+
+		switch ptrKind := srcFieldValue.Type().Elem().Kind(); ptrKind {
+		case reflect.Bool:
+			if prepend {
+				if dstFieldValue.IsNil() {
+					dstFieldValue.Set(reflect.ValueOf(BoolPtr(srcFieldValue.Elem().Bool())))
+				}
+			} else {
+				// For append, replace the original value.
+				dstFieldValue.Set(reflect.ValueOf(BoolPtr(srcFieldValue.Elem().Bool())))
+			}
+		case reflect.String:
+			if prepend {
+				if dstFieldValue.IsNil() {
+					dstFieldValue.Set(reflect.ValueOf(StringPtr(srcFieldValue.Elem().String())))
+				}
+			} else {
+				// For append, replace the original value.
+				dstFieldValue.Set(reflect.ValueOf(StringPtr(srcFieldValue.Elem().String())))
+			}
+		default:
+			panic(fmt.Errorf("unexpected pointer kind %s", ptrKind))
+		}
+	}
+}
+
 type getStructEmptyError struct{}
 
 func (getStructEmptyError) Error() string { return "interface containing nil pointer" }

unpack.go

@@ -240,27 +240,7 @@ func unpackStructValue(namePrefix string, structValue reflect.Value,
 
 		var newErrs []error
 
-		switch kind := fieldValue.Kind(); kind {
-		case reflect.Bool:
-			newErrs = unpackBool(fieldValue, packedProperty.property)
-		case reflect.String:
-			newErrs = unpackString(fieldValue, packedProperty.property)
-		case reflect.Slice:
-			newErrs = unpackSlice(fieldValue, packedProperty.property)
-		case reflect.Ptr:
-			switch ptrKind := fieldValue.Type().Elem().Kind(); ptrKind {
-			case reflect.Bool:
-				newValue := reflect.New(fieldValue.Type().Elem())
-				newErrs = unpackBool(newValue.Elem(), packedProperty.property)
-				fieldValue.Set(newValue)
-			case reflect.String:
-				newValue := reflect.New(fieldValue.Type().Elem())
-				newErrs = unpackString(newValue.Elem(), packedProperty.property)
-				fieldValue.Set(newValue)
-			default:
-				panic(fmt.Errorf("unexpected pointer kind %s", ptrKind))
-			}
-		case reflect.Struct:
+		if fieldValue.Kind() == reflect.Struct {
 			localFilterKey, localFilterValue := filterKey, filterValue
 			if k, v, err := HasFilter(field.Tag); err != nil {
 				errs = append(errs, err)
@@ -280,66 +260,87 @@ func unpackStructValue(namePrefix string, structValue reflect.Value,
 			}
 			newErrs = unpackStruct(propertyName+".", fieldValue,
 				packedProperty.property, propertyMap, localFilterKey, localFilterValue)
-		default:
-			panic(fmt.Errorf("unexpected kind %s", kind))
+
+			errs = append(errs, newErrs...)
+			if len(errs) >= maxErrors {
+				return errs
+			}
+
+			continue
 		}
-		errs = append(errs, newErrs...)
-		if len(errs) >= maxErrors {
-			return errs
+
+		// Handle basic types and pointers to basic types
+
+		propertyValue, err := propertyToValue(fieldValue.Type(), packedProperty.property)
+		if err != nil {
+			errs = append(errs, err)
+			if len(errs) >= maxErrors {
+				return errs
+			}
 		}
+
+		proptools.ExtendBasicType(fieldValue, propertyValue, proptools.Append)
 	}
 
 	return errs
 }
 
-func unpackBool(boolValue reflect.Value, property *parser.Property) []error {
-	b, ok := property.Value.Eval().(*parser.Bool)
-	if !ok {
-		return []error{
-			fmt.Errorf("%s: can't assign %s value to bool property %q",
-				property.Value.Pos(), property.Value.Type(), property.Name),
-		}
-	}
-	boolValue.SetBool(b.Value)
-	return nil
-}
+func propertyToValue(typ reflect.Type, property *parser.Property) (reflect.Value, error) {
+	var value reflect.Value
 
-func unpackString(stringValue reflect.Value,
-	property *parser.Property) []error {
-
-	s, ok := property.Value.Eval().(*parser.String)
-	if !ok {
-		return []error{
-			fmt.Errorf("%s: can't assign %s value to string property %q",
-				property.Value.Pos(), property.Value.Type(), property.Name),
-		}
-	}
-	stringValue.SetString(s.Value)
-	return nil
-}
-
-func unpackSlice(sliceValue reflect.Value, property *parser.Property) []error {
-
-	l, ok := property.Value.Eval().(*parser.List)
-	if !ok {
-		return []error{
-			fmt.Errorf("%s: can't assign %s value to list property %q",
-				property.Value.Pos(), property.Value.Type(), property.Name),
-		}
+	var ptr bool
+	if typ.Kind() == reflect.Ptr {
+		typ = typ.Elem()
+		ptr = true
 	}
 
-	list := make([]string, len(l.Values))
-	for i, value := range l.Values {
-		s, ok := value.Eval().(*parser.String)
+	switch kind := typ.Kind(); kind {
+	case reflect.Bool:
+		b, ok := property.Value.Eval().(*parser.Bool)
 		if !ok {
-			// The parser should not produce this.
-			panic(fmt.Errorf("non-string value %q found in list", value))
+			return value, fmt.Errorf("%s: can't assign %s value to bool property %q",
+				property.Value.Pos(), property.Value.Type(), property.Name)
 		}
-		list[i] = s.Value
+		value = reflect.ValueOf(b.Value)
+
+	case reflect.String:
+		s, ok := property.Value.Eval().(*parser.String)
+		if !ok {
+			return value, fmt.Errorf("%s: can't assign %s value to string property %q",
+				property.Value.Pos(), property.Value.Type(), property.Name)
+		}
+		value = reflect.ValueOf(s.Value)
+
+	case reflect.Slice:
+		l, ok := property.Value.Eval().(*parser.List)
+		if !ok {
+			return value, fmt.Errorf("%s: can't assign %s value to list property %q",
+				property.Value.Pos(), property.Value.Type(), property.Name)
+		}
+
+		list := make([]string, len(l.Values))
+		for i, value := range l.Values {
+			s, ok := value.Eval().(*parser.String)
+			if !ok {
+				// The parser should not produce this.
+				panic(fmt.Errorf("non-string value %q found in list", value))
+			}
+			list[i] = s.Value
+		}
+
+		value = reflect.ValueOf(list)
+
+	default:
+		panic(fmt.Errorf("unexpected kind %s", kind))
 	}
 
-	sliceValue.Set(reflect.ValueOf(list))
-	return nil
+	if ptr {
+		ptrValue := reflect.New(value.Type())
+		ptrValue.Elem().Set(value)
+		value = ptrValue
+	}
+
+	return value, nil
 }
 
 func unpackStruct(namePrefix string, structValue reflect.Value,

unpack_test.go

@@ -498,6 +498,49 @@ var validUnpackTestCases = []struct {
 			},
 		},
 	},
+
+	// Factory set properties
+	{
+		input: `
+			m {
+				string: "abc",
+				string_ptr: "abc",
+				bool: false,
+				bool_ptr: false,
+				list: ["a", "b", "c"],
+			}
+		`,
+		output: []interface{}{
+			struct {
+				String     string
+				String_ptr *string
+				Bool       bool
+				Bool_ptr   *bool
+				List       []string
+			}{
+				String:     "012abc",
+				String_ptr: proptools.StringPtr("abc"),
+				Bool:       true,
+				Bool_ptr:   proptools.BoolPtr(false),
+				List:       []string{"0", "1", "2", "a", "b", "c"},
+			},
+		},
+		empty: []interface{}{
+			&struct {
+				String     string
+				String_ptr *string
+				Bool       bool
+				Bool_ptr   *bool
+				List       []string
+			}{
+				String:     "012",
+				String_ptr: proptools.StringPtr("012"),
+				Bool:       true,
+				Bool_ptr:   proptools.BoolPtr(true),
+				List:       []string{"0", "1", "2"},
+			},
+		},
+	},
 }
 
 type EmbeddedStruct struct{ Name string }