Merge pull request #278 from colincross/filtershard

Make FilterPropertyStructSharded smarter

bootstrap/bpdoc/bpdoc.go

@@ -145,14 +145,6 @@ func assembleModuleTypeInfo(r *Reader, name string, factory reflect.Value,
 				return nil, fmt.Errorf("nesting point %q not found", nestedName)
 			}
 
-			key, value, err := proptools.HasFilter(nestPoint.Tag)
-			if err != nil {
-				return nil, err
-			}
-			if key != "" {
-				nested.IncludeByTag(key, value)
-			}
-
 			nestPoint.Nest(nested)
 		}
 		mt.PropertyStructs = append(mt.PropertyStructs, ps)

proptools/filter.go

@@ -15,12 +15,60 @@
 package proptools
 
 import (
+	"fmt"
 	"reflect"
+	"strconv"
 )
 
 type FilterFieldPredicate func(field reflect.StructField, string string) (bool, reflect.StructField)
 
-func filterPropertyStructFields(fields []reflect.StructField, prefix string, predicate FilterFieldPredicate) (filteredFields []reflect.StructField, filtered bool) {
+type cantFitPanic struct {
+	field reflect.StructField
+	size  int
+}
+
+func (x cantFitPanic) Error() string {
+	return fmt.Sprintf("Can't fit field %s %s %s size %d into %d",
+		x.field.Name, x.field.Type.String(), strconv.Quote(string(x.field.Tag)),
+		fieldToTypeNameSize(x.field, true)+2, x.size)
+}
+
+// All runtime created structs will have a name that starts with "struct {" and ends with "}"
+const emptyStructTypeNameSize = len("struct {}")
+
+func filterPropertyStructFields(fields []reflect.StructField, prefix string, maxTypeNameSize int,
+	predicate FilterFieldPredicate) (filteredFieldsShards [][]reflect.StructField, filtered bool) {
+
+	structNameSize := emptyStructTypeNameSize
+
+	var filteredFields []reflect.StructField
+
+	appendAndShardIfNameFull := func(field reflect.StructField) {
+		fieldTypeNameSize := fieldToTypeNameSize(field, true)
+		// Every field will have a space before it and either a semicolon or space after it.
+		fieldTypeNameSize += 2
+
+		if maxTypeNameSize > 0 && structNameSize+fieldTypeNameSize > maxTypeNameSize {
+			if len(filteredFields) == 0 {
+				if field.Type.Kind() == reflect.Struct ||
+					field.Type.Kind() == reflect.Ptr && field.Type.Elem().Kind() == reflect.Struct {
+					// An error fitting the nested struct should have been caught when recursing
+					// into the nested struct.
+					panic(fmt.Errorf("Shouldn't happen: can't fit nested struct %q (%d) into %d",
+						field.Type.String(), len(field.Type.String()), maxTypeNameSize-structNameSize))
+				}
+				panic(cantFitPanic{field, maxTypeNameSize - structNameSize})
+
+			}
+			filteredFieldsShards = append(filteredFieldsShards, filteredFields)
+			filteredFields = nil
+			structNameSize = emptyStructTypeNameSize
+		}
+
+		filteredFields = append(filteredFields, field)
+		structNameSize += fieldTypeNameSize
+	}
+
 	for _, field := range fields {
 		var keep bool
 		if keep, field = predicate(field, prefix); !keep {
@@ -33,32 +81,61 @@ func filterPropertyStructFields(fields []reflect.StructField, prefix string, pre
 			subPrefix = prefix + "." + subPrefix
 		}
 
-		// Recurse into structs
-		switch field.Type.Kind() {
-		case reflect.Struct:
-			var subFiltered bool
-			field.Type, subFiltered = filterPropertyStruct(field.Type, subPrefix, predicate)
-			filtered = filtered || subFiltered
-			if field.Type == nil {
-				continue
-			}
-		case reflect.Ptr:
-			if field.Type.Elem().Kind() == reflect.Struct {
-				nestedType, subFiltered := filterPropertyStruct(field.Type.Elem(), subPrefix, predicate)
-				filtered = filtered || subFiltered
-				if nestedType == nil {
-					continue
-				}
-				field.Type = reflect.PtrTo(nestedType)
-			}
-		case reflect.Interface:
-			panic("Interfaces are not supported in filtered property structs")
+		ptrToStruct := false
+		if field.Type.Kind() == reflect.Ptr && field.Type.Elem().Kind() == reflect.Struct {
+			ptrToStruct = true
 		}
 
-		filteredFields = append(filteredFields, field)
+		// Recurse into structs
+		if ptrToStruct || field.Type.Kind() == reflect.Struct {
+			subMaxTypeNameSize := maxTypeNameSize
+			if maxTypeNameSize > 0 {
+				// In the worst case where only this nested struct will fit in the outer struct, the
+				// outer struct will contribute struct{}, the name and tag of the field that contains
+				// the nested struct, and one space before and after the field.
+				subMaxTypeNameSize -= emptyStructTypeNameSize + fieldToTypeNameSize(field, false) + 2
+			}
+			typ := field.Type
+			if ptrToStruct {
+				subMaxTypeNameSize -= len("*")
+				typ = typ.Elem()
+			}
+			nestedTypes, subFiltered := filterPropertyStruct(typ, subPrefix, subMaxTypeNameSize, predicate)
+			filtered = filtered || subFiltered
+			if nestedTypes == nil {
+				continue
+			}
+
+			for _, nestedType := range nestedTypes {
+				if ptrToStruct {
+					nestedType = reflect.PtrTo(nestedType)
+				}
+				field.Type = nestedType
+				appendAndShardIfNameFull(field)
+			}
+		} else {
+			appendAndShardIfNameFull(field)
+		}
 	}
 
-	return filteredFields, filtered
+	if len(filteredFields) > 0 {
+		filteredFieldsShards = append(filteredFieldsShards, filteredFields)
+	}
+
+	return filteredFieldsShards, filtered
+}
+
+func fieldToTypeNameSize(field reflect.StructField, withType bool) int {
+	nameSize := len(field.Name)
+	nameSize += len(" ")
+	if withType {
+		nameSize += len(field.Type.String())
+	}
+	if field.Tag != "" {
+		nameSize += len(" ")
+		nameSize += len(strconv.Quote(string(field.Tag)))
+	}
+	return nameSize
 }
 
 // FilterPropertyStruct takes a reflect.Type that is either a struct or a pointer to a struct, and returns a
@@ -66,10 +143,20 @@ func filterPropertyStructFields(fields []reflect.StructField, prefix string, pre
 // that is true if the new struct type has fewer fields than the original type.  If there are no fields in the
 // original type for which predicate returns true it returns nil and true.
 func FilterPropertyStruct(prop reflect.Type, predicate FilterFieldPredicate) (filteredProp reflect.Type, filtered bool) {
-	return filterPropertyStruct(prop, "", predicate)
+	filteredFieldsShards, filtered := filterPropertyStruct(prop, "", -1, predicate)
+	switch len(filteredFieldsShards) {
+	case 0:
+		return nil, filtered
+	case 1:
+		return filteredFieldsShards[0], filtered
+	default:
+		panic("filterPropertyStruct should only return 1 struct if maxNameSize < 0")
+	}
 }
 
-func filterPropertyStruct(prop reflect.Type, prefix string, predicate FilterFieldPredicate) (filteredProp reflect.Type, filtered bool) {
+func filterPropertyStruct(prop reflect.Type, prefix string, maxNameSize int,
+	predicate FilterFieldPredicate) (filteredProp []reflect.Type, filtered bool) {
+
 	var fields []reflect.StructField
 
 	ptr := prop.Kind() == reflect.Ptr
@@ -81,22 +168,26 @@ func filterPropertyStruct(prop reflect.Type, prefix string, predicate FilterFiel
 		fields = append(fields, prop.Field(i))
 	}
 
-	filteredFields, filtered := filterPropertyStructFields(fields, prefix, predicate)
+	filteredFieldsShards, filtered := filterPropertyStructFields(fields, prefix, maxNameSize, predicate)
 
-	if len(filteredFields) == 0 {
+	if len(filteredFieldsShards) == 0 {
 		return nil, true
 	}
 
 	if !filtered {
 		if ptr {
-			return reflect.PtrTo(prop), false
+			return []reflect.Type{reflect.PtrTo(prop)}, false
 		}
-		return prop, false
+		return []reflect.Type{prop}, false
 	}
 
-	ret := reflect.StructOf(filteredFields)
-	if ptr {
-		ret = reflect.PtrTo(ret)
+	var ret []reflect.Type
+	for _, filteredFields := range filteredFieldsShards {
+		p := reflect.StructOf(filteredFields)
+		if ptr {
+			p = reflect.PtrTo(p)
+		}
+		ret = append(ret, p)
 	}
 
 	return ret, true
@@ -109,51 +200,6 @@ func filterPropertyStruct(prop reflect.Type, prefix string, predicate FilterFiel
 // level fields in it to attempt to avoid hitting the 65535 byte type name length limit in reflect.StructOf
 // (reflect.nameFrom: name too long), although the limit can still be reached with a single struct field with many
 // fields in it.
-func FilterPropertyStructSharded(prop reflect.Type, predicate FilterFieldPredicate) (filteredProp []reflect.Type, filtered bool) {
-	var fields []reflect.StructField
-
-	ptr := prop.Kind() == reflect.Ptr
-	if ptr {
-		prop = prop.Elem()
-	}
-
-	for i := 0; i < prop.NumField(); i++ {
-		fields = append(fields, prop.Field(i))
-	}
-
-	fields, filtered = filterPropertyStructFields(fields, "", predicate)
-	if !filtered {
-		if ptr {
-			return []reflect.Type{reflect.PtrTo(prop)}, false
-		}
-		return []reflect.Type{prop}, false
-	}
-
-	if len(fields) == 0 {
-		return nil, true
-	}
-
-	shards := shardFields(fields, 10)
-
-	for _, shard := range shards {
-		s := reflect.StructOf(shard)
-		if ptr {
-			s = reflect.PtrTo(s)
-		}
-		filteredProp = append(filteredProp, s)
-	}
-
-	return filteredProp, true
-}
-
-func shardFields(fields []reflect.StructField, shardSize int) [][]reflect.StructField {
-	ret := make([][]reflect.StructField, 0, (len(fields)+shardSize-1)/shardSize)
-	for len(fields) > shardSize {
-		ret = append(ret, fields[0:shardSize])
-		fields = fields[shardSize:]
-	}
-	if len(fields) > 0 {
-		ret = append(ret, fields)
-	}
-	return ret
+func FilterPropertyStructSharded(prop reflect.Type, maxTypeNameSize int, predicate FilterFieldPredicate) (filteredProp []reflect.Type, filtered bool) {
+	return filterPropertyStruct(prop, "", maxTypeNameSize, predicate)
 }

proptools/filter_test.go

@@ -16,6 +16,7 @@ package proptools
 
 import (
 	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -237,3 +238,281 @@ func TestFilterPropertyStruct(t *testing.T) {
 		})
 	}
 }
+
+func TestFilterPropertyStructSharded(t *testing.T) {
+	tests := []struct {
+		name        string
+		maxNameSize int
+		in          interface{}
+		out         []interface{}
+		filtered    bool
+	}{
+		// Property tests
+		{
+			name:        "basic",
+			maxNameSize: 20,
+			in: &struct {
+				A *string `keep:"true"`
+				B *string `keep:"true"`
+				C *string
+			}{},
+			out: []interface{}{
+				&struct {
+					A *string
+				}{},
+				&struct {
+					B *string
+				}{},
+			},
+			filtered: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			out, filtered := filterPropertyStruct(reflect.TypeOf(test.in), "", test.maxNameSize,
+				func(field reflect.StructField, prefix string) (bool, reflect.StructField) {
+					if HasTag(field, "keep", "true") {
+						field.Tag = ""
+						return true, field
+					}
+					return false, field
+				})
+			if filtered != test.filtered {
+				t.Errorf("expected filtered %v, got %v", test.filtered, filtered)
+			}
+			var expected []reflect.Type
+			for _, t := range test.out {
+				expected = append(expected, reflect.TypeOf(t))
+			}
+			if !reflect.DeepEqual(out, expected) {
+				t.Errorf("expected type %v, got %v", expected, out)
+			}
+		})
+	}
+}
+
+func Test_fieldToTypeNameSize(t *testing.T) {
+	tests := []struct {
+		name  string
+		field reflect.StructField
+	}{
+		{
+			name: "string",
+			field: reflect.StructField{
+				Name: "Foo",
+				Type: reflect.TypeOf(""),
+			},
+		},
+		{
+			name: "string pointer",
+			field: reflect.StructField{
+				Name: "Foo",
+				Type: reflect.TypeOf(StringPtr("")),
+			},
+		},
+		{
+			name: "anonymous struct",
+			field: reflect.StructField{
+				Name: "Foo",
+				Type: reflect.TypeOf(struct{ foo string }{}),
+			},
+		}, {
+			name: "anonymous struct pointer",
+			field: reflect.StructField{
+				Name: "Foo",
+				Type: reflect.TypeOf(&struct{ foo string }{}),
+			},
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			typeName := reflect.StructOf([]reflect.StructField{test.field}).String()
+			typeName = strings.TrimPrefix(typeName, "struct { ")
+			typeName = strings.TrimSuffix(typeName, " }")
+			if g, w := fieldToTypeNameSize(test.field, true), len(typeName); g != w {
+				t.Errorf("want fieldToTypeNameSize(..., true) = %v, got %v", w, g)
+			}
+			if g, w := fieldToTypeNameSize(test.field, false), len(typeName)-len(test.field.Type.String()); g != w {
+				t.Errorf("want fieldToTypeNameSize(..., false) = %v, got %v", w, g)
+			}
+		})
+	}
+}
+
+func Test_filterPropertyStructFields(t *testing.T) {
+	type args struct {
+	}
+	tests := []struct {
+		name            string
+		maxTypeNameSize int
+		in              interface{}
+		out             []interface{}
+	}{
+		{
+			name:            "empty",
+			maxTypeNameSize: -1,
+			in:              struct{}{},
+			out:             nil,
+		},
+		{
+			name:            "one",
+			maxTypeNameSize: -1,
+			in: struct {
+				A *string
+			}{},
+			out: []interface{}{
+				struct {
+					A *string
+				}{},
+			},
+		},
+		{
+			name:            "two",
+			maxTypeNameSize: 20,
+			in: struct {
+				A *string
+				B *string
+			}{},
+			out: []interface{}{
+				struct {
+					A *string
+				}{},
+				struct {
+					B *string
+				}{},
+			},
+		},
+		{
+			name:            "nested",
+			maxTypeNameSize: 36,
+			in: struct {
+				AAAAA struct {
+					A string
+				}
+				BBBBB struct {
+					B string
+				}
+			}{},
+			out: []interface{}{
+				struct {
+					AAAAA struct {
+						A string
+					}
+				}{},
+				struct {
+					BBBBB struct {
+						B string
+					}
+				}{},
+			},
+		},
+		{
+			name:            "nested pointer",
+			maxTypeNameSize: 37,
+			in: struct {
+				AAAAA *struct {
+					A string
+				}
+				BBBBB *struct {
+					B string
+				}
+			}{},
+			out: []interface{}{
+				struct {
+					AAAAA *struct {
+						A string
+					}
+				}{},
+				struct {
+					BBBBB *struct {
+						B string
+					}
+				}{},
+			},
+		},
+		{
+			name:            "doubly nested",
+			maxTypeNameSize: 49,
+			in: struct {
+				AAAAA struct {
+					A struct {
+						A string
+					}
+				}
+				BBBBB struct {
+					B struct {
+						B string
+					}
+				}
+			}{},
+			out: []interface{}{
+				struct {
+					AAAAA struct {
+						A struct {
+							A string
+						}
+					}
+				}{},
+				struct {
+					BBBBB struct {
+						B struct {
+							B string
+						}
+					}
+				}{},
+			},
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			inType := reflect.TypeOf(test.in)
+			var in []reflect.StructField
+			for i := 0; i < inType.NumField(); i++ {
+				in = append(in, inType.Field(i))
+			}
+
+			keep := func(field reflect.StructField, string string) (bool, reflect.StructField) {
+				return true, field
+			}
+
+			// Test that maxTypeNameSize is the
+			if test.maxTypeNameSize > 0 {
+				correctPanic := false
+				func() {
+					defer func() {
+						if r := recover(); r != nil {
+							if _, ok := r.(cantFitPanic); ok {
+								correctPanic = true
+							} else {
+								panic(r)
+							}
+						}
+					}()
+
+					_, _ = filterPropertyStructFields(in, "", test.maxTypeNameSize-1, keep)
+				}()
+
+				if !correctPanic {
+					t.Errorf("filterPropertyStructFields() with size-1 should produce cantFitPanic")
+				}
+			}
+
+			filteredFieldsShards, _ := filterPropertyStructFields(in, "", test.maxTypeNameSize, keep)
+
+			var out []interface{}
+			for _, filteredFields := range filteredFieldsShards {
+				typ := reflect.StructOf(filteredFields)
+				if test.maxTypeNameSize > 0 && len(typ.String()) > test.maxTypeNameSize {
+					t.Errorf("out %q expected size <= %d, got %d",
+						typ.String(), test.maxTypeNameSize, len(typ.String()))
+				}
+				out = append(out, reflect.Zero(typ).Interface())
+			}
+
+			if g, w := out, test.out; !reflect.DeepEqual(g, w) {
+				t.Errorf("filterPropertyStructFields() want %v, got %v", w, g)
+			}
+		})
+	}
+}

proptools/unpack.go

@@ -17,8 +17,6 @@ package proptools
 import (
 	"fmt"
 	"reflect"
-	"strconv"
-	"strings"
 	"text/scanner"
 
 	"github.com/google/blueprint/parser"
@@ -60,7 +58,7 @@ func UnpackProperties(propertyDefs []*parser.Property,
 			panic("properties must be a pointer to a struct")
 		}
 
-		newErrs := unpackStructValue("", propertiesValue, propertyMap, "", "")
+		newErrs := unpackStructValue("", propertiesValue, propertyMap)
 		errs = append(errs, newErrs...)
 
 		if len(errs) >= maxUnpackErrors {
@@ -130,7 +128,7 @@ func buildPropertyMap(namePrefix string, propertyDefs []*parser.Property,
 }
 
 func unpackStructValue(namePrefix string, structValue reflect.Value,
-	propertyMap map[string]*packedProperty, filterKey, filterValue string) []error {
+	propertyMap map[string]*packedProperty) []error {
 
 	structType := structValue.Type()
 
@@ -215,7 +213,7 @@ func unpackStructValue(namePrefix string, structValue reflect.Value,
 		}
 
 		if field.Anonymous && fieldValue.Kind() == reflect.Struct {
-			newErrs := unpackStructValue(namePrefix, fieldValue, propertyMap, filterKey, filterValue)
+			newErrs := unpackStructValue(namePrefix, fieldValue, propertyMap)
 			errs = append(errs, newErrs...)
 			continue
 		}
@@ -239,40 +237,11 @@ func unpackStructValue(namePrefix string, structValue reflect.Value,
 			continue
 		}
 
-		if filterKey != "" && !HasTag(field, filterKey, filterValue) {
-			errs = append(errs,
-				&UnpackError{
-					Err: fmt.Errorf("filtered field %s cannot be set in a Blueprint file", propertyName),
-					Pos: packedProperty.property.ColonPos,
-				})
-			if len(errs) >= maxUnpackErrors {
-				return errs
-			}
-			continue
-		}
-
 		var newErrs []error
 
 		if fieldValue.Kind() == reflect.Struct {
-			localFilterKey, localFilterValue := filterKey, filterValue
-			if k, v, err := HasFilter(field.Tag); err != nil {
-				errs = append(errs, err)
-				if len(errs) >= maxUnpackErrors {
-					return errs
-				}
-			} else if k != "" {
-				if filterKey != "" {
-					errs = append(errs, fmt.Errorf("nested filter tag not supported on field %q",
-						field.Name))
-					if len(errs) >= maxUnpackErrors {
-						return errs
-					}
-				} else {
-					localFilterKey, localFilterValue = k, v
-				}
-			}
 			newErrs = unpackStruct(propertyName+".", fieldValue,
-				packedProperty.property, propertyMap, localFilterKey, localFilterValue)
+				packedProperty.property, propertyMap)
 
 			errs = append(errs, newErrs...)
 			if len(errs) >= maxUnpackErrors {
@@ -365,8 +334,7 @@ func propertyToValue(typ reflect.Type, property *parser.Property) (reflect.Value
 }
 
 func unpackStruct(namePrefix string, structValue reflect.Value,
-	property *parser.Property, propertyMap map[string]*packedProperty,
-	filterKey, filterValue string) []error {
+	property *parser.Property, propertyMap map[string]*packedProperty) []error {
 
 	m, ok := property.Value.Eval().(*parser.Map)
 	if !ok {
@@ -381,31 +349,5 @@ func unpackStruct(namePrefix string, structValue reflect.Value,
 		return errs
 	}
 
-	return unpackStructValue(namePrefix, structValue, propertyMap, filterKey, filterValue)
-}
-
-func HasFilter(field reflect.StructTag) (k, v string, err error) {
-	tag := field.Get("blueprint")
-	for _, entry := range strings.Split(tag, ",") {
-		if strings.HasPrefix(entry, "filter") {
-			if !strings.HasPrefix(entry, "filter(") || !strings.HasSuffix(entry, ")") {
-				return "", "", fmt.Errorf("unexpected format for filter %q: missing ()", entry)
-			}
-			entry = strings.TrimPrefix(entry, "filter(")
-			entry = strings.TrimSuffix(entry, ")")
-
-			s := strings.Split(entry, ":")
-			if len(s) != 2 {
-				return "", "", fmt.Errorf("unexpected format for filter %q: expected single ':'", entry)
-			}
-			k = s[0]
-			v, err = strconv.Unquote(s[1])
-			if err != nil {
-				return "", "", fmt.Errorf("unexpected format for filter %q: %s", entry, err.Error())
-			}
-			return k, v, nil
-		}
-	}
-
-	return "", "", nil
+	return unpackStructValue(namePrefix, structValue, propertyMap)
 }

proptools/unpack_test.go

@@ -16,7 +16,6 @@ package proptools
 
 import (
 	"bytes"
-	"fmt"
 	"reflect"
 	"testing"
 	"text/scanner"
@@ -219,39 +218,6 @@ var validUnpackTestCases = []struct {
 		},
 	},
 
-	{
-		input: `
-			m {
-				nested: {
-					foo: "abc",
-				},
-				bar: false,
-				baz: ["def", "ghi"],
-			}
-		`,
-		output: []interface{}{
-			struct {
-				Nested struct {
-					Foo string
-				} `blueprint:"filter(allowNested:\"true\")"`
-				Bar bool
-				Baz []string
-			}{
-				Nested: struct{ Foo string }{
-					Foo: "",
-				},
-				Bar: false,
-				Baz: []string{"def", "ghi"},
-			},
-		},
-		errs: []error{
-			&UnpackError{
-				Err: fmt.Errorf("filtered field nested.foo cannot be set in a Blueprint file"),
-				Pos: mkpos(30, 4, 9),
-			},
-		},
-	},
-
 	// Anonymous struct
 	{
 		input: `