go/importer: add ForCompiler, which accepts a token.FileSet

The importer.For function logically requires a FileSet, but did not
when it was first created because export data did not then record
position information. This change adds a new function, ForCompiler,
that has an additional FileSet parameter, and deprecates the For
function.

Before this change, cmd/vet would report confusing spurious
positions for token.Pos values produced by the importer.
The bug is essentially unfixable in cmd/vet.

This CL includes a test that the FileSet is correctly populated.

The changes to cmd/vendor will be applied upstream in a follow-up.

Fixes #28995

Change-Id: I9271bcb1f28e96845c913e15f0304bac93d4d4c4
Reviewed-on: https://go-review.googlesource.com/c/152258
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Robert Griesemer <gri@golang.org>

src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go

@@ -181,6 +181,11 @@ func readConfig(filename string) (*Config, error) {
 	return cfg, nil
 }
 
+var importerForCompiler = func(_ *token.FileSet, compiler string, lookup importer.Lookup) types.Importer {
+	// broken legacy implementation (github.com/golang/go/issues/28995)
+	return importer.For(compiler, lookup)
+}
+
 func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]result, error) {
 	// Load, parse, typecheck.
 	var files []*ast.File
@@ -196,7 +201,7 @@ func run(fset *token.FileSet, cfg *Config, analyzers []*analysis.Analyzer) ([]re
 		}
 		files = append(files, f)
 	}
-	compilerImporter := importer.For(cfg.Compiler, func(path string) (io.ReadCloser, error) {
+	compilerImporter := importerForCompiler(fset, cfg.Compiler, func(path string) (io.ReadCloser, error) {
 		// path is a resolved package path, not an import path.
 		file, ok := cfg.PackageFile[path]
 		if !ok {

src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker112.go

+// +build go1.12
+
+package unitchecker
+
+import "go/importer"
+
+func init() {
+	importerForCompiler = importer.ForCompiler
+}

src/go/importer/importer.go

@@ -20,7 +20,7 @@ import (
 // a given import path, or an error if no matching package is found.
 type Lookup func(path string) (io.ReadCloser, error)
 
-// For returns an Importer for importing from installed packages
+// ForCompiler returns an Importer for importing from installed packages
 // for the compilers "gc" and "gccgo", or for importing directly
 // from the source if the compiler argument is "source". In this
 // latter case, importing may fail under circumstances where the
@@ -39,10 +39,11 @@ type Lookup func(path string) (io.ReadCloser, error)
 // (not relative or absolute ones); it is assumed that the translation
 // to canonical import paths is being done by the client of the
 // importer.
-func For(compiler string, lookup Lookup) types.Importer {
+func ForCompiler(fset *token.FileSet, compiler string, lookup Lookup) types.Importer {
 	switch compiler {
 	case "gc":
 		return &gcimports{
+			fset:     fset,
 			packages: make(map[string]*types.Package),
 			lookup:   lookup,
 		}
@@ -63,13 +64,21 @@ func For(compiler string, lookup Lookup) types.Importer {
 			panic("source importer for custom import path lookup not supported (issue #13847).")
 		}
 
-		return srcimporter.New(&build.Default, token.NewFileSet(), make(map[string]*types.Package))
+		return srcimporter.New(&build.Default, fset, make(map[string]*types.Package))
 	}
 
 	// compiler not supported
 	return nil
 }
 
+// For calls ForCompiler with a new FileSet.
+//
+// Deprecated: use ForCompiler, which populates a FileSet
+// with the positions of objects created by the importer.
+func For(compiler string, lookup Lookup) types.Importer {
+	return ForCompiler(token.NewFileSet(), compiler, lookup)
+}
+
 // Default returns an Importer for the compiler that built the running binary.
 // If available, the result implements types.ImporterFrom.
 func Default() types.Importer {
@@ -79,6 +88,7 @@ func Default() types.Importer {
 // gc importer
 
 type gcimports struct {
+	fset     *token.FileSet
 	packages map[string]*types.Package
 	lookup   Lookup
 }
@@ -91,7 +101,7 @@ func (m *gcimports) ImportFrom(path, srcDir string, mode types.ImportMode) (*typ
 	if mode != 0 {
 		panic("mode must be 0")
 	}
-	return gcimporter.Import(m.packages, path, srcDir, m.lookup)
+	return gcimporter.Import(m.fset, m.packages, path, srcDir, m.lookup)
 }
 
 // gccgo importer

src/go/importer/importer_test.go

@@ -5,15 +5,18 @@
 package importer
 
 import (
+	"go/token"
 	"internal/testenv"
 	"io"
+	"io/ioutil"
 	"os"
 	"os/exec"
+	"runtime"
 	"strings"
 	"testing"
 )
 
-func TestFor(t *testing.T) {
+func TestForCompiler(t *testing.T) {
 	testenv.MustHaveGoBuild(t)
 
 	const thePackage = "math/big"
@@ -32,8 +35,10 @@ func TestFor(t *testing.T) {
 		t.Skip("golang.org/issue/22500")
 	}
 
+	fset := token.NewFileSet()
+
 	t.Run("LookupDefault", func(t *testing.T) {
-		imp := For(compiler, nil)
+		imp := ForCompiler(fset, compiler, nil)
 		pkg, err := imp.Import(thePackage)
 		if err != nil {
 			t.Fatal(err)
@@ -41,6 +46,21 @@ func TestFor(t *testing.T) {
 		if pkg.Path() != thePackage {
 			t.Fatalf("Path() = %q, want %q", pkg.Path(), thePackage)
 		}
+
+		// Check that the fileset positions are accurate.
+		// https://github.com/golang/go#28995
+		mathBigInt := pkg.Scope().Lookup("Int")
+		posn := fset.Position(mathBigInt.Pos()) // "$GOROOT/src/math/big/int.go:25:1"
+		filename := strings.Replace(posn.Filename, "$GOROOT", runtime.GOROOT(), 1)
+		data, err := ioutil.ReadFile(filename)
+		if err != nil {
+			t.Fatalf("can't read file containing declaration of math/big.Int: %v", err)
+		}
+		lines := strings.Split(string(data), "\n")
+		if posn.Line > len(lines) || !strings.HasPrefix(lines[posn.Line-1], "type Int") {
+			t.Fatalf("Object %v position %s does not contain its declaration",
+				mathBigInt, posn)
+		}
 	})
 
 	t.Run("LookupCustom", func(t *testing.T) {
@@ -54,7 +74,7 @@ func TestFor(t *testing.T) {
 			}
 			return f, nil
 		}
-		imp := For(compiler, lookup)
+		imp := ForCompiler(fset, compiler, lookup)
 		pkg, err := imp.Import("math/bigger")
 		if err != nil {
 			t.Fatal(err)

src/go/internal/gcimporter/gcimporter.go

@@ -85,7 +85,7 @@ func FindPkg(path, srcDir string) (filename, id string) {
 // the corresponding package object to the packages map, and returns the object.
 // The packages map must contain all packages already imported.
 //
-func Import(packages map[string]*types.Package, path, srcDir string, lookup func(path string) (io.ReadCloser, error)) (pkg *types.Package, err error) {
+func Import(fset *token.FileSet, packages map[string]*types.Package, path, srcDir string, lookup func(path string) (io.ReadCloser, error)) (pkg *types.Package, err error) {
 	var rc io.ReadCloser
 	var id string
 	if lookup != nil {
@@ -152,10 +152,6 @@ func Import(packages map[string]*types.Package, path, srcDir string, lookup func
 			break
 		}
 
-		// TODO(gri): allow clients of go/importer to provide a FileSet.
-		// Or, define a new standard go/types/gcexportdata package.
-		fset := token.NewFileSet()
-
 		// The indexed export format starts with an 'i'; the older
 		// binary export format starts with a 'c', 'd', or 'v'
 		// (from "version"). Select appropriate importer.

src/go/internal/gcimporter/gcimporter_test.go

@@ -17,6 +17,7 @@ import (
 	"testing"
 	"time"
 
+	"go/token"
 	"go/types"
 )
 
@@ -55,7 +56,8 @@ func compile(t *testing.T, dirname, filename, outdirname string) string {
 
 func testPath(t *testing.T, path, srcDir string) *types.Package {
 	t0 := time.Now()
-	pkg, err := Import(make(map[string]*types.Package), path, srcDir, nil)
+	fset := token.NewFileSet()
+	pkg, err := Import(fset, make(map[string]*types.Package), path, srcDir, nil)
 	if err != nil {
 		t.Errorf("testPath(%s): %s", path, err)
 		return nil
@@ -158,6 +160,8 @@ func TestVersionHandling(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	fset := token.NewFileSet()
+
 	for _, f := range list {
 		name := f.Name()
 		if !strings.HasSuffix(name, ".a") {
@@ -173,7 +177,7 @@ func TestVersionHandling(t *testing.T) {
 		}
 
 		// test that export data can be imported
-		_, err := Import(make(map[string]*types.Package), pkgpath, dir, nil)
+		_, err := Import(fset, make(map[string]*types.Package), pkgpath, dir, nil)
 		if err != nil {
 			// ok to fail if it fails with a newer version error for select files
 			if strings.Contains(err.Error(), "newer version") {
@@ -209,7 +213,7 @@ func TestVersionHandling(t *testing.T) {
 		ioutil.WriteFile(filename, data, 0666)
 
 		// test that importing the corrupted file results in an error
-		_, err = Import(make(map[string]*types.Package), pkgpath, corruptdir, nil)
+		_, err = Import(fset, make(map[string]*types.Package), pkgpath, corruptdir, nil)
 		if err == nil {
 			t.Errorf("import corrupted %q succeeded", pkgpath)
 		} else if msg := err.Error(); !strings.Contains(msg, "version skew") {
@@ -266,6 +270,7 @@ func TestImportedTypes(t *testing.T) {
 		t.Skipf("gc-built packages not available (compiler = %s)", runtime.Compiler)
 	}
 
+	fset := token.NewFileSet()
 	for _, test := range importedObjectTests {
 		s := strings.Split(test.name, ".")
 		if len(s) != 2 {
@@ -274,7 +279,7 @@ func TestImportedTypes(t *testing.T) {
 		importPath := s[0]
 		objName := s[1]
 
-		pkg, err := Import(make(map[string]*types.Package), importPath, ".", nil)
+		pkg, err := Import(fset, make(map[string]*types.Package), importPath, ".", nil)
 		if err != nil {
 			t.Error(err)
 			continue
@@ -371,7 +376,8 @@ func TestCorrectMethodPackage(t *testing.T) {
 	}
 
 	imports := make(map[string]*types.Package)
-	_, err := Import(imports, "net/http", ".", nil)
+	fset := token.NewFileSet()
+	_, err := Import(fset, imports, "net/http", ".", nil)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -433,8 +439,9 @@ func TestIssue13898(t *testing.T) {
 	}
 
 	// import go/internal/gcimporter which imports go/types partially
+	fset := token.NewFileSet()
 	imports := make(map[string]*types.Package)
-	_, err := Import(imports, "go/internal/gcimporter", ".", nil)
+	_, err := Import(fset, imports, "go/internal/gcimporter", ".", nil)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -502,8 +509,9 @@ func TestIssue15517(t *testing.T) {
 	// file and package path are different, exposing the problem if present.
 	// The same issue occurs with vendoring.)
 	imports := make(map[string]*types.Package)
+	fset := token.NewFileSet()
 	for i := 0; i < 3; i++ {
-		if _, err := Import(imports, "./././testdata/p", tmpdir, nil); err != nil {
+		if _, err := Import(fset, imports, "./././testdata/p", tmpdir, nil); err != nil {
 			t.Fatal(err)
 		}
 	}
@@ -582,7 +590,8 @@ func TestIssue25596(t *testing.T) {
 }
 
 func importPkg(t *testing.T, path, srcDir string) *types.Package {
-	pkg, err := Import(make(map[string]*types.Package), path, srcDir, nil)
+	fset := token.NewFileSet()
+	pkg, err := Import(fset, make(map[string]*types.Package), path, srcDir, nil)
 	if err != nil {
 		t.Fatal(err)
 	}