From 42cb28f66e5b1d20324ec905d0d3e53f1a8f3c5a Mon Sep 17 00:00:00 2001 From: Liz Kammer Date: Fri, 21 May 2021 17:56:53 -0400 Subject: [PATCH] Add support for maps in blueprint files. This limits support to allow-listed property names to prevent proliferation of map types requiring additional support to migrate. Test: go test blueprint tests Test: m nothing && diff build.ninja & Android-aosp_arm.mk -- no changes Change-Id: Id12637462f19ac5de1b562f63507de989a51600d --- parser/ast.go | 50 +++++++++- parser/parser.go | 42 +++++++- parser/parser_test.go | 137 +++++++++++++++++++++++++ proptools/unpack.go | 210 ++++++++++++++++++++++++++++++++------- proptools/unpack_test.go | 109 +++++++++++++++++++- 5 files changed, 504 insertions(+), 44 deletions(-) diff --git a/parser/ast.go b/parser/ast.go index 8d8b10e..cb311ee 100644 --- a/parser/ast.go +++ b/parser/ast.go @@ -107,6 +107,29 @@ func (p *Property) String() string { func (p *Property) Pos() scanner.Position { return p.NamePos } func (p *Property) End() scanner.Position { return p.Value.End() } +// A MapItem is a key: value pair within a Map, corresponding to map type, rather than a struct. +type MapItem struct { + ColonPos scanner.Position + Key *String + Value Expression +} + +func (m *MapItem) Copy() *MapItem { + ret := MapItem{ + ColonPos: m.ColonPos, + Key: m.Key.Copy().(*String), + Value: m.Value.Copy(), + } + return &ret +} + +func (m *MapItem) String() string { + return fmt.Sprintf("%s@%s: %s", m.Key, m.ColonPos, m.Value) +} + +func (m *MapItem) Pos() scanner.Position { return m.Key.Pos() } +func (m *MapItem) End() scanner.Position { return m.Value.End() } + // An Expression is a Value in a Property or Assignment. It can be a literal (String or Bool), a // Map, a List, an Operator that combines two expressions of the same type, or a Variable that // references and Assignment. @@ -244,6 +267,7 @@ type Map struct { LBracePos scanner.Position RBracePos scanner.Position Properties []*Property + MapItems []*MapItem } func (x *Map) Pos() scanner.Position { return x.LBracePos } @@ -255,20 +279,36 @@ func (x *Map) Copy() Expression { for i := range x.Properties { ret.Properties[i] = x.Properties[i].Copy() } + ret.MapItems = make([]*MapItem, len(x.MapItems)) + for i := range x.MapItems { + ret.MapItems[i] = x.MapItems[i].Copy() + } return &ret } func (x *Map) Eval() Expression { + if len(x.Properties) > 0 && len(x.MapItems) > 0 { + panic("Cannot support both Properties and MapItems") + } return x } func (x *Map) String() string { - propertyStrings := make([]string, len(x.Properties)) - for i, property := range x.Properties { - propertyStrings[i] = property.String() + var s string + if len(x.MapItems) > 0 { + mapStrings := make([]string, len(x.MapItems)) + for i, mapItem := range x.MapItems { + mapStrings[i] = mapItem.String() + } + s = strings.Join(mapStrings, ", ") + } else { + propertyStrings := make([]string, len(x.Properties)) + for i, property := range x.Properties { + propertyStrings[i] = property.String() + } + s = strings.Join(propertyStrings, ", ") } - return fmt.Sprintf("@%s-%s{%s}", x.LBracePos, x.RBracePos, - strings.Join(propertyStrings, ", ")) + return fmt.Sprintf("@%s-%s{%s}", x.LBracePos, x.RBracePos, s) } func (x *Map) Type() Type { return MapType } diff --git a/parser/parser.go b/parser/parser.go index 9b6aa18..8d2b519 100644 --- a/parser/parser.go +++ b/parser/parser.go @@ -293,6 +293,37 @@ func (p *parser) parsePropertyList(isModule, compat bool) (properties []*Propert return } +func (p *parser) parseMapItemList() []*MapItem { + var items []*MapItem + // this is a map, not a struct, we only know we're at the end if we hit a '}' + for p.tok != '}' { + items = append(items, p.parseMapItem()) + + if p.tok != ',' { + // There was no comma, so the list is done. + break + } + p.accept(',') + } + return items +} + +func (p *parser) parseMapItem() *MapItem { + keyExpression := p.parseExpression() + if keyExpression.Type() != StringType { + p.errorf("only strings are supported as map keys: %s (%s)", keyExpression.Type(), keyExpression.String()) + } + key := keyExpression.(*String) + p.accept(':') + pos := p.scanner.Position + value := p.parseExpression() + return &MapItem{ + ColonPos: pos, + Key: key, + Value: value, + } +} + func (p *parser) parseProperty(isModule, compat bool) (property *Property) { property = new(Property) @@ -575,7 +606,15 @@ func (p *parser) parseMapValue() *Map { return nil } - properties := p.parsePropertyList(false, false) + var properties []*Property + var mapItems []*MapItem + // if the next item is an identifier, this is a property + if p.tok == scanner.Ident { + properties = p.parsePropertyList(false, false) + } else { + // otherwise, we assume that this is a map + mapItems = p.parseMapItemList() + } rBracePos := p.scanner.Position p.accept('}') @@ -584,6 +623,7 @@ func (p *parser) parseMapValue() *Map { LBracePos: lBracePos, RBracePos: rBracePos, Properties: properties, + MapItems: mapItems, } } diff --git a/parser/parser_test.go b/parser/parser_test.go index c9d284b..96ddefc 100644 --- a/parser/parser_test.go +++ b/parser/parser_test.go @@ -193,6 +193,121 @@ var validParseTestCases = []struct { nil, }, + {` + foo { + stuff: { + "key1": 1, + "key2": 2, + }, + } + `, + []Definition{ + &Module{ + Type: "foo", + TypePos: mkpos(3, 2, 3), + Map: Map{ + LBracePos: mkpos(7, 2, 7), + RBracePos: mkpos(59, 7, 3), + Properties: []*Property{ + { + Name: "stuff", + NamePos: mkpos(12, 3, 4), + ColonPos: mkpos(17, 3, 9), + Value: &Map{ + LBracePos: mkpos(19, 3, 11), + RBracePos: mkpos(54, 6, 4), + MapItems: []*MapItem{ + &MapItem{ + ColonPos: mkpos(33, 4, 13), + Key: &String{ + LiteralPos: mkpos(25, 4, 5), + Value: "key1", + }, + Value: &Int64{ + LiteralPos: mkpos(33, 4, 13), + Value: 1, + Token: "1", + }, + }, + &MapItem{ + ColonPos: mkpos(48, 5, 13), + Key: &String{ + LiteralPos: mkpos(40, 5, 5), + Value: "key2", + }, + Value: &Int64{ + LiteralPos: mkpos(48, 5, 13), + Value: 2, + Token: "2", + }, + }, + }, + }, + }, + }, + }, + }, + }, + nil, + }, + + {` + foo { + stuff: { + "key1": { + a: "abc", + }, + }, + } + `, + []Definition{ + &Module{ + Type: "foo", + TypePos: mkpos(3, 2, 3), + Map: Map{ + LBracePos: mkpos(7, 2, 7), + RBracePos: mkpos(65, 8, 3), + Properties: []*Property{ + { + Name: "stuff", + NamePos: mkpos(12, 3, 4), + ColonPos: mkpos(17, 3, 9), + Value: &Map{ + LBracePos: mkpos(19, 3, 11), + RBracePos: mkpos(60, 7, 4), + MapItems: []*MapItem{ + &MapItem{ + ColonPos: mkpos(33, 4, 13), + Key: &String{ + LiteralPos: mkpos(25, 4, 5), + Value: "key1", + }, + Value: &Map{ + LBracePos: mkpos(33, 4, 13), + RBracePos: mkpos(54, 6, 5), + Properties: []*Property{ + &Property{ + Name: "a", + NamePos: mkpos(40, 5, 6), + ColonPos: mkpos(41, 5, 7), + Value: &String{ + LiteralPos: mkpos(43, 5, 9), + Value: "abc", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + nil, + }, + { ` foo { @@ -1215,6 +1330,28 @@ func TestParseValidInput(t *testing.T) { // TODO: Test error strings +func TestMapParserError(t *testing.T) { + input := + ` + foo { + stuff: { + 1: "value1", + 2: "value2", + }, + } + ` + expectedErr := `:4:6: only strings are supported as map keys: int64 ('\x01'@:4:5)` + _, errs := ParseAndEval("", bytes.NewBufferString(input), NewScope(nil)) + if len(errs) == 0 { + t.Fatalf("Expected errors, got none.") + } + for _, err := range errs { + if expectedErr != err.Error() { + t.Errorf("Unexpected err: %s", err) + } + } +} + func TestParserEndPos(t *testing.T) { in := ` module { diff --git a/proptools/unpack.go b/proptools/unpack.go index f6d9e95..28a68b5 100644 --- a/proptools/unpack.go +++ b/proptools/unpack.go @@ -27,6 +27,12 @@ import ( const maxUnpackErrors = 10 +var ( + // Hard-coded list of allowlisted property names of type map. This is to limit use of maps to + // where absolutely necessary. + validMapProperties = []string{} +) + type UnpackError struct { Err error Pos scanner.Position @@ -45,8 +51,9 @@ type packedProperty struct { // unpackContext keeps compound names and their values in a map. It is initialized from // parsed properties. type unpackContext struct { - propertyMap map[string]*packedProperty - errs []error + propertyMap map[string]*packedProperty + validMapProperties map[string]bool + errs []error } // UnpackProperties populates the list of runtime values ("property structs") from the parsed properties. @@ -67,11 +74,19 @@ type unpackContext struct { // The same property can initialize fields in multiple runtime values. It is an error if any property // value was not used to initialize at least one field. func UnpackProperties(properties []*parser.Property, objects ...interface{}) (map[string]*parser.Property, []error) { + return unpackProperties(properties, validMapProperties, objects...) +} + +func unpackProperties(properties []*parser.Property, validMapProps []string, objects ...interface{}) (map[string]*parser.Property, []error) { var unpackContext unpackContext unpackContext.propertyMap = make(map[string]*packedProperty) if !unpackContext.buildPropertyMap("", properties) { return nil, unpackContext.errs } + unpackContext.validMapProperties = make(map[string]bool, len(validMapProps)) + for _, p := range validMapProps { + unpackContext.validMapProperties[p] = true + } for _, obj := range objects { valueObject := reflect.ValueOf(obj) @@ -138,7 +153,33 @@ func (ctx *unpackContext) buildPropertyMap(prefix string, properties []*parser.P ctx.propertyMap[name] = &packedProperty{property, false} switch propValue := property.Value.Eval().(type) { case *parser.Map: + // If this is a map and the values are not primitive types, we need to unroll it for further + // mapping. Keys are limited to string types. ctx.buildPropertyMap(name, propValue.Properties) + if len(propValue.MapItems) == 0 { + continue + } + items := propValue.MapItems + keysType := items[0].Key.Type() + valsAreBasic := primitiveType(items[0].Value.Type()) + if keysType != parser.StringType { + ctx.addError(&UnpackError{Err: fmt.Errorf("complex key types are unsupported: %s", keysType)}) + return false + } else if valsAreBasic { + continue + } + itemProperties := make([]*parser.Property, len(items), len(items)) + for i, item := range items { + itemProperties[i] = &parser.Property{ + Name: fmt.Sprintf("%s{value:%d}", property.Name, i), + NamePos: property.NamePos, + ColonPos: property.ColonPos, + Value: item.Value, + } + } + if !ctx.buildPropertyMap(prefix, itemProperties) { + return false + } case *parser.List: // If it is a list, unroll it unless its elements are of primitive type // (no further mapping will be needed in that case, so we avoid cluttering @@ -146,7 +187,7 @@ func (ctx *unpackContext) buildPropertyMap(prefix string, properties []*parser.P if len(propValue.Values) == 0 { continue } - if t := propValue.Values[0].Type(); t == parser.StringType || t == parser.Int64Type || t == parser.BoolType { + if primitiveType(propValue.Values[0].Type()) { continue } @@ -168,6 +209,11 @@ func (ctx *unpackContext) buildPropertyMap(prefix string, properties []*parser.P return len(ctx.errs) == nOldErrors } +// primitiveType returns whether typ is a primitive type +func primitiveType(typ parser.Type) bool { + return typ == parser.StringType || typ == parser.Int64Type || typ == parser.BoolType +} + func fieldPath(prefix, fieldName string) string { if prefix == "" { return fieldName @@ -219,6 +265,15 @@ func (ctx *unpackContext) unpackToStruct(namePrefix string, structValue reflect. switch kind := fieldValue.Kind(); kind { case reflect.Bool, reflect.String, reflect.Struct, reflect.Slice: // Do nothing + case reflect.Map: + // Restrict names of map properties that _can_ be set in bp files + if _, ok := ctx.validMapProperties[propertyName]; !ok { + if !HasTag(field, "blueprint", "mutated") { + ctx.addError(&UnpackError{ + Err: fmt.Errorf("Uses of maps for properties must be allowlisted. %q is an unsupported use case", propertyName), + }) + } + } case reflect.Interface: if fieldValue.IsNil() { panic(fmt.Errorf("field %s contains a nil interface", propertyName)) @@ -299,6 +354,13 @@ func (ctx *unpackContext) unpackToStruct(namePrefix string, structValue reflect. if len(ctx.errs) >= maxUnpackErrors { return } + } else if fieldValue.Type().Kind() == reflect.Map { + if unpackedValue, ok := ctx.unpackToMap(propertyName, property, fieldValue.Type()); ok { + ExtendBasicType(fieldValue, unpackedValue, Append) + } + if len(ctx.errs) >= maxUnpackErrors { + return + } } else { unpackedValue, err := propertyToValue(fieldValue.Type(), property) @@ -310,6 +372,61 @@ func (ctx *unpackContext) unpackToStruct(namePrefix string, structValue reflect. } } +// unpackToMap unpacks given parser.property into a go map of type mapType +func (ctx *unpackContext) unpackToMap(mapName string, property *parser.Property, mapType reflect.Type) (reflect.Value, bool) { + propValueAsMap, ok := property.Value.Eval().(*parser.Map) + // Verify this property is a map + if !ok { + ctx.addError(&UnpackError{ + fmt.Errorf("can't assign %q value to map property %q", property.Value.Type(), property.Name), + property.Value.Pos(), + }) + return reflect.MakeMap(mapType), false + } + // And is not a struct + if len(propValueAsMap.Properties) > 0 { + ctx.addError(&UnpackError{ + fmt.Errorf("can't assign property to a map (%s) property %q", property.Value.Type(), property.Name), + property.Value.Pos(), + }) + return reflect.MakeMap(mapType), false + } + + items := propValueAsMap.MapItems + m := reflect.MakeMap(mapType) + if len(items) == 0 { + return m, true + } + keyConstructor := ctx.itemConstructor(items[0].Key.Type()) + keyType := mapType.Key() + valueConstructor := ctx.itemConstructor(items[0].Value.Type()) + valueType := mapType.Elem() + + itemProperty := &parser.Property{NamePos: property.NamePos, ColonPos: property.ColonPos} + for i, item := range items { + itemProperty.Name = fmt.Sprintf("%s{key:%d}", mapName, i) + itemProperty.Value = item.Key + if packedProperty, ok := ctx.propertyMap[itemProperty.Name]; ok { + packedProperty.used = true + } + keyValue, ok := itemValue(keyConstructor, itemProperty, keyType) + if !ok { + continue + } + itemProperty.Name = fmt.Sprintf("%s{value:%d}", mapName, i) + itemProperty.Value = item.Value + if packedProperty, ok := ctx.propertyMap[itemProperty.Name]; ok { + packedProperty.used = true + } + value, ok := itemValue(valueConstructor, itemProperty, valueType) + if ok { + m.SetMapIndex(keyValue, value) + } + } + + return m, true +} + // unpackSlice creates a value of a given slice type from the property which should be a list func (ctx *unpackContext) unpackToSlice( sliceName string, property *parser.Property, sliceType reflect.Type) (reflect.Value, bool) { @@ -328,11 +445,50 @@ func (ctx *unpackContext) unpackToSlice( return value, true } + itemConstructor := ctx.itemConstructor(exprs[0].Type()) + itemType := sliceType.Elem() + + itemProperty := &parser.Property{NamePos: property.NamePos, ColonPos: property.ColonPos} + for i, expr := range exprs { + itemProperty.Name = sliceName + "[" + strconv.Itoa(i) + "]" + itemProperty.Value = expr + if packedProperty, ok := ctx.propertyMap[itemProperty.Name]; ok { + packedProperty.used = true + } + if itemValue, ok := itemValue(itemConstructor, itemProperty, itemType); ok { + value = reflect.Append(value, itemValue) + } + } + return value, true +} + +// constructItem is a function to construct a reflect.Value from given parser.Property of reflect.Type +type constructItem func(*parser.Property, reflect.Type) (reflect.Value, bool) + +// itemValue creates a new item of type t with value determined by f +func itemValue(f constructItem, property *parser.Property, t reflect.Type) (reflect.Value, bool) { + isPtr := t.Kind() == reflect.Ptr + if isPtr { + t = t.Elem() + } + val, ok := f(property, t) + if !ok { + return val, ok + } + if isPtr { + ptrValue := reflect.New(val.Type()) + ptrValue.Elem().Set(val) + return ptrValue, true + } + return val, true +} + +// itemConstructor returns a function to construct an item of typ +func (ctx *unpackContext) itemConstructor(typ parser.Type) constructItem { // The function to construct an item value depends on the type of list elements. - var getItemFunc func(*parser.Property, reflect.Type) (reflect.Value, bool) - switch exprs[0].Type() { + switch typ { case parser.BoolType, parser.StringType, parser.Int64Type: - getItemFunc = func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { + return func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { value, err := propertyToValue(t, property) if err != nil { ctx.addError(err) @@ -341,46 +497,26 @@ func (ctx *unpackContext) unpackToSlice( return value, true } case parser.ListType: - getItemFunc = func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { + return func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { return ctx.unpackToSlice(property.Name, property, t) } case parser.MapType: - getItemFunc = func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { - itemValue := reflect.New(t).Elem() - ctx.unpackToStruct(property.Name, itemValue) - return itemValue, true + return func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { + if t.Kind() == reflect.Map { + return ctx.unpackToMap(property.Name, property, t) + } else { + itemValue := reflect.New(t).Elem() + ctx.unpackToStruct(property.Name, itemValue) + return itemValue, true + } } case parser.NotEvaluatedType: - getItemFunc = func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { + return func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { return reflect.New(t), false } default: - panic(fmt.Errorf("bizarre property expression type: %v", exprs[0].Type())) + panic(fmt.Errorf("bizarre property expression type: %v", typ)) } - - itemProperty := &parser.Property{NamePos: property.NamePos, ColonPos: property.ColonPos} - elemType := sliceType.Elem() - isPtr := elemType.Kind() == reflect.Ptr - - for i, expr := range exprs { - itemProperty.Name = sliceName + "[" + strconv.Itoa(i) + "]" - itemProperty.Value = expr - if packedProperty, ok := ctx.propertyMap[itemProperty.Name]; ok { - packedProperty.used = true - } - if isPtr { - if itemValue, ok := getItemFunc(itemProperty, elemType.Elem()); ok { - ptrValue := reflect.New(itemValue.Type()) - ptrValue.Elem().Set(itemValue) - value = reflect.Append(value, ptrValue) - } - } else { - if itemValue, ok := getItemFunc(itemProperty, elemType); ok { - value = reflect.Append(value, itemValue) - } - } - } - return value, true } // propertyToValue creates a value of a given value type from the property. diff --git a/proptools/unpack_test.go b/proptools/unpack_test.go index 7e2751d..5c6e3d0 100644 --- a/proptools/unpack_test.go +++ b/proptools/unpack_test.go @@ -128,6 +128,82 @@ var validUnpackTestCases = []struct { }, }, + { + name: "map", + input: ` + m { + stuff: { "asdf": "jkl;", "qwert": "uiop"}, + empty: {}, + nested: { + other_stuff: {}, + }, + } + `, + output: []interface{}{ + &struct { + Stuff map[string]string + Empty map[string]string + Nil map[string]string + NonString map[string]struct{ S string } `blueprint:"mutated"` + Nested struct { + Other_stuff map[string]string + } + }{ + Stuff: map[string]string{"asdf": "jkl;", "qwert": "uiop"}, + Empty: map[string]string{}, + Nil: nil, + NonString: nil, + Nested: struct{ Other_stuff map[string]string }{ + Other_stuff: map[string]string{}, + }, + }, + }, + }, + + { + name: "map with slice", + input: ` + m { + stuff: { "asdf": ["jkl;"], "qwert": []}, + empty: {}, + } + `, + output: []interface{}{ + &struct { + Stuff map[string][]string + Empty map[string][]string + Nil map[string][]string + NonString map[string]struct{ S string } `blueprint:"mutated"` + }{ + Stuff: map[string][]string{"asdf": []string{"jkl;"}, "qwert": []string{}}, + Empty: map[string][]string{}, + Nil: nil, + NonString: nil, + }, + }, + }, + + { + name: "map with struct", + input: ` + m { + stuff: { "asdf": {s:"a"}}, + empty: {}, + } + `, + output: []interface{}{ + &struct { + Stuff map[string]struct{ S string } + Empty map[string]struct{ S string } + Nil map[string]struct{ S string } + }{ + Stuff: map[string]struct{ S string }{"asdf": struct{ S string }{"a"}}, + Empty: map[string]struct{ S string }{}, + Nil: nil, + }, + }, + }, + { name: "double nested", input: ` @@ -755,7 +831,7 @@ func TestUnpackProperties(t *testing.T) { } } - _, errs = UnpackProperties(module.Properties, output...) + _, errs = unpackProperties(module.Properties, []string{"stuff", "empty", "nil", "nested.other_stuff"}, output...) if len(errs) != 0 && len(testCase.errs) == 0 { t.Errorf("test case: %s", testCase.input) t.Errorf("unexpected unpack errors:") @@ -962,6 +1038,37 @@ func TestUnpackErrors(t *testing.T) { `:3:16: can't assign string value to list property "map_list"`, }, }, + { + name: "invalid use of maps", + input: ` + m { + map: {"foo": "bar"}, + } + `, + output: []interface{}{ + &struct { + Map map[string]string + }{}, + }, + errors: []string{ + `: Uses of maps for properties must be allowlisted. "map" is an unsupported use case`, + }, + }, + { + name: "invalid use of maps, not used in bp file", + input: ` + m { + } + `, + output: []interface{}{ + &struct { + Map map[string]string + }{}, + }, + errors: []string{ + `: Uses of maps for properties must be allowlisted. "map" is an unsupported use case`, + }, + }, } for _, testCase := range testCases {