From f83202143a0aabdc3937720cd87d9f61948bff9e Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Wed, 10 Nov 2021 15:05:07 -0800 Subject: [PATCH] Allow generic if statements Previously, only comparisons between strings/variable references and other strings/variable references or certain function calls were allowed. After this cl, any two starlark expressions can be compared. If they have different types, they will be converted to strings first. Fixes: 205995738 Bug: 201700692 Test: go test Change-Id: Ib463de7b24d3abcd032dbdcba7c3f1351c314d01 --- mk2rbc/expr.go | 51 +++++++++ mk2rbc/mk2rbc.go | 247 +++++++++++++++++++++--------------------- mk2rbc/mk2rbc_test.go | 30 +++++ 3 files changed, 204 insertions(+), 124 deletions(-) diff --git a/mk2rbc/expr.go b/mk2rbc/expr.go index 0bb8b9530..8279d2e17 100644 --- a/mk2rbc/expr.go +++ b/mk2rbc/expr.go @@ -197,6 +197,51 @@ func (v *variableRefExpr) emitListVarCopy(gctx *generationContext) { } } +type toStringExpr struct { + expr starlarkExpr +} + +func (s *toStringExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) { + if x, same := s.expr.eval(valueMap); same { + res = s + } else { + res = &toStringExpr{expr: x} + } + return +} + +func (s *toStringExpr) emit(ctx *generationContext) { + switch s.expr.typ() { + case starlarkTypeString, starlarkTypeUnknown: + // Assume unknown types are strings already. + s.expr.emit(ctx) + case starlarkTypeList: + ctx.write(`" ".join(`) + s.expr.emit(ctx) + ctx.write(")") + case starlarkTypeInt: + ctx.write(`("%d" % (`) + s.expr.emit(ctx) + ctx.write("))") + case starlarkTypeBool: + ctx.write("((") + s.expr.emit(ctx) + ctx.write(`) ? "true" : "")`) + case starlarkTypeVoid: + ctx.write(`""`) + default: + panic("Unknown starlark type!") + } +} + +func (s *toStringExpr) typ() starlarkType { + return starlarkTypeString +} + +func (s *toStringExpr) emitListVarCopy(gctx *generationContext) { + s.emit(gctx) +} + type notExpr struct { expr starlarkExpr } @@ -255,6 +300,12 @@ func (eq *eqExpr) emit(gctx *generationContext) { return } + + if eq.left.typ() != eq.right.typ() { + eq.left = &toStringExpr{expr: eq.left} + eq.right = &toStringExpr{expr: eq.right} + } + // General case eq.left.emit(gctx) if eq.isEq { diff --git a/mk2rbc/mk2rbc.go b/mk2rbc/mk2rbc.go index 7ce183495..b87ab4171 100644 --- a/mk2rbc/mk2rbc.go +++ b/mk2rbc/mk2rbc.go @@ -1044,48 +1044,54 @@ func (ctx *parseContext) parseCompare(cond *mkparser.Directive) starlarkExpr { args[1].TrimLeftSpaces() isEq := !strings.HasSuffix(cond.Name, "neq") - switch xLeft := ctx.parseMakeString(cond, args[0]).(type) { - case *stringLiteralExpr, *variableRefExpr: - switch xRight := ctx.parseMakeString(cond, args[1]).(type) { - case *stringLiteralExpr, *variableRefExpr: - return &eqExpr{left: xLeft, right: xRight, isEq: isEq} - case *badExpr: - return xRight - default: - expr, ok := ctx.parseCheckFunctionCallResult(cond, xLeft, args[1]) - if ok { - return expr - } - return ctx.newBadExpr(cond, "right operand is too complex: %s", args[1].Dump()) - } - case *badExpr: - return xLeft - default: - switch xRight := ctx.parseMakeString(cond, args[1]).(type) { - case *stringLiteralExpr, *variableRefExpr: - expr, ok := ctx.parseCheckFunctionCallResult(cond, xRight, args[0]) - if ok { - return expr - } - return ctx.newBadExpr(cond, "left operand is too complex: %s", args[0].Dump()) - case *badExpr: - return xRight - default: - return ctx.newBadExpr(cond, "operands are too complex: (%s,%s)", args[0].Dump(), args[1].Dump()) - } + xLeft := ctx.parseMakeString(cond, args[0]) + xRight := ctx.parseMakeString(cond, args[1]) + if bad, ok := xLeft.(*badExpr); ok { + return bad } + if bad, ok := xRight.(*badExpr); ok { + return bad + } + + if expr, ok := ctx.parseCompareSpecialCases(cond, xLeft, xRight); ok { + return expr + } + + return &eqExpr{left: xLeft, right: xRight, isEq: isEq} } -func (ctx *parseContext) parseCheckFunctionCallResult(directive *mkparser.Directive, xValue starlarkExpr, - varArg *mkparser.MakeString) (starlarkExpr, bool) { - mkSingleVar, ok := varArg.SingleVariable() - if !ok { +// Given an if statement's directive and the left/right starlarkExprs, +// check if the starlarkExprs are one of a few hardcoded special cases +// that can be converted to a simpler equalify expression than simply comparing +// the two. +func (ctx *parseContext) parseCompareSpecialCases(directive *mkparser.Directive, left starlarkExpr, + right starlarkExpr) (starlarkExpr, bool) { + isEq := !strings.HasSuffix(directive.Name, "neq") + + // All the special cases require a call on one side and a + // string literal/variable on the other. Turn the left/right variables into + // call/value variables, and return false if that's not possible. + var value starlarkExpr = nil + call, ok := left.(*callExpr) + if ok { + switch right.(type) { + case *stringLiteralExpr, *variableRefExpr: + value = right + } + } else { + call, _ = right.(*callExpr) + switch left.(type) { + case *stringLiteralExpr, *variableRefExpr: + value = left + } + } + + if call == nil || value == nil { return nil, false } - expr := ctx.parseReference(directive, mkSingleVar) - negate := strings.HasSuffix(directive.Name, "neq") + checkIsSomethingFunction := func(xCall *callExpr) starlarkExpr { - s, ok := maybeString(xValue) + s, ok := maybeString(value) if !ok || s != "true" { return ctx.newBadExpr(directive, fmt.Sprintf("the result of %s can be compared only to 'true'", xCall.name)) @@ -1095,97 +1101,90 @@ func (ctx *parseContext) parseCheckFunctionCallResult(directive *mkparser.Direct } return nil } - switch x := expr.(type) { - case *callExpr: - switch x.name { - case "filter": - return ctx.parseCompareFilterFuncResult(directive, x, xValue, !negate), true - case "filter-out": - return ctx.parseCompareFilterFuncResult(directive, x, xValue, negate), true - case "wildcard": - return ctx.parseCompareWildcardFuncResult(directive, x, xValue, negate), true - case "findstring": - return ctx.parseCheckFindstringFuncResult(directive, x, xValue, negate), true - case "strip": - return ctx.parseCompareStripFuncResult(directive, x, xValue, negate), true - case "is-board-platform": - if xBad := checkIsSomethingFunction(x); xBad != nil { - return xBad, true - } - return &eqExpr{ - left: &variableRefExpr{ctx.addVariable("TARGET_BOARD_PLATFORM"), false}, - right: x.args[0], - isEq: !negate, - }, true - case "is-board-platform-in-list": - if xBad := checkIsSomethingFunction(x); xBad != nil { - return xBad, true - } - return &inExpr{ - expr: &variableRefExpr{ctx.addVariable("TARGET_BOARD_PLATFORM"), false}, - list: maybeConvertToStringList(x.args[0]), - isNot: negate, - }, true - case "is-product-in-list": - if xBad := checkIsSomethingFunction(x); xBad != nil { - return xBad, true - } - return &inExpr{ - expr: &variableRefExpr{ctx.addVariable("TARGET_PRODUCT"), true}, - list: maybeConvertToStringList(x.args[0]), - isNot: negate, - }, true - case "is-vendor-board-platform": - if xBad := checkIsSomethingFunction(x); xBad != nil { - return xBad, true - } - s, ok := maybeString(x.args[0]) - if !ok { - return ctx.newBadExpr(directive, "cannot handle non-constant argument to is-vendor-board-platform"), true - } - return &inExpr{ - expr: &variableRefExpr{ctx.addVariable("TARGET_BOARD_PLATFORM"), false}, - list: &variableRefExpr{ctx.addVariable(s + "_BOARD_PLATFORMS"), true}, - isNot: negate, - }, true - case "is-board-platform2", "is-board-platform-in-list2": - if s, ok := maybeString(xValue); !ok || s != "" { - return ctx.newBadExpr(directive, - fmt.Sprintf("the result of %s can be compared only to empty", x.name)), true - } - if len(x.args) != 1 { - return ctx.newBadExpr(directive, "%s requires an argument", x.name), true - } - cc := &callExpr{ - name: x.name, - args: []starlarkExpr{x.args[0]}, - returnType: starlarkTypeBool, - } - if !negate { - return ¬Expr{cc}, true - } - return cc, true - case "is-vendor-board-qcom": - if s, ok := maybeString(xValue); !ok || s != "" { - return ctx.newBadExpr(directive, - fmt.Sprintf("the result of %s can be compared only to empty", x.name)), true - } - // if the expression is ifneq (,$(call is-vendor-board-platform,...)), negate==true, - // so we should set inExpr.isNot to false - return &inExpr{ - expr: &variableRefExpr{ctx.addVariable("TARGET_BOARD_PLATFORM"), false}, - list: &variableRefExpr{ctx.addVariable("QCOM_BOARD_PLATFORMS"), true}, - isNot: !negate, - }, true - default: - return ctx.newBadExpr(directive, "Unknown function in ifeq: %s", x.name), true + switch call.name { + case "filter": + return ctx.parseCompareFilterFuncResult(directive, call, value, isEq), true + case "filter-out": + return ctx.parseCompareFilterFuncResult(directive, call, value, !isEq), true + case "wildcard": + return ctx.parseCompareWildcardFuncResult(directive, call, value, !isEq), true + case "findstring": + return ctx.parseCheckFindstringFuncResult(directive, call, value, !isEq), true + case "strip": + return ctx.parseCompareStripFuncResult(directive, call, value, !isEq), true + case "is-board-platform": + if xBad := checkIsSomethingFunction(call); xBad != nil { + return xBad, true } - case *badExpr: - return x, true - default: - return nil, false + return &eqExpr{ + left: &variableRefExpr{ctx.addVariable("TARGET_BOARD_PLATFORM"), false}, + right: call.args[0], + isEq: isEq, + }, true + case "is-board-platform-in-list": + if xBad := checkIsSomethingFunction(call); xBad != nil { + return xBad, true + } + return &inExpr{ + expr: &variableRefExpr{ctx.addVariable("TARGET_BOARD_PLATFORM"), false}, + list: maybeConvertToStringList(call.args[0]), + isNot: !isEq, + }, true + case "is-product-in-list": + if xBad := checkIsSomethingFunction(call); xBad != nil { + return xBad, true + } + return &inExpr{ + expr: &variableRefExpr{ctx.addVariable("TARGET_PRODUCT"), true}, + list: maybeConvertToStringList(call.args[0]), + isNot: !isEq, + }, true + case "is-vendor-board-platform": + if xBad := checkIsSomethingFunction(call); xBad != nil { + return xBad, true + } + s, ok := maybeString(call.args[0]) + if !ok { + return ctx.newBadExpr(directive, "cannot handle non-constant argument to is-vendor-board-platform"), true + } + return &inExpr{ + expr: &variableRefExpr{ctx.addVariable("TARGET_BOARD_PLATFORM"), false}, + list: &variableRefExpr{ctx.addVariable(s + "_BOARD_PLATFORMS"), true}, + isNot: !isEq, + }, true + + case "is-board-platform2", "is-board-platform-in-list2": + if s, ok := maybeString(value); !ok || s != "" { + return ctx.newBadExpr(directive, + fmt.Sprintf("the result of %s can be compared only to empty", call.name)), true + } + if len(call.args) != 1 { + return ctx.newBadExpr(directive, "%s requires an argument", call.name), true + } + cc := &callExpr{ + name: call.name, + args: []starlarkExpr{call.args[0]}, + returnType: starlarkTypeBool, + } + if isEq { + return ¬Expr{cc}, true + } + return cc, true + case "is-vendor-board-qcom": + if s, ok := maybeString(value); !ok || s != "" { + return ctx.newBadExpr(directive, + fmt.Sprintf("the result of %s can be compared only to empty", call.name)), true + } + // if the expression is ifneq (,$(call is-vendor-board-platform,...)), negate==true, + // so we should set inExpr.isNot to false + return &inExpr{ + expr: &variableRefExpr{ctx.addVariable("TARGET_BOARD_PLATFORM"), false}, + list: &variableRefExpr{ctx.addVariable("QCOM_BOARD_PLATFORMS"), true}, + isNot: isEq, + }, true } + return nil, false } func (ctx *parseContext) parseCompareFilterFuncResult(cond *mkparser.Directive, diff --git a/mk2rbc/mk2rbc_test.go b/mk2rbc/mk2rbc_test.go index 1e79552cd..c2c46543e 100644 --- a/mk2rbc/mk2rbc_test.go +++ b/mk2rbc/mk2rbc_test.go @@ -350,6 +350,21 @@ def init(g, handle): cfg["PRODUCT_MODEL"] = "pix21" if "aosp_x86" != g["TARGET_PRODUCT"]: cfg["PRODUCT_MODEL"] = "pix3" +`, + }, + { + desc: "ifeq with soong_config_get", + mkname: "product.mk", + in: ` +ifeq (true,$(call soong_config_get,art_module,source_build)) +endif +`, + expected: `load("//build/make/core:product_config.rbc", "rblf") + +def init(g, handle): + cfg = rblf.cfg(handle) + if "true" == rblf.soong_config_get(g, "art_module", "source_build"): + pass `, }, { @@ -512,6 +527,21 @@ def init(g, handle): pass if rblf.file_wildcard_exists("foo*.mk"): pass +`, + }, + { + desc: "if with interpolation", + mkname: "product.mk", + in: ` +ifeq ($(VARIABLE1)text$(VARIABLE2),true) +endif +`, + expected: `load("//build/make/core:product_config.rbc", "rblf") + +def init(g, handle): + cfg = rblf.cfg(handle) + if "%stext%s" % (g.get("VARIABLE1", ""), g.get("VARIABLE2", "")) == "true": + pass `, }, {