cmd/vet: avoid false positives with non-comments

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

@@ -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
 		}
 	}

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

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
+`