Fix android.Expand and ninja escaping

RuleBuilder does its own ninja escaping, so values that will be
passed to RuleBuilder must not be pre-escaped.  Add a new
android.ExpandNinjaEscaped method that explicitly handles ninja
escaping.  Some of the expansion functions return ninja values
(like "${in}") that need to stay unescaped, so add a bool return
value to the expansion function in android.ExpandNinjaEscaped.

Test: expand_test.go
Change-Id: Ib03d4db38b5e3e5bffbd87acf14f55e276a53d04

android/expand.go

@@ -18,12 +18,30 @@ import (
 	"fmt"
 	"strings"
 	"unicode"
+
+	"github.com/google/blueprint/proptools"
 )
 
+// ExpandNinjaEscaped substitutes $() variables in a string
+// $(var) is passed to mapping(var), which should return the expanded value, a bool for whether the result should
+// be left unescaped when using in a ninja value (generally false, true if the expanded value is a ninja variable like
+// '${in}'), and an error.
+// $$ is converted to $, which is escaped back to $$.
+func ExpandNinjaEscaped(s string, mapping func(string) (string, bool, error)) (string, error) {
+	return expand(s, true, mapping)
+}
+
 // Expand substitutes $() variables in a string
-// $(var) is passed to Expander(var)
+// $(var) is passed to mapping(var), which should return the expanded value and an error.
-// $$ is converted to $
+// $$ is converted to $.
 func Expand(s string, mapping func(string) (string, error)) (string, error) {
+	return expand(s, false, func(s string) (string, bool, error) {
+		s, err := mapping(s)
+		return s, false, err
+	})
+}
+
+func expand(s string, ninjaEscape bool, mapping func(string) (string, bool, error)) (string, error) {
 	// based on os.Expand
 	buf := make([]byte, 0, 2*len(s))
 	i := 0
@@ -33,10 +51,13 @@ func Expand(s string, mapping func(string) (string, error)) (string, error) {
 				return "", fmt.Errorf("expected character after '$'")
 			}
 			buf = append(buf, s[i:j]...)
-			value, w, err := getMapping(s[j+1:], mapping)
+			value, ninjaVariable, w, err := getMapping(s[j+1:], mapping)
 			if err != nil {
 				return "", err
 			}
+			if !ninjaVariable && ninjaEscape {
+				value = proptools.NinjaEscape(value)
+			}
 			buf = append(buf, value...)
 			j += w
 			i = j + 1
@@ -45,26 +66,26 @@ func Expand(s string, mapping func(string) (string, error)) (string, error) {
 	return string(buf) + s[i:], nil
 }
 
-func getMapping(s string, mapping func(string) (string, error)) (string, int, error) {
+func getMapping(s string, mapping func(string) (string, bool, error)) (string, bool, int, error) {
 	switch s[0] {
 	case '(':
 		// Scan to closing brace
 		for i := 1; i < len(s); i++ {
 			if s[i] == ')' {
-				ret, err := mapping(strings.TrimSpace(s[1:i]))
+				ret, ninjaVariable, err := mapping(strings.TrimSpace(s[1:i]))
-				return ret, i + 1, err
+				return ret, ninjaVariable, i + 1, err
 			}
 		}
-		return "", len(s), fmt.Errorf("missing )")
+		return "", false, len(s), fmt.Errorf("missing )")
 	case '$':
-		return "$$", 1, nil
+		return "$", false, 1, nil
 	default:
 		i := strings.IndexFunc(s, unicode.IsSpace)
 		if i == 0 {
-			return "", 0, fmt.Errorf("unexpected character '%c' after '$'", s[0])
+			return "", false, 0, fmt.Errorf("unexpected character '%c' after '$'", s[0])
 		} else if i == -1 {
 			i = len(s)
 		}
-		return "", 0, fmt.Errorf("expected '(' after '$', did you mean $(%s)?", s[:i])
+		return "", false, 0, fmt.Errorf("expected '(' after '$', did you mean $(%s)?", s[:i])
 	}
 }

android/expand_test.go

@@ -20,88 +20,111 @@ import (
 )
 
 var vars = map[string]string{
-	"var1": "abc",
+	"var1":   "abc",
-	"var2": "",
+	"var2":   "",
-	"var3": "def",
+	"var3":   "def",
-	"💩":    "😃",
+	"💩":      "😃",
+	"escape": "${in}",
 }
 
-func expander(s string) (string, error) {
+func expander(s string) (string, bool, error) {
 	if val, ok := vars[s]; ok {
-		return val, nil
+		return val, s == "escape", nil
 	} else {
-		return "", fmt.Errorf("unknown variable %q", s)
+		return "", false, fmt.Errorf("unknown variable %q", s)
 	}
 }
 
 var expandTestCases = []struct {
-	in  string
+	in          string
-	out string
+	out         string
-	err bool
+	out_escaped string
+	err         bool
 }{
 	{
-		in:  "$(var1)",
+		in:          "$(var1)",
-		out: "abc",
+		out:         "abc",
+		out_escaped: "abc",
 	},
 	{
-		in:  "$( var1 )",
+		in:          "$( var1 )",
-		out: "abc",
+		out:         "abc",
+		out_escaped: "abc",
 	},
 	{
-		in:  "def$(var1)",
+		in:          "def$(var1)",
-		out: "defabc",
+		out:         "defabc",
+		out_escaped: "defabc",
 	},
 	{
-		in:  "$(var1)def",
+		in:          "$(var1)def",
-		out: "abcdef",
+		out:         "abcdef",
+		out_escaped: "abcdef",
 	},
 	{
-		in:  "def$(var1)def",
+		in:          "def$(var1)def",
-		out: "defabcdef",
+		out:         "defabcdef",
+		out_escaped: "defabcdef",
 	},
 	{
-		in:  "$(var2)",
+		in:          "$(var2)",
-		out: "",
+		out:         "",
+		out_escaped: "",
 	},
 	{
-		in:  "def$(var2)",
+		in:          "def$(var2)",
-		out: "def",
+		out:         "def",
+		out_escaped: "def",
 	},
 	{
-		in:  "$(var2)def",
+		in:          "$(var2)def",
-		out: "def",
+		out:         "def",
+		out_escaped: "def",
 	},
 	{
-		in:  "def$(var2)def",
+		in:          "def$(var2)def",
-		out: "defdef",
+		out:         "defdef",
+		out_escaped: "defdef",
 	},
 	{
-		in:  "$(var1)$(var3)",
+		in:          "$(var1)$(var3)",
-		out: "abcdef",
+		out:         "abcdef",
+		out_escaped: "abcdef",
 	},
 	{
-		in:  "$(var1)g$(var3)",
+		in:          "$(var1)g$(var3)",
-		out: "abcgdef",
+		out:         "abcgdef",
+		out_escaped: "abcgdef",
 	},
 	{
-		in:  "$$",
+		in:          "$$",
-		out: "$$",
+		out:         "$",
+		out_escaped: "$$",
 	},
 	{
-		in:  "$$(var1)",
+		in:          "$$(var1)",
-		out: "$$(var1)",
+		out:         "$(var1)",
+		out_escaped: "$$(var1)",
 	},
 	{
-		in:  "$$$(var1)",
+		in:          "$$$(var1)",
-		out: "$$abc",
+		out:         "$abc",
+		out_escaped: "$$abc",
 	},
 	{
-		in:  "$(var1)$$",
+		in:          "$(var1)$$",
-		out: "abc$$",
+		out:         "abc$",
+		out_escaped: "abc$$",
 	},
 	{
-		in:  "$(💩)",
+		in:          "$(💩)",
-		out: "😃",
+		out:         "😃",
+		out_escaped: "😃",
+	},
+	{
+		in:          "$$a$(escape)$$b",
+		out:         "$a${in}$b",
+		out_escaped: "$$a${in}$$b",
 	},
 
 	// Errors
@@ -141,7 +164,10 @@ var expandTestCases = []struct {
 
 func TestExpand(t *testing.T) {
 	for _, test := range expandTestCases {
-		got, err := Expand(test.in, expander)
+		got, err := Expand(test.in, func(s string) (string, error) {
+			s, _, err := expander(s)
+			return s, err
+		})
 		if err != nil && !test.err {
 			t.Errorf("%q: unexpected error %s", test.in, err.Error())
 		} else if err == nil && test.err {
@@ -151,3 +177,16 @@ func TestExpand(t *testing.T) {
 		}
 	}
 }
+
+func TestExpandNinjaEscaped(t *testing.T) {
+	for _, test := range expandTestCases {
+		got, err := ExpandNinjaEscaped(test.in, expander)
+		if err != nil && !test.err {
+			t.Errorf("%q: unexpected error %s", test.in, err.Error())
+		} else if err == nil && test.err {
+			t.Errorf("%q: expected error, got %q", test.in, got)
+		} else if !test.err && got != test.out_escaped {
+			t.Errorf("%q: expected %q, got %q", test.in, test.out, got)
+		}
+	}
+}

genrule/genrule.go

@@ -284,12 +284,12 @@ func (g *Module) GenerateAndroidBuildActions(ctx android.ModuleContext) {
 
 	referencedDepfile := false
 
-	rawCommand, err := android.Expand(task.cmd, func(name string) (string, error) {
+	rawCommand, err := android.ExpandNinjaEscaped(task.cmd, func(name string) (string, bool, error) {
 		// report the error directly without returning an error to android.Expand to catch multiple errors in a
 		// single run
-		reportError := func(fmt string, args ...interface{}) (string, error) {
+		reportError := func(fmt string, args ...interface{}) (string, bool, error) {
 			ctx.PropertyErrorf("cmd", fmt, args...)
-			return "SOONG_ERROR", nil
+			return "SOONG_ERROR", false, nil
 		}
 
 		switch name {
@@ -304,19 +304,19 @@ func (g *Module) GenerateAndroidBuildActions(ctx android.ModuleContext) {
 				return reportError("default label %q has multiple files, use $(locations %s) to reference it",
 					firstLabel, firstLabel)
 			}
-			return locationLabels[firstLabel][0], nil
+			return locationLabels[firstLabel][0], false, nil
 		case "in":
-			return "${in}", nil
+			return "${in}", true, nil
 		case "out":
-			return "__SBOX_OUT_FILES__", nil
+			return "__SBOX_OUT_FILES__", false, nil
 		case "depfile":
 			referencedDepfile = true
 			if !Bool(g.properties.Depfile) {
 				return reportError("$(depfile) used without depfile property")
 			}
-			return "__SBOX_DEPFILE__", nil
+			return "__SBOX_DEPFILE__", false, nil
 		case "genDir":
-			return "__SBOX_OUT_DIR__", nil
+			return "__SBOX_OUT_DIR__", false, nil
 		default:
 			if strings.HasPrefix(name, "location ") {
 				label := strings.TrimSpace(strings.TrimPrefix(name, "location "))
@@ -327,7 +327,7 @@ func (g *Module) GenerateAndroidBuildActions(ctx android.ModuleContext) {
 						return reportError("label %q has multiple files, use $(locations %s) to reference it",
 							label, label)
 					}
-					return paths[0], nil
+					return paths[0], false, nil
 				} else {
 					return reportError("unknown location label %q", label)
 				}
@@ -337,7 +337,7 @@ func (g *Module) GenerateAndroidBuildActions(ctx android.ModuleContext) {
 					if len(paths) == 0 {
 						return reportError("label %q has no files", label)
 					}
-					return strings.Join(paths, " "), nil
+					return strings.Join(paths, " "), false, nil
 				} else {
 					return reportError("unknown locations label %q", label)
 				}

java/droiddoc.go

@@ -732,19 +732,19 @@ func (j *Javadoc) collectDeps(ctx android.ModuleContext) deps {
 	}
 
 	var err error
-	j.args, err = android.Expand(String(j.properties.Args), func(name string) (string, error) {
+	j.args, err = android.ExpandNinjaEscaped(String(j.properties.Args), func(name string) (string, bool, error) {
 		if strings.HasPrefix(name, "location ") {
 			label := strings.TrimSpace(strings.TrimPrefix(name, "location "))
 			if paths, ok := argFilesMap[label]; ok {
-				return paths, nil
+				return paths, false, nil
 			} else {
-				return "", fmt.Errorf("unknown location label %q, expecting one of %q",
+				return "", false, fmt.Errorf("unknown location label %q, expecting one of %q",
 					label, strings.Join(argFileLabels, ", "))
 			}
 		} else if name == "genDir" {
-			return android.PathForModuleGen(ctx).String(), nil
+			return android.PathForModuleGen(ctx).String(), false, nil
 		}
-		return "", fmt.Errorf("unknown variable '$(%s)'", name)
+		return "", false, fmt.Errorf("unknown variable '$(%s)'", name)
 	})
 
 	if err != nil {