cmd/go/internal/generate: stop premature variable substitution in commands

go:generate commands passed no arguments are currently subject
to premature variable substitution due to mistakenly assuming append
guarantees a copy.  The change fixes this by forcing a slice copy at
each invocation of a command.

The previous code assumed that append would always generate a
copy of its inputs. However, append wouldn't create a copy if there was
no need to increase capacity and it would just return the original
input slice. This resulted in premature variable substitutions in
the "master word list" of generate commands, thus yielding incorrect
results across multiple invocations of the same command when the
body contained substitutions e.g. environment variables, moreover
these can change during the lifetime of go:generate processing a
file.

Note that this behavior would not manifest itself if any arguments were
passed to the command, because append would make a copy of the slice
as it needed to increase its capacity.   The "hacky" work-around was to
always pass at least one argument to any command, even if the
command ignores it.  e.g.,
       //go:generate MyNoArgsCmd ' '

This CL fixes that issue and removes the need for the hack mentioned
above.

Fixes #31608

Change-Id: I782ac2234bd7035a37f61c101ee4aee38ed8d29f
GitHub-Last-Rev: 796d343019
GitHub-Pull-Request: golang/go#31527
Reviewed-on: https://go-review.googlesource.com/c/go/+/172580
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

src/cmd/go/internal/generate/generate.go

@@ -374,7 +374,12 @@ Words:
 	// Substitute command if required.
 	if len(words) > 0 && g.commands[words[0]] != nil {
 		// Replace 0th word by command substitution.
-		words = append(g.commands[words[0]], words[1:]...)
+		//
+		// Force a copy of the command definition to
+		// ensure words doesn't end up as a reference
+		// to the g.commands content.
+		tmpCmdWords := append([]string(nil), (g.commands[words[0]])...)
+		words = append(tmpCmdWords, words[1:]...)
 	}
 	// Substitute environment variables.
 	for i, word := range words {

src/cmd/go/internal/generate/generate_test.go

@@ -5,6 +5,7 @@
 package generate
 
 import (
+	"os"
 	"reflect"
 	"runtime"
 	"testing"
@@ -15,6 +16,15 @@ type splitTest struct {
 	out []string
 }
 
+// Same as above, except including source line number to set
+type splitTestWithLine struct {
+	in         string
+	out        []string
+	lineNumber int
+}
+
+const anyLineNo = 0
+
 var splitTests = []splitTest{
 	{"", nil},
 	{"x", []string{"x"}},
@@ -54,3 +64,191 @@ func TestGenerateCommandParse(t *testing.T) {
 		}
 	}
 }
+
+// These environment variables will be undefined before the splitTestWithLine tests
+var undefEnvList = []string{
+	"_XYZZY_",
+}
+
+// These environment variables will be defined before the splitTestWithLine tests
+var defEnvMap = map[string]string{
+	"_PLUGH_": "SomeVal",
+	"_X":      "Y",
+}
+
+// TestGenerateCommandShortHand - similar to TestGenerateCommandParse,
+// except:
+//    1. if the result starts with -command, record that shorthand
+//       before moving on to the next test.
+//    2. If a source line number is specified, set that in the parser
+//       before executing the test.  i.e., execute the split as if it
+//       processing that source line.
+func TestGenerateCommandShorthand(t *testing.T) {
+	g := &Generator{
+		r:        nil, // Unused here.
+		path:     "/usr/ken/sys/proc.go",
+		dir:      "/usr/ken/sys",
+		file:     "proc.go",
+		pkg:      "sys",
+		commands: make(map[string][]string),
+	}
+
+	var inLine string
+	var expected, got []string
+
+	g.setEnv()
+
+	// Set up the system environment variables
+	for i := range undefEnvList {
+		os.Unsetenv(undefEnvList[i])
+	}
+	for k := range defEnvMap {
+		os.Setenv(k, defEnvMap[k])
+	}
+
+	// simple command from environment variable
+	inLine = "//go:generate -command CMD0 \"ab${_X}cd\""
+	expected = []string{"-command", "CMD0", "abYcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	// try again, with an extra level of indirection (should leave variable in command)
+	inLine = "//go:generate -command CMD0 \"ab${DOLLAR}{_X}cd\""
+	expected = []string{"-command", "CMD0", "ab${_X}cd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	// Now the interesting part, record that output as a command
+	g.setShorthand(got)
+
+	// see that the command still substitutes correctly from env. variable
+	inLine = "//go:generate CMD0"
+	expected = []string{"abYcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	// Now change the value of $X and see if the recorded definition is
+	// still intact (vs. having the $_X already substituted out)
+
+	os.Setenv("_X", "Z")
+	inLine = "//go:generate CMD0"
+	expected = []string{"abZcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	// What if the variable is now undefined?  Should be empty substitution.
+
+	os.Unsetenv("_X")
+	inLine = "//go:generate CMD0"
+	expected = []string{"abcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	// Try another undefined variable as an extra check
+	os.Unsetenv("_Z")
+	inLine = "//go:generate -command CMD1 \"ab${_Z}cd\""
+	expected = []string{"-command", "CMD1", "abcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	g.setShorthand(got)
+
+	inLine = "//go:generate CMD1"
+	expected = []string{"abcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	const val = "someNewValue"
+	os.Setenv("_Z", val)
+
+	// try again with the properly-escaped variable.
+
+	inLine = "//go:generate -command CMD2 \"ab${DOLLAR}{_Z}cd\""
+	expected = []string{"-command", "CMD2", "ab${_Z}cd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	g.setShorthand(got)
+
+	inLine = "//go:generate CMD2"
+	expected = []string{"ab" + val + "cd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+}
+
+// Command-related tests for TestGenerateCommandShortHand2
+// -- Note line numbers included to check substitutions from "build-in" variable - $GOLINE
+var splitTestsLines = []splitTestWithLine{
+	{"-command TEST1 $GOLINE", []string{"-command", "TEST1", "22"}, 22},
+	{"-command TEST2 ${DOLLAR}GOLINE", []string{"-command", "TEST2", "$GOLINE"}, 26},
+	{"TEST1", []string{"22"}, 33},
+	{"TEST2", []string{"66"}, 66},
+	{"TEST1 ''", []string{"22", "''"}, 99},
+	{"TEST2 ''", []string{"44", "''"}, 44},
+}
+
+// TestGenerateCommandShortHand - similar to TestGenerateCommandParse,
+// except:
+//    1. if the result starts with -command, record that shorthand
+//       before moving on to the next test.
+//    2. If a source line number is specified, set that in the parser
+//       before executing the test.  i.e., execute the split as if it
+//       processing that source line.
+func TestGenerateCommandShortHand2(t *testing.T) {
+	g := &Generator{
+		r:        nil, // Unused here.
+		path:     "/usr/ken/sys/proc.go",
+		dir:      "/usr/ken/sys",
+		file:     "proc.go",
+		pkg:      "sys",
+		commands: make(map[string][]string),
+	}
+	g.setEnv()
+	for _, test := range splitTestsLines {
+		// if the test specified a line number, reflect that
+		if test.lineNumber != anyLineNo {
+			g.lineNum = test.lineNumber
+			g.setEnv()
+		}
+		// First with newlines.
+		got := g.split("//go:generate " + test.in + "\n")
+		if !reflect.DeepEqual(got, test.out) {
+			t.Errorf("split(%q): got %q expected %q", test.in, got, test.out)
+		}
+		// Then with CRLFs, thank you Windows.
+		got = g.split("//go:generate " + test.in + "\r\n")
+		if !reflect.DeepEqual(got, test.out) {
+			t.Errorf("split(%q): got %q expected %q", test.in, got, test.out)
+		}
+		if got[0] == "-command" { // record commands
+			g.setShorthand(got)
+		}
+	}
+}