go/buildutil: permit comma delimiters in build tags

The buildutil.TagsFlag must maintain parity with the go build
command, but the duplicated implementations have drifted apart
over time. Starting in go 1.13 the go build command began
honoring comma xor space delimiters while buildutil.TagsFlag
only recognized spaces as of go 1.22.

Test coverage has been updated to use a table with more cases to
affirm expected behaviors, and to detect regressions that occur
should the go build command change its implementation again.

Fixes golang/go#44787

Change-Id: I210074a8fc5aaaed504e81633fc93aeacb142d9e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/558115
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>

go/buildutil/tags.go

@@ -4,17 +4,22 @@
 
 package buildutil
 
-// This logic was copied from stringsFlag from $GOROOT/src/cmd/go/build.go.
+// This duplicated logic must be kept in sync with that from go build:
+//   $GOROOT/src/cmd/go/internal/work/build.go (tagsFlag.Set)
+//   $GOROOT/src/cmd/go/internal/base/flag.go (StringsFlag.Set)
+//   $GOROOT/src/cmd/internal/quoted/quoted.go (isSpaceByte, Split)
 
-import "fmt"
+import (
+	"fmt"
+	"strings"
+)
 
 const TagsFlagDoc = "a list of `build tags` to consider satisfied during the build. " +
 	"For more information about build tags, see the description of " +
 	"build constraints in the documentation for the go/build package"
 
 // TagsFlag is an implementation of the flag.Value and flag.Getter interfaces that parses
-// a flag value in the same manner as go build's -tags flag and
-// populates a []string slice.
+// a flag value the same as go build's -tags flag and populates a []string slice.
 //
 // See $GOROOT/src/go/build/doc.go for description of build tags.
 // See $GOROOT/src/cmd/go/doc.go for description of 'go build -tags' flag.
@@ -25,19 +30,32 @@ const TagsFlagDoc = "a list of `build tags` to consider satisfied during the bui
 type TagsFlag []string
 
 func (v *TagsFlag) Set(s string) error {
-	var err error
-	*v, err = splitQuotedFields(s)
-	if *v == nil {
-		*v = []string{}
+	// See $GOROOT/src/cmd/go/internal/work/build.go (tagsFlag.Set)
+	// For compatibility with Go 1.12 and earlier, allow "-tags='a b c'" or even just "-tags='a'".
+	if strings.Contains(s, " ") || strings.Contains(s, "'") {
+		var err error
+		*v, err = splitQuotedFields(s)
+		if *v == nil {
+			*v = []string{}
+		}
+		return err
+	}
+
+	// Starting in Go 1.13, the -tags flag is a comma-separated list of build tags.
+	*v = []string{}
+	for _, s := range strings.Split(s, ",") {
+		if s != "" {
+			*v = append(*v, s)
+		}
 	}
-	return err
+	return nil
 }
 
 func (v *TagsFlag) Get() interface{} { return *v }
 
 func splitQuotedFields(s string) ([]string, error) {
-	// Split fields allowing '' or "" around elements.
-	// Quotes further inside the string do not count.
+	// See $GOROOT/src/cmd/internal/quoted/quoted.go (Split)
+	// This must remain in sync with that logic.
 	var f []string
 	for len(s) > 0 {
 		for len(s) > 0 && isSpaceByte(s[0]) {
@@ -76,5 +94,7 @@ func (v *TagsFlag) String() string {
 }
 
 func isSpaceByte(c byte) bool {
+	// See $GOROOT/src/cmd/internal/quoted/quoted.go (isSpaceByte, Split)
+	// This list must remain in sync with that.
 	return c == ' ' || c == '\t' || c == '\n' || c == '\r'
 }

go/buildutil/tags_test.go

@@ -5,28 +5,124 @@
 package buildutil_test
 
 import (
+	"bytes"
 	"flag"
 	"go/build"
+	"os/exec"
 	"reflect"
+	"strings"
 	"testing"
 
 	"golang.org/x/tools/go/buildutil"
+	"golang.org/x/tools/internal/testenv"
 )
 
 func TestTags(t *testing.T) {
-	f := flag.NewFlagSet("TestTags", flag.PanicOnError)
-	var ctxt build.Context
-	f.Var((*buildutil.TagsFlag)(&ctxt.BuildTags), "tags", buildutil.TagsFlagDoc)
-	f.Parse([]string{"-tags", ` 'one'"two"	'three "four"'`, "rest"})
-
-	// BuildTags
-	want := []string{"one", "two", "three \"four\""}
-	if !reflect.DeepEqual(ctxt.BuildTags, want) {
-		t.Errorf("BuildTags = %q, want %q", ctxt.BuildTags, want)
+
+	type tagTestCase struct {
+		tags    string
+		want    []string
+		wantErr bool
 	}
 
-	// Args()
-	if want := []string{"rest"}; !reflect.DeepEqual(f.Args(), want) {
-		t.Errorf("f.Args() = %q, want %q", f.Args(), want)
+	for name, tc := range map[string]tagTestCase{
+		// Normal valid cases
+		"empty": {
+			tags: "",
+			want: []string{},
+		},
+		"commas": {
+			tags: "tag1,tag_2,🐹,tag/3,tag-4",
+			want: []string{"tag1", "tag_2", "🐹", "tag/3", "tag-4"},
+		},
+		"delimiters are spaces": {
+			tags: "a b\tc\rd\ne",
+			want: []string{"a", "b", "c", "d", "e"},
+		},
+		"old quote and space form": {
+			tags: "'a' 'b' 'c'",
+			want: []string{"a", "b", "c"},
+		},
+
+		// Normal error cases
+		"unterminated": {
+			tags:    `"missing closing quote`,
+			want:    []string{},
+			wantErr: true,
+		},
+		"unterminated single": {
+			tags:    `'missing closing quote`,
+			want:    []string{},
+			wantErr: true,
+		},
+
+		// Maybe surprising difference for unterminated quotes, no spaces
+		"unterminated no spaces": {
+			tags: `"missing_closing_quote`,
+			want: []string{"\"missing_closing_quote"},
+		},
+		"unterminated no spaces single": {
+			tags:    `'missing_closing_quote`,
+			want:    []string{},
+			wantErr: true,
+		},
+
+		// Permitted but not recommended
+		"delimiters contiguous spaces": {
+			tags: "a \t\r\n, b \t\r\nc,d\te\tf",
+			want: []string{"a", ",", "b", "c,d", "e", "f"},
+		},
+		"quotes and spaces": {
+			tags: ` 'one'"two"	'three "four"'`,
+			want: []string{"one", "two", "three \"four\""},
+		},
+		"quotes single no spaces": {
+			tags: `'t1','t2',"t3"`,
+			want: []string{"t1", ",'t2',\"t3\""},
+		},
+		"quotes double no spaces": {
+			tags: `"t1","t2","t3"`,
+			want: []string{`"t1"`, `"t2"`, `"t3"`},
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			f := flag.NewFlagSet("TestTags", flag.ContinueOnError)
+			var ctxt build.Context
+			f.Var((*buildutil.TagsFlag)(&ctxt.BuildTags), "tags", buildutil.TagsFlagDoc)
+
+			// Normal case valid parsed tags
+			f.Parse([]string{"-tags", tc.tags, "rest"})
+
+			// BuildTags
+			if !reflect.DeepEqual(ctxt.BuildTags, tc.want) {
+				t.Errorf("Case = %s, BuildTags = %q, want %q", name, ctxt.BuildTags, tc.want)
+			}
+
+			// Args()
+			if want := []string{"rest"}; !reflect.DeepEqual(f.Args(), want) {
+				t.Errorf("Case = %s, f.Args() = %q, want %q", name, f.Args(), want)
+			}
+
+			// Regression check against base go tooling
+			cmd := testenv.Command(t, "go", "list", "-f", "{{context.BuildTags}}", "-tags", tc.tags, ".")
+			var out bytes.Buffer
+			cmd.Stdout = &out
+			if err := cmd.Run(); err != nil {
+				if ee, ok := err.(*exec.ExitError); ok && len(ee.Stderr) > 0 {
+					t.Logf("stderr:\n%s", ee.Stderr)
+				}
+				if !tc.wantErr {
+					t.Errorf("%v: %v", cmd, err)
+				}
+			} else if tc.wantErr {
+				t.Errorf("Expected failure for %v", cmd)
+			} else {
+				wantDescription := strings.Join(tc.want, " ")
+				output := strings.Trim(strings.TrimSuffix(out.String(), "\n"), "[]")
+				if output != wantDescription {
+					t.Errorf("Output = %s, want %s", output, wantDescription)
+				}
+			}
+		})
 	}
 }