Skip to content

Commit

Permalink
go/build: reject //go:build without // +build
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@rsc
rsc committed Oct 13, 2020
1 parent 85f829d commit c8fdfa7
Show file tree
Hide file tree
Showing 2 changed files with 218 additions and 43 deletions.
138 changes: 107 additions & 31 deletions src/go/build/build.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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) {
Expand All @@ -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.
Expand Down
123 changes: 111 additions & 12 deletions src/go/build/build_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package build

import (
"flag"
"fmt"
"internal/testenv"
"io"
"io/ioutil"
Expand Down Expand Up @@ -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" +
Expand All @@ -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" +
Expand All @@ -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)
}
})
}
Expand Down

0 comments on commit c8fdfa7

Please sign in to comment.