[go] cmd/vet: avoid false positives with non-comments
142 views
Skip to first unread message
Daniel Martí (Gerrit)
May 4, 2018, 1:59:41 AM5/4/18
to Ian Lance Taylor, Rob Pike, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Daniel Martí has uploaded this change for review.
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
---
M src/cmd/vet/buildtag.go
M src/cmd/vet/main.go
M src/cmd/vet/testdata/buildtag/buildtag.go
3 files changed, 48 insertions(+), 16 deletions(-)
diff --git a/src/cmd/vet/buildtag.go b/src/cmd/vet/buildtag.go
index 80d8f81..f8d8935 100644
--- a/src/cmd/vet/buildtag.go
+++ b/src/cmd/vet/buildtag.go
@@ -19,11 +19,36 @@
)
// 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{}) {
+ format = "%s:%d:" + format + "\n"
+ args = append([]interface{}{f.name, line}, args...)
+ fmt.Fprintf(os.Stderr, format, args...)
+ 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 records all source lines that contain a //-style
+ // comment in a Go source file. If the current source file is not Go,
+ // the map is nil.
+ var lineWithComment map[int]bool
+ if f.file != nil {
+ lineWithComment = make(map[int]bool)
+ for _, group := range f.file.Comments {
+ for _, comment := range group.List {
+ line := f.fset.Position(comment.Pos()).Line
+ lineWithComment[line] = true
+ }
+ }
+ }
// Determine cutpoint where +build comments are no longer valid.
// They are valid in leading // comments in the file followed by
@@ -46,18 +71,23 @@
if !bytes.HasPrefix(line, slashSlash) {
continue
}
+ if lineWithComment != nil && !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 +95,13 @@
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
}
}
@@ -83,8 +111,7 @@
}
// 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)
+ badf(i+1, "possible malformed +build comment")
continue
}
}
diff --git a/src/cmd/vet/main.go b/src/cmd/vet/main.go
index 7265aa6..7ce8660 100644
--- a/src/cmd/vet/main.go
+++ b/src/cmd/vet/main.go
@@ -405,23 +405,24 @@
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 f12f895..6ee08da 100644
--- a/src/cmd/vet/testdata/buildtag/buildtag.go
+++ b/src/cmd/vet/testdata/buildtag/buildtag.go
@@ -12,3 +12,7 @@
// +build toolate // ERROR "build comment must appear before package clause and be followed by a blank line"
var _ = 3
+
+var _ = `
+// +build notacomment
+`
To view, visit change 111415. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
May 4, 2018, 1:59:53 AM5/4/18
to Daniel Martí, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
TryBots beginning. Status page: https://farmer.golang.org/try?commit=6c4f7841
Gobot Gobot (Gerrit)
May 4, 2018, 2:10:02 AM5/4/18
to Daniel Martí, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
TryBots are happy.
Patch set 1:TryBot-Result +1
Ian Lance Taylor (Gerrit)
May 4, 2018, 2:13:15 PM5/4/18
to Daniel Martí, goph...@pubsubhelper.golang.org, Gobot Gobot, golang-co...@googlegroups.com
2 comments:
Patch Set #1, Line 29:
format = "%s:%d:" + format + "\n"Personally I think it's clearer to write
msg := fmt.Sprintf(format, args)
fmt.Fprintf(os.Stderr, "%s:%d: %s\n", f.name, line, msg)Patch Set #1, Line 44:
lineWithComment = make(map[int]bool)This is pretty minor, but there are a lot more line comments than there are lines that look like build tags. I think it would make more sense to look for build tags first, and, if we find one, look through the comments to verify that it's a real comment.
To view, visit change 111415. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Martí (Gerrit)
May 5, 2018, 2:42:54 AM5/5/18
to Gobot Gobot, goph...@pubsubhelper.golang.org, Ian Lance Taylor, golang-co...@googlegroups.com
Daniel Martí uploaded patch set #2 to this change.
3 files changed, 58 insertions(+), 17 deletions(-)
To view, visit change 111415. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
May 5, 2018, 2:43:03 AM5/5/18
to Daniel Martí, goph...@pubsubhelper.golang.org, Ian Lance Taylor, golang-co...@googlegroups.com
Gobot Gobot (Gerrit)
May 5, 2018, 2:55:25 AM5/5/18
to Daniel Martí, goph...@pubsubhelper.golang.org, Ian Lance Taylor, golang-co...@googlegroups.com
TryBots are happy.
Patch set 2:TryBot-Result +1
Daniel Martí (Gerrit)
May 8, 2018, 6:37:24 AM5/8/18
to goph...@pubsubhelper.golang.org, Gobot Gobot, Ian Lance Taylor, golang-co...@googlegroups.com
Hmm, I wrote replies to your comments three days ago, but apparently forgot to send them.
In any case, please take a look.
2 comments:
Patch Set #1, Line 29:
msg := fmt.Sprintf(format, args)Personally I think it's clearer to write […]
Done
Patch Set #1, Line 44:
return trueThis is pretty minor, but there are a lot more line comments than there are lines that look like bui […]
You mean do a linear search for every line that looks like a build tag? I assume you are suggesting this because in most cases, it would reduce work by not having to build the map at all.
Will do.
To view, visit change 111415. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Martí (Gerrit)
May 9, 2018, 12:03:37 AM5/9/18
to goph...@pubsubhelper.golang.org, Josh Bleecher Snyder, Robert Griesemer, Rob Pike, Gobot Gobot, Ian Lance Taylor, golang-co...@googlegroups.com
Adding CCs from the recent discussion on GitHub.
Daniel Martí (Gerrit)
May 29, 2018, 10:59:03 AM5/29/18
to goph...@pubsubhelper.golang.org, Josh Bleecher Snyder, Robert Griesemer, Rob Pike, Gobot Gobot, Ian Lance Taylor, golang-co...@googlegroups.com
Still waiting for more reviews :) This change is non-trivial, so the more weeks it can be live on master until the final 1.11 release, the better.
Ian Lance Taylor (Gerrit)
May 29, 2018, 12:58:02 PM5/29/18
to Daniel Martí, goph...@pubsubhelper.golang.org, Josh Bleecher Snyder, Robert Griesemer, Rob Pike, Gobot Gobot, golang-co...@googlegroups.com
Sorry, dropped this accidentally. LGTM.
Patch set 2:Code-Review +2
Daniel Martí (Gerrit)
May 29, 2018, 1:02:02 PM5/29/18
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Ian Lance Taylor, Josh Bleecher Snyder, Robert Griesemer, Rob Pike, Gobot Gobot, golang-co...@googlegroups.com
Daniel Martí merged this change.
Approvals:
Ian Lance Taylor: Looks good to me, approved
Daniel Martí: Run TryBots
Gobot Gobot: TryBots succeeded
Reviewed-on: https://go-review.googlesource.com/111415
Run-TryBot: Daniel Martí <mv...@mvdan.cc>
TryBot-Result: Gobot Gobot <go...@golang.org>
Reviewed-by: Ian Lance Taylor <ia...@golang.org>
---
M src/cmd/vet/buildtag.go
M src/cmd/vet/main.go
M src/cmd/vet/testdata/buildtag/buildtag.go
3 files changed, 58 insertions(+), 17 deletions(-)
diff --git a/src/cmd/vet/buildtag.go b/src/cmd/vet/buildtag.go
index 80d8f81..d1fedec 100644
--- a/src/cmd/vet/buildtag.go
+++ b/src/cmd/vet/buildtag.go
@@ -19,11 +19,39 @@
)
// 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 @@
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 @@
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 @@
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 4422add..50af846 100644
--- a/src/cmd/vet/main.go
+++ b/src/cmd/vet/main.go
@@ -415,23 +415,24 @@
Reply all
Reply to author
Forward
0 new messages