Remove starlarkExpr.Eval()

It was only used to substitute variable references to
predefined variables with the predefined value, which
is an easy condition to directly parse into instead
of having a separate evalutation pass.

Bug: 201700692
Test: go test
Change-Id: I543d20a1d6435bfabd9faa90ffb09af3084ed28c

mk2rbc/expr.go

@@ -16,18 +16,13 @@ package mk2rbc
 
 import (
 	"fmt"
-	"strconv"
 	"strings"
 )
 
-// Represents an expression in the Starlark code. An expression has
+// Represents an expression in the Starlark code. An expression has a type.
-// a type, and it can be evaluated.
 type starlarkExpr interface {
 	starlarkNode
 	typ() starlarkType
-	// Try to substitute variable values. Return substitution result
-	// and whether it is the same as the original expression.
-	eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool)
 	// Emit the code to copy the expression, otherwise we will end up
 	// with source and target pointing to the same list.
 	emitListVarCopy(gctx *generationContext)
@@ -50,12 +45,6 @@ type stringLiteralExpr struct {
 	literal string
 }
 
-func (s *stringLiteralExpr) eval(_ map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	res = s
-	same = true
-	return
-}
-
 func (s *stringLiteralExpr) emit(gctx *generationContext) {
 	gctx.writef("%q", s.literal)
 }
@@ -81,12 +70,6 @@ type intLiteralExpr struct {
 	literal int
 }
 
-func (s *intLiteralExpr) eval(_ map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	res = s
-	same = true
-	return
-}
-
 func (s *intLiteralExpr) emit(gctx *generationContext) {
 	gctx.writef("%d", s.literal)
 }
@@ -112,10 +95,6 @@ type boolLiteralExpr struct {
 	literal bool
 }
 
-func (b *boolLiteralExpr) eval(_ map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	return b, true
-}
-
 func (b *boolLiteralExpr) emit(gctx *generationContext) {
 	if b.literal {
 		gctx.write("True")
@@ -148,6 +127,35 @@ type interpolateExpr struct {
 	args   []starlarkExpr
 }
 
+func NewInterpolateExpr(parts []starlarkExpr) starlarkExpr {
+	result := &interpolateExpr{}
+	needString := true
+	for _, part := range parts {
+		if needString {
+			if strLit, ok := part.(*stringLiteralExpr); ok {
+				result.chunks = append(result.chunks, strLit.literal)
+			} else {
+				result.chunks = append(result.chunks, "")
+			}
+			needString = false
+		} else {
+			if strLit, ok := part.(*stringLiteralExpr); ok {
+				result.chunks[len(result.chunks)-1] += strLit.literal
+			} else {
+				result.args = append(result.args, part)
+				needString = true
+			}
+		}
+	}
+	if len(result.chunks) == len(result.args) {
+		result.chunks = append(result.chunks, "")
+	}
+	if len(result.args) == 0 {
+		return &stringLiteralExpr{literal: strings.Join(result.chunks, "")}
+	}
+	return result
+}
+
 func (xi *interpolateExpr) emit(gctx *generationContext) {
 	if len(xi.chunks) != len(xi.args)+1 {
 		panic(fmt.Errorf("malformed interpolateExpr: #chunks(%d) != #args(%d)+1",
@@ -181,37 +189,6 @@ func (xi *interpolateExpr) emit(gctx *generationContext) {
 	}
 }
 
-func (xi *interpolateExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	same = true
-	newChunks := []string{xi.chunks[0]}
-	var newArgs []starlarkExpr
-	for i, arg := range xi.args {
-		newArg, sameArg := arg.eval(valueMap)
-		same = same && sameArg
-		switch x := newArg.(type) {
-		case *stringLiteralExpr:
-			newChunks[len(newChunks)-1] += x.literal + xi.chunks[i+1]
-			same = false
-			continue
-		case *intLiteralExpr:
-			newChunks[len(newChunks)-1] += strconv.Itoa(x.literal) + xi.chunks[i+1]
-			same = false
-			continue
-		default:
-			newChunks = append(newChunks, xi.chunks[i+1])
-			newArgs = append(newArgs, newArg)
-		}
-	}
-	if same {
-		res = xi
-	} else if len(newChunks) == 1 {
-		res = &stringLiteralExpr{newChunks[0]}
-	} else {
-		res = &interpolateExpr{chunks: newChunks, args: newArgs}
-	}
-	return
-}
-
 func (_ *interpolateExpr) typ() starlarkType {
 	return starlarkTypeString
 }
@@ -238,14 +215,11 @@ type variableRefExpr struct {
 	isDefined bool
 }
 
-func (v *variableRefExpr) eval(map[string]starlarkExpr) (res starlarkExpr, same bool) {
+func NewVariableRefExpr(ref variable, isDefined bool) starlarkExpr {
-	predefined, ok := v.ref.(*predefinedVariable)
+	if predefined, ok := ref.(*predefinedVariable); ok {
-	if same = !ok; same {
+		return predefined.value
-		res = v
-	} else {
-		res = predefined.value
 	}
-	return
+	return &variableRefExpr{ref, isDefined}
 }
 
 func (v *variableRefExpr) emit(gctx *generationContext) {
@@ -275,15 +249,6 @@ 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:
@@ -329,15 +294,6 @@ type notExpr struct {
 	expr starlarkExpr
 }
 
-func (n *notExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	if x, same := n.expr.eval(valueMap); same {
-		res = n
-	} else {
-		res = &notExpr{expr: x}
-	}
-	return
-}
-
 func (n *notExpr) emit(ctx *generationContext) {
 	ctx.write("not ")
 	n.expr.emit(ctx)
@@ -365,17 +321,6 @@ type eqExpr struct {
 	isEq        bool // if false, it's !=
 }
 
-func (eq *eqExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	xLeft, sameLeft := eq.left.eval(valueMap)
-	xRight, sameRight := eq.right.eval(valueMap)
-	if same = sameLeft && sameRight; same {
-		res = eq
-	} else {
-		res = &eqExpr{left: xLeft, right: xRight, isEq: eq.isEq}
-	}
-	return
-}
-
 func (eq *eqExpr) emit(gctx *generationContext) {
 	var stringOperand string
 	var otherOperand starlarkExpr
@@ -444,13 +389,6 @@ type variableDefinedExpr struct {
 	v variable
 }
 
-func (v *variableDefinedExpr) eval(_ map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	res = v
-	same = true
-	return
-
-}
-
 func (v *variableDefinedExpr) emit(gctx *generationContext) {
 	if v.v != nil {
 		v.v.emitDefined(gctx)
@@ -476,22 +414,6 @@ type listExpr struct {
 	items []starlarkExpr
 }
 
-func (l *listExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	newItems := make([]starlarkExpr, len(l.items))
-	same = true
-	for i, item := range l.items {
-		var sameItem bool
-		newItems[i], sameItem = item.eval(valueMap)
-		same = same && sameItem
-	}
-	if same {
-		res = l
-	} else {
-		res = &listExpr{newItems}
-	}
-	return
-}
-
 func (l *listExpr) emit(gctx *generationContext) {
 	if !gctx.inAssignment || len(l.items) < 2 {
 		gctx.write("[")
@@ -578,22 +500,6 @@ func (c *concatExpr) emit(gctx *generationContext) {
 	gctx.indentLevel -= 2
 }
 
-func (c *concatExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	same = true
-	xConcat := &concatExpr{items: make([]starlarkExpr, len(c.items))}
-	for i, item := range c.items {
-		var sameItem bool
-		xConcat.items[i], sameItem = item.eval(valueMap)
-		same = same && sameItem
-	}
-	if same {
-		res = c
-	} else {
-		res = xConcat
-	}
-	return
-}
-
 func (_ *concatExpr) typ() starlarkType {
 	return starlarkTypeList
 }
@@ -622,19 +528,6 @@ type inExpr struct {
 	isNot bool
 }
 
-func (i *inExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	x := &inExpr{isNot: i.isNot}
-	var sameExpr, sameList bool
-	x.expr, sameExpr = i.expr.eval(valueMap)
-	x.list, sameList = i.list.eval(valueMap)
-	if same = sameExpr && sameList; same {
-		res = i
-	} else {
-		res = x
-	}
-	return
-}
-
 func (i *inExpr) emit(gctx *generationContext) {
 	i.expr.emit(gctx)
 	if i.isNot {
@@ -679,17 +572,6 @@ func (ix *indexExpr) typ() starlarkType {
 	return starlarkTypeString
 }
 
-func (ix *indexExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	newArray, isSameArray := ix.array.eval(valueMap)
-	newIndex, isSameIndex := ix.index.eval(valueMap)
-	if same = isSameArray && isSameIndex; same {
-		res = ix
-	} else {
-		res = &indexExpr{newArray, newIndex}
-	}
-	return
-}
-
 func (ix *indexExpr) emitListVarCopy(gctx *generationContext) {
 	ix.emit(gctx)
 }
@@ -711,27 +593,6 @@ type callExpr struct {
 	returnType starlarkType
 }
 
-func (cx *callExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	newCallExpr := &callExpr{name: cx.name, args: make([]starlarkExpr, len(cx.args)),
-		returnType: cx.returnType}
-	if cx.object != nil {
-		newCallExpr.object, same = cx.object.eval(valueMap)
-	} else {
-		same = true
-	}
-	for i, args := range cx.args {
-		var s bool
-		newCallExpr.args[i], s = args.eval(valueMap)
-		same = same && s
-	}
-	if same {
-		res = cx
-	} else {
-		res = newCallExpr
-	}
-	return
-}
-
 func (cx *callExpr) emit(gctx *generationContext) {
 	sep := ""
 	if cx.object != nil {
@@ -793,22 +654,6 @@ type ifExpr struct {
 	ifFalse   starlarkExpr
 }
 
-func (i *ifExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	cond, condSame := i.condition.eval(valueMap)
-	t, tSame := i.ifTrue.eval(valueMap)
-	f, fSame := i.ifFalse.eval(valueMap)
-	same = condSame && tSame && fSame
-	if same {
-		return i, same
-	} else {
-		return &ifExpr{
-			condition: cond,
-			ifTrue:    t,
-			ifFalse:   f,
-		}, same
-	}
-}
-
 func (i *ifExpr) emit(gctx *generationContext) {
 	gctx.write("(")
 	i.ifTrue.emit(gctx)
@@ -851,10 +696,6 @@ type identifierExpr struct {
 	name string
 }
 
-func (i *identifierExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	return i, true
-}
-
 func (i *identifierExpr) emit(gctx *generationContext) {
 	gctx.write(i.name)
 }
@@ -881,21 +722,6 @@ type foreachExpr struct {
 	action  starlarkExpr
 }
 
-func (f *foreachExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	list, listSame := f.list.eval(valueMap)
-	action, actionSame := f.action.eval(valueMap)
-	same = listSame && actionSame
-	if same {
-		return f, same
-	} else {
-		return &foreachExpr{
-			varName: f.varName,
-			list:    list,
-			action:  action,
-		}, same
-	}
-}
-
 func (f *foreachExpr) emit(gctx *generationContext) {
 	gctx.write("[")
 	f.action.emit(gctx)
@@ -927,12 +753,6 @@ type badExpr struct {
 	message       string
 }
 
-func (b *badExpr) eval(_ map[string]starlarkExpr) (res starlarkExpr, same bool) {
-	res = b
-	same = true
-	return
-}
-
 func (b *badExpr) emit(gctx *generationContext) {
 	gctx.emitConversionError(b.errorLocation, b.message)
 }

mk2rbc/mk2rbc.go

@@ -411,7 +411,6 @@ type parseContext struct {
 	ifNestLevel      int
 	moduleNameCount  map[string]int // count of imported modules with given basename
 	fatalError       error
-	builtinMakeVars  map[string]starlarkExpr
 	outputSuffix     string
 	errorLogger      ErrorLogger
 	tracedVariables  map[string]bool // variables to be traced in the generated script
@@ -464,7 +463,6 @@ func newParseContext(ss *StarlarkScript, nodes []mkparser.Node) *parseContext {
 		currentNodeIndex: 0,
 		ifNestLevel:      0,
 		moduleNameCount:  make(map[string]int),
-		builtinMakeVars:  map[string]starlarkExpr{},
 		variables:        make(map[string]variable),
 		dependentModules: make(map[string]*moduleInfo),
 		soongNamespaces:  make(map[string]map[string]bool),
@@ -600,9 +598,6 @@ func (ctx *parseContext) handleAssignment(a *mkparser.Assignment) {
 		}
 	}
 
-	// TODO(asmundak): move evaluation to a separate pass
-	asgn.value, _ = asgn.value.eval(ctx.builtinMakeVars)
-
 	asgn.previous = ctx.lastAssignment(name)
 	ctx.setLastAssignment(name, asgn)
 	switch a.Type {
@@ -629,7 +624,6 @@ func (ctx *parseContext) handleSoongNsAssignment(name string, asgn *mkparser.Ass
 		ctx.wrapBadExpr(xBad)
 		return
 	}
-	val, _ = val.eval(ctx.builtinMakeVars)
 
 	// Unfortunately, Soong namespaces can be set up by directly setting corresponding Make
 	// variables instead of via add_soong_config_namespace + add_soong_config_var_value.
@@ -785,7 +779,6 @@ func (ctx *parseContext) newDependentModule(path string, optional bool) *moduleI
 
 func (ctx *parseContext) handleSubConfig(
 	v mkparser.Node, pathExpr starlarkExpr, loadAlways bool, processModule func(inheritedModule)) {
-	pathExpr, _ = pathExpr.eval(ctx.builtinMakeVars)
 
 	// In a simple case, the name of a module to inherit/include is known statically.
 	if path, ok := maybeString(pathExpr); ok {
@@ -1122,7 +1115,7 @@ func (ctx *parseContext) parseCompareSpecialCases(directive *mkparser.Directive,
 			return xBad, true
 		}
 		return &eqExpr{
-			left:  &variableRefExpr{ctx.addVariable("TARGET_BOARD_PLATFORM"), false},
+			left:  NewVariableRefExpr(ctx.addVariable("TARGET_BOARD_PLATFORM"), false),
 			right: call.args[0],
 			isEq:  isEq,
 		}, true
@@ -1131,7 +1124,7 @@ func (ctx *parseContext) parseCompareSpecialCases(directive *mkparser.Directive,
 			return xBad, true
 		}
 		return &inExpr{
-			expr:  &variableRefExpr{ctx.addVariable("TARGET_BOARD_PLATFORM"), false},
+			expr:  NewVariableRefExpr(ctx.addVariable("TARGET_BOARD_PLATFORM"), false),
 			list:  maybeConvertToStringList(call.args[0]),
 			isNot: !isEq,
 		}, true
@@ -1140,7 +1133,7 @@ func (ctx *parseContext) parseCompareSpecialCases(directive *mkparser.Directive,
 			return xBad, true
 		}
 		return &inExpr{
-			expr:  &variableRefExpr{ctx.addVariable("TARGET_PRODUCT"), true},
+			expr:  NewVariableRefExpr(ctx.addVariable("TARGET_PRODUCT"), true),
 			list:  maybeConvertToStringList(call.args[0]),
 			isNot: !isEq,
 		}, true
@@ -1153,8 +1146,8 @@ func (ctx *parseContext) parseCompareSpecialCases(directive *mkparser.Directive,
 			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},
+			expr:  NewVariableRefExpr(ctx.addVariable("TARGET_BOARD_PLATFORM"), false),
-			list:  &variableRefExpr{ctx.addVariable(s + "_BOARD_PLATFORMS"), true},
+			list:  NewVariableRefExpr(ctx.addVariable(s+"_BOARD_PLATFORMS"), true),
 			isNot: !isEq,
 		}, true
 
@@ -1183,8 +1176,8 @@ func (ctx *parseContext) parseCompareSpecialCases(directive *mkparser.Directive,
 		// 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},
+			expr:  NewVariableRefExpr(ctx.addVariable("TARGET_BOARD_PLATFORM"), false),
-			list:  &variableRefExpr{ctx.addVariable("QCOM_BOARD_PLATFORMS"), true},
+			list:  NewVariableRefExpr(ctx.addVariable("QCOM_BOARD_PLATFORMS"), true),
 			isNot: isEq,
 		}, true
 	}
@@ -1366,12 +1359,12 @@ func (ctx *parseContext) parseReference(node mkparser.Node, ref *mkparser.MakeSt
 				args: []starlarkExpr{
 					&stringLiteralExpr{literal: substParts[0]},
 					&stringLiteralExpr{literal: substParts[1]},
-					&variableRefExpr{v, ctx.lastAssignment(v.name()) != nil},
+					NewVariableRefExpr(v, ctx.lastAssignment(v.name()) != nil),
 				},
 			}
 		}
 		if v := ctx.addVariable(refDump); v != nil {
-			return &variableRefExpr{v, ctx.lastAssignment(v.name()) != nil}
+			return NewVariableRefExpr(v, ctx.lastAssignment(v.name()) != nil)
 		}
 		return ctx.newBadExpr(node, "unknown variable %s", refDump)
 	}
@@ -1416,7 +1409,7 @@ func (ctx *parseContext) parseReference(node mkparser.Node, ref *mkparser.MakeSt
 	case "firstword", "lastword":
 		return ctx.parseFirstOrLastwordFunc(node, expr.name, args)
 	case "my-dir":
-		return &variableRefExpr{ctx.addVariable("LOCAL_PATH"), true}
+		return NewVariableRefExpr(ctx.addVariable("LOCAL_PATH"), true)
 	case "subst", "patsubst":
 		return ctx.parseSubstFunc(node, expr.name, args)
 	default:
@@ -1579,16 +1572,18 @@ func (ctx *parseContext) parseMakeString(node mkparser.Node, mk *mkparser.MakeSt
 	// If we reached here, it's neither string literal nor a simple variable,
 	// we need a full-blown interpolation node that will generate
 	// "a%b%c" % (X, Y) for a$(X)b$(Y)c
-	xInterp := &interpolateExpr{args: make([]starlarkExpr, len(mk.Variables))}
+	parts := make([]starlarkExpr, len(mk.Variables)+len(mk.Strings))
-	for i, ref := range mk.Variables {
+	for i := 0; i < len(parts); i++ {
-		arg := ctx.parseReference(node, ref.Name)
+		if i%2 == 0 {
-		if x, ok := arg.(*badExpr); ok {
+			parts[i] = &stringLiteralExpr{literal: mk.Strings[i/2]}
-			return x
+		} else {
+			parts[i] = ctx.parseReference(node, mk.Variables[i/2].Name)
+			if x, ok := parts[i].(*badExpr); ok {
+				return x
+			}
 		}
-		xInterp.args[i] = arg
 	}
-	xInterp.chunks = append(xInterp.chunks, mk.Strings...)
+	return NewInterpolateExpr(parts)
-	return xInterp
 }
 
 // Handles the statements whose treatment is the same in all contexts: comment,