go/build: reject //go:build without // +build

We are converting from using error-prone ad-hoc syntax // +build lines
to less error-prone, standard boolean syntax //go:build lines.
The timeline is:

Go 1.16: prepare for transition
 - Builds still use // +build for file selection.
 - Source files may not contain //go:build without // +build.
 - Builds fail when a source file contains //go:build lines without // +build lines. <<<

Go 1.17: start transition
 - Builds prefer //go:build for file selection, falling back to // +build
   for files containing only // +build.
 - Source files may contain //go:build without // +build (but they won't build with Go 1.16).
 - Gofmt moves //go:build and // +build lines to proper file locations.
 - Gofmt introduces //go:build lines into files with only // +build lines.
 - Go vet rejects files with mismatched //go:build and // +build lines.

Go 1.18: complete transition
 - Go fix removes // +build lines, leaving behind equivalent // +build lines.

This CL provides part of the <<< marked line above in the Go 1.16 step:
rejecting files containing //go:build but not // +build.

For #41184.

Change-Id: I29b8a789ab1526ab5057f613d5533bd2060ba9cd
Reviewed-on: https://go-review.googlesource.com/c/go/+/240600
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

2 files changed, 218 insertions, 43 deletions

diff --git a/src/go/build/build.go b/src/go/build/build.go
index 86daf7c057..7a96680e77 100644
--- a/src/go/build/build.go
+++ b/src/go/build/build.go
@@ -1371,9 +1371,12 @@ func (ctxt *Context) matchFile(dir, name string, allTags map[string]bool, binary
 	}
 
 	// Look for +build comments to accept or reject the file.
-	ok, sawBinaryOnly := ctxt.shouldBuild(data, allTags)
+	ok, sawBinaryOnly, err := ctxt.shouldBuild(data, allTags)
+	if err != nil {
+		return // non-nil err
+	}
 	if !ok && !ctxt.UseAllFiles {
-		return
+		return // nil err
 	}
 
 	if binaryOnly != nil && sawBinaryOnly {
@@ -1402,7 +1405,25 @@ func ImportDir(dir string, mode ImportMode) (*Package, error) {
 	return Default.ImportDir(dir, mode)
 }
 
-var slashslash = []byte("//")
+var (
+	bSlashSlash = []byte(slashSlash)
+	bStarSlash  = []byte(starSlash)
+	bSlashStar  = []byte(slashStar)
+
+	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)
+)
+
+func isGoBuildComment(line []byte) bool {
+	if !bytes.HasPrefix(line, goBuildComment) {
+		return false
+	}
+	line = bytes.TrimSpace(line)
+	rest := line[len(goBuildComment):]
+	return len(rest) == 0 || len(bytes.TrimSpace(rest)) < len(rest)
+}
 
 // Special comment denoting a binary-only package.
 // See https://golang.org/design/2775-binary-only-packages
@@ -1426,34 +1447,20 @@ 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 bool, binaryOnly bool) {
-	sawBinaryOnly := false
+func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool) (shouldBuild, binaryOnly bool, err error) {
 
 	// Pass 1. Identify leading run of // comments and blank lines,
 	// which must be followed by a blank line.
-	end := 0
-	p := content
-	for len(p) > 0 {
-		line := p
-		if i := bytes.IndexByte(line, '\n'); i >= 0 {
-			line, p = line[:i], p[i+1:]
-		} else {
-			p = p[len(p):]
-		}
-		line = bytes.TrimSpace(line)
-		if len(line) == 0 { // Blank line
-			end = len(content) - len(p)
-			continue
-		}
-		if !bytes.HasPrefix(line, slashslash) { // Not comment line
-			break
-		}
+	// Also identify any //go:build comments.
+	content, goBuild, sawBinaryOnly, err := parseFileHeader(content)
+	if err != nil {
+		return false, false, err
 	}
-	content = content[:end]
 
-	// Pass 2.  Process each line in the run.
-	p = content
+	// Pass 2.  Process each +build line in the run.
+	p := content
 	shouldBuild = true
+	sawBuild := false
 	for len(p) > 0 {
 		line := p
 		if i := bytes.IndexByte(line, '\n'); i >= 0 {
@@ -1462,17 +1469,15 @@ func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool) (shoul
 			p = p[len(p):]
 		}
 		line = bytes.TrimSpace(line)
-		if !bytes.HasPrefix(line, slashslash) {
+		if !bytes.HasPrefix(line, bSlashSlash) {
 			continue
 		}
-		if bytes.Equal(line, binaryOnlyComment) {
-			sawBinaryOnly = true
-		}
-		line = bytes.TrimSpace(line[len(slashslash):])
+		line = bytes.TrimSpace(line[len(bSlashSlash):])
 		if len(line) > 0 && line[0] == '+' {
 			// Looks like a comment +line.
 			f := strings.Fields(string(line))
 			if f[0] == "+build" {
+				sawBuild = true
 				ok := false
 				for _, tok := range f[1:] {
 					if ctxt.match(tok, allTags) {
@@ -1486,7 +1491,78 @@ func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool) (shoul
 		}
 	}
 
-	return shouldBuild, sawBinaryOnly
+	if goBuild != nil && !sawBuild {
+		return false, false, errGoBuildWithoutBuild
+	}
+
+	return shouldBuild, sawBinaryOnly, nil
+}
+
+func parseFileHeader(content []byte) (trimmed, goBuild []byte, sawBinaryOnly bool, err error) {
+	end := 0
+	p := content
+	ended := false       // found non-blank, non-// line, so stopped accepting // +build lines
+	inSlashStar := false // in /* */ comment
+
+Lines:
+	for len(p) > 0 {
+		line := p
+		if i := bytes.IndexByte(line, '\n'); i >= 0 {
+			line, p = line[:i], p[i+1:]
+		} else {
+			p = p[len(p):]
+		}
+		line = bytes.TrimSpace(line)
+		if len(line) == 0 && !ended { // Blank line
+			// Remember position of most recent blank line.
+			// When we find the first non-blank, non-// line,
+			// this "end" position marks the latest file position
+			// where a // +build line can appear.
+			// (It must appear _before_ a blank line before the non-blank, non-// line.
+			// Yes, that's confusing, which is part of why we moved to //go:build lines.)
+			// Note that ended==false here means that inSlashStar==false,
+			// since seeing a /* would have set ended==true.
+			end = len(content) - len(p)
+			continue Lines
+		}
+		if !bytes.HasPrefix(line, slashSlash) { // Not comment line
+			ended = true
+		}
+
+		if !inSlashStar && isGoBuildComment(line) {
+			if false && goBuild != nil { // enabled in Go 1.N
+				return nil, nil, false, errMultipleGoBuild
+			}
+			goBuild = line
+		}
+		if !inSlashStar && bytes.Equal(line, binaryOnlyComment) {
+			sawBinaryOnly = true
+		}
+
+	Comments:
+		for len(line) > 0 {
+			if inSlashStar {
+				if i := bytes.Index(line, starSlash); i >= 0 {
+					inSlashStar = false
+					line = bytes.TrimSpace(line[i+len(starSlash):])
+					continue Comments
+				}
+				continue Lines
+			}
+			if bytes.HasPrefix(line, bSlashSlash) {
+				continue Lines
+			}
+			if bytes.HasPrefix(line, bSlashStar) {
+				inSlashStar = true
+				line = bytes.TrimSpace(line[len(bSlashStar):])
+				continue Comments
+			}
+			// Found non-comment text.
+			break Lines
+		}
+	}
+
+	return content[:end], goBuild, sawBinaryOnly, nil
 }
 
 // saveCgo saves the information from the #cgo lines in the import "C" comment.
diff --git a/src/go/build/build_test.go b/src/go/build/build_test.go
index cec5186a30..3a4ad22f46 100644
--- a/src/go/build/build_test.go
+++ b/src/go/build/build_test.go
@@ -6,7 +6,6 @@ package build
 
 import (
 	"flag"
-	"fmt"
 	"internal/testenv"
 	"io"
 	"io/ioutil"
@@ -140,30 +139,36 @@ func TestLocalDirectory(t *testing.T) {
 }
 
 var shouldBuildTests = []struct {
+	name        string
 	content     string
 	tags        map[string]bool
 	binaryOnly  bool
 	shouldBuild bool
+	err         error
 }{
 	{
+		name: "Yes",
 		content: "// +build yes\n\n" +
 			"package main\n",
 		tags:        map[string]bool{"yes": true},
 		shouldBuild: true,
 	},
 	{
+		name: "Or",
 		content: "// +build no yes\n\n" +
 			"package main\n",
 		tags:        map[string]bool{"yes": true, "no": true},
 		shouldBuild: true,
 	},
 	{
-		content: "// +build no,yes no\n\n" +
+		name: "And",
+		content: "// +build no,yes\n\n" +
 			"package main\n",
 		tags:        map[string]bool{"yes": true, "no": true},
 		shouldBuild: false,
 	},
 	{
+		name: "Cgo",
 		content: "// +build cgo\n\n" +
 			"// Copyright The Go Authors.\n\n" +
 			"// This package implements parsing of tags like\n" +
@@ -173,6 +178,7 @@ var shouldBuildTests = []struct {
 		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" +
@@ -182,33 +188,126 @@ var shouldBuildTests = []struct {
 		shouldBuild: true,
 	},
 	{
-		// too close to package line
+		name: "TooClose",
 		content: "// +build yes\n" +
 			"package main\n",
 		tags:        map[string]bool{},
 		shouldBuild: true,
 	},
 	{
-		// too close to package line
+		name: "TooCloseNo",
 		content: "// +build no\n" +
 			"package main\n",
 		tags:        map[string]bool{},
 		shouldBuild: true,
 	},
+	{
+		name: "BinaryOnly",
+		content: "//go:binary-only-package\n" +
+			"// +build yes\n" +
+			"package main\n",
+		tags:        map[string]bool{},
+		binaryOnly:  true,
+		shouldBuild: true,
+	},
+	{
+		name: "ValidGoBuild",
+		content: "// +build yes\n\n" +
+			"//go:build no\n" +
+			"package main\n",
+		tags:        map[string]bool{"yes": true},
+		shouldBuild: true,
+	},
+	{
+		name: "MissingBuild",
+		content: "//go:build no\n" +
+			"package main\n",
+		tags:        map[string]bool{},
+		shouldBuild: false,
+		err:         errGoBuildWithoutBuild,
+	},
+	{
+		name: "MissingBuild2",
+		content: "/* */\n" +
+			"// +build yes\n\n" +
+			"//go:build no\n" +
+			"package main\n",
+		tags:        map[string]bool{},
+		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",
+		content: "/*\n" +
+			"//go:build no\n" +
+			"*/\n\n" +
+			"package main\n",
+		tags:        map[string]bool{},
+		shouldBuild: true,
+	},
+	{
+		name: "Comment2",
+		content: "/*\n" +
+			"text\n" +
+			"*/\n\n" +
+			"//go:build no\n" +
+			"package main\n",
+		tags:        map[string]bool{},
+		shouldBuild: false,
+		err:         errGoBuildWithoutBuild,
+	},
+	{
+		name: "Comment3",
+		content: "/*/*/ /* hi *//* \n" +
+			"text\n" +
+			"*/\n\n" +
+			"//go:build no\n" +
+			"package main\n",
+		tags:        map[string]bool{},
+		shouldBuild: false,
+		err:         errGoBuildWithoutBuild,
+	},
+	{
+		name: "Comment4",
+		content: "/**///go:build no\n" +
+			"package main\n",
+		tags:        map[string]bool{},
+		shouldBuild: true,
+	},
+	{
+		name: "Comment5",
+		content: "/**/\n" +
+			"//go:build no\n" +
+			"package main\n",
+		tags:        map[string]bool{},
+		shouldBuild: false,
+		err:         errGoBuildWithoutBuild,
+	},
 }
 
 func TestShouldBuild(t *testing.T) {
-	for i, tt := range shouldBuildTests {
-		t.Run(fmt.Sprint(i), func(t *testing.T) {
+	for _, tt := range shouldBuildTests {
+		t.Run(tt.name, func(t *testing.T) {
 			ctx := &Context{BuildTags: []string{"yes"}}
 			tags := map[string]bool{}
-			shouldBuild, binaryOnly := ctx.shouldBuild([]byte(tt.content), tags)
-			if shouldBuild != tt.shouldBuild || binaryOnly != tt.binaryOnly || !reflect.DeepEqual(tags, tt.tags) {
+			shouldBuild, binaryOnly, err := ctx.shouldBuild([]byte(tt.content), tags)
+			if shouldBuild != tt.shouldBuild || binaryOnly != tt.binaryOnly || !reflect.DeepEqual(tags, tt.tags) || err != tt.err {
 				t.Errorf("mismatch:\n"+
-					"have shouldBuild=%v, binaryOnly=%v, tags=%v\n"+
-					"want shouldBuild=%v, binaryOnly=%v, tags=%v",
-					shouldBuild, binaryOnly, tags,
-					tt.shouldBuild, tt.binaryOnly, tt.tags)
+					"have shouldBuild=%v, binaryOnly=%v, tags=%v, err=%v\n"+
+					"want shouldBuild=%v, binaryOnly=%v, tags=%v, err=%v",
+					shouldBuild, binaryOnly, tags, err,
+					tt.shouldBuild, tt.binaryOnly, tt.tags, tt.err)
 			}
 		})
 	}