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 <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>

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 {
+		for _, cgroup := range f.Comments {
-			posn := pass.Fset.Position(c.Pos())
+			for _, c := range cgroup.List {
-			sanitize(gopath, &posn)
+				posn := pass.Fset.Position(c.Pos())
-			text := strings.TrimSpace(c.Text())
+				sanitize(gopath, &posn)
 
-			// Any comment starting with "want" is treated
+				text := strings.TrimPrefix(c.Text, "//")
-			// as an expectation, even without following whitespace.
+				if text == c.Text {
-			if rest := strings.TrimPrefix(text, "want"); rest != text {
+					continue // not a //-comment
-				expects, err := parseExpectations(rest)
-				if err != nil {
-					t.Errorf("%s: in 'want' comment: %s", posn, err)
-					continue
 				}
-				if false {
+				text = strings.TrimSpace(text)
-					log.Printf("%s: %v", posn, expects)
+
+				// 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
 			}
 		}
 	}

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,
+	)
 }

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

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")
+}

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

go/analysis/passes/vet/buildtag.go

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

go/analysis/passes/vet/testdata/buildtag/buildtag.go

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

go/analysis/passes/vet/testdata/buildtag/buildtag_bad.go

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