go/analysis: add IgnoredFiles, check ignored files in buildtag check

This CL fixes a long-standing TODO from the original conversion
of go vet to the go/analysis framework: apply the buildtag check
to all source files, not just the ones selected for the current GOOS/GOARCH.

Note that this only triggers when IgnoredFiles is passed from the
go command, which only happens in the current Go 1.16 development tree.

Change-Id: I37692f0bbe7b2b6e96f016e3c4827ebed1eeafa4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/261725
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
diff --git a/go/analysis/analysis.go b/go/analysis/analysis.go
index 8c99773..8c3c2e7 100644
--- a/go/analysis/analysis.go
+++ b/go/analysis/analysis.go
@@ -95,12 +95,13 @@
 	Analyzer *Analyzer // the identity of the current analyzer
 
 	// syntax and type information
-	Fset       *token.FileSet // file position information
-	Files      []*ast.File    // the abstract syntax tree of each file
-	OtherFiles []string       // names of non-Go files of this package
-	Pkg        *types.Package // type information about the package
-	TypesInfo  *types.Info    // type information about the syntax trees
-	TypesSizes types.Sizes    // function for computing sizes of types
+	Fset         *token.FileSet // file position information
+	Files        []*ast.File    // the abstract syntax tree of each file
+	OtherFiles   []string       // names of non-Go files of this package
+	IgnoredFiles []string       // names of ignored source files in this package
+	Pkg          *types.Package // type information about the package
+	TypesInfo    *types.Info    // type information about the syntax trees
+	TypesSizes   types.Sizes    // function for computing sizes of types
 
 	// Report reports a Diagnostic, a finding about a specific location
 	// in the analyzed source code such as a potential mistake.
diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go
index bb79cde..ae56302 100644
--- a/go/analysis/analysistest/analysistest.go
+++ b/go/analysis/analysistest/analysistest.go
@@ -355,7 +355,7 @@
 		}
 	}
 
-	// Extract 'want' comments from Go files.
+	// Extract 'want' comments from parsed Go files.
 	for _, f := range pass.Files {
 		for _, cgroup := range f.Comments {
 			for _, c := range cgroup.List {
@@ -398,6 +398,16 @@
 		linenum := 0
 		for _, line := range strings.Split(string(data), "\n") {
 			linenum++
+
+			// Hack: treat a comment of the form "//...// want..."
+			// or "/*...// want... */
+			// as if it starts at 'want'.
+			// This allows us to add comments on comments,
+			// as required when testing the buildtag analyzer.
+			if i := strings.Index(line, "// want"); i >= 0 {
+				line = line[i:]
+			}
+
 			if i := strings.Index(line, "//"); i >= 0 {
 				line = line[i+len("//"):]
 				processComment(filename, linenum, line)
diff --git a/go/analysis/doc.go b/go/analysis/doc.go
index fb17a0e..9fa3302 100644
--- a/go/analysis/doc.go
+++ b/go/analysis/doc.go
@@ -121,13 +121,14 @@
 reporting diagnostics and other information back to the driver.
 
 	type Pass struct {
-		Fset       *token.FileSet
-		Files      []*ast.File
-		OtherFiles []string
-		Pkg        *types.Package
-		TypesInfo  *types.Info
-		ResultOf   map[*Analyzer]interface{}
-		Report     func(Diagnostic)
+		Fset         *token.FileSet
+		Files        []*ast.File
+		OtherFiles   []string
+		IgnoredFiles []string
+		Pkg          *types.Package
+		TypesInfo    *types.Info
+		ResultOf     map[*Analyzer]interface{}
+		Report       func(Diagnostic)
 		...
 	}
 
@@ -139,6 +140,12 @@
 or "buildtags" analyzers for examples of loading non-Go files and reporting
 diagnostics against them.
 
+The IgnoredFiles field provides the names, but not the contents,
+of ignored Go and non-Go source files that are not part of this package
+with the current build configuration but may be part of other build
+configurations. See the "buildtags" analyzer for an example of loading
+and checking IgnoredFiles.
+
 The ResultOf field provides the results computed by the analyzers
 required by this one, as expressed in its Analyzer.Requires field. The
 driver runs the required analyzers first and makes their results
diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go
index 5ccfb16..34f5b47 100644
--- a/go/analysis/internal/checker/checker.go
+++ b/go/analysis/internal/checker/checker.go
@@ -639,6 +639,7 @@
 		Fset:              act.pkg.Fset,
 		Files:             act.pkg.Syntax,
 		OtherFiles:        act.pkg.OtherFiles,
+		IgnoredFiles:      act.pkg.IgnoredFiles,
 		Pkg:               act.pkg.Types,
 		TypesInfo:         act.pkg.TypesInfo,
 		TypesSizes:        act.pkg.TypesSizes,
diff --git a/go/analysis/passes/buildtag/buildtag.go b/go/analysis/passes/buildtag/buildtag.go
index 78176f1..841b928 100644
--- a/go/analysis/passes/buildtag/buildtag.go
+++ b/go/analysis/passes/buildtag/buildtag.go
@@ -9,6 +9,7 @@
 	"bytes"
 	"fmt"
 	"go/ast"
+	"go/parser"
 	"strings"
 	"unicode"
 
@@ -33,6 +34,20 @@
 			return nil, err
 		}
 	}
+	for _, name := range pass.IgnoredFiles {
+		if strings.HasSuffix(name, ".go") {
+			f, err := parser.ParseFile(pass.Fset, name, nil, parser.ParseComments)
+			if err != nil {
+				// Not valid Go source code - not our job to diagnose, so ignore.
+				return nil, nil
+			}
+			checkGoFile(pass, f)
+		} else {
+			if err := checkOtherFile(pass, name); err != nil {
+				return nil, err
+			}
+		}
+	}
 	return nil, nil
 }
 
@@ -132,13 +147,6 @@
 }
 
 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, "!!") {
diff --git a/go/analysis/passes/buildtag/buildtag_test.go b/go/analysis/passes/buildtag/buildtag_test.go
index e95cc1a..110343c 100644
--- a/go/analysis/passes/buildtag/buildtag_test.go
+++ b/go/analysis/passes/buildtag/buildtag_test.go
@@ -7,11 +7,28 @@
 import (
 	"testing"
 
+	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/analysistest"
 	"golang.org/x/tools/go/analysis/passes/buildtag"
 )
 
 func Test(t *testing.T) {
-	testdata := analysistest.TestData()
-	analysistest.Run(t, testdata, buildtag.Analyzer, "a")
+	analyzer := *buildtag.Analyzer
+	analyzer.Run = func(pass *analysis.Pass) (interface{}, error) {
+		defer func() {
+			// The buildtag pass is unusual in that it checks the IgnoredFiles.
+			// After analysis, add IgnoredFiles to OtherFiles so that
+			// the test harness checks for expected diagnostics in those.
+			// (The test harness shouldn't do this by default because most
+			// passes can't do anything with the IgnoredFiles without type
+			// information, which is unavailable because they are ignored.)
+			var files []string
+			files = append(files, pass.OtherFiles...)
+			files = append(files, pass.IgnoredFiles...)
+			pass.OtherFiles = files
+		}()
+
+		return buildtag.Analyzer.Run(pass)
+	}
+	analysistest.Run(t, analysistest.TestData(), &analyzer, "a")
 }
diff --git a/go/analysis/passes/buildtag/testdata/src/a/buildtag2.go b/go/analysis/passes/buildtag/testdata/src/a/buildtag2.go
new file mode 100644
index 0000000..3b71ca5
--- /dev/null
+++ b/go/analysis/passes/buildtag/testdata/src/a/buildtag2.go
@@ -0,0 +1,9 @@
+// Copyright 2020 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.
+
+// +build no
+
+package a
+
+// +build toolate // want "build comment must appear before package clause and be followed by a blank line$"
diff --git a/go/analysis/unitchecker/unitchecker.go b/go/analysis/unitchecker/unitchecker.go
index 2ed2749..713e138 100644
--- a/go/analysis/unitchecker/unitchecker.go
+++ b/go/analysis/unitchecker/unitchecker.go
@@ -63,6 +63,7 @@
 	ImportPath                string
 	GoFiles                   []string
 	NonGoFiles                []string
+	IgnoredFiles              []string
 	ImportMap                 map[string]string
 	PackageFile               map[string]string
 	Standard                  map[string]bool
@@ -333,6 +334,7 @@
 				Fset:              fset,
 				Files:             files,
 				OtherFiles:        cfg.NonGoFiles,
+				IgnoredFiles:      cfg.IgnoredFiles,
 				Pkg:               pkg,
 				TypesInfo:         info,
 				TypesSizes:        tc.Sizes,
diff --git a/go/packages/golist.go b/go/packages/golist.go
index bc04503..d7dac20 100644
--- a/go/packages/golist.go
+++ b/go/packages/golist.go
@@ -381,32 +381,34 @@
 // Fields must match go list;
 // see $GOROOT/src/cmd/go/internal/load/pkg.go.
 type jsonPackage struct {
-	ImportPath      string
-	Dir             string
-	Name            string
-	Export          string
-	GoFiles         []string
-	CompiledGoFiles []string
-	CFiles          []string
-	CgoFiles        []string
-	CXXFiles        []string
-	MFiles          []string
-	HFiles          []string
-	FFiles          []string
-	SFiles          []string
-	SwigFiles       []string
-	SwigCXXFiles    []string
-	SysoFiles       []string
-	Imports         []string
-	ImportMap       map[string]string
-	Deps            []string
-	Module          *Module
-	TestGoFiles     []string
-	TestImports     []string
-	XTestGoFiles    []string
-	XTestImports    []string
-	ForTest         string // q in a "p [q.test]" package, else ""
-	DepOnly         bool
+	ImportPath        string
+	Dir               string
+	Name              string
+	Export            string
+	GoFiles           []string
+	CompiledGoFiles   []string
+	IgnoredGoFiles    []string
+	IgnoredOtherFiles []string
+	CFiles            []string
+	CgoFiles          []string
+	CXXFiles          []string
+	MFiles            []string
+	HFiles            []string
+	FFiles            []string
+	SFiles            []string
+	SwigFiles         []string
+	SwigCXXFiles      []string
+	SysoFiles         []string
+	Imports           []string
+	ImportMap         map[string]string
+	Deps              []string
+	Module            *Module
+	TestGoFiles       []string
+	TestImports       []string
+	XTestGoFiles      []string
+	XTestImports      []string
+	ForTest           string // q in a "p [q.test]" package, else ""
+	DepOnly           bool
 
 	Error *jsonPackageError
 }
@@ -558,6 +560,7 @@
 			GoFiles:         absJoin(p.Dir, p.GoFiles, p.CgoFiles),
 			CompiledGoFiles: absJoin(p.Dir, p.CompiledGoFiles),
 			OtherFiles:      absJoin(p.Dir, otherFiles(p)...),
+			IgnoredFiles:    absJoin(p.Dir, p.IgnoredGoFiles, p.IgnoredOtherFiles),
 			forTest:         p.ForTest,
 			Module:          p.Module,
 		}
diff --git a/go/packages/golist_overlay.go b/go/packages/golist_overlay.go
index 885aec3..de2c1dc 100644
--- a/go/packages/golist_overlay.go
+++ b/go/packages/golist_overlay.go
@@ -1,3 +1,7 @@
+// Copyright 2018 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 packages
 
 import (
diff --git a/go/packages/packages.go b/go/packages/packages.go
index 04053f1..970c480 100644
--- a/go/packages/packages.go
+++ b/go/packages/packages.go
@@ -289,6 +289,11 @@
 	// including assembly, C, C++, Fortran, Objective-C, SWIG, and so on.
 	OtherFiles []string
 
+	// IgnoredFiles lists source files that are not part of the package
+	// using the current build configuration but that might be part of
+	// the package using other build configurations.
+	IgnoredFiles []string
+
 	// ExportFile is the absolute path to a file containing type
 	// information for the package as provided by the build system.
 	ExportFile string
@@ -404,6 +409,7 @@
 	GoFiles         []string          `json:",omitempty"`
 	CompiledGoFiles []string          `json:",omitempty"`
 	OtherFiles      []string          `json:",omitempty"`
+	IgnoredFiles    []string          `json:",omitempty"`
 	ExportFile      string            `json:",omitempty"`
 	Imports         map[string]string `json:",omitempty"`
 }
@@ -426,6 +432,7 @@
 		GoFiles:         p.GoFiles,
 		CompiledGoFiles: p.CompiledGoFiles,
 		OtherFiles:      p.OtherFiles,
+		IgnoredFiles:    p.IgnoredFiles,
 		ExportFile:      p.ExportFile,
 	}
 	if len(p.Imports) > 0 {
@@ -712,7 +719,8 @@
 		result[i] = lpkg.Package
 	}
 	for i := range ld.pkgs {
-		// Clear all unrequested fields, for extra de-Hyrum-ization.
+		// Clear all unrequested fields,
+		// to catch programs that use more than they request.
 		if ld.requestedMode&NeedName == 0 {
 			ld.pkgs[i].Name = ""
 			ld.pkgs[i].PkgPath = ""
@@ -720,6 +728,7 @@
 		if ld.requestedMode&NeedFiles == 0 {
 			ld.pkgs[i].GoFiles = nil
 			ld.pkgs[i].OtherFiles = nil
+			ld.pkgs[i].IgnoredFiles = nil
 		}
 		if ld.requestedMode&NeedCompiledGoFiles == 0 {
 			ld.pkgs[i].CompiledGoFiles = nil
diff --git a/go/packages/packages_test.go b/go/packages/packages_test.go
index 4015b07..a5351ab 100644
--- a/go/packages/packages_test.go
+++ b/go/packages/packages_test.go
@@ -185,19 +185,20 @@
 
 	// Check node information: kind, name, srcs.
 	for _, test := range []struct {
-		id       string
-		wantName string
-		wantKind string
-		wantSrcs string
+		id          string
+		wantName    string
+		wantKind    string
+		wantSrcs    string
+		wantIgnored string
 	}{
-		{"golang.org/fake/a", "a", "package", "a.go"},
-		{"golang.org/fake/b", "b", "package", "b.go"},
-		{"golang.org/fake/c", "c", "package", "c.go"}, // c2.go is ignored
-		{"golang.org/fake/e", "main", "command", "e.go e2.go"},
-		{"container/list", "list", "package", "list.go"},
-		{"golang.org/fake/subdir/d", "d", "package", "d.go"},
-		{"golang.org/fake/subdir/d.test", "main", "command", "0.go"},
-		{"unsafe", "unsafe", "package", ""},
+		{"golang.org/fake/a", "a", "package", "a.go", ""},
+		{"golang.org/fake/b", "b", "package", "b.go", ""},
+		{"golang.org/fake/c", "c", "package", "c.go", "c2.go"}, // c2.go is ignored
+		{"golang.org/fake/e", "main", "command", "e.go e2.go", ""},
+		{"container/list", "list", "package", "list.go", ""},
+		{"golang.org/fake/subdir/d", "d", "package", "d.go", ""},
+		{"golang.org/fake/subdir/d.test", "main", "command", "0.go", ""},
+		{"unsafe", "unsafe", "package", "", ""},
 	} {
 		p, ok := all[test.id]
 		if !ok {
@@ -222,6 +223,9 @@
 		if srcs := strings.Join(srcs(p), " "); srcs != test.wantSrcs {
 			t.Errorf("%s.Srcs = [%s], want [%s]", test.id, srcs, test.wantSrcs)
 		}
+		if ignored := strings.Join(cleanPaths(p.IgnoredFiles), " "); ignored != test.wantIgnored {
+			t.Errorf("%s.Srcs = [%s], want [%s]", test.id, ignored, test.wantIgnored)
+		}
 	}
 
 	// Test an ad-hoc package, analogous to "go run hello.go".
@@ -1092,6 +1096,9 @@
 			for _, filename := range pkg.OtherFiles {
 				checkFile(filename)
 			}
+			for _, filename := range pkg.IgnoredFiles {
+				checkFile(filename)
+			}
 		}
 	}
 }
@@ -1248,6 +1255,7 @@
 		pkg.GoFiles = cleanPaths(pkg.GoFiles)
 		pkg.CompiledGoFiles = cleanPaths(pkg.CompiledGoFiles)
 		pkg.OtherFiles = cleanPaths(pkg.OtherFiles)
+		pkg.IgnoredFiles = cleanPaths(pkg.IgnoredFiles)
 		if err := enc.Encode(pkg); err != nil {
 			t.Fatal(err)
 		}
@@ -2629,7 +2637,7 @@
 }
 
 func srcs(p *packages.Package) []string {
-	return cleanPaths(append(p.GoFiles, p.OtherFiles...))
+	return cleanPaths(append(p.GoFiles[:len(p.GoFiles):len(p.GoFiles)], p.OtherFiles...))
 }
 
 // cleanPaths attempts to reduce path names to stable forms