]> Cypherpunks repositories - gostls13.git/commitdiff
go/build: reject //go:build without // +build
authorRuss Cox <rsc@golang.org>
Fri, 22 May 2020 18:46:52 +0000 (14:46 -0400)
committerRuss Cox <rsc@golang.org>
Tue, 13 Oct 2020 03:12:23 +0000 (03:12 +0000)
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>
src/go/build/build.go
src/go/build/build_test.go

index 86daf7c05784415be0f59c418232db911ba957ea..7a96680e77b04d935beee6fff67c27e2c07cc82b 100644 (file)
@@ -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.
index cec5186a3063038e55a2b4b20eeacf6e3efd83bf..3a4ad22f46b4e8b5fc24da1d493cf6b09544a8d1 100644 (file)
@@ -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)
                        }
                })
        }