From 290e675625d299900e124e6e28796038a74aa203 Mon Sep 17 00:00:00 2001 From: Usta Shrestha Date: Tue, 2 Aug 2022 10:51:08 -0400 Subject: [PATCH] Revert "Add support for maps in blueprint files." This reverts commit 42cb28f66e5b1d20324ec905d0d3e53f1a8f3c5a. Reason for revert: Dead code - map type properties in Module Change-Id: Ie944a311963cc54258cbc4ba3fc974882e5539ce --- 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, 44 insertions(+), 504 deletions(-) diff --git a/parser/ast.go b/parser/ast.go index a1ffa3b..89f7a39 100644 --- a/parser/ast.go +++ b/parser/ast.go @@ -107,29 +107,6 @@ 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. @@ -267,7 +244,6 @@ type Map struct { LBracePos scanner.Position RBracePos scanner.Position Properties []*Property - MapItems []*MapItem } func (x *Map) Pos() scanner.Position { return x.LBracePos } @@ -279,36 +255,20 @@ 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 { - 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, ", ") + propertyStrings := make([]string, len(x.Properties)) + for i, property := range x.Properties { + propertyStrings[i] = property.String() } - return fmt.Sprintf("@%s-%s{%s}", x.LBracePos, x.RBracePos, s) + return fmt.Sprintf("@%s-%s{%s}", x.LBracePos, x.RBracePos, + strings.Join(propertyStrings, ", ")) } func (x *Map) Type() Type { return MapType } diff --git a/parser/parser.go b/parser/parser.go index bb8817e..df694c5 100644 --- a/parser/parser.go +++ b/parser/parser.go @@ -301,37 +301,6 @@ 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) @@ -614,15 +583,7 @@ func (p *parser) parseMapValue() *Map { return nil } - 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() - } + properties := p.parsePropertyList(false, false) rBracePos := p.scanner.Position p.accept('}') @@ -631,7 +592,6 @@ 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 b32581e..4cc4e1b 100644 --- a/parser/parser_test.go +++ b/parser/parser_test.go @@ -194,121 +194,6 @@ 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 { @@ -1331,28 +1216,6 @@ 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 28a68b5..f6d9e95 100644 --- a/proptools/unpack.go +++ b/proptools/unpack.go @@ -27,12 +27,6 @@ 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 @@ -51,9 +45,8 @@ 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 - validMapProperties map[string]bool - errs []error + propertyMap map[string]*packedProperty + errs []error } // UnpackProperties populates the list of runtime values ("property structs") from the parsed properties. @@ -74,19 +67,11 @@ 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) @@ -153,33 +138,7 @@ 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 @@ -187,7 +146,7 @@ func (ctx *unpackContext) buildPropertyMap(prefix string, properties []*parser.P if len(propValue.Values) == 0 { continue } - if primitiveType(propValue.Values[0].Type()) { + if t := propValue.Values[0].Type(); t == parser.StringType || t == parser.Int64Type || t == parser.BoolType { continue } @@ -209,11 +168,6 @@ 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 @@ -265,15 +219,6 @@ 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)) @@ -354,13 +299,6 @@ 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) @@ -372,61 +310,6 @@ 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) { @@ -445,50 +328,11 @@ 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. - switch typ { + var getItemFunc func(*parser.Property, reflect.Type) (reflect.Value, bool) + switch exprs[0].Type() { case parser.BoolType, parser.StringType, parser.Int64Type: - return func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { + getItemFunc = func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { value, err := propertyToValue(t, property) if err != nil { ctx.addError(err) @@ -497,26 +341,46 @@ func (ctx *unpackContext) itemConstructor(typ parser.Type) constructItem { return value, true } case parser.ListType: - return func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { + getItemFunc = func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { return ctx.unpackToSlice(property.Name, property, t) } case parser.MapType: - 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 - } + getItemFunc = func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { + itemValue := reflect.New(t).Elem() + ctx.unpackToStruct(property.Name, itemValue) + return itemValue, true } case parser.NotEvaluatedType: - return func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { + getItemFunc = func(property *parser.Property, t reflect.Type) (reflect.Value, bool) { return reflect.New(t), false } default: - panic(fmt.Errorf("bizarre property expression type: %v", typ)) + panic(fmt.Errorf("bizarre property expression type: %v", exprs[0].Type())) } + + 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 5c6e3d0..7e2751d 100644 --- a/proptools/unpack_test.go +++ b/proptools/unpack_test.go @@ -128,82 +128,6 @@ 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: ` @@ -831,7 +755,7 @@ func TestUnpackProperties(t *testing.T) { } } - _, errs = unpackProperties(module.Properties, []string{"stuff", "empty", "nil", "nested.other_stuff"}, output...) + _, errs = UnpackProperties(module.Properties, output...) if len(errs) != 0 && len(testCase.errs) == 0 { t.Errorf("test case: %s", testCase.input) t.Errorf("unexpected unpack errors:") @@ -1038,37 +962,6 @@ 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 {