From 8a9223ac31b60f7f1aa9236865d8396b7cb310fe Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 4 Oct 2018 14:28:38 -0400 Subject: [PATCH] go/analysis/passes/buildtag: split out of vet buildtag checker: - This checker has been modified from the version in vet to handle Go and non-Go files differently, to avoid having to re-read-and-parse Go files in the common case. - The old cmd/vet driver would run this check on all the files in a directory whereas new drivers will run it only on the files selected for a particular configuration, so some of the checks (those in checkArguments) will never fire. But this is not a regression relative to 'go vet', because it too presents cmd/vet with only the files selected as part of the package. analysistest: - fix bug that processed a block of //-comments as one. - treat "...// want..." within a //-comment as a want comment. This is required for adding expectations on lines that are already comments. Change-Id: Iacf3684864e07532f77176481afbf059a9638f3b Reviewed-on: https://go-review.googlesource.com/c/139797 Reviewed-by: Michael Matloob Run-TryBot: Michael Matloob --- go/analysis/analysistest/analysistest.go | 43 ++-- go/analysis/cmd/analyze/analyze.go | 6 +- go/analysis/passes/buildtag/buildtag.go | 204 ++++++++++++++++++ go/analysis/passes/buildtag/buildtag_test.go | 14 ++ .../buildtag/testdata/src/a/buildtag.go | 21 ++ go/analysis/passes/vet/buildtag.go | 128 ----------- .../passes/vet/testdata/buildtag/buildtag.go | 18 -- .../vet/testdata/buildtag/buildtag_bad.go | 15 -- 8 files changed, 273 insertions(+), 176 deletions(-) create mode 100644 go/analysis/passes/buildtag/buildtag.go create mode 100644 go/analysis/passes/buildtag/buildtag_test.go create mode 100644 go/analysis/passes/buildtag/testdata/src/a/buildtag.go delete mode 100644 go/analysis/passes/vet/buildtag.go delete mode 100644 go/analysis/passes/vet/testdata/buildtag/buildtag.go delete mode 100644 go/analysis/passes/vet/testdata/buildtag/buildtag_bad.go diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go index 0f8415aa..db84fb06 100644 --- a/go/analysis/analysistest/analysistest.go +++ b/go/analysis/analysistest/analysistest.go @@ -159,23 +159,38 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis } want := make(map[key][]expectation) for _, f := range pass.Files { - for _, c := range f.Comments { - posn := pass.Fset.Position(c.Pos()) - sanitize(gopath, &posn) - text := strings.TrimSpace(c.Text()) + for _, cgroup := range f.Comments { + for _, c := range cgroup.List { + posn := pass.Fset.Position(c.Pos()) + sanitize(gopath, &posn) - // Any comment starting with "want" is treated - // as an expectation, even without following whitespace. - if rest := strings.TrimPrefix(text, "want"); rest != text { - expects, err := parseExpectations(rest) - if err != nil { - t.Errorf("%s: in 'want' comment: %s", posn, err) - continue + text := strings.TrimPrefix(c.Text, "//") + if text == c.Text { + continue // not a //-comment } - if false { - log.Printf("%s: %v", posn, expects) + text = strings.TrimSpace(text) + + // Hack: treat a comment of the form "//...// want..." + // as if it starts after the second "//". + // This allows us to add comments on comments, + // as required when testing the buildtag analyzer. + if i := strings.Index(text, "// want"); i >= 0 { + text = text[i+len("// "):] + } + + // Any comment starting with "want" is treated + // as an expectation, even without following whitespace. + if rest := strings.TrimPrefix(text, "want"); rest != text { + expects, err := parseExpectations(rest) + if err != nil { + t.Errorf("%s: in 'want' comment: %s", posn, err) + continue + } + if false { + log.Printf("%s: %v", posn, expects) + } + want[key{posn.Filename, posn.Line}] = expects } - want[key{posn.Filename, posn.Line}] = expects } } } diff --git a/go/analysis/cmd/analyze/analyze.go b/go/analysis/cmd/analyze/analyze.go index 50fd8bfe..fcc67eb6 100644 --- a/go/analysis/cmd/analyze/analyze.go +++ b/go/analysis/cmd/analyze/analyze.go @@ -15,6 +15,7 @@ import ( "golang.org/x/tools/go/analysis/multichecker" // analysis plug-ins + "golang.org/x/tools/go/analysis/passes/buildtag" "golang.org/x/tools/go/analysis/passes/findcall" ) @@ -22,5 +23,8 @@ func main() { log.SetFlags(0) log.SetPrefix("analyze: ") - multichecker.Main(findcall.Analyzer) + multichecker.Main( + findcall.Analyzer, + buildtag.Analyzer, + ) } diff --git a/go/analysis/passes/buildtag/buildtag.go b/go/analysis/passes/buildtag/buildtag.go new file mode 100644 index 00000000..63687864 --- /dev/null +++ b/go/analysis/passes/buildtag/buildtag.go @@ -0,0 +1,204 @@ +// Copyright 2013 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package buildtag + +import ( + "bytes" + "fmt" + "go/ast" + "go/token" + "io/ioutil" + "strings" + "unicode" + + "golang.org/x/tools/go/analysis" +) + +var Analyzer = &analysis.Analyzer{ + Name: "buildtag", + Doc: "check that +build tags are well-formed and correctly located", + Run: runBuildTag, +} + +func runBuildTag(pass *analysis.Pass) (interface{}, error) { + for _, f := range pass.Files { + checkGoFile(pass, f) + } + for _, name := range pass.OtherFiles { + if err := checkOtherFile(pass, name); err != nil { + return nil, err + } + } + return nil, nil +} + +func checkGoFile(pass *analysis.Pass, f *ast.File) { + pastCutoff := false + for _, group := range f.Comments { + // A +build comment is ignored after or adjoining the package declaration. + if group.End()+1 >= f.Package { + pastCutoff = true + } + + // "+build" is ignored within or after a /*...*/ comment. + if !strings.HasPrefix(group.List[0].Text, "//") { + pastCutoff = true + continue + } + + // Check each line of a //-comment. + for _, c := range group.List { + if !strings.Contains(c.Text, "+build") { + continue + } + if err := checkLine(c.Text, pastCutoff); err != nil { + pass.Reportf(c.Pos(), "%s", err) + } + } + } +} + +func checkOtherFile(pass *analysis.Pass, filename string) error { + content, tf, err := readFile(pass.Fset, filename) + if err != nil { + return err + } + + // We must look at the raw lines, as build tags may appear in non-Go + // files such as assembly files. + lines := bytes.SplitAfter(content, nl) + + // Determine cutpoint where +build comments are no longer valid. + // They are valid in leading // comments in the file followed by + // a blank line. + // + // This must be done as a separate pass because of the + // requirement that the comment be followed by a blank line. + var cutoff int + for i, line := range lines { + line = bytes.TrimSpace(line) + if !bytes.HasPrefix(line, slashSlash) { + if len(line) > 0 { + break + } + cutoff = i + } + } + + for i, line := range lines { + line = bytes.TrimSpace(line) + if !bytes.HasPrefix(line, slashSlash) { + continue + } + if !bytes.Contains(line, []byte("+build")) { + continue + } + if err := checkLine(string(line), i >= cutoff); err != nil { + pass.Reportf(lineStart(tf, i+1), "%s", err) + continue + } + } + return nil +} + +// checkLine checks a line that starts with "//" and contains "+build". +func checkLine(line string, pastCutoff bool) error { + line = strings.TrimPrefix(line, "//") + line = strings.TrimSpace(line) + + if strings.HasPrefix(line, "+build") { + fields := strings.Fields(line) + if fields[0] != "+build" { + // Comment is something like +buildasdf not +build. + return fmt.Errorf("possible malformed +build comment") + } + if pastCutoff { + return fmt.Errorf("+build comment must appear before package clause and be followed by a blank line") + } + if err := checkArguments(fields); err != nil { + return err + } + } else { + // Comment with +build but not at beginning. + if !pastCutoff { + return fmt.Errorf("possible malformed +build comment") + } + } + return nil +} + +func checkArguments(fields []string) error { + // The original version of this checker in vet could examine + // files with malformed build tags that would cause the file to + // be always ignored by "go build". However, drivers for the new + // analysis API will analyze only the files selected to form a + // package, so these checks will never fire. + // TODO(adonovan): rethink this. + + for _, arg := range fields[1:] { + for _, elem := range strings.Split(arg, ",") { + if strings.HasPrefix(elem, "!!") { + return fmt.Errorf("invalid double negative in build constraint: %s", arg) + } + elem = strings.TrimPrefix(elem, "!") + for _, c := range elem { + if !unicode.IsLetter(c) && !unicode.IsDigit(c) && c != '_' && c != '.' { + return fmt.Errorf("invalid non-alphanumeric build constraint: %s", arg) + } + } + } + } + return nil +} + +var ( + nl = []byte("\n") + slashSlash = []byte("//") +) + +// -- these declarations are copied from asmdecl -- + +// readFile reads a file and adds it to the FileSet +// so that we can report errors against it using lineStart. +func readFile(fset *token.FileSet, filename string) ([]byte, *token.File, error) { + content, err := ioutil.ReadFile(filename) + if err != nil { + return nil, nil, err + } + tf := fset.AddFile(filename, -1, len(content)) + tf.SetLinesForContent(content) + return content, tf, nil +} + +// lineStart returns the position of the start of the specified line +// within file f, or NoPos if there is no line of that number. +func lineStart(f *token.File, line int) token.Pos { + // Use binary search to find the start offset of this line. + // + // TODO(adonovan): eventually replace this function with the + // simpler and more efficient (*go/token.File).LineStart, added + // in go1.12. + + min := 0 // inclusive + max := f.Size() // exclusive + for { + offset := (min + max) / 2 + pos := f.Pos(offset) + posn := f.Position(pos) + if posn.Line == line { + return pos - (token.Pos(posn.Column) - 1) + } + + if min+1 >= max { + return token.NoPos + } + + if posn.Line < line { + min = offset + } else { + max = offset + } + } +} diff --git a/go/analysis/passes/buildtag/buildtag_test.go b/go/analysis/passes/buildtag/buildtag_test.go new file mode 100644 index 00000000..98aec64a --- /dev/null +++ b/go/analysis/passes/buildtag/buildtag_test.go @@ -0,0 +1,14 @@ +package buildtag_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/buildtag" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + // loads testdata/src/a/a.go + analysistest.Run(t, testdata, buildtag.Analyzer, "a") +} diff --git a/go/analysis/passes/buildtag/testdata/src/a/buildtag.go b/go/analysis/passes/buildtag/testdata/src/a/buildtag.go new file mode 100644 index 00000000..dcc980c1 --- /dev/null +++ b/go/analysis/passes/buildtag/testdata/src/a/buildtag.go @@ -0,0 +1,21 @@ +// Copyright 2013 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// This file contains tests for the buildtag checker. + +// +builder // want `possible malformed \+build comment` +// +build !ignore + +// Mention +build // want `possible malformed \+build comment` + +// +build nospace // want "build comment must appear before package clause and be followed by a blank line" +package a + +// +build toolate // want "build comment must appear before package clause and be followed by a blank line$" + +var _ = 3 + +var _ = ` +// +build notacomment +` diff --git a/go/analysis/passes/vet/buildtag.go b/go/analysis/passes/vet/buildtag.go deleted file mode 100644 index ebc2bf89..00000000 --- a/go/analysis/passes/vet/buildtag.go +++ /dev/null @@ -1,128 +0,0 @@ -// +build ignore - -// Copyright 2013 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package main - -import ( - "bytes" - "fmt" - "os" - "strings" - "unicode" -) - -var ( - nl = []byte("\n") - slashSlash = []byte("//") - plusBuild = []byte("+build") -) - -func badfLine(f *File, 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) -} - -// checkBuildTag checks that build tags are in the correct location and well-formed. -func checkBuildTag(f *File) { - if !vet("buildtags") { - return - } - - // 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 - // a blank line. - var cutoff int - for i, line := range lines { - line = bytes.TrimSpace(line) - if len(line) == 0 { - cutoff = i - continue - } - if bytes.HasPrefix(line, slashSlash) { - continue - } - break - } - - for i, line := range lines { - line = bytes.TrimSpace(line) - 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. - badfLine(f, i+1, "possible malformed +build comment") - continue - } - if i >= cutoff { - badfLine(f, i+1, "+build comment must appear before package clause and be followed by a blank line") - continue - } - // Check arguments. - Args: - for _, arg := range fields[1:] { - for _, elem := range strings.Split(string(arg), ",") { - if strings.HasPrefix(elem, "!!") { - badfLine(f, 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 != '.' { - badfLine(f, i+1, "invalid non-alphanumeric build constraint: %s", arg) - break Args - } - } - } - } - continue - } - // Comment with +build but not at beginning. - if i < cutoff { - badfLine(f, i+1, "possible malformed +build comment") - continue - } - } -} diff --git a/go/analysis/passes/vet/testdata/buildtag/buildtag.go b/go/analysis/passes/vet/testdata/buildtag/buildtag.go deleted file mode 100644 index c2fd6aaa..00000000 --- a/go/analysis/passes/vet/testdata/buildtag/buildtag.go +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright 2013 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// This file contains tests for the buildtag checker. - -// +builder // ERROR "possible malformed \+build comment" -// +build !ignore - -package testdata - -// +build toolate // ERROR "build comment must appear before package clause and be followed by a blank line$" - -var _ = 3 - -var _ = ` -// +build notacomment -` diff --git a/go/analysis/passes/vet/testdata/buildtag/buildtag_bad.go b/go/analysis/passes/vet/testdata/buildtag/buildtag_bad.go deleted file mode 100644 index fbe10cf7..00000000 --- a/go/analysis/passes/vet/testdata/buildtag/buildtag_bad.go +++ /dev/null @@ -1,15 +0,0 @@ -// This file contains misplaced or malformed build constraints. -// The Go tool will skip it, because the constraints are invalid. -// It serves only to test the tag checker during make test. - -// Mention +build // ERROR "possible malformed \+build comment" - -// +build !!bang // ERROR "invalid double negative in build constraint" -// +build @#$ // ERROR "invalid non-alphanumeric build constraint" - -// +build toolate // ERROR "build comment must appear before package clause and be followed by a blank line" -package bad - -// This is package 'bad' rather than 'main' so the erroneous build -// tag doesn't end up looking like a package doc for the vet command -// when examined by godoc.