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

@@ -50,15 +50,12 @@ const (
 	soongNsPrefix = "SOONG_CONFIG_"
 
 	// And here are the functions and variables:
-	cfnGetCfg          = baseName + ".cfg"
+	cfnGetCfg         = baseName + ".cfg"
-	cfnMain            = baseName + ".product_configuration"
+	cfnMain           = baseName + ".product_configuration"
-	cfnBoardMain       = baseName + ".board_configuration"
+	cfnBoardMain      = baseName + ".board_configuration"
-	cfnPrintVars       = baseName + ".printvars"
+	cfnPrintVars      = baseName + ".printvars"
-	cfnWarning         = baseName + ".warning"
+	cfnInherit        = baseName + ".inherit"
-	cfnLocalAppend     = baseName + ".local_append"
+	cfnSetListDefault = baseName + ".setdefault"
-	cfnLocalSetDefault = baseName + ".local_set_default"
-	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 = asgnAppend
-			asgn.flavor = asgnMaybeAppend
-		} else {
-			asgn.flavor = asgnAppend
-		}
 	case "?=":
 		asgn.flavor = asgnMaybeSet
 	default:

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"]
 `,
 	},
 	{

mk2rbc/node.go

@@ -184,10 +184,9 @@ type assignmentFlavor int
 
 const (
 	// Assignment flavors
-	asgnSet         assignmentFlavor = iota // := or =
+	asgnSet      assignmentFlavor = iota // := or =
-	asgnMaybeSet    assignmentFlavor = iota // ?= and variable may be unset
+	asgnMaybeSet assignmentFlavor = iota // ?=
-	asgnAppend      assignmentFlavor = iota // += and variable has been set before
+	asgnAppend   assignmentFlavor = iota // +=
-	asgnMaybeAppend assignmentFlavor = iota // += and variable may be unset
 )
 
 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
 }

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
+		if needsSetDefault {
-		asgn.value.transform(func(expr starlarkExpr) starlarkExpr {
-			if ref, ok := expr.(*variableRefExpr); ok && ref.ref.name() == pcv.name() {
-				isSelfReferential = true
-			}
-			return nil
-		})
-		if isSelfReferential {
 			emitSetDefault()
 		}
 		emitAssignment()
 	case asgnAppend:
-		emitAppend()
+		if needsSetDefault {
-	case asgnMaybeAppend:
+			emitSetDefault()
-		// If we are not sure variable has been assigned before, emit setdefault
+		}
-		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()
+		if needsSetDefault {
-	case asgnMaybeAppend:
+			gctx.writef("g.setdefault(%q, %s)", scv.name(), scv.defaultValueString())
-		// If we are not sure variable has been assigned before, emit setdefault
+			gctx.newLine()
-		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(")")
 	}
 }