]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/vet: avoid false positives with non-comments
authorDaniel Martí <mvdan@mvdan.cc>
Fri, 4 May 2018 04:22:53 +0000 (11:22 +0700)
committerDaniel Martí <mvdan@mvdan.cc>
Tue, 29 May 2018 17:01:58 +0000 (17:01 +0000)
vet's buildtag check looks for malformed build tag comments. Since these
can appear in Go files as well as non-Go files (such as assembly files),
it must read the file line by line instead of using go/token or go/ast
directly.

However, this method runs into false positives if there are any lines in
the code that look like comments, but are not. For example:

$ cat f.go
package main
const foo = `
//+build ignore
`
$ go vet f.go
./f.go:3: +build comment must appear before package clause and be followed by a blank line

This bug has been popping up more frequently since vet started being run
with go test, so it is important to make the check as precise as
possible.

To avoid the false positive, when checking a Go file, cross-check that a
line that looks like a comment actually corresponds to a comment in the
go/ast syntax tree. Since vet already obtains the syntax trees for all
the Go files, it checks, this change means very little extra work for
the check.

While at it, add a badf helper function to simplify the code that
reports warnings in the buildtag check.

Fixes #13533.

Change-Id: I484a16da01363b409ec418c313634171bf85250b
Reviewed-on: https://go-review.googlesource.com/111415
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
src/cmd/vet/buildtag.go
src/cmd/vet/main.go
src/cmd/vet/testdata/buildtag/buildtag.go

index 80d8f819240c4eec9f1f816ae7b1515938509fc3..d1fedec554918c19628e6185030e41bcdb32b6a3 100644 (file)
@@ -19,11 +19,39 @@ var (
 )
 
 // checkBuildTag checks that build tags are in the correct location and well-formed.
-func checkBuildTag(name string, data []byte) {
+func checkBuildTag(f *File) {
        if !vet("buildtags") {
                return
        }
-       lines := bytes.SplitAfter(data, nl)
+       // badf is like File.Badf, but it uses a line number instead of
+       // token.Pos.
+       badf := func(line int, format string, args ...interface{}) {
+               msg := fmt.Sprintf(format, args)
+               fmt.Fprintf(os.Stderr, "%s:%d: %s\n", f.name, line, msg)
+               setExit(1)
+       }
+
+       // we must look at the raw lines, as build tags may appear in non-Go
+       // files such as assembly files.
+       lines := bytes.SplitAfter(f.content, nl)
+
+       // lineWithComment reports whether a line corresponds to a comment in
+       // the source file. If the source file wasn't Go, the function always
+       // returns true.
+       lineWithComment := func(line int) bool {
+               if f.file == nil {
+                       // Current source file is not Go, so be conservative.
+                       return true
+               }
+               for _, group := range f.file.Comments {
+                       startLine := f.fset.Position(group.Pos()).Line
+                       endLine := f.fset.Position(group.End()).Line
+                       if startLine <= line && line <= endLine {
+                               return true
+                       }
+               }
+               return false
+       }
 
        // Determine cutpoint where +build comments are no longer valid.
        // They are valid in leading // comments in the file followed by
@@ -46,18 +74,29 @@ func checkBuildTag(name string, data []byte) {
                if !bytes.HasPrefix(line, slashSlash) {
                        continue
                }
+               if !bytes.Contains(line, plusBuild) {
+                       // Check that the comment contains "+build" early, to
+                       // avoid unnecessary lineWithComment calls that may
+                       // incur linear searches.
+                       continue
+               }
+               if !lineWithComment(i + 1) {
+                       // This is a line in a Go source file that looks like a
+                       // comment, but actually isn't - such as part of a raw
+                       // string.
+                       continue
+               }
+
                text := bytes.TrimSpace(line[2:])
                if bytes.HasPrefix(text, plusBuild) {
                        fields := bytes.Fields(text)
                        if !bytes.Equal(fields[0], plusBuild) {
                                // Comment is something like +buildasdf not +build.
-                               fmt.Fprintf(os.Stderr, "%s:%d: possible malformed +build comment\n", name, i+1)
-                               setExit(1)
+                               badf(i+1, "possible malformed +build comment")
                                continue
                        }
                        if i >= cutoff {
-                               fmt.Fprintf(os.Stderr, "%s:%d: +build comment must appear before package clause and be followed by a blank line\n", name, i+1)
-                               setExit(1)
+                               badf(i+1, "+build comment must appear before package clause and be followed by a blank line")
                                continue
                        }
                        // Check arguments.
@@ -65,15 +104,13 @@ func checkBuildTag(name string, data []byte) {
                        for _, arg := range fields[1:] {
                                for _, elem := range strings.Split(string(arg), ",") {
                                        if strings.HasPrefix(elem, "!!") {
-                                               fmt.Fprintf(os.Stderr, "%s:%d: invalid double negative in build constraint: %s\n", name, i+1, arg)
-                                               setExit(1)
+                                               badf(i+1, "invalid double negative in build constraint: %s", arg)
                                                break Args
                                        }
                                        elem = strings.TrimPrefix(elem, "!")
                                        for _, c := range elem {
                                                if !unicode.IsLetter(c) && !unicode.IsDigit(c) && c != '_' && c != '.' {
-                                                       fmt.Fprintf(os.Stderr, "%s:%d: invalid non-alphanumeric build constraint: %s\n", name, i+1, arg)
-                                                       setExit(1)
+                                                       badf(i+1, "invalid non-alphanumeric build constraint: %s", arg)
                                                        break Args
                                                }
                                        }
@@ -82,9 +119,8 @@ func checkBuildTag(name string, data []byte) {
                        continue
                }
                // Comment with +build but not at beginning.
-               if bytes.Contains(line, plusBuild) && i < cutoff {
-                       fmt.Fprintf(os.Stderr, "%s:%d: possible malformed +build comment\n", name, i+1)
-                       setExit(1)
+               if i < cutoff {
+                       badf(i+1, "possible malformed +build comment")
                        continue
                }
        }
index 4422add72f24250c2b4ba2b722fcda179cd35e6e..50af846c59980cb34a35b051035a7b2c5ff8069d 100644 (file)
@@ -415,23 +415,24 @@ func doPackage(names []string, basePkg *Package) *Package {
                        warnf("%s: %s", name, err)
                        return nil
                }
-               checkBuildTag(name, data)
                var parsedFile *ast.File
                if strings.HasSuffix(name, ".go") {
-                       parsedFile, err = parser.ParseFile(fs, name, data, 0)
+                       parsedFile, err = parser.ParseFile(fs, name, data, parser.ParseComments)
                        if err != nil {
                                warnf("%s: %s", name, err)
                                return nil
                        }
                        astFiles = append(astFiles, parsedFile)
                }
-               files = append(files, &File{
+               file := &File{
                        fset:    fs,
                        content: data,
                        name:    name,
                        file:    parsedFile,
                        dead:    make(map[ast.Node]bool),
-               })
+               }
+               checkBuildTag(file)
+               files = append(files, file)
        }
        if len(astFiles) == 0 {
                return nil
index f12f895dfb19b6de3dd3148b5ce1cbe41c273731..6ee08da638da8db1a4bc895e8bcb9978e84e51f1 100644 (file)
@@ -12,3 +12,7 @@ package testdata
 // +build toolate // ERROR "build comment must appear before package clause and be followed by a blank line"
 
 var _ = 3
+
+var _ = `
+// +build notacomment
+`