go/build: prefer //go:build over // +build lines

Part of //go:build change (#41184).
See https://golang.org/design/draft-gobuild

- Reject files with multiple //go:build lines.
- If a file has both //go:build and // +build lines, only use the //go:build line.
- Otherwise fall back to // +build lines
- Use go/build/constraint for parsing both //go:build and // +build lines.

For Go 1.17.

Change-Id: I32e2404d8ce266230f767718dc7cc24e77b425e8
Reviewed-on: https://go-review.googlesource.com/c/go/+/240607
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

src/go/build/build.go

@@ -9,6 +9,7 @@ import (
 	"errors"
 	"fmt"
 	"go/ast"
+	"go/build/constraint"
 	"go/doc"
 	"go/token"
 	exec "internal/execabs"
@@ -1423,7 +1424,7 @@ func (ctxt *Context) matchFile(dir, name string, allTags map[string]bool, binary
 	// Look for +build comments to accept or reject the file.
 	ok, sawBinaryOnly, err := ctxt.shouldBuild(info.header, allTags)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("%s: %v", name, err)
 	}
 	if !ok && !ctxt.UseAllFiles {
 		return nil, nil
@@ -1459,11 +1460,12 @@ var (
 	bSlashSlash = []byte(slashSlash)
 	bStarSlash  = []byte(starSlash)
 	bSlashStar  = []byte(slashStar)
+	bPlusBuild  = []byte("+build")
 
 	goBuildComment = []byte("//go:build")
 
 	errGoBuildWithoutBuild = errors.New("//go:build comment without // +build comment")
-	errMultipleGoBuild     = errors.New("multiple //go:build comments") // unused in Go 1.(N-1)
+	errMultipleGoBuild     = errors.New("multiple //go:build comments")
 )
 
 func isGoBuildComment(line []byte) bool {
@@ -1498,8 +1500,7 @@ var binaryOnlyComment = []byte("//go:binary-only-package")
 // shouldBuild reports whether the file should be built
 // and whether a //go:binary-only-package comment was found.
 func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool) (shouldBuild, binaryOnly bool, err error) {
-
+	// Identify leading run of // comments and blank lines,
-	// Pass 1. Identify leading run of // comments and blank lines,
 	// which must be followed by a blank line.
 	// Also identify any //go:build comments.
 	content, goBuild, sawBinaryOnly, err := parseFileHeader(content)
@@ -1507,44 +1508,42 @@ func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool) (shoul
 		return false, false, err
 	}
 
-	// Pass 2.  Process each +build line in the run.
+	// If //go:build line is present, it controls.
-	p := content
+	// Otherwise fall back to +build processing.
-	shouldBuild = true
+	switch {
-	sawBuild := false
+	case goBuild != nil:
-	for len(p) > 0 {
+		x, err := constraint.Parse(string(goBuild))
-		line := p
+		if err != nil {
-		if i := bytes.IndexByte(line, '\n'); i >= 0 {
+			return false, false, fmt.Errorf("parsing //go:build line: %v", err)
-			line, p = line[:i], p[i+1:]
-		} else {
-			p = p[len(p):]
 		}
-		line = bytes.TrimSpace(line)
+		shouldBuild = ctxt.eval(x, allTags)
-		if !bytes.HasPrefix(line, bSlashSlash) {
+
-			continue
+	default:
-		}
+		shouldBuild = true
-		line = bytes.TrimSpace(line[len(bSlashSlash):])
+		p := content
-		if len(line) > 0 && line[0] == '+' {
+		for len(p) > 0 {
-			// Looks like a comment +line.
+			line := p
-			f := strings.Fields(string(line))
+			if i := bytes.IndexByte(line, '\n'); i >= 0 {
-			if f[0] == "+build" {
+				line, p = line[:i], p[i+1:]
-				sawBuild = true
+			} else {
-				ok := false
+				p = p[len(p):]
-				for _, tok := range f[1:] {
+			}
-					if ctxt.match(tok, allTags) {
+			line = bytes.TrimSpace(line)
-						ok = true
+			if !bytes.HasPrefix(line, bSlashSlash) || !bytes.Contains(line, bPlusBuild) {
-					}
+				continue
-				}
+			}
-				if !ok {
+			text := string(line)
+			if !constraint.IsPlusBuild(text) {
+				continue
+			}
+			if x, err := constraint.Parse(text); err == nil {
+				if !ctxt.eval(x, allTags) {
 					shouldBuild = false
 				}
 			}
 		}
 	}
 
-	if goBuild != nil && !sawBuild {
-		return false, false, errGoBuildWithoutBuild
-	}
-
 	return shouldBuild, sawBinaryOnly, nil
 }
 
@@ -1580,7 +1579,7 @@ Lines:
 		}
 
 		if !inSlashStar && isGoBuildComment(line) {
-			if false && goBuild != nil { // enabled in Go 1.N
+			if goBuild != nil {
 				return nil, nil, false, errMultipleGoBuild
 			}
 			goBuild = line
@@ -1649,7 +1648,7 @@ func (ctxt *Context) saveCgo(filename string, di *Package, cg *ast.CommentGroup)
 		if len(cond) > 0 {
 			ok := false
 			for _, c := range cond {
-				if ctxt.match(c, nil) {
+				if ctxt.matchAuto(c, nil) {
 					ok = true
 					break
 				}
@@ -1831,50 +1830,44 @@ func splitQuoted(s string) (r []string, err error) {
 	return args, err
 }
 
-// match reports whether the name is one of:
+// matchAuto interprets text as either a +build or //go:build expression (whichever works),
+// reporting whether the expression matches the build context.
 //
+// matchAuto is only used for testing of tag evaluation
+// and in #cgo lines, which accept either syntax.
+func (ctxt *Context) matchAuto(text string, allTags map[string]bool) bool {
+	if strings.ContainsAny(text, "&|()") {
+		text = "//go:build " + text
+	} else {
+		text = "// +build " + text
+	}
+	x, err := constraint.Parse(text)
+	if err != nil {
+		return false
+	}
+	return ctxt.eval(x, allTags)
+}
+
+func (ctxt *Context) eval(x constraint.Expr, allTags map[string]bool) bool {
+	return x.Eval(func(tag string) bool { return ctxt.matchTag(tag, allTags) })
+}
+
+// matchTag reports whether the name is one of:
+//
+//	cgo (if cgo is enabled)
 //	$GOOS
 //	$GOARCH
-//	cgo (if cgo is enabled)
-//	!cgo (if cgo is disabled)
 //	ctxt.Compiler
-//	!ctxt.Compiler
+//	linux (if GOOS = android)
+//	solaris (if GOOS = illumos)
 //	tag (if tag is listed in ctxt.BuildTags or ctxt.ReleaseTags)
-//	!tag (if tag is not listed in ctxt.BuildTags or ctxt.ReleaseTags)
-//	a comma-separated list of any of these
 //
-func (ctxt *Context) match(name string, allTags map[string]bool) bool {
+// It records all consulted tags in allTags.
-	if name == "" {
+func (ctxt *Context) matchTag(name string, allTags map[string]bool) bool {
-		if allTags != nil {
-			allTags[name] = true
-		}
-		return false
-	}
-	if i := strings.Index(name, ","); i >= 0 {
-		// comma-separated list
-		ok1 := ctxt.match(name[:i], allTags)
-		ok2 := ctxt.match(name[i+1:], allTags)
-		return ok1 && ok2
-	}
-	if strings.HasPrefix(name, "!!") { // bad syntax, reject always
-		return false
-	}
-	if strings.HasPrefix(name, "!") { // negation
-		return len(name) > 1 && !ctxt.match(name[1:], allTags)
-	}
-
 	if allTags != nil {
 		allTags[name] = true
 	}
 
-	// Tags must be letters, digits, underscores or dots.
-	// Unlike in Go identifiers, all digits are fine (e.g., "386").
-	for _, c := range name {
-		if !unicode.IsLetter(c) && !unicode.IsDigit(c) && c != '_' && c != '.' {
-			return false
-		}
-	}
-
 	// special tags
 	if ctxt.CgoEnabled && name == "cgo" {
 		return true
@@ -1946,10 +1939,10 @@ func (ctxt *Context) goodOSArchFile(name string, allTags map[string]bool) bool {
 	}
 	n := len(l)
 	if n >= 2 && knownOS[l[n-2]] && knownArch[l[n-1]] {
-		return ctxt.match(l[n-1], allTags) && ctxt.match(l[n-2], allTags)
+		return ctxt.matchTag(l[n-1], allTags) && ctxt.matchTag(l[n-2], allTags)
 	}
 	if n >= 1 && (knownOS[l[n-1]] || knownArch[l[n-1]]) {
-		return ctxt.match(l[n-1], allTags)
+		return ctxt.matchTag(l[n-1], allTags)
 	}
 	return true
 }

src/go/build/build_test.go

@@ -30,7 +30,7 @@ func TestMatch(t *testing.T) {
 	match := func(tag string, want map[string]bool) {
 		t.Helper()
 		m := make(map[string]bool)
-		if !ctxt.match(tag, m) {
+		if !ctxt.matchAuto(tag, m) {
 			t.Errorf("%s context should match %s, does not", what, tag)
 		}
 		if !reflect.DeepEqual(m, want) {
@@ -40,7 +40,7 @@ func TestMatch(t *testing.T) {
 	nomatch := func(tag string, want map[string]bool) {
 		t.Helper()
 		m := make(map[string]bool)
-		if ctxt.match(tag, m) {
+		if ctxt.matchAuto(tag, m) {
 			t.Errorf("%s context should NOT match %s, does", what, tag)
 		}
 		if !reflect.DeepEqual(m, want) {
@@ -153,6 +153,13 @@ var shouldBuildTests = []struct {
 		tags:        map[string]bool{"yes": true},
 		shouldBuild: true,
 	},
+	{
+		name: "Yes2",
+		content: "//go:build yes\n" +
+			"package main\n",
+		tags:        map[string]bool{"yes": true},
+		shouldBuild: true,
+	},
 	{
 		name: "Or",
 		content: "// +build no yes\n\n" +
@@ -160,6 +167,13 @@ var shouldBuildTests = []struct {
 		tags:        map[string]bool{"yes": true, "no": true},
 		shouldBuild: true,
 	},
+	{
+		name: "Or2",
+		content: "//go:build no || yes\n" +
+			"package main\n",
+		tags:        map[string]bool{"yes": true, "no": true},
+		shouldBuild: true,
+	},
 	{
 		name: "And",
 		content: "// +build no,yes\n\n" +
@@ -167,6 +181,13 @@ var shouldBuildTests = []struct {
 		tags:        map[string]bool{"yes": true, "no": true},
 		shouldBuild: false,
 	},
+	{
+		name: "And2",
+		content: "//go:build no && yes\n" +
+			"package main\n",
+		tags:        map[string]bool{"yes": true, "no": true},
+		shouldBuild: false,
+	},
 	{
 		name: "Cgo",
 		content: "// +build cgo\n\n" +
@@ -177,12 +198,23 @@ var shouldBuildTests = []struct {
 		tags:        map[string]bool{"cgo": true},
 		shouldBuild: false,
 	},
+	{
+		name: "Cgo2",
+		content: "//go:build cgo\n" +
+			"// Copyright The Go Authors.\n\n" +
+			"// This package implements parsing of tags like\n" +
+			"// +build tag1\n" +
+			"package build",
+		tags:        map[string]bool{"cgo": true},
+		shouldBuild: false,
+	},
 	{
 		name: "AfterPackage",
 		content: "// Copyright The Go Authors.\n\n" +
 			"package build\n\n" +
 			"// shouldBuild checks tags given by lines of the form\n" +
 			"// +build tag\n" +
+			"//go:build tag\n" +
 			"func shouldBuild(content []byte)\n",
 		tags:        map[string]bool{},
 		shouldBuild: true,
@@ -194,6 +226,13 @@ var shouldBuildTests = []struct {
 		tags:        map[string]bool{},
 		shouldBuild: true,
 	},
+	{
+		name: "TooClose2",
+		content: "//go:build yes\n" +
+			"package main\n",
+		tags:        map[string]bool{"yes": true},
+		shouldBuild: true,
+	},
 	{
 		name: "TooCloseNo",
 		content: "// +build no\n" +
@@ -201,6 +240,13 @@ var shouldBuildTests = []struct {
 		tags:        map[string]bool{},
 		shouldBuild: true,
 	},
+	{
+		name: "TooCloseNo2",
+		content: "//go:build no\n" +
+			"package main\n",
+		tags:        map[string]bool{"no": true},
+		shouldBuild: false,
+	},
 	{
 		name: "BinaryOnly",
 		content: "//go:binary-only-package\n" +
@@ -210,21 +256,22 @@ var shouldBuildTests = []struct {
 		binaryOnly:  true,
 		shouldBuild: true,
 	},
+	{
+		name: "BinaryOnly2",
+		content: "//go:binary-only-package\n" +
+			"//go:build no\n" +
+			"package main\n",
+		tags:        map[string]bool{"no": true},
+		binaryOnly:  true,
+		shouldBuild: false,
+	},
 	{
 		name: "ValidGoBuild",
 		content: "// +build yes\n\n" +
 			"//go:build no\n" +
 			"package main\n",
-		tags:        map[string]bool{"yes": true},
+		tags:        map[string]bool{"no": true},
-		shouldBuild: true,
-	},
-	{
-		name: "MissingBuild",
-		content: "//go:build no\n" +
-			"package main\n",
-		tags:        map[string]bool{},
 		shouldBuild: false,
-		err:         errGoBuildWithoutBuild,
 	},
 	{
 		name: "MissingBuild2",
@@ -232,20 +279,8 @@ var shouldBuildTests = []struct {
 			"// +build yes\n\n" +
 			"//go:build no\n" +
 			"package main\n",
-		tags:        map[string]bool{},
+		tags:        map[string]bool{"no": true},
 		shouldBuild: false,
-		err:         errGoBuildWithoutBuild,
-	},
-	{
-		name: "MissingBuild2",
-		content: "/*\n" +
-			"// +build yes\n\n" +
-			"*/\n" +
-			"//go:build no\n" +
-			"package main\n",
-		tags:        map[string]bool{},
-		shouldBuild: false,
-		err:         errGoBuildWithoutBuild,
 	},
 	{
 		name: "Comment1",
@@ -263,9 +298,8 @@ var shouldBuildTests = []struct {
 			"*/\n\n" +
 			"//go:build no\n" +
 			"package main\n",
-		tags:        map[string]bool{},
+		tags:        map[string]bool{"no": true},
 		shouldBuild: false,
-		err:         errGoBuildWithoutBuild,
 	},
 	{
 		name: "Comment3",
@@ -274,9 +308,8 @@ var shouldBuildTests = []struct {
 			"*/\n\n" +
 			"//go:build no\n" +
 			"package main\n",
-		tags:        map[string]bool{},
+		tags:        map[string]bool{"no": true},
 		shouldBuild: false,
-		err:         errGoBuildWithoutBuild,
 	},
 	{
 		name: "Comment4",
@@ -290,9 +323,8 @@ var shouldBuildTests = []struct {
 		content: "/**/\n" +
 			"//go:build no\n" +
 			"package main\n",
-		tags:        map[string]bool{},
+		tags:        map[string]bool{"no": true},
 		shouldBuild: false,
-		err:         errGoBuildWithoutBuild,
 	},
 }
 

src/go/build/deps_test.go

@@ -295,7 +295,7 @@ var depsRules = `
 	FMT
 	< go/build/constraint;
 
-	go/doc, go/parser, internal/goroot, internal/goversion
+	go/build/constraint, go/doc, go/parser, internal/goroot, internal/goversion
 	< go/build;
 
 	DEBUG, go/build, go/types, text/scanner