go/internal/gcimporter: remove support for type parameters subscripts
Now that the compiler no longer includes subscripts in its export data,
we can remove support for parsing them in the gcimporter.
Make this change, as well as add support for exporting the new names to
iexport.go (tests fail if these changes are made in separate CLs). This
proved a bit tricky to debug, so along the way add some hooks for easier
tracing of import/export.
Fix a bug in iimport.go where rparams were being shadowed, and fix the
export tests to actually compare methods.
Change-Id: Ie08aa5e5ee971d357d352b8f4a4673f6d2a38779
Reviewed-on: https://go-review.googlesource.com/c/tools/+/356534
Trust: Robert Findley <rfindley@google.com>
Trust: Dan Scales <danscales@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
diff --git a/go/internal/gcimporter/bexport.go b/go/internal/gcimporter/bexport.go
index 072005a..0a3cdb9 100644
--- a/go/internal/gcimporter/bexport.go
+++ b/go/internal/gcimporter/bexport.go
@@ -34,9 +34,6 @@
// (suspected) format errors, and whenever a change is made to the format.
const debugFormat = false // default: false
-// If trace is set, debugging output is printed to std out.
-const trace = false // default: false
-
// Current export format version. Increase with each format change.
// Note: The latest binary (non-indexed) export format is at version 6.
// This exporter is still at level 4, but it doesn't matter since
diff --git a/go/internal/gcimporter/bexport_test.go b/go/internal/gcimporter/bexport_test.go
index 7300ba8..8a80427 100644
--- a/go/internal/gcimporter/bexport_test.go
+++ b/go/internal/gcimporter/bexport_test.go
@@ -15,6 +15,7 @@
"path/filepath"
"reflect"
"runtime"
+ "sort"
"strings"
"testing"
@@ -242,6 +243,32 @@
if err := equalTypeArgs(typeparams.NamedTypeArgs(x), typeparams.NamedTypeArgs(y)); err != nil {
return fmt.Errorf("type arguments: %s", err)
}
+ if x.NumMethods() != y.NumMethods() {
+ return fmt.Errorf("unequal methods: %d vs %d",
+ x.NumMethods(), y.NumMethods())
+ }
+ // Unfortunately method sorting is not canonical, so sort before comparing.
+ var xms, yms []*types.Func
+ for i := 0; i < x.NumMethods(); i++ {
+ xms = append(xms, x.Method(i))
+ yms = append(yms, y.Method(i))
+ }
+ for _, ms := range [][]*types.Func{xms, yms} {
+ sort.Slice(ms, func(i, j int) bool {
+ return ms[i].Name() < ms[j].Name()
+ })
+ }
+ for i, xm := range xms {
+ ym := yms[i]
+ if xm.Name() != ym.Name() {
+ return fmt.Errorf("mismatched %dth method: %s vs %s", i, xm, ym)
+ }
+ // Calling equalType here leads to infinite recursion, so just compare
+ // strings.
+ if sanitizeName(xm.String()) != sanitizeName(ym.String()) {
+ return fmt.Errorf("unequal methods: %s vs %s", x, y)
+ }
+ }
case *types.Pointer:
y := y.(*types.Pointer)
if err := equalType(x.Elem(), y.Elem()); err != nil {
diff --git a/go/internal/gcimporter/gcimporter.go b/go/internal/gcimporter/gcimporter.go
index e8cba6b..519ef3b 100644
--- a/go/internal/gcimporter/gcimporter.go
+++ b/go/internal/gcimporter/gcimporter.go
@@ -29,8 +29,14 @@
"text/scanner"
)
-// debugging/development support
-const debug = false
+const (
+ // Enable debug during development: it adds some additional checks, and
+ // prevents errors from being recovered.
+ debug = false
+
+ // If trace is set, debugging output is printed to std out.
+ trace = false
+)
var pkgExts = [...]string{".a", ".o"}
diff --git a/go/internal/gcimporter/gcimporter_test.go b/go/internal/gcimporter/gcimporter_test.go
index e5eb9ed..249af96 100644
--- a/go/internal/gcimporter/gcimporter_test.go
+++ b/go/internal/gcimporter/gcimporter_test.go
@@ -162,6 +162,9 @@
}
func TestVersionHandling(t *testing.T) {
+ if debug {
+ t.Skip("TestVersionHandling panics in debug mode")
+ }
skipSpecialPlatforms(t) // we really only need to exclude nacl platforms, but this is fine
// This package only handles gc export data.
diff --git a/go/internal/gcimporter/iexport.go b/go/internal/gcimporter/iexport.go
index be8b745..c8fa0ac 100644
--- a/go/internal/gcimporter/iexport.go
+++ b/go/internal/gcimporter/iexport.go
@@ -11,6 +11,7 @@
import (
"bytes"
"encoding/binary"
+ "fmt"
"go/ast"
"go/constant"
"go/token"
@@ -19,6 +20,7 @@
"math/big"
"reflect"
"sort"
+ "strings"
"golang.org/x/tools/internal/typeparams"
)
@@ -60,6 +62,7 @@
allPkgs: map[*types.Package]bool{},
stringIndex: map[string]uint64{},
declIndex: map[types.Object]uint64{},
+ tparamNames: map[types.Object]string{},
typIndex: map[types.Type]uint64{},
}
if !bundle {
@@ -136,8 +139,12 @@
// non-compiler tools and includes a complete package description
// (i.e., name and height).
func (w *exportWriter) writeIndex(index map[types.Object]uint64) {
+ type pkgObj struct {
+ obj types.Object
+ name string // qualified name; differs from obj.Name for type params
+ }
// Build a map from packages to objects from that package.
- pkgObjs := map[*types.Package][]types.Object{}
+ pkgObjs := map[*types.Package][]pkgObj{}
// For the main index, make sure to include every package that
// we reference, even if we're not exporting (or reexporting)
@@ -150,7 +157,8 @@
}
for obj := range index {
- pkgObjs[obj.Pkg()] = append(pkgObjs[obj.Pkg()], obj)
+ name := w.p.indexName(obj)
+ pkgObjs[obj.Pkg()] = append(pkgObjs[obj.Pkg()], pkgObj{obj, name})
}
var pkgs []*types.Package
@@ -158,7 +166,7 @@
pkgs = append(pkgs, pkg)
sort.Slice(objs, func(i, j int) bool {
- return indexName(objs[i]) < indexName(objs[j])
+ return objs[i].name < objs[j].name
})
}
@@ -175,22 +183,18 @@
objs := pkgObjs[pkg]
w.uint64(uint64(len(objs)))
for _, obj := range objs {
- w.string(indexName(obj))
- w.uint64(index[obj])
+ w.string(obj.name)
+ w.uint64(index[obj.obj])
}
}
}
// indexName returns the 'indexed' name of an object. It differs from
-// obj.Name() only for type parameter names, where we include the subscripted
-// type parameter ID.
-//
-// TODO(rfindley): remove this once we no longer need subscripts.
-func indexName(obj types.Object) (res string) {
- if _, ok := obj.(*types.TypeName); ok {
- if tparam, ok := obj.Type().(*typeparams.TypeParam); ok {
- return types.TypeString(tparam, func(*types.Package) string { return "" })
- }
+// obj.Name() only for type parameter names, where the name is qualified by
+// owner.
+func (p *iexporter) indexName(obj types.Object) (res string) {
+ if name := p.tparamNames[obj]; name != "" {
+ return name
}
return obj.Name()
}
@@ -211,9 +215,21 @@
strings intWriter
stringIndex map[string]uint64
- data0 intWriter
- declIndex map[types.Object]uint64
- typIndex map[types.Type]uint64
+ data0 intWriter
+ declIndex map[types.Object]uint64
+ tparamNames map[types.Object]string // typeparam->qualified name
+ typIndex map[types.Type]uint64
+
+ indent int // for tracing support
+}
+
+func (p *iexporter) trace(format string, args ...interface{}) {
+ if !debug {
+ // Call sites should also be guarded, but having this check here allows
+ // easily enabling/disabling debug trace statements.
+ return
+ }
+ fmt.Printf(strings.Repeat("..", p.indent)+format+"\n", args...)
}
// stringOff returns the offset of s within the string section.
@@ -239,7 +255,7 @@
return
}
- p.declIndex[obj] = ^uint64(0) // mark n present in work queue
+ p.declIndex[obj] = ^uint64(0) // mark obj present in work queue
p.declTodo.pushTail(obj)
}
@@ -262,6 +278,14 @@
}
func (p *iexporter) doDecl(obj types.Object) {
+ if trace {
+ p.trace("exporting decl %v (%T)", obj, obj)
+ p.indent++
+ defer func() {
+ p.indent--
+ p.trace("=> %s", obj)
+ }()
+ }
w := p.newWriter()
w.setPkg(obj.Pkg(), false)
@@ -291,7 +315,7 @@
// other places in the signature and function that they
// are used.
if tparams := typeparams.ForSignature(sig); tparams.Len() > 0 {
- w.tparamList(tparams, obj.Pkg())
+ w.tparamList(obj, tparams, obj.Pkg())
}
w.signature(sig)
@@ -331,7 +355,7 @@
w.pos(obj.Pos())
if typeparams.ForNamed(named).Len() > 0 {
- w.tparamList(typeparams.ForNamed(named), obj.Pkg())
+ w.tparamList(obj, typeparams.ForNamed(named), obj.Pkg())
}
underlying := obj.Type().Underlying()
@@ -348,6 +372,15 @@
w.pos(m.Pos())
w.string(m.Name())
sig, _ := m.Type().(*types.Signature)
+
+ // Receiver type parameters are type arguments of the receiver type, so
+ // their name must be qualified before exporting recv.
+ rparams := typeparams.RecvTypeParams(sig)
+ for i := 0; i < rparams.Len(); i++ {
+ rparam := rparams.At(i)
+ name := obj.Name() + "." + m.Name() + "." + rparam.Obj().Name()
+ w.p.tparamNames[rparam.Obj()] = name
+ }
w.param(sig.Recv())
w.signature(sig)
}
@@ -447,9 +480,12 @@
}
func (w *exportWriter) qualifiedIdent(obj types.Object) {
+ name := w.p.indexName(obj)
+
// Ensure any referenced declarations are written out too.
w.p.pushDecl(obj)
- w.string(indexName(obj))
+ w.p.trace("writing ident %s for %s", name, obj)
+ w.string(name)
w.pkg(obj.Pkg())
}
@@ -483,6 +519,14 @@
}
func (w *exportWriter) doTyp(t types.Type, pkg *types.Package) {
+ if trace {
+ w.p.trace("exporting type %s (%T)", t, t)
+ w.p.indent++
+ defer func() {
+ w.p.indent--
+ w.p.trace("=> %s", t)
+ }()
+ }
switch t := t.(type) {
case *types.Named:
if targs := typeparams.NamedTypeArgs(t); targs.Len() > 0 {
@@ -619,10 +663,14 @@
}
}
-func (w *exportWriter) tparamList(list *typeparams.TypeParamList, pkg *types.Package) {
+func (w *exportWriter) tparamList(owner types.Object, list *typeparams.TypeParamList, pkg *types.Package) {
ll := uint64(list.Len())
w.uint64(ll)
for i := 0; i < list.Len(); i++ {
+ tparam := list.At(i)
+ // Qualify the type parameter name before exporting its type.
+ name := owner.Name() + "." + tparam.Obj().Name()
+ w.p.tparamNames[tparam.Obj()] = name
w.typ(list.At(i), pkg)
}
}
@@ -832,7 +880,7 @@
return
}
- name := indexName(obj)
+ name := obj.Name()
if name == "_" {
w.string("_")
return
diff --git a/go/internal/gcimporter/iimport.go b/go/internal/gcimporter/iimport.go
index 8843db0..58d4e2b 100644
--- a/go/internal/gcimporter/iimport.go
+++ b/go/internal/gcimporter/iimport.go
@@ -154,7 +154,7 @@
pkgIndex: make(map[*types.Package]map[string]uint64),
typCache: make(map[uint64]types.Type),
// Separate map for typeparams, keyed by their package and unique
- // name (name with subscript).
+ // name.
tparamIndex: make(map[ident]types.Type),
fake: fakeFileSet{
@@ -262,9 +262,28 @@
fake fakeFileSet
interfaceList []*types.Interface
+
+ indent int // for tracing support
+}
+
+func (p *iimporter) trace(format string, args ...interface{}) {
+ if !trace {
+ // Call sites should also be guarded, but having this check here allows
+ // easily enabling/disabling debug trace statements.
+ return
+ }
+ fmt.Printf(strings.Repeat("..", p.indent)+format+"\n", args...)
}
func (p *iimporter) doDecl(pkg *types.Package, name string) {
+ if debug {
+ p.trace("import decl %s", name)
+ p.indent++
+ defer func() {
+ p.indent--
+ p.trace("=> %s", name)
+ }()
+ }
// See if we've already imported this declaration.
if obj := pkg.Scope().Lookup(name); obj != nil {
return
@@ -381,15 +400,13 @@
// If the receiver has any targs, set those as the
// rparams of the method (since those are the
// typeparams being used in the method sig/body).
- targs := typeparams.NamedTypeArgs(baseType(recv.Type()))
+ base := baseType(recv.Type())
+ assert(base != nil)
+ targs := typeparams.NamedTypeArgs(base)
var rparams []*typeparams.TypeParam
if targs.Len() > 0 {
- rparams := make([]*typeparams.TypeParam, targs.Len())
+ rparams = make([]*typeparams.TypeParam, targs.Len())
for i := range rparams {
- // TODO(rfindley): this is less tolerant than the standard library
- // go/internal/gcimporter, which calls under(...) and is tolerant
- // of nil rparams. Bring them in sync by making the standard
- // library importer stricter.
rparams[i] = targs.At(i).(*typeparams.TypeParam)
}
}
@@ -406,25 +423,15 @@
if r.p.exportVersion < iexportVersionGenerics {
errorf("unexpected type param type")
}
- // Temporarily strip both type parameter subscripts and path prefixes,
- // while we replace subscripts with prefixes in the compiler.
- // TODO(rfindley): remove support for subscripts once the compiler changes
- // have landed.
- name0, _ := parseSubscript(name)
+ // Remove the "path" from the type param name that makes it unique
ix := strings.LastIndex(name, ".")
- name0 = name0[ix+1:]
+ if ix < 0 {
+ errorf("missing path for type param")
+ }
+ name0 := name[ix+1:]
tn := types.NewTypeName(pos, r.currPkg, name0, nil)
t := typeparams.NewTypeParam(tn, nil)
- // The check below is disabled so that we can support both path-prefixed
- // and subscripted type parameter names.
- // if sub == 0 {
- // errorf("name %q missing subscript", name)
- // }
-
- // TODO(rfindley): can we use a different, stable ID?
- // t.SetId(sub)
-
// To handle recursive references to the typeparam within its
// bound, save the partial type in tparamIndex before reading the bounds.
id := ident{r.currPkg.Name(), name}
@@ -638,8 +645,17 @@
func (r *importReader) pkg() *types.Package { return r.p.pkgAt(r.uint64()) }
func (r *importReader) string() string { return r.p.stringAt(r.uint64()) }
-func (r *importReader) doType(base *types.Named) types.Type {
- switch k := r.kind(); k {
+func (r *importReader) doType(base *types.Named) (res types.Type) {
+ k := r.kind()
+ if debug {
+ r.p.trace("importing type %d (base: %s)", k, base)
+ r.p.indent++
+ defer func() {
+ r.p.indent--
+ r.p.trace("=> %s", res)
+ }()
+ }
+ switch k {
default:
errorf("unexpected kind tag in %q: %v", r.p.ipath, k)
return nil
@@ -832,23 +848,3 @@
n, _ := typ.(*types.Named)
return n
}
-
-func parseSubscript(name string) (string, uint64) {
- // Extract the subscript value from the type param name. We export
- // and import the subscript value, so that all type params have
- // unique names.
- sub := uint64(0)
- startsub := -1
- for i, r := range name {
- if '₀' <= r && r < '₀'+10 {
- if startsub == -1 {
- startsub = i
- }
- sub = sub*10 + uint64(r-'₀')
- }
- }
- if startsub >= 0 {
- name = name[:startsub]
- }
- return name, sub
-}
