Add property support for pointers to bools and strings

The only append semantics for bool that result in a no-op when the zero
value is appended is to OR the two values together, but that is rarely
the desired semantics.  Add support for *bool and *string as property
types, where appending a nil pointer is a no-op.  For *bool, appending a
non-nil pointer replaces the destination with the value.  For *string,
appending a non-nil pointer appends the value.

This also provides a more reliable replacement for
ModuleContext.ContainsProperty, as the  build logic can tell that the
property was set, even if it was set by a  mutator and not by the
blueprints file, by testing against nil.

[]string already provides these semantics for lists.

Setting a *bool or *string property from a blueprints file is the same
syntax as setting a bool or a string property.

proptools/clone.go

@@ -91,28 +91,35 @@ func CopyProperties(dstValue, srcValue reflect.Value) {
 			}
 			fallthrough
 		case reflect.Ptr:
-			if srcFieldValue.Type().Elem().Kind() != reflect.Struct {
-				panic(fmt.Errorf("can't clone field %q: points to a non-struct",
-					field.Name))
-			}
-
 			if srcFieldValue.IsNil() {
 				dstFieldValue.Set(srcFieldValue)
 				break
 			}
 
+			srcFieldValue := srcFieldValue.Elem()
+
+			switch srcFieldValue.Kind() {
+			case reflect.Struct:
 				if !dstFieldValue.IsNil() {
 					// Re-use the existing allocation.
-				CopyProperties(dstFieldValue.Elem(), srcFieldValue.Elem())
+					CopyProperties(dstFieldValue.Elem(), srcFieldValue)
 					break
 				} else {
-				newValue := CloneProperties(srcFieldValue.Elem())
+					newValue := CloneProperties(srcFieldValue)
 					if dstFieldInterfaceValue.IsValid() {
 						dstFieldInterfaceValue.Set(newValue)
 					} else {
 						dstFieldValue.Set(newValue)
 					}
 				}
+			case reflect.Bool, reflect.String:
+				newValue := reflect.New(srcFieldValue.Type())
+				newValue.Elem().Set(srcFieldValue)
+				dstFieldValue.Set(newValue)
+			default:
+				panic(fmt.Errorf("can't clone field %q: points to a %s",
+					field.Name, srcFieldValue.Kind()))
+			}
 		default:
 			panic(fmt.Errorf("unexpected kind for property struct field %q: %s",
 				field.Name, srcFieldValue.Kind()))
@@ -153,18 +160,19 @@ func ZeroProperties(structValue reflect.Value) {
 			}
 			fallthrough
 		case reflect.Ptr:
-			// We leave the pointer intact and zero out the struct that's
-			// pointed to.
-			if fieldValue.Type().Elem().Kind() != reflect.Struct {
-				panic(fmt.Errorf("can't zero field %q: points to a non-struct",
-					field.Name))
-			}
-
+			switch fieldValue.Type().Elem().Kind() {
+			case reflect.Struct:
 				if fieldValue.IsNil() {
 					break
 				}
-
 				ZeroProperties(fieldValue.Elem())
+			case reflect.Bool, reflect.String:
+				fieldValue.Set(reflect.Zero(fieldValue.Type()))
+			default:
+				panic(fmt.Errorf("can't zero field %q: points to a %s",
+					field.Name, fieldValue.Elem().Kind()))
+			}
+
 		default:
 			panic(fmt.Errorf("unexpected kind for property struct field %q: %s",
 				field.Name, fieldValue.Kind()))
@@ -217,21 +225,24 @@ func cloneEmptyProperties(dstValue, srcValue reflect.Value) {
 			dstFieldValue = newValue
 			fallthrough
 		case reflect.Ptr:
-			if srcFieldValue.Type().Elem().Kind() != reflect.Struct {
-				panic(fmt.Errorf("can't clone field %q: points to a non-struct",
-					field.Name))
-			}
-
+			switch srcFieldValue.Type().Elem().Kind() {
+			case reflect.Struct:
 				if srcFieldValue.IsNil() {
 					break
 				}
-
 				newValue := CloneEmptyProperties(srcFieldValue.Elem())
 				if dstFieldInterfaceValue.IsValid() {
 					dstFieldInterfaceValue.Set(newValue)
 				} else {
 					dstFieldValue.Set(newValue)
 				}
+			case reflect.Bool, reflect.String:
+				// Nothing
+			default:
+				panic(fmt.Errorf("can't clone empty field %q: points to a %s",
+					field.Name, srcFieldValue.Elem().Kind()))
+			}
+
 		default:
 			panic(fmt.Errorf("unexpected kind for property struct field %q: %s",
 				field.Name, srcFieldValue.Kind()))

proptools/clone_test.go

@@ -71,6 +71,25 @@ var clonePropertiesTestCases = []struct {
 		out: &struct{ S []string }{},
 	},
 	{
+		// Clone pointer to bool
+		in: &struct{ B1, B2 *bool }{
+			B1: BoolPtr(true),
+			B2: BoolPtr(false),
+		},
+		out: &struct{ B1, B2 *bool }{
+			B1: BoolPtr(true),
+			B2: BoolPtr(false),
+		},
+	},
+	{
+		// Clone pointer to string
+		in: &struct{ S *string }{
+			S: StringPtr("string1"),
+		},
+		out: &struct{ S *string }{
+			S: StringPtr("string1"),
+		},
+	},
 	{
 		// Clone struct
 		in: &struct{ S struct{ S string } }{
@@ -201,6 +220,20 @@ var cloneEmptyPropertiesTestCases = []struct {
 		out: &struct{ S []string }{},
 	},
 	{
+		// Clone pointer to bool
+		in: &struct{ B1, B2 *bool }{
+			B1: BoolPtr(true),
+			B2: BoolPtr(false),
+		},
+		out: &struct{ B1, B2 *bool }{},
+	},
+	{
+		// Clone pointer to string
+		in: &struct{ S *string }{
+			S: StringPtr("string1"),
+		},
+		out: &struct{ S *string }{},
+	},
 	{
 		// Clone struct
 		in: &struct{ S struct{ S string } }{

proptools/extend.go

@@ -29,8 +29,9 @@ import (
 // An error returned by AppendProperties that applies to a specific property will be an
 // *ExtendPropertyError, and can have the property name and error extracted from it.
 //
-// The append operation is defined as appending string and slices of strings normally, OR-ing
-// bool values, and recursing into embedded structs, pointers to structs, and interfaces containing
+// The append operation is defined as appending strings, pointers to strings, and slices of
+// strings normally, OR-ing bool values, replacing non-nil pointers to booleans, and recursing into
+// embedded structs, pointers to structs, and interfaces containing
 // pointers to structs.  Appending the zero value of a property will always be a no-op.
 func AppendProperties(dst interface{}, src interface{}, filter ExtendPropertyFilterFunc) error {
 	return extendProperties(dst, src, filter, false)
@@ -46,8 +47,9 @@ func AppendProperties(dst interface{}, src interface{}, filter ExtendPropertyFil
 // An error returned by PrependProperties that applies to a specific property will be an
 // *ExtendPropertyError, and can have the property name and error extracted from it.
 //
-// The prepend operation is defined as prepending string and slices of strings normally, OR-ing
-// bool values, and recursing into embedded structs, pointers to structs, and interfaces containing
+// The prepend operation is defined as prepending strings, pointers to strings, and slices of
+// strings normally, OR-ing bool values, replacing non-nil pointers to booleans, and recursing into
+// embedded structs, pointers to structs, and interfaces containing
 // pointers to structs.  Prepending the zero value of a property will always be a no-op.
 func PrependProperties(dst interface{}, src interface{}, filter ExtendPropertyFilterFunc) error {
 	return extendProperties(dst, src, filter, true)
@@ -65,8 +67,9 @@ func PrependProperties(dst interface{}, src interface{}, filter ExtendPropertyFi
 // An error returned by AppendMatchingProperties that applies to a specific property will be an
 // *ExtendPropertyError, and can have the property name and error extracted from it.
 //
-// The append operation is defined as appending string and slices of strings normally, OR-ing
-// bool values, and recursing into embedded structs, pointers to structs, and interfaces containing
+// The append operation is defined as appending strings, pointers to strings, and slices of
+// strings normally, OR-ing bool values, replacing non-nil pointers to booleans, and recursing into
+// embedded structs, pointers to structs, and interfaces containing
 // pointers to structs.  Appending the zero value of a property will always be a no-op.
 func AppendMatchingProperties(dst []interface{}, src interface{},
 	filter ExtendPropertyFilterFunc) error {
@@ -85,8 +88,9 @@ func AppendMatchingProperties(dst []interface{}, src interface{},
 // An error returned by PrependProperties that applies to a specific property will be an
 // *ExtendPropertyError, and can have the property name and error extracted from it.
 //
-// The prepend operation is defined as prepending string and slices of strings normally, OR-ing
-// bool values, and recursing into embedded structs, pointers to structs, and interfaces containing
+// The prepend operation is defined as prepending strings, pointers to strings, and slices of
+// strings normally, OR-ing bool values, replacing non-nil pointers to booleans, and recursing into
+// embedded structs, pointers to structs, and interfaces containing
 // pointers to structs.  Prepending the zero value of a property will always be a no-op.
 func PrependMatchingProperties(dst []interface{}, src interface{},
 	filter ExtendPropertyFilterFunc) error {
@@ -213,6 +217,18 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
 
 				fallthrough
 			case reflect.Ptr:
+				ptrKind := srcFieldValue.Type().Elem().Kind()
+				if ptrKind == reflect.Bool || ptrKind == reflect.String {
+					if srcFieldValue.Type() != dstFieldValue.Type() {
+						return extendPropertyErrorf(propertyName, "mismatched pointer types %s and %s",
+							dstFieldValue.Type(), srcFieldValue.Type())
+					}
+					break
+				} else if ptrKind != reflect.Struct {
+					return extendPropertyErrorf(propertyName, "pointer is a %s", ptrKind)
+				}
+
+				// Pointer to a struct
 				if dstFieldValue.IsNil() != srcFieldValue.IsNil() {
 					return extendPropertyErrorf(propertyName, "nilitude mismatch")
 				}
@@ -223,10 +239,6 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
 				dstFieldValue = dstFieldValue.Elem()
 				srcFieldValue = srcFieldValue.Elem()
 
-				if srcFieldValue.Kind() != reflect.Struct || dstFieldValue.Kind() != reflect.Struct {
-					return extendPropertyErrorf(propertyName, "pointer not to a struct")
-				}
-
 				fallthrough
 			case reflect.Struct:
 				if sameTypes && dstFieldValue.Type() != srcFieldValue.Type() {
@@ -293,6 +305,34 @@ func extendPropertiesRecursive(dstValues []reflect.Value, srcValue reflect.Value
 					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:
+					dstStr := ""
+					if !dstFieldValue.IsNil() {
+						dstStr = dstFieldValue.Elem().String()
+					}
+					if prepend {
+						dstFieldValue.Set(reflect.ValueOf(StringPtr(srcFieldValue.Elem().String() + dstStr)))
+					} else {
+						dstFieldValue.Set(reflect.ValueOf(StringPtr(dstStr + srcFieldValue.Elem().String())))
+					}
+				default:
+					panic(fmt.Errorf("unexpected pointer kind %s", ptrKind))
+				}
 			}
 		}
 		if !found {

proptools/extend_test.go

@@ -100,6 +100,114 @@ var appendPropertiesTestCases = []struct {
 		},
 		prepend: true,
 	},
+	{
+		// Append pointer to bool
+		in1: &struct{ B1, B2, B3, B4, B5, B6, B7, B8, B9 *bool }{
+			B1: BoolPtr(true),
+			B2: BoolPtr(false),
+			B3: nil,
+			B4: BoolPtr(true),
+			B5: BoolPtr(false),
+			B6: nil,
+			B7: BoolPtr(true),
+			B8: BoolPtr(false),
+			B9: nil,
+		},
+		in2: &struct{ B1, B2, B3, B4, B5, B6, B7, B8, B9 *bool }{
+			B1: nil,
+			B2: nil,
+			B3: nil,
+			B4: BoolPtr(true),
+			B5: BoolPtr(true),
+			B6: BoolPtr(true),
+			B7: BoolPtr(false),
+			B8: BoolPtr(false),
+			B9: BoolPtr(false),
+		},
+		out: &struct{ B1, B2, B3, B4, B5, B6, B7, B8, B9 *bool }{
+			B1: BoolPtr(true),
+			B2: BoolPtr(false),
+			B3: nil,
+			B4: BoolPtr(true),
+			B5: BoolPtr(true),
+			B6: BoolPtr(true),
+			B7: BoolPtr(false),
+			B8: BoolPtr(false),
+			B9: BoolPtr(false),
+		},
+	},
+	{
+		// Prepend pointer to bool
+		in1: &struct{ B1, B2, B3, B4, B5, B6, B7, B8, B9 *bool }{
+			B1: BoolPtr(true),
+			B2: BoolPtr(false),
+			B3: nil,
+			B4: BoolPtr(true),
+			B5: BoolPtr(false),
+			B6: nil,
+			B7: BoolPtr(true),
+			B8: BoolPtr(false),
+			B9: nil,
+		},
+		in2: &struct{ B1, B2, B3, B4, B5, B6, B7, B8, B9 *bool }{
+			B1: nil,
+			B2: nil,
+			B3: nil,
+			B4: BoolPtr(true),
+			B5: BoolPtr(true),
+			B6: BoolPtr(true),
+			B7: BoolPtr(false),
+			B8: BoolPtr(false),
+			B9: BoolPtr(false),
+		},
+		out: &struct{ B1, B2, B3, B4, B5, B6, B7, B8, B9 *bool }{
+			B1: BoolPtr(true),
+			B2: BoolPtr(false),
+			B3: nil,
+			B4: BoolPtr(true),
+			B5: BoolPtr(false),
+			B6: BoolPtr(true),
+			B7: BoolPtr(true),
+			B8: BoolPtr(false),
+			B9: BoolPtr(false),
+		},
+		prepend: true,
+	},
+	{
+		// Append pointer to strings
+		in1: &struct{ S1, S2, S3, S4 *string }{
+			S1: StringPtr("string1"),
+			S2: StringPtr("string2"),
+		},
+		in2: &struct{ S1, S2, S3, S4 *string }{
+			S1: StringPtr("string3"),
+			S3: StringPtr("string4"),
+		},
+		out: &struct{ S1, S2, S3, S4 *string }{
+			S1: StringPtr("string1string3"),
+			S2: StringPtr("string2"),
+			S3: StringPtr("string4"),
+			S4: nil,
+		},
+	},
+	{
+		// Prepend pointer to strings
+		in1: &struct{ S1, S2, S3, S4 *string }{
+			S1: StringPtr("string1"),
+			S2: StringPtr("string2"),
+		},
+		in2: &struct{ S1, S2, S3, S4 *string }{
+			S1: StringPtr("string3"),
+			S3: StringPtr("string4"),
+		},
+		out: &struct{ S1, S2, S3, S4 *string }{
+			S1: StringPtr("string3string1"),
+			S2: StringPtr("string2"),
+			S3: StringPtr("string4"),
+			S4: nil,
+		},
+		prepend: true,
+	},
 	{
 		// Append slice
 		in1: &struct{ S []string }{
@@ -439,7 +547,7 @@ var appendPropertiesTestCases = []struct {
 		out: &struct{ S *[]string }{
 			S: &[]string{"string1"},
 		},
-		err: extendPropertyErrorf("s", "pointer not to a struct"),
+		err: extendPropertyErrorf("s", "pointer is a slice"),
 	},
 	{
 		// Error in nested struct

proptools/proptools.go

@@ -49,3 +49,31 @@ func HasTag(field reflect.StructField, name, value string) bool {
 
 	return false
 }
+
+// BoolPtr returns a pointer to a new bool containing the given value.
+func BoolPtr(b bool) *bool {
+	return &b
+}
+
+// StringPtr returns a pointer to a new string containing the given value.
+func StringPtr(s string) *string {
+	return &s
+}
+
+// Bool takes a pointer to a bool and returns true iff the pointer is non-nil and points to a true
+// value.
+func Bool(b *bool) bool {
+	if b != nil {
+		return *b
+	}
+	return false
+}
+
+// String takes a pointer to a string and returns the value of the string if the pointer is non-nil,
+// or an empty string.
+func String(s *string) string {
+	if s != nil {
+		return *s
+	}
+	return ""
+}

unpack.go

@@ -161,15 +161,18 @@ func unpackStructValue(namePrefix string, structValue reflect.Value,
 			}
 			fallthrough
 		case reflect.Ptr:
+			switch ptrKind := fieldValue.Type().Elem().Kind(); ptrKind {
+			case reflect.Struct:
 				if fieldValue.IsNil() {
 					panic(fmt.Errorf("field %s contains a nil pointer",
 						field.Name))
 				}
 				fieldValue = fieldValue.Elem()
-			elemType := fieldValue.Type()
-			if elemType.Kind() != reflect.Struct {
-				panic(fmt.Errorf("field %s contains a non-struct pointer",
-					field.Name))
+			case reflect.Bool, reflect.String:
+				// Nothing
+			default:
+				panic(fmt.Errorf("field %s contains a pointer to %s",
+					field.Name, ptrKind))
 			}
 
 		case reflect.Int, reflect.Uint:
@@ -225,9 +228,19 @@ func unpackStructValue(namePrefix string, structValue reflect.Value,
 			newErrs = unpackString(fieldValue, packedProperty.property)
 		case reflect.Slice:
 			newErrs = unpackSlice(fieldValue, packedProperty.property)
-		case reflect.Ptr, reflect.Interface:
-			fieldValue = fieldValue.Elem()
-			fallthrough
+		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:
 			localFilterKey, localFilterValue := filterKey, filterValue
 			if k, v, err := HasFilter(field.Tag); err != nil {
@@ -248,6 +261,8 @@ 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 {

unpack_test.go

@@ -30,6 +30,24 @@ var validUnpackTestCases = []struct {
 	output interface{}
 	errs   []error
 }{
+	{`
+		m {
+			name: "abc",
+			blank: "",
+		}
+		`,
+		struct {
+			Name  *string
+			Blank *string
+			Unset *string
+		}{
+			Name:  proptools.StringPtr("abc"),
+			Blank: proptools.StringPtr(""),
+			Unset: nil,
+		},
+		nil,
+	},
+
 	{`
 		m {
 			name: "abc",
@@ -56,6 +74,24 @@ var validUnpackTestCases = []struct {
 		nil,
 	},
 
+	{`
+		m {
+			isGood: true,
+			isBad: false,
+		}
+		`,
+		struct {
+			IsGood *bool
+			IsBad  *bool
+			IsUgly *bool
+		}{
+			IsGood: proptools.BoolPtr(true),
+			IsBad:  proptools.BoolPtr(false),
+			IsUgly: nil,
+		},
+		nil,
+	},
+
 	{`
 		m {
 			stuff: ["asdf", "jkl;", "qwert",