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>

diff --git a/src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go b/src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go
index ec3103b27e298c249ac2c3d0d455cfbe669c39a9..018191a5e7a25cf38b2c479ecdfc4ea496739cbf 100644 (file)
--- a/src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker.go
+++ b/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 {

diff --git a/src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker112.go b/src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker112.go
new file mode 100644 (file)
index 0000000..683b7e9
--- /dev/null
+++ b/src/cmd/vendor/golang.org/x/tools/go/analysis/unitchecker/unitchecker112.go
@@ -0,0 +1,9 @@
+// +build go1.12
+
+package unitchecker
+
+import "go/importer"
+
+func init() {
+       importerForCompiler = importer.ForCompiler
+}

diff --git a/src/go/importer/importer.go b/src/go/importer/importer.go
index f0a1ca2b76a18bc05cce274934791e744286126c..c809c9ab8678c3ffd15435ae9957a973d7c5b5ed 100644 (file)
--- a/src/go/importer/importer.go
+++ b/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

diff --git a/src/go/importer/importer_test.go b/src/go/importer/importer_test.go
index 56e83136fb1a0b09b4c6af27d69eeccf598d9252..ff6e12c0da5233a5d340058e353a3768273a8722 100644 (file)
--- a/src/go/importer/importer_test.go
+++ b/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)

diff --git a/src/go/internal/gcimporter/gcimporter.go b/src/go/internal/gcimporter/gcimporter.go
index d117f6fe4d3f4b91ddbd6a59c2083636b24acce0..3aed6de6ae0b33624cc2c1a528059171e0bdb031 100644 (file)
--- a/src/go/internal/gcimporter/gcimporter.go
+++ b/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.

diff --git a/src/go/internal/gcimporter/gcimporter_test.go b/src/go/internal/gcimporter/gcimporter_test.go
index 222b36c883688e8cacfb0d0b8d1d36f6913ee919..3b7636806e7e5a8ad8f06ee83b9d24000abf2796 100644 (file)
--- a/src/go/internal/gcimporter/gcimporter_test.go
+++ b/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)
        }