From e2a37988ffa9577b3b60d2612d07d10421b31eb7 Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Wed, 9 Mar 2022 16:00:17 -0800 Subject: [PATCH] Simplify and correct variable assignments - Remove asgnMaybeAppend, it was only used to indicate that the asignment needs a setDefault. But really, all types of variable assignments need setDefault if they're self-referential. - Remove local_append/local_set_default because there was no implementation for them written in product_config.rbc anyways. - Correct productConfigVariable.emitDefined using the global variables instead of the product config variables. - Emit setDefaults for all types of assignments if they're self referential. Bug: 222737841 Test: go test Change-Id: I06a0f90f16d5900049d473281e6d5ef5e93e67da --- mk2rbc/mk2rbc.go | 21 ++++++----------- mk2rbc/mk2rbc_test.go | 37 +++++++++++++++++++++++++++++ mk2rbc/node.go | 21 +++++++++++++---- mk2rbc/variable.go | 54 +++++++++++++++++++++---------------------- 4 files changed, 88 insertions(+), 45 deletions(-) diff --git a/mk2rbc/mk2rbc.go b/mk2rbc/mk2rbc.go index d108a0d04..d8b88b2cd 100644 --- a/mk2rbc/mk2rbc.go +++ b/mk2rbc/mk2rbc.go @@ -50,15 +50,12 @@ const ( soongNsPrefix = "SOONG_CONFIG_" // And here are the functions and variables: - cfnGetCfg = baseName + ".cfg" - cfnMain = baseName + ".product_configuration" - cfnBoardMain = baseName + ".board_configuration" - cfnPrintVars = baseName + ".printvars" - cfnWarning = baseName + ".warning" - cfnLocalAppend = baseName + ".local_append" - cfnLocalSetDefault = baseName + ".local_set_default" - cfnInherit = baseName + ".inherit" - cfnSetListDefault = baseName + ".setdefault" + cfnGetCfg = baseName + ".cfg" + cfnMain = baseName + ".product_configuration" + cfnBoardMain = baseName + ".board_configuration" + cfnPrintVars = baseName + ".printvars" + cfnInherit = baseName + ".inherit" + cfnSetListDefault = baseName + ".setdefault" ) const ( @@ -571,11 +568,7 @@ func (ctx *parseContext) handleAssignment(a *mkparser.Assignment) []starlarkNode case "=", ":=": asgn.flavor = asgnSet case "+=": - if asgn.previous == nil && !asgn.lhs.isPreset() { - asgn.flavor = asgnMaybeAppend - } else { - asgn.flavor = asgnAppend - } + asgn.flavor = asgnAppend case "?=": asgn.flavor = asgnMaybeSet default: diff --git a/mk2rbc/mk2rbc_test.go b/mk2rbc/mk2rbc_test.go index 556dcaa0c..35c54d268 100644 --- a/mk2rbc/mk2rbc_test.go +++ b/mk2rbc/mk2rbc_test.go @@ -898,6 +898,43 @@ def init(g, handle): cfg["PRODUCT_LIST1"] += ["c"] rblf.setdefault(handle, "PRODUCT_LIST2") cfg["PRODUCT_LIST2"] += ["c"] +`, + }, + { + desc: "assigment setdefaults", + mkname: "product.mk", + in: ` +# All of these should have a setdefault because they're self-referential and not defined before +PRODUCT_LIST1 = a $(PRODUCT_LIST1) +PRODUCT_LIST2 ?= a $(PRODUCT_LIST2) +PRODUCT_LIST3 += a + +# Now doing them again should not have a setdefault because they've already been set +PRODUCT_LIST1 = a $(PRODUCT_LIST1) +PRODUCT_LIST2 ?= a $(PRODUCT_LIST2) +PRODUCT_LIST3 += a +`, + expected: `# All of these should have a setdefault because they're self-referential and not defined before +load("//build/make/core:product_config.rbc", "rblf") + +def init(g, handle): + cfg = rblf.cfg(handle) + rblf.setdefault(handle, "PRODUCT_LIST1") + cfg["PRODUCT_LIST1"] = (["a"] + + cfg.get("PRODUCT_LIST1", [])) + if cfg.get("PRODUCT_LIST2") == None: + rblf.setdefault(handle, "PRODUCT_LIST2") + cfg["PRODUCT_LIST2"] = (["a"] + + cfg.get("PRODUCT_LIST2", [])) + rblf.setdefault(handle, "PRODUCT_LIST3") + cfg["PRODUCT_LIST3"] += ["a"] + # Now doing them again should not have a setdefault because they've already been set + cfg["PRODUCT_LIST1"] = (["a"] + + cfg["PRODUCT_LIST1"]) + if cfg.get("PRODUCT_LIST2") == None: + cfg["PRODUCT_LIST2"] = (["a"] + + cfg["PRODUCT_LIST2"]) + cfg["PRODUCT_LIST3"] += ["a"] `, }, { diff --git a/mk2rbc/node.go b/mk2rbc/node.go index 5d98d7bc1..9d5af91c4 100644 --- a/mk2rbc/node.go +++ b/mk2rbc/node.go @@ -184,10 +184,9 @@ type assignmentFlavor int const ( // Assignment flavors - asgnSet assignmentFlavor = iota // := or = - asgnMaybeSet assignmentFlavor = iota // ?= and variable may be unset - asgnAppend assignmentFlavor = iota // += and variable has been set before - asgnMaybeAppend assignmentFlavor = iota // += and variable may be unset + asgnSet assignmentFlavor = iota // := or = + asgnMaybeSet assignmentFlavor = iota // ?= + asgnAppend assignmentFlavor = iota // += ) type assignmentNode struct { @@ -215,6 +214,20 @@ func (asgn *assignmentNode) emit(gctx *generationContext) { } } +func (asgn *assignmentNode) isSelfReferential() bool { + if asgn.flavor == asgnAppend { + return true + } + isSelfReferential := false + asgn.value.transform(func(expr starlarkExpr) starlarkExpr { + if ref, ok := expr.(*variableRefExpr); ok && ref.ref.name() == asgn.lhs.name() { + isSelfReferential = true + } + return nil + }) + return isSelfReferential +} + type exprNode struct { expr starlarkExpr } diff --git a/mk2rbc/variable.go b/mk2rbc/variable.go index 6805744a7..3241a3854 100644 --- a/mk2rbc/variable.go +++ b/mk2rbc/variable.go @@ -97,29 +97,27 @@ func (pcv productConfigVariable) emitSet(gctx *generationContext, asgn *assignme gctx.newLine() } + // If we are not sure variable has been assigned before, emit setdefault + needsSetDefault := asgn.previous == nil && !pcv.isPreset() && asgn.isSelfReferential() + switch asgn.flavor { case asgnSet: - isSelfReferential := false - asgn.value.transform(func(expr starlarkExpr) starlarkExpr { - if ref, ok := expr.(*variableRefExpr); ok && ref.ref.name() == pcv.name() { - isSelfReferential = true - } - return nil - }) - if isSelfReferential { + if needsSetDefault { emitSetDefault() } emitAssignment() case asgnAppend: - emitAppend() - case asgnMaybeAppend: - // If we are not sure variable has been assigned before, emit setdefault - emitSetDefault() + if needsSetDefault { + emitSetDefault() + } emitAppend() case asgnMaybeSet: gctx.writef("if cfg.get(%q) == None:", pcv.nam) gctx.indentLevel++ gctx.newLine() + if needsSetDefault { + emitSetDefault() + } emitAssignment() gctx.indentLevel-- } @@ -134,7 +132,7 @@ func (pcv productConfigVariable) emitGet(gctx *generationContext, isDefined bool } func (pcv productConfigVariable) emitDefined(gctx *generationContext) { - gctx.writef("g.get(%q) != None", pcv.name()) + gctx.writef("cfg.get(%q) != None", pcv.name()) } type otherGlobalVariable struct { @@ -159,20 +157,30 @@ func (scv otherGlobalVariable) emitSet(gctx *generationContext, asgn *assignment value.emit(gctx) } + // If we are not sure variable has been assigned before, emit setdefault + needsSetDefault := asgn.previous == nil && !scv.isPreset() && asgn.isSelfReferential() + switch asgn.flavor { case asgnSet: + if needsSetDefault { + gctx.writef("g.setdefault(%q, %s)", scv.name(), scv.defaultValueString()) + gctx.newLine() + } emitAssignment() case asgnAppend: - emitAppend() - case asgnMaybeAppend: - // If we are not sure variable has been assigned before, emit setdefault - gctx.writef("g.setdefault(%q, %s)", scv.name(), scv.defaultValueString()) - gctx.newLine() + if needsSetDefault { + gctx.writef("g.setdefault(%q, %s)", scv.name(), scv.defaultValueString()) + gctx.newLine() + } emitAppend() case asgnMaybeSet: gctx.writef("if g.get(%q) == None:", scv.nam) gctx.indentLevel++ gctx.newLine() + if needsSetDefault { + gctx.writef("g.setdefault(%q, %s)", scv.name(), scv.defaultValueString()) + gctx.newLine() + } emitAssignment() gctx.indentLevel-- } @@ -204,7 +212,7 @@ func (lv localVariable) String() string { func (lv localVariable) emitSet(gctx *generationContext, asgn *assignmentNode) { switch asgn.flavor { - case asgnSet: + case asgnSet, asgnMaybeSet: gctx.writef("%s = ", lv) asgn.value.emitListVarCopy(gctx) case asgnAppend: @@ -216,14 +224,6 @@ func (lv localVariable) emitSet(gctx *generationContext, asgn *assignmentNode) { value = &toStringExpr{expr: value} } value.emit(gctx) - case asgnMaybeAppend: - gctx.writef("%s(%q, ", cfnLocalAppend, lv) - asgn.value.emit(gctx) - gctx.write(")") - case asgnMaybeSet: - gctx.writef("%s(%q, ", cfnLocalSetDefault, lv) - asgn.value.emit(gctx) - gctx.write(")") } }