From c8fdfa756ead0a48b0227f6cec02641797124a92 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Fri, 22 May 2020 14:46:52 -0400 Subject: [PATCH] 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 Run-TryBot: Russ Cox TryBot-Result: Go Bot Reviewed-by: Ian Lance Taylor --- src/go/build/build.go | 138 ++++++++++++++++++++++++++++--------- src/go/build/build_test.go | 123 +++++++++++++++++++++++++++++---- 2 files changed, 218 insertions(+), 43 deletions(-) diff --git a/src/go/build/build.go b/src/go/build/build.go index 86daf7c057844..7a96680e77b04 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 cec5186a30630..3a4ad22f46b4e 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) } }) }