From 07d384b9de6b58bfe8cf9c6543179654353944b4 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Daniel=20Mart=C3=AD?= Date: Fri, 4 May 2018 11:22:53 +0700 Subject: [PATCH] cmd/vet: avoid false positives with non-comments MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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í TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- src/cmd/vet/buildtag.go | 62 ++++++++++++++++++----- src/cmd/vet/main.go | 9 ++-- src/cmd/vet/testdata/buildtag/buildtag.go | 4 ++ 3 files changed, 58 insertions(+), 17 deletions(-) diff --git a/src/cmd/vet/buildtag.go b/src/cmd/vet/buildtag.go index 80d8f81924..d1fedec554 100644 --- a/src/cmd/vet/buildtag.go +++ b/src/cmd/vet/buildtag.go @@ -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 } } diff --git a/src/cmd/vet/main.go b/src/cmd/vet/main.go index 4422add72f..50af846c59 100644 --- a/src/cmd/vet/main.go +++ b/src/cmd/vet/main.go @@ -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 diff --git a/src/cmd/vet/testdata/buildtag/buildtag.go b/src/cmd/vet/testdata/buildtag/buildtag.go index f12f895dfb..6ee08da638 100644 --- a/src/cmd/vet/testdata/buildtag/buildtag.go +++ b/src/cmd/vet/testdata/buildtag/buildtag.go @@ -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 +` -- 2.50.0