From f92c9f2809465b826fc4a9b4b7414c3ed13b2b07 Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Mon, 14 Mar 2022 14:35:50 -0700 Subject: [PATCH] Add type hints to mk2rbc Type hints have the format #RBC# type_hint MY_VAR list and must be specified at the top of the Makefile. Setting one will cause that variable to have that type for the remainder of the Makefile. This can be used where mk2rbc's type inference detects the wrong type and it must be manually changed. Bug: 224601891 Test: go test Change-Id: I6db2c50056d0298227e1d2801a522adf8bbd1df8 --- mk2rbc/mk2rbc.go | 42 +++++++++++++++++++++++++++++++++++++- mk2rbc/mk2rbc_test.go | 47 +++++++++++++++++++++++++++++++++++++++++++ mk2rbc/variable.go | 14 +++++++++---- 3 files changed, 98 insertions(+), 5 deletions(-) diff --git a/mk2rbc/mk2rbc.go b/mk2rbc/mk2rbc.go index d108a0d04..245750adf 100644 --- a/mk2rbc/mk2rbc.go +++ b/mk2rbc/mk2rbc.go @@ -410,6 +410,8 @@ type parseContext struct { dependentModules map[string]*moduleInfo soongNamespaces map[string]map[string]bool includeTops []string + typeHints map[string]starlarkType + atTopOfMakefile bool } func newParseContext(ss *StarlarkScript, nodes []mkparser.Node) *parseContext { @@ -453,6 +455,8 @@ func newParseContext(ss *StarlarkScript, nodes []mkparser.Node) *parseContext { dependentModules: make(map[string]*moduleInfo), soongNamespaces: make(map[string]map[string]bool), includeTops: []string{}, + typeHints: make(map[string]starlarkType), + atTopOfMakefile: true, } ctx.pushVarAssignments() for _, item := range predefined { @@ -1687,7 +1691,8 @@ func (ctx *parseContext) handleSimpleStatement(node mkparser.Node) []starlarkNod // Clear the includeTops after each non-comment statement // so that include annotations placed on certain statements don't apply // globally for the rest of the makefile was well. - if _, wasComment := node.(*mkparser.Comment); !wasComment && len(ctx.includeTops) > 0 { + if _, wasComment := node.(*mkparser.Comment); !wasComment { + ctx.atTopOfMakefile = false ctx.includeTops = []string{} } @@ -1697,6 +1702,12 @@ func (ctx *parseContext) handleSimpleStatement(node mkparser.Node) []starlarkNod return result } +// The types allowed in a type_hint +var typeHintMap = map[string]starlarkType{ + "string": starlarkTypeString, + "list": starlarkTypeList, +} + // Processes annotation. An annotation is a comment that starts with #RBC# and provides // a conversion hint -- say, where to look for the dynamically calculated inherit/include // paths. Returns true if the comment was a successfully-handled annotation. @@ -1721,6 +1732,35 @@ func (ctx *parseContext) maybeHandleAnnotation(cnode *mkparser.Comment) (starlar } ctx.includeTops = append(ctx.includeTops, p) return nil, true + } else if p, ok := maybeTrim(annotation, "type_hint"); ok { + // Type hints must come at the beginning the file, to avoid confusion + // if a type hint was specified later and thus only takes effect for half + // of the file. + if !ctx.atTopOfMakefile { + return ctx.newBadNode(cnode, "type_hint annotations must come before the first Makefile statement"), true + } + + parts := strings.Fields(p) + if len(parts) <= 1 { + return ctx.newBadNode(cnode, "Invalid type_hint annotation: %s. Must be a variable type followed by a list of variables of that type", p), true + } + + var varType starlarkType + if varType, ok = typeHintMap[parts[0]]; !ok { + varType = starlarkTypeUnknown + } + if varType == starlarkTypeUnknown { + return ctx.newBadNode(cnode, "Invalid type_hint annotation. Only list/string types are accepted, found %s", parts[0]), true + } + + for _, name := range parts[1:] { + // Don't allow duplicate type hints + if _, ok := ctx.typeHints[name]; ok { + return ctx.newBadNode(cnode, "Duplicate type hint for variable %s", name), true + } + ctx.typeHints[name] = varType + } + return nil, true } return ctx.newBadNode(cnode, "unsupported annotation %s", cnode.Comment), true } diff --git a/mk2rbc/mk2rbc_test.go b/mk2rbc/mk2rbc_test.go index 556dcaa0c..fc78d474c 100644 --- a/mk2rbc/mk2rbc_test.go +++ b/mk2rbc/mk2rbc_test.go @@ -1367,6 +1367,53 @@ def init(g, handle): pass if int("100%s" % g.get("MY_VAR", "")) >= 10: pass +`, + }, + { + desc: "Type hints", + mkname: "product.mk", + in: ` +# Test type hints +#RBC# type_hint list MY_VAR MY_VAR_2 +# Unsupported type +#RBC# type_hint bool MY_VAR_3 +# Invalid syntax +#RBC# type_hint list +# Duplicated variable +#RBC# type_hint list MY_VAR_2 +#RBC# type_hint list my-local-var-with-dashes + +MY_VAR := foo +MY_VAR_UNHINTED := foo + +# Vars set after other statements still get the hint +MY_VAR_2 := foo + +# You can't specify a type hint after the first statement +#RBC# type_hint list MY_VAR_4 +MY_VAR_4 := foo + +my-local-var-with-dashes := foo +`, + expected: `# Test type hints +# Unsupported type +load("//build/make/core:product_config.rbc", "rblf") + +def init(g, handle): + cfg = rblf.cfg(handle) + rblf.mk2rbc_error("product.mk:5", "Invalid type_hint annotation. Only list/string types are accepted, found bool") + # Invalid syntax + rblf.mk2rbc_error("product.mk:7", "Invalid type_hint annotation: list. Must be a variable type followed by a list of variables of that type") + # Duplicated variable + rblf.mk2rbc_error("product.mk:9", "Duplicate type hint for variable MY_VAR_2") + g["MY_VAR"] = ["foo"] + g["MY_VAR_UNHINTED"] = "foo" + # Vars set after other statements still get the hint + g["MY_VAR_2"] = ["foo"] + # You can't specify a type hint after the first statement + rblf.mk2rbc_error("product.mk:19", "type_hint annotations must come before the first Makefile statement") + g["MY_VAR_4"] = "foo" + _my_local_var_with_dashes = ["foo"] `, }, } diff --git a/mk2rbc/variable.go b/mk2rbc/variable.go index 6805744a7..69297b94d 100644 --- a/mk2rbc/variable.go +++ b/mk2rbc/variable.go @@ -291,6 +291,12 @@ var presetVariables = map[string]bool{ // addVariable returns a variable with a given name. A variable is // added if it does not exist yet. func (ctx *parseContext) addVariable(name string) variable { + // Get the hintType before potentially changing the variable name + var hintType starlarkType + var ok bool + if hintType, ok = ctx.typeHints[name]; !ok { + hintType = starlarkTypeUnknown + } // Heuristics: if variable's name is all lowercase, consider it local // string variable. isLocalVariable := name == strings.ToLower(name) @@ -301,8 +307,8 @@ func (ctx *parseContext) addVariable(name string) variable { } v, found := ctx.variables[name] if !found { - _, preset := presetVariables[name] if vi, found := KnownVariables[name]; found { + _, preset := presetVariables[name] switch vi.class { case VarClassConfig: v = &productConfigVariable{baseVariable{nam: name, typ: vi.valueType, preset: preset}} @@ -310,10 +316,10 @@ func (ctx *parseContext) addVariable(name string) variable { v = &otherGlobalVariable{baseVariable{nam: name, typ: vi.valueType, preset: preset}} } } else if isLocalVariable { - v = &localVariable{baseVariable{nam: name, typ: starlarkTypeUnknown}} + v = &localVariable{baseVariable{nam: name, typ: hintType}} } else { - vt := starlarkTypeUnknown - if strings.HasPrefix(name, "LOCAL_") { + vt := hintType + if strings.HasPrefix(name, "LOCAL_") && vt == starlarkTypeUnknown { // Heuristics: local variables that contribute to corresponding config variables if cfgVarName, found := localProductConfigVariables[name]; found { vi, found2 := KnownVariables[cfgVarName]