Skip to content

Commit c8fdfa7

Browse files
rscrsc
committed
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 golang#41184. Change-Id: I29b8a789ab1526ab5057f613d5533bd2060ba9cd Reviewed-on: https://go-review.googlesource.com/c/go/+/240600 Trust: Russ Cox <[email protected]> Run-TryBot: Russ Cox <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
1 parent 85f829d commit c8fdfa7

2 files changed

Lines changed: 218 additions & 43 deletions

File tree

‎src/go/build/build.go‎

Lines changed: 107 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1371,9 +1371,12 @@ func (ctxt *Context) matchFile(dir, name string, allTags map[string]bool, binary
13711371
}
13721372

13731373
// Look for +build comments to accept or reject the file.
1374-
ok, sawBinaryOnly := ctxt.shouldBuild(data, allTags)
1374+
ok, sawBinaryOnly, err := ctxt.shouldBuild(data, allTags)
1375+
if err != nil {
1376+
return // non-nil err
1377+
}
13751378
if !ok && !ctxt.UseAllFiles {
1376-
return
1379+
return // nil err
13771380
}
13781381

13791382
if binaryOnly != nil && sawBinaryOnly {
@@ -1402,7 +1405,25 @@ func ImportDir(dir string, mode ImportMode) (*Package, error) {
14021405
return Default.ImportDir(dir, mode)
14031406
}
14041407

1405-
var slashslash = []byte("//")
1408+
var (
1409+
bSlashSlash = []byte(slashSlash)
1410+
bStarSlash = []byte(starSlash)
1411+
bSlashStar = []byte(slashStar)
1412+
1413+
goBuildComment = []byte("//go:build")
1414+
1415+
errGoBuildWithoutBuild = errors.New("//go:build comment without // +build comment")
1416+
errMultipleGoBuild = errors.New("multiple //go:build comments") // unused in Go 1.(N-1)
1417+
)
1418+
1419+
func isGoBuildComment(line []byte) bool {
1420+
if !bytes.HasPrefix(line, goBuildComment) {
1421+
return false
1422+
}
1423+
line = bytes.TrimSpace(line)
1424+
rest := line[len(goBuildComment):]
1425+
return len(rest) == 0 || len(bytes.TrimSpace(rest)) < len(rest)
1426+
}
14061427

14071428
// Special comment denoting a binary-only package.
14081429
// See https://golang.org/design/2775-binary-only-packages
@@ -1426,34 +1447,20 @@ var binaryOnlyComment = []byte("//go:binary-only-package")
14261447
//
14271448
// shouldBuild reports whether the file should be built
14281449
// and whether a //go:binary-only-package comment was found.
1429-
func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool) (shouldBuild bool, binaryOnly bool) {
1430-
sawBinaryOnly := false
1450+
func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool) (shouldBuild, binaryOnly bool, err error) {
14311451

14321452
// Pass 1. Identify leading run of // comments and blank lines,
14331453
// which must be followed by a blank line.
1434-
end := 0
1435-
p := content
1436-
for len(p) > 0 {
1437-
line := p
1438-
if i := bytes.IndexByte(line, '\n'); i >= 0 {
1439-
line, p = line[:i], p[i+1:]
1440-
} else {
1441-
p = p[len(p):]
1442-
}
1443-
line = bytes.TrimSpace(line)
1444-
if len(line) == 0 { // Blank line
1445-
end = len(content) - len(p)
1446-
continue
1447-
}
1448-
if !bytes.HasPrefix(line, slashslash) { // Not comment line
1449-
break
1450-
}
1454+
// Also identify any //go:build comments.
1455+
content, goBuild, sawBinaryOnly, err := parseFileHeader(content)
1456+
if err != nil {
1457+
return false, false, err
14511458
}
1452-
content = content[:end]
14531459

1454-
// Pass 2. Process each line in the run.
1455-
p = content
1460+
// Pass 2. Process each +build line in the run.
1461+
p := content
14561462
shouldBuild = true
1463+
sawBuild := false
14571464
for len(p) > 0 {
14581465
line := p
14591466
if i := bytes.IndexByte(line, '\n'); i >= 0 {
@@ -1462,17 +1469,15 @@ func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool) (shoul
14621469
p = p[len(p):]
14631470
}
14641471
line = bytes.TrimSpace(line)
1465-
if !bytes.HasPrefix(line, slashslash) {
1472+
if !bytes.HasPrefix(line, bSlashSlash) {
14661473
continue
14671474
}
1468-
if bytes.Equal(line, binaryOnlyComment) {
1469-
sawBinaryOnly = true
1470-
}
1471-
line = bytes.TrimSpace(line[len(slashslash):])
1475+
line = bytes.TrimSpace(line[len(bSlashSlash):])
14721476
if len(line) > 0 && line[0] == '+' {
14731477
// Looks like a comment +line.
14741478
f := strings.Fields(string(line))
14751479
if f[0] == "+build" {
1480+
sawBuild = true
14761481
ok := false
14771482
for _, tok := range f[1:] {
14781483
if ctxt.match(tok, allTags) {
@@ -1486,7 +1491,78 @@ func (ctxt *Context) shouldBuild(content []byte, allTags map[string]bool) (shoul
14861491
}
14871492
}
14881493

1489-
return shouldBuild, sawBinaryOnly
1494+
if goBuild != nil && !sawBuild {
1495+
return false, false, errGoBuildWithoutBuild
1496+
}
1497+
1498+
return shouldBuild, sawBinaryOnly, nil
1499+
}
1500+
1501+
func parseFileHeader(content []byte) (trimmed, goBuild []byte, sawBinaryOnly bool, err error) {
1502+
end := 0
1503+
p := content
1504+
ended := false // found non-blank, non-// line, so stopped accepting // +build lines
1505+
inSlashStar := false // in /* */ comment
1506+
1507+
Lines:
1508+
for len(p) > 0 {
1509+
line := p
1510+
if i := bytes.IndexByte(line, '\n'); i >= 0 {
1511+
line, p = line[:i], p[i+1:]
1512+
} else {
1513+
p = p[len(p):]
1514+
}
1515+
line = bytes.TrimSpace(line)
1516+
if len(line) == 0 && !ended { // Blank line
1517+
// Remember position of most recent blank line.
1518+
// When we find the first non-blank, non-// line,
1519+
// this "end" position marks the latest file position
1520+
// where a // +build line can appear.
1521+
// (It must appear _before_ a blank line before the non-blank, non-// line.
1522+
// Yes, that's confusing, which is part of why we moved to //go:build lines.)
1523+
// Note that ended==false here means that inSlashStar==false,
1524+
// since seeing a /* would have set ended==true.
1525+
end = len(content) - len(p)
1526+
continue Lines
1527+
}
1528+
if !bytes.HasPrefix(line, slashSlash) { // Not comment line
1529+
ended = true
1530+
}
1531+
1532+
if !inSlashStar && isGoBuildComment(line) {
1533+
if false && goBuild != nil { // enabled in Go 1.N
1534+
return nil, nil, false, errMultipleGoBuild
1535+
}
1536+
goBuild = line
1537+
}
1538+
if !inSlashStar && bytes.Equal(line, binaryOnlyComment) {
1539+
sawBinaryOnly = true
1540+
}
1541+
1542+
Comments:
1543+
for len(line) > 0 {
1544+
if inSlashStar {
1545+
if i := bytes.Index(line, starSlash); i >= 0 {
1546+
inSlashStar = false
1547+
line = bytes.TrimSpace(line[i+len(starSlash):])
1548+
continue Comments
1549+
}
1550+
continue Lines
1551+
}
1552+
if bytes.HasPrefix(line, bSlashSlash) {
1553+
continue Lines
1554+
}
1555+
if bytes.HasPrefix(line, bSlashStar) {
1556+
inSlashStar = true
1557+
line = bytes.TrimSpace(line[len(bSlashStar):])
1558+
continue Comments
1559+
}
1560+
// Found non-comment text.
1561+
break Lines
1562+
}
1563+
}
1564+
1565+
return content[:end], goBuild, sawBinaryOnly, nil
14901566
}
14911567

14921568
// saveCgo saves the information from the #cgo lines in the import "C" comment.

‎src/go/build/build_test.go‎

Lines changed: 111 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ package build
66

77
import (
88
"flag"
9-
"fmt"
109
"internal/testenv"
1110
"io"
1211
"io/ioutil"
@@ -140,30 +139,36 @@ func TestLocalDirectory(t *testing.T) {
140139
}
141140

142141
var shouldBuildTests = []struct {
142+
name string
143143
content string
144144
tags map[string]bool
145145
binaryOnly bool
146146
shouldBuild bool
147+
err error
147148
}{
148149
{
150+
name: "Yes",
149151
content: "// +build yes\n\n" +
150152
"package main\n",
151153
tags: map[string]bool{"yes": true},
152154
shouldBuild: true,
153155
},
154156
{
157+
name: "Or",
155158
content: "// +build no yes\n\n" +
156159
"package main\n",
157160
tags: map[string]bool{"yes": true, "no": true},
158161
shouldBuild: true,
159162
},
160163
{
161-
content: "// +build no,yes no\n\n" +
164+
name: "And",
165+
content: "// +build no,yes\n\n" +
162166
"package main\n",
163167
tags: map[string]bool{"yes": true, "no": true},
164168
shouldBuild: false,
165169
},
166170
{
171+
name: "Cgo",
167172
content: "// +build cgo\n\n" +
168173
"// Copyright The Go Authors.\n\n" +
169174
"// This package implements parsing of tags like\n" +
@@ -173,6 +178,7 @@ var shouldBuildTests = []struct {
173178
shouldBuild: false,
174179
},
175180
{
181+
name: "AfterPackage",
176182
content: "// Copyright The Go Authors.\n\n" +
177183
"package build\n\n" +
178184
"// shouldBuild checks tags given by lines of the form\n" +
@@ -182,33 +188,126 @@ var shouldBuildTests = []struct {
182188
shouldBuild: true,
183189
},
184190
{
185-
// too close to package line
191+
name: "TooClose",
186192
content: "// +build yes\n" +
187193
"package main\n",
188194
tags: map[string]bool{},
189195
shouldBuild: true,
190196
},
191197
{
192-
// too close to package line
198+
name: "TooCloseNo",
193199
content: "// +build no\n" +
194200
"package main\n",
195201
tags: map[string]bool{},
196202
shouldBuild: true,
197203
},
204+
{
205+
name: "BinaryOnly",
206+
content: "//go:binary-only-package\n" +
207+
"// +build yes\n" +
208+
"package main\n",
209+
tags: map[string]bool{},
210+
binaryOnly: true,
211+
shouldBuild: true,
212+
},
213+
{
214+
name: "ValidGoBuild",
215+
content: "// +build yes\n\n" +
216+
"//go:build no\n" +
217+
"package main\n",
218+
tags: map[string]bool{"yes": true},
219+
shouldBuild: true,
220+
},
221+
{
222+
name: "MissingBuild",
223+
content: "//go:build no\n" +
224+
"package main\n",
225+
tags: map[string]bool{},
226+
shouldBuild: false,
227+
err: errGoBuildWithoutBuild,
228+
},
229+
{
230+
name: "MissingBuild2",
231+
content: "/* */\n" +
232+
"// +build yes\n\n" +
233+
"//go:build no\n" +
234+
"package main\n",
235+
tags: map[string]bool{},
236+
shouldBuild: false,
237+
err: errGoBuildWithoutBuild,
238+
},
239+
{
240+
name: "MissingBuild2",
241+
content: "/*\n" +
242+
"// +build yes\n\n" +
243+
"*/\n" +
244+
"//go:build no\n" +
245+
"package main\n",
246+
tags: map[string]bool{},
247+
shouldBuild: false,
248+
err: errGoBuildWithoutBuild,
249+
},
250+
{
251+
name: "Comment1",
252+
content: "/*\n" +
253+
"//go:build no\n" +
254+
"*/\n\n" +
255+
"package main\n",
256+
tags: map[string]bool{},
257+
shouldBuild: true,
258+
},
259+
{
260+
name: "Comment2",
261+
content: "/*\n" +
262+
"text\n" +
263+
"*/\n\n" +
264+
"//go:build no\n" +
265+
"package main\n",
266+
tags: map[string]bool{},
267+
shouldBuild: false,
268+
err: errGoBuildWithoutBuild,
269+
},
270+
{
271+
name: "Comment3",
272+
content: "/*/*/ /* hi *//* \n" +
273+
"text\n" +
274+
"*/\n\n" +
275+
"//go:build no\n" +
276+
"package main\n",
277+
tags: map[string]bool{},
278+
shouldBuild: false,
279+
err: errGoBuildWithoutBuild,
280+
},
281+
{
282+
name: "Comment4",
283+
content: "/**///go:build no\n" +
284+
"package main\n",
285+
tags: map[string]bool{},
286+
shouldBuild: true,
287+
},
288+
{
289+
name: "Comment5",
290+
content: "/**/\n" +
291+
"//go:build no\n" +
292+
"package main\n",
293+
tags: map[string]bool{},
294+
shouldBuild: false,
295+
err: errGoBuildWithoutBuild,
296+
},
198297
}
199298

200299
func TestShouldBuild(t *testing.T) {
201-
for i, tt := range shouldBuildTests {
202-
t.Run(fmt.Sprint(i), func(t *testing.T) {
300+
for _, tt := range shouldBuildTests {
301+
t.Run(tt.name, func(t *testing.T) {
203302
ctx := &Context{BuildTags: []string{"yes"}}
204303
tags := map[string]bool{}
205-
shouldBuild, binaryOnly := ctx.shouldBuild([]byte(tt.content), tags)
206-
if shouldBuild != tt.shouldBuild || binaryOnly != tt.binaryOnly || !reflect.DeepEqual(tags, tt.tags) {
304+
shouldBuild, binaryOnly, err := ctx.shouldBuild([]byte(tt.content), tags)
305+
if shouldBuild != tt.shouldBuild || binaryOnly != tt.binaryOnly || !reflect.DeepEqual(tags, tt.tags) || err != tt.err {
207306
t.Errorf("mismatch:\n"+
208-
"have shouldBuild=%v, binaryOnly=%v, tags=%v\n"+
209-
"want shouldBuild=%v, binaryOnly=%v, tags=%v",
210-
shouldBuild, binaryOnly, tags,
211-
tt.shouldBuild, tt.binaryOnly, tt.tags)
307+
"have shouldBuild=%v, binaryOnly=%v, tags=%v, err=%v\n"+
308+
"want shouldBuild=%v, binaryOnly=%v, tags=%v, err=%v",
309+
shouldBuild, binaryOnly, tags, err,
310+
tt.shouldBuild, tt.binaryOnly, tt.tags, tt.err)
212311
}
213312
})
214313
}

0 commit comments

Comments
 (0)