From 021cc8f5b80c5e19a5087ec22ae80831ceb7c9ee Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Thu, 28 Mar 2024 16:20:12 -0700 Subject: [PATCH] Add support for unset select branches Currently, with the arch/os mutator, you can override a property using the default value for just a few arch types, for example: cc_defaults { name: "my_defaults", target: { windows: { enabled: true, } } } cc_binary { name: "foo", enabled: false, defaults: ["my_defaults"], } You could make a select statment that acts like the above if it were all in one module, but currently with select statements you can't make a defaults module that can be generically applied to any other module and have the same behavior as the above. After this cl, the defaults module could look like: cc_defaults { name: "my_defaults", enabled: select(variant("arch"), { "windows": true, _: unset, }), } Which would have the same behavior. Unset may also be useful for setting the property under some configurations, but wanting to leave the implementation-specific default value in others. Bug: 323382414 Test: m nothing --no-skip-soong-tests Change-Id: I3ea3277ea8b9a0ac5e613b4378945388b9df036a --- parser/ast.go | 63 ++++++++++++++++++++++++++---------- parser/parser.go | 58 ++++++++++++++++++++++++++++++--- parser/printer.go | 6 +++- parser/printer_test.go | 67 +++++++++++++++++++++++++++++++++++++++ proptools/configurable.go | 24 ++++++++------ proptools/extend_test.go | 16 +++++----- proptools/unpack.go | 62 ++++++++++++++++++++++++------------ proptools/unpack_test.go | 34 ++++++++++---------- 8 files changed, 252 insertions(+), 78 deletions(-) diff --git a/parser/ast.go b/parser/ast.go index cf8bb29..68f15ed 100644 --- a/parser/ast.go +++ b/parser/ast.go @@ -189,6 +189,7 @@ const ( ListType MapType NotEvaluatedType + UnsetType ) func (t Type) String() string { @@ -205,6 +206,8 @@ func (t Type) String() string { return "map" case NotEvaluatedType: return "notevaluated" + case UnsetType: + return "unset" default: panic(fmt.Errorf("Unknown type %d", t)) } @@ -596,13 +599,14 @@ func (s SelectType) String() string { } type Select struct { - KeywordPos scanner.Position // the keyword "select" - Typ SelectType - Condition String - LBracePos scanner.Position - RBracePos scanner.Position - Cases []*SelectCase // the case statements - Append Expression + KeywordPos scanner.Position // the keyword "select" + Typ SelectType + Condition String + LBracePos scanner.Position + RBracePos scanner.Position + Cases []*SelectCase // the case statements + Append Expression + ExpressionType Type } func (s *Select) Pos() scanner.Position { return s.KeywordPos } @@ -629,16 +633,10 @@ func (s *Select) String() string { } func (s *Select) Type() Type { - if len(s.Cases) == 0 { - panic("select with no cases") + if s.ExpressionType == UnsetType && s.Append != nil { + return s.Append.Type() } - ty := s.Cases[0].Value.Type() - for _, c := range s.Cases[1:] { - if c.Value.Type() != ty { - panic(fmt.Sprintf("Found select statement with differing types %q and %q in its cases", ty.String(), c.Value.Type().String())) - } - } - return ty + return s.ExpressionType } type SelectCase struct { @@ -660,3 +658,36 @@ func (c *SelectCase) String() string { func (c *SelectCase) Pos() scanner.Position { return c.Pattern.LiteralPos } func (c *SelectCase) End() scanner.Position { return c.Value.End() } + +// UnsetProperty is the expression type of the "unset" keyword that can be +// used in select statements to make the property unset. For example: +// +// my_module_type { +// name: "foo", +// some_prop: select(soong_config_variable("my_namespace", "my_var"), { +// "foo": unset, +// "default": "bar", +// }) +// } +type UnsetProperty struct { + Position scanner.Position +} + +func (n UnsetProperty) Copy() Expression { + return UnsetProperty{Position: n.Position} +} + +func (n UnsetProperty) String() string { + return "unset" +} + +func (n UnsetProperty) Type() Type { + return UnsetType +} + +func (n UnsetProperty) Eval() Expression { + return UnsetProperty{Position: n.Position} +} + +func (n UnsetProperty) Pos() scanner.Position { return n.Position } +func (n UnsetProperty) End() scanner.Position { return n.Position } diff --git a/parser/parser.go b/parser/parser.go index 31096f2..76cc654 100644 --- a/parser/parser.go +++ b/parser/parser.go @@ -289,7 +289,12 @@ func (p *parser) parseModule(typ string, typPos scanner.Position) *Module { func (p *parser) parsePropertyList(isModule, compat bool) (properties []*Property) { for p.tok == scanner.Ident { property := p.parseProperty(isModule, compat) - properties = append(properties, property) + + // If a property is set to an empty select or a select where all branches are "unset", + // skip emitting the property entirely. + if property.Value.Type() != UnsetType { + properties = append(properties, property) + } if p.tok != ',' { // There was no comma, so the list is done. @@ -350,7 +355,14 @@ func (p *parser) parseExpression() (value Expression) { } func (p *parser) evaluateOperator(value1, value2 Expression, operator rune, - pos scanner.Position) (*Operator, error) { + pos scanner.Position) (Expression, error) { + + if value1.Type() == UnsetType { + return value2, nil + } + if value2.Type() == UnsetType { + return value1, nil + } value := value1 @@ -454,7 +466,7 @@ func (p *parser) addMaps(map1, map2 []*Property, pos scanner.Position) ([]*Prope return ret, nil } -func (p *parser) parseOperator(value1 Expression) *Operator { +func (p *parser) parseOperator(value1 Expression) Expression { operator := p.tok pos := p.scanner.Position p.accept(operator) @@ -595,6 +607,7 @@ func (p *parser) parseSelect() Expression { return nil } + hasNonUnsetValue := false for p.tok == scanner.String { c := &SelectCase{} if s := p.parseStringValue(); s != nil { @@ -610,7 +623,13 @@ func (p *parser) parseSelect() Expression { if !p.accept(':') { return nil } - c.Value = p.parseExpression() + if p.tok == scanner.Ident && p.scanner.TokenText() == "unset" { + c.Value = UnsetProperty{Position: p.scanner.Position} + p.accept(scanner.Ident) + } else { + hasNonUnsetValue = true + c.Value = p.parseExpression() + } if !p.accept(',') { return nil } @@ -633,13 +652,42 @@ func (p *parser) parseSelect() Expression { if !p.accept(':') { return nil } - c.Value = p.parseExpression() + if p.tok == scanner.Ident && p.scanner.TokenText() == "unset" { + c.Value = UnsetProperty{Position: p.scanner.Position} + p.accept(scanner.Ident) + } else { + hasNonUnsetValue = true + c.Value = p.parseExpression() + } if !p.accept(',') { return nil } result.Cases = append(result.Cases, c) } + // If all branches have the value "unset", then this is equivalent + // to an empty select. + if !hasNonUnsetValue { + result.Typ = SelectTypeUnconfigured + result.Condition.Value = "" + result.Cases = nil + } + + ty := UnsetType + for _, c := range result.Cases { + otherTy := c.Value.Type() + // Any other type can override UnsetType + if ty == UnsetType { + ty = otherTy + } + if otherTy != UnsetType && otherTy != ty { + p.errorf("Found select statement with differing types %q and %q in its cases", ty.String(), otherTy.String()) + return nil + } + } + + result.ExpressionType = ty + result.RBracePos = p.scanner.Position if !p.accept('}') { return nil diff --git a/parser/printer.go b/parser/printer.go index 09a519d..47f8121 100644 --- a/parser/printer.go +++ b/parser/printer.go @@ -187,7 +187,11 @@ func (p *printer) printSelect(s *Select) { } p.printToken(":", c.ColonPos) p.requestSpace() - p.printExpression(c.Value) + if unset, ok := c.Value.(UnsetProperty); ok { + p.printToken(unset.String(), unset.Pos()) + } else { + p.printExpression(c.Value) + } p.printToken(",", c.Value.Pos()) } p.requestNewline() diff --git a/parser/printer_test.go b/parser/printer_test.go index 166bb3c..8aefa5d 100644 --- a/parser/printer_test.go +++ b/parser/printer_test.go @@ -635,6 +635,73 @@ foo { _: "f2", }), } +`, + }, + { + name: "Select with unset property", + input: ` +foo { + stuff: select(soong_config_variable("my_namespace", "my_variable"), { + "foo": unset, + _: "c2", + }), +} +`, + output: ` +foo { + stuff: select(soong_config_variable("my_namespace", "my_variable"), { + "foo": unset, + _: "c2", + }), +} +`, + }, + { + name: "Select with only unsets is removed", + input: ` +foo { + stuff: select(soong_config_variable("my_namespace", "my_variable"), { + "foo": unset, + _: unset, + }), +} +`, + output: ` +foo { + +} +`, + }, + { + name: "Additions of unset selects are removed", + input: ` +foo { + stuff: select(soong_config_variable("my_namespace", "my_variable"), { + "foo": "a", + _: "b", + }) + select(soong_config_variable("my_namespace", "my_variable2"), { + "foo": unset, + _: unset, + }) + select(soong_config_variable("my_namespace", "my_variable3"), { + "foo": "c", + _: "d", + }), +} +`, + // TODO(b/323382414): This is not good formatting, revisit later. + // But at least it removes the useless middle select + output: ` +foo { + stuff: select(soong_config_variable("my_namespace", "my_variable"), { + "foo": "a", + _: "b", + }) + + + select(soong_config_variable("my_namespace", "my_variable3"), { + "foo": "c", + _: "d", + }), +} `, }, } diff --git a/proptools/configurable.go b/proptools/configurable.go index 416614c..53cdf23 100644 --- a/proptools/configurable.go +++ b/proptools/configurable.go @@ -69,7 +69,7 @@ type Configurable[T ConfigurableElements] struct { propertyName string typ parser.SelectType condition string - cases map[string]T + cases map[string]*T appendWrapper *appendWrapper[T] } @@ -119,12 +119,12 @@ func (c *Configurable[T]) evaluateNonTransitive(evaluator ConfigurableEvaluator) } panic(fmt.Sprintf("Expected the single branch of an unconfigured select to be %q, got %q", default_select_branch_name, actual)) } - return &result + return result } val, defined := evaluator.EvaluateConfiguration(c.typ, c.propertyName, c.condition) if !defined { if result, ok := c.cases[default_select_branch_name]; ok { - return &result + return result } evaluator.PropertyErrorf(c.propertyName, "%s %q was not defined", c.typ.String(), c.condition) return nil @@ -133,10 +133,10 @@ func (c *Configurable[T]) evaluateNonTransitive(evaluator ConfigurableEvaluator) panic("Evaluator cannot return the default branch") } if result, ok := c.cases[val]; ok { - return &result + return result } if result, ok := c.cases[default_select_branch_name]; ok { - return &result + return result } evaluator.PropertyErrorf(c.propertyName, "%s %q had value %q, which was not handled by the select statement", c.typ.String(), c.condition, val) return nil @@ -212,7 +212,7 @@ func (c *Configurable[T]) initialize(propertyName string, typ parser.SelectType, c.propertyName = propertyName c.typ = typ c.condition = condition - c.cases = cases.(map[string]T) + c.cases = cases.(map[string]*T) c.appendWrapper = &appendWrapper[T]{} } @@ -251,7 +251,7 @@ func (c *Configurable[T]) clone() *Configurable[T] { } } - casesCopy := make(map[string]T, len(c.cases)) + casesCopy := make(map[string]*T, len(c.cases)) for k, v := range c.cases { casesCopy[k] = copyConfiguredValue(v) } @@ -265,10 +265,14 @@ func (c *Configurable[T]) clone() *Configurable[T] { } } -func copyConfiguredValue[T ConfigurableElements](t T) T { - switch t2 := any(t).(type) { +func copyConfiguredValue[T ConfigurableElements](t *T) *T { + if t == nil { + return nil + } + switch t2 := any(*t).(type) { case []string: - return any(slices.Clone(t2)).(T) + result := any(slices.Clone(t2)).(T) + return &result default: return t } diff --git a/proptools/extend_test.go b/proptools/extend_test.go index 61d3c40..498ce29 100644 --- a/proptools/extend_test.go +++ b/proptools/extend_test.go @@ -1262,7 +1262,7 @@ func appendPropertiesTestCases() []appendPropertyTestCase { S: Configurable[[]string]{ typ: parser.SelectTypeSoongConfigVariable, condition: "foo", - cases: map[string][]string{ + cases: map[string]*[]string{ "a": {"1", "2"}, }, appendWrapper: &appendWrapper[[]string]{}, @@ -1272,7 +1272,7 @@ func appendPropertiesTestCases() []appendPropertyTestCase { S: Configurable[[]string]{ typ: parser.SelectTypeReleaseVariable, condition: "bar", - cases: map[string][]string{ + cases: map[string]*[]string{ "b": {"3", "4"}, }, appendWrapper: &appendWrapper[[]string]{}, @@ -1282,14 +1282,14 @@ func appendPropertiesTestCases() []appendPropertyTestCase { S: Configurable[[]string]{ typ: parser.SelectTypeSoongConfigVariable, condition: "foo", - cases: map[string][]string{ + cases: map[string]*[]string{ "a": {"1", "2"}, }, appendWrapper: &appendWrapper[[]string]{ append: Configurable[[]string]{ typ: parser.SelectTypeReleaseVariable, condition: "bar", - cases: map[string][]string{ + cases: map[string]*[]string{ "b": {"3", "4"}, }, appendWrapper: &appendWrapper[[]string]{}, @@ -1305,7 +1305,7 @@ func appendPropertiesTestCases() []appendPropertyTestCase { S: Configurable[[]string]{ typ: parser.SelectTypeSoongConfigVariable, condition: "foo", - cases: map[string][]string{ + cases: map[string]*[]string{ "a": {"1", "2"}, }, appendWrapper: &appendWrapper[[]string]{}, @@ -1315,7 +1315,7 @@ func appendPropertiesTestCases() []appendPropertyTestCase { S: Configurable[[]string]{ typ: parser.SelectTypeReleaseVariable, condition: "bar", - cases: map[string][]string{ + cases: map[string]*[]string{ "b": {"3", "4"}, }, appendWrapper: &appendWrapper[[]string]{}, @@ -1325,14 +1325,14 @@ func appendPropertiesTestCases() []appendPropertyTestCase { S: Configurable[[]string]{ typ: parser.SelectTypeReleaseVariable, condition: "bar", - cases: map[string][]string{ + cases: map[string]*[]string{ "b": {"3", "4"}, }, appendWrapper: &appendWrapper[[]string]{ append: Configurable[[]string]{ typ: parser.SelectTypeSoongConfigVariable, condition: "foo", - cases: map[string][]string{ + cases: map[string]*[]string{ "a": {"1", "2"}, }, appendWrapper: &appendWrapper[[]string]{}, diff --git a/proptools/unpack.go b/proptools/unpack.go index faef56a..019bdc9 100644 --- a/proptools/unpack.go +++ b/proptools/unpack.go @@ -357,8 +357,8 @@ func (ctx *unpackContext) unpackToConfigurable(propertyName string, property *pa result := Configurable[string]{ propertyName: property.Name, typ: parser.SelectTypeUnconfigured, - cases: map[string]string{ - default_select_branch_name: v.Value, + cases: map[string]*string{ + default_select_branch_name: &v.Value, }, appendWrapper: &appendWrapper[string]{}, } @@ -375,8 +375,8 @@ func (ctx *unpackContext) unpackToConfigurable(propertyName string, property *pa result := Configurable[bool]{ propertyName: property.Name, typ: parser.SelectTypeUnconfigured, - cases: map[string]bool{ - default_select_branch_name: v.Value, + cases: map[string]*bool{ + default_select_branch_name: &v.Value, }, appendWrapper: &appendWrapper[bool]{}, } @@ -392,9 +392,9 @@ func (ctx *unpackContext) unpackToConfigurable(propertyName string, property *pa } switch configuredType.Elem().Kind() { case reflect.String: - var cases map[string][]string + var cases map[string]*[]string if v.Values != nil { - cases = map[string][]string{default_select_branch_name: make([]string, 0, len(v.Values))} + value := make([]string, 0, len(v.Values)) itemProperty := &parser.Property{NamePos: property.NamePos, ColonPos: property.ColonPos} for i, expr := range v.Values { itemProperty.Name = propertyName + "[" + strconv.Itoa(i) + "]" @@ -404,8 +404,9 @@ func (ctx *unpackContext) unpackToConfigurable(propertyName string, property *pa ctx.addError(err) return reflect.ValueOf(Configurable[[]string]{}), false } - cases[default_select_branch_name] = append(cases[default_select_branch_name], exprUnpacked.Interface().(string)) + value = append(value, exprUnpacked.Interface().(string)) } + cases = map[string]*[]string{default_select_branch_name: &value} } result := Configurable[[]string]{ propertyName: property.Name, @@ -423,16 +424,21 @@ func (ctx *unpackContext) unpackToConfigurable(propertyName string, property *pa case *parser.Select: resultPtr := reflect.New(configurableType) result := resultPtr.Elem() - cases := reflect.MakeMapWithSize(reflect.MapOf(reflect.TypeOf(""), configuredType), len(v.Cases)) + cases := reflect.MakeMapWithSize(reflect.MapOf(reflect.TypeOf(""), reflect.PointerTo(configuredType)), len(v.Cases)) for i, c := range v.Cases { + p := &parser.Property{ + Name: property.Name + "[" + strconv.Itoa(i) + "]", + NamePos: c.ColonPos, + Value: c.Value, + } + // Map the "unset" keyword to a nil pointer in the cases map + if _, ok := c.Value.(parser.UnsetProperty); ok { + cases.SetMapIndex(reflect.ValueOf(c.Pattern.Value), reflect.Zero(reflect.PointerTo(configuredType))) + continue + } switch configuredType.Kind() { case reflect.String, reflect.Bool: - p := &parser.Property{ - Name: property.Name + "[" + strconv.Itoa(i) + "]", - NamePos: c.ColonPos, - Value: c.Value, - } - val, err := propertyToValue(configuredType, p) + val, err := propertyToValue(reflect.PointerTo(configuredType), p) if err != nil { ctx.addError(&UnpackError{ err, @@ -445,12 +451,7 @@ func (ctx *unpackContext) unpackToConfigurable(propertyName string, property *pa if configuredType.Elem().Kind() != reflect.String { panic("This should be unreachable because ConfigurableElements only accepts slices of strings") } - p := &parser.Property{ - Name: property.Name + "[" + strconv.Itoa(i) + "]", - NamePos: c.ColonPos, - Value: c.Value, - } - val, ok := ctx.unpackToSlice(p.Name, p, configuredType) + val, ok := ctx.unpackToSlice(p.Name, p, reflect.PointerTo(configuredType)) if !ok { return reflect.New(configurableType), false } @@ -504,8 +505,27 @@ func (ctx *unpackContext) reportSelectOnNonConfigurablePropertyError( return true } -// unpackSlice creates a value of a given slice type from the property which should be a list +// unpackSlice creates a value of a given slice or pointer to 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) { + if sliceType.Kind() == reflect.Pointer { + sliceType = sliceType.Elem() + result := reflect.New(sliceType) + slice, ok := ctx.unpackToSliceInner(sliceName, property, sliceType) + if !ok { + return result, ok + } + result.Elem().Set(slice) + return result, true + } + return ctx.unpackToSliceInner(sliceName, property, sliceType) +} + +// unpackToSliceInner creates a value of a given slice type from the property, +// which should be a list. It doesn't support pointers to slice types like unpackToSlice +// does. +func (ctx *unpackContext) unpackToSliceInner( sliceName string, property *parser.Property, sliceType reflect.Type) (reflect.Value, bool) { propValueAsList, ok := property.Value.Eval().(*parser.List) if !ok { diff --git a/proptools/unpack_test.go b/proptools/unpack_test.go index b51ed7a..96cbfb9 100644 --- a/proptools/unpack_test.go +++ b/proptools/unpack_test.go @@ -735,8 +735,8 @@ var validUnpackTestCases = []struct { Foo: Configurable[string]{ propertyName: "foo", typ: parser.SelectTypeUnconfigured, - cases: map[string]string{ - default_select_branch_name: "bar", + cases: map[string]*string{ + default_select_branch_name: StringPtr("bar"), }, appendWrapper: &appendWrapper[string]{}, }, @@ -757,8 +757,8 @@ var validUnpackTestCases = []struct { Foo: Configurable[bool]{ propertyName: "foo", typ: parser.SelectTypeUnconfigured, - cases: map[string]bool{ - default_select_branch_name: true, + cases: map[string]*bool{ + default_select_branch_name: BoolPtr(true), }, appendWrapper: &appendWrapper[bool]{}, }, @@ -779,7 +779,7 @@ var validUnpackTestCases = []struct { Foo: Configurable[[]string]{ propertyName: "foo", typ: parser.SelectTypeUnconfigured, - cases: map[string][]string{ + cases: map[string]*[]string{ default_select_branch_name: {"a", "b"}, }, appendWrapper: &appendWrapper[[]string]{}, @@ -806,10 +806,10 @@ var validUnpackTestCases = []struct { propertyName: "foo", typ: parser.SelectTypeSoongConfigVariable, condition: "my_namespace:my_variable", - cases: map[string]string{ - "a": "a2", - "b": "b2", - default_select_branch_name: "c2", + cases: map[string]*string{ + "a": StringPtr("a2"), + "b": StringPtr("b2"), + default_select_branch_name: StringPtr("c2"), }, appendWrapper: &appendWrapper[string]{}, }, @@ -839,20 +839,20 @@ var validUnpackTestCases = []struct { propertyName: "foo", typ: parser.SelectTypeSoongConfigVariable, condition: "my_namespace:my_variable", - cases: map[string]string{ - "a": "a2", - "b": "b2", - default_select_branch_name: "c2", + cases: map[string]*string{ + "a": StringPtr("a2"), + "b": StringPtr("b2"), + default_select_branch_name: StringPtr("c2"), }, appendWrapper: &appendWrapper[string]{ append: Configurable[string]{ propertyName: "foo", typ: parser.SelectTypeSoongConfigVariable, condition: "my_namespace:my_2nd_variable", - cases: map[string]string{ - "d": "d2", - "e": "e2", - default_select_branch_name: "f2", + cases: map[string]*string{ + "d": StringPtr("d2"), + "e": StringPtr("e2"), + default_select_branch_name: StringPtr("f2"), }, appendWrapper: &appendWrapper[string]{}, },