Merge pull request #15 from buildkite/making-it-rain

Fix nested escaped dollarsigns

go.mod

@@ -1,3 +1,5 @@
 module github.com/buildkite/interpolate
 
 go 1.22
+
+require github.com/google/go-cmp v0.6.0

go.sum

@@ -0,0 +1,2 @@
+github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
+github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=

interpolate.go

@@ -1,8 +1,8 @@
 package interpolate
 
 import (
-	"bytes"
 	"fmt"
+	"strings"
 )
 
 // Interpolate takes a set of environment and interpolates it into the provided string using shell script expansions
@@ -28,7 +28,13 @@ func Identifiers(str string) ([]string, error) {
 
 // An expansion is something that takes in ENV and returns a string or an error
 type Expansion interface {
+	// Expand expands the expansion using variables from env.
 	Expand(env Env) (string, error)
+
+	// Identifiers returns any variable names referenced within the expansion.
+	// Escaped expansions do something special and return identifiers
+	// (starting with $) that *would* become referenced after a round of
+	// unescaping.
 	Identifiers() []string
 }
 
@@ -84,15 +90,17 @@ func (e UnsetValueExpansion) Expand(env Env) (string, error) {
 
 // EscapedExpansion is an expansion that is delayed until later on (usually by a later process)
 type EscapedExpansion struct {
-	Identifier string
+	// PotentialIdentifier is an identifier for the purpose of Identifiers,
+	// but not for the purpose of Expand.
+	PotentialIdentifier string
 }
 
 func (e EscapedExpansion) Identifiers() []string {
-	return []string{"$" + e.Identifier}
+	return []string{"$" + e.PotentialIdentifier}
 }
 
 func (e EscapedExpansion) Expand(Env) (string, error) {
-	return "$" + e.Identifier, nil
+	return "$", nil
 }
 
 // SubstringExpansion returns a substring (or slice) of the env
@@ -193,17 +201,17 @@ func (e Expression) Identifiers() []string {
 }
 
 func (e Expression) Expand(env Env) (string, error) {
-	buf := &bytes.Buffer{}
+	var buf strings.Builder
 
 	for _, item := range e {
 		if item.Expansion != nil {
 			result, err := item.Expansion.Expand(env)
 			if err != nil {
 				return "", err
 			}
-			_, _ = buf.WriteString(result)
+			buf.WriteString(result)
 		} else {
-			_, _ = buf.WriteString(item.Text)
+			buf.WriteString(item.Text)
 		}
 	}
 

interpolate_test.go

@@ -281,23 +281,52 @@ func TestEscapingVariables(t *testing.T) {
 	t.Parallel()
 
 	for _, tc := range []struct {
-		Str      string
-		Expected string
+		input string
+		want  string
 	}{
-		{`Do this $$ESCAPE_PARTY`, `Do this $ESCAPE_PARTY`},
-		{`Do this \$ESCAPE_PARTY`, `Do this $ESCAPE_PARTY`},
-		{`Do this $${SUCH_ESCAPE}`, `Do this ${SUCH_ESCAPE}`},
-		{`Do this \${SUCH_ESCAPE}`, `Do this ${SUCH_ESCAPE}`},
-		{`Do this $${SUCH_ESCAPE:-$OTHERWISE}`, `Do this ${SUCH_ESCAPE:-$OTHERWISE}`},
-		{`Do this \${SUCH_ESCAPE:-$OTHERWISE}`, `Do this ${SUCH_ESCAPE:-$OTHERWISE}`},
-		{`echo "my favourite mountain is cotopaxi" | grep 'xi$$'`, `echo "my favourite mountain is cotopaxi" | grep 'xi$'`},
+		{
+			input: "Do this $$ESCAPE_PARTY",
+			want:  "Do this $ESCAPE_PARTY",
+		},
+		{
+			input: `Do this \$ESCAPE_PARTY`,
+			want:  "Do this $ESCAPE_PARTY",
+		},
+		{
+			input: "Do this $${SUCH_ESCAPE}",
+			want:  "Do this ${SUCH_ESCAPE}",
+		},
+		{
+			input: `Do this \${SUCH_ESCAPE}`,
+			want:  "Do this ${SUCH_ESCAPE}",
+		},
+		{
+			input: "Do this $${SUCH_ESCAPE:-$OTHERWISE}",
+			want:  "Do this ${SUCH_ESCAPE:-}",
+		},
+		{
+			input: `Do this \${SUCH_ESCAPE:-$OTHERWISE}`,
+			want:  "Do this ${SUCH_ESCAPE:-}",
+		},
+		{
+			input: "Do this $${SUCH_ESCAPE:-$$OTHERWISE}",
+			want:  "Do this ${SUCH_ESCAPE:-$OTHERWISE}",
+		},
+		{
+			input: `Do this \${SUCH_ESCAPE:-\$OTHERWISE}`,
+			want:  "Do this ${SUCH_ESCAPE:-$OTHERWISE}",
+		},
+		{
+			input: `echo "my favourite mountain is cotopaxi" | grep 'xi$$'`,
+			want:  `echo "my favourite mountain is cotopaxi" | grep 'xi$'`,
+		},
 	} {
-		result, err := interpolate.Interpolate(nil, tc.Str)
+		result, err := interpolate.Interpolate(nil, tc.input)
 		if err != nil {
-			t.Fatal(err)
+			t.Errorf("interpolate.Interpolate(nil, %q) error = %v", tc.input, err)
 		}
-		if result != tc.Expected {
-			t.Fatalf("Test %q failed: Expected substring %q, got %q", tc.Str, tc.Expected, result)
+		if result != tc.want {
+			t.Errorf("interpolate.Interpolate(nil, %q) = %q, want %q", tc.input, tc.want, result)
 		}
 	}
 }

parser.go

@@ -87,7 +87,7 @@ func (p *Parser) parseExpression(stop ...rune) (Expression, error) {
 				return nil, err
 			}
 
-			expr = append(expr, ee)
+			expr = append(expr, ExpressionItem{Expansion: ee})
 			continue
 		}
 
@@ -122,28 +122,39 @@ func (p *Parser) parseExpression(stop ...rune) (Expression, error) {
 	return expr, nil
 }
 
-func (p *Parser) parseEscapedExpansion() (ExpressionItem, error) {
+// parseEscapedExpansion attempts to extract a *potential* identifier or brace
+// expression from the text following the escaped dollarsign.
+func (p *Parser) parseEscapedExpansion() (EscapedExpansion, error) {
+	// Since it's not an expansion, we should treat the following text as text.
+	start := p.pos
+	defer func() { p.pos = start }()
+
 	next := p.peekRune()
 	switch {
 	case next == '{':
-		// if it's an escaped brace expansion, (eg $${MY_COOL_VAR:-5}) consume text until the close brace
-		id := p.scanUntil(func(r rune) bool { return r == '}' })
-		id = id + string(p.nextRune()) // we know that the next rune is a close brace, chuck it on the end
-		return ExpressionItem{Expansion: EscapedExpansion{Identifier: id}}, nil
+		// it *could be* an escaped brace expansion
+		if _, err := p.parseBraceExpansion(); err != nil {
+			return EscapedExpansion{}, nil
+		}
+		// it was! instead of storing the expansion itself, store the string
+		// that produced it.
+		return EscapedExpansion{PotentialIdentifier: p.input[start:p.pos]}, nil
 
 	case unicode.IsLetter(next):
-		// it's an escaped identifier (eg $$MY_COOL_VAR)
+		// it *could be* an escaped identifier (eg $$MY_COOL_VAR)
 		id, err := p.scanIdentifier()
 		if err != nil {
-			return ExpressionItem{}, err
+			// this should never happen, since scanIdentifier only errors if the
+			// first rune is not a letter, and we just checked that.
+			return EscapedExpansion{}, nil
 		}
 
-		return ExpressionItem{Expansion: EscapedExpansion{Identifier: id}}, nil
+		return EscapedExpansion{PotentialIdentifier: id}, nil
 
 	default:
-		// there's no identifier or brace afterward, so it's probably a literal escaped dollar sign
-		// just return a text item with the dollar sign
-		return ExpressionItem{Text: "$"}, nil
+		// there's no identifier or brace afterward, so it's probably a literal
+		// escaped dollar sign
+		return EscapedExpansion{}, nil
 	}
 }
 
@@ -162,7 +173,9 @@ func (p *Parser) parseExpansion() (Expansion, error) {
 		return nil, err
 	}
 
-	return VariableExpansion{Identifier: identifier}, nil
+	return VariableExpansion{
+		Identifier: identifier,
+	}, nil
 }
 
 func (p *Parser) parseBraceExpansion() (Expansion, error) {
@@ -177,7 +190,9 @@ func (p *Parser) parseBraceExpansion() (Expansion, error) {
 
 	if c := p.peekRune(); c == '}' {
 		_ = p.nextRune()
-		return VariableExpansion{Identifier: identifier}, nil
+		return VariableExpansion{
+			Identifier: identifier,
+		}, nil
 	}
 
 	var operator string
@@ -298,8 +313,8 @@ func (p *Parser) scanIdentifier() (string, error) {
 	if c := p.peekRune(); !unicode.IsLetter(c) {
 		return "", fmt.Errorf("Expected identifier to start with a letter, got %c", c)
 	}
-	var notIdentifierChar = func(r rune) bool {
-		return (!unicode.IsLetter(r) && !unicode.IsNumber(r) && r != '_')
+	notIdentifierChar := func(r rune) bool {
+		return !(unicode.IsLetter(r) || unicode.IsNumber(r) || r == '_')
 	}
 	return p.scanUntil(notIdentifierChar), nil
 }