internal/lsp: fix find-implementation for promoted methods

We weren't returning promoted methods as implementations when the
promoted method was defined in a different package than the type
implementing the interface.

Fix by properly mapping the implementer types.Object to its containing
source.Package.

I generalized the implementations() result to just contain the
implementer objects and their containing package. This allowed me to
get rid of some result prep code in Implementation().

Fixes golang/go#35972.

Change-Id: I867f2114c34e2ad39515ee3c8b6354c1cd35f7af
Reviewed-on: https://go-review.googlesource.com/c/tools/+/210280
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/implementation.go

@@ -50,7 +50,7 @@ func (s *Server) implementation(ctx context.Context, params *protocol.Implementa
 
 		locs, err := ident.Implementation(ctx)
 		if err != nil {
-			if err == source.ErrNotAMethod {
+			if err == source.ErrNotAnInterface {
 				return nil, err
 			}
 			log.Error(ctx, "failed to find Implemenation", err)

internal/lsp/source/implementation.go

@@ -2,20 +2,12 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-// The code in this file is based largely on the code in
-// cmd/guru/implements.go. The guru implementation supports
-// looking up "implementers" of methods also, but that
-// code has been cut out here for now for simplicity.
-
 package source
 
 import (
 	"context"
-	"fmt"
-	"go/token"
 	"go/types"
 
-	"golang.org/x/tools/go/types/typeutil"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/telemetry"
 	"golang.org/x/tools/internal/telemetry/log"
@@ -25,75 +17,35 @@ import (
 func (i *IdentifierInfo) Implementation(ctx context.Context) ([]protocol.Location, error) {
 	ctx = telemetry.Package.With(ctx, i.pkg.ID())
 
-	res, err := i.implementations(ctx)
+	impls, err := i.implementations(ctx)
 	if err != nil {
 		return nil, err
 	}
 
-	var objs []types.Object
-	pkgs := map[token.Pos]Package{}
-
-	if res.toMethod != nil {
-		// If we looked up a method, results are in toMethod.
-		for _, s := range res.toMethod {
-			if pkgs[s.Obj().Pos()] != nil {
-				continue
-			}
-			// Determine package of receiver.
-			recv := s.Recv()
-			if p, ok := recv.(*types.Pointer); ok {
-				recv = p.Elem()
-			}
-			if n, ok := recv.(*types.Named); ok {
-				pkg := res.pkgs[n]
-				pkgs[s.Obj().Pos()] = pkg
-			}
-			// Add object to objs.
-			objs = append(objs, s.Obj())
-		}
-	} else {
-		// Otherwise, the results are in to.
-		for _, t := range res.to {
-			// We'll provide implementations that are named types and pointers to named types.
-			if p, ok := t.(*types.Pointer); ok {
-				t = p.Elem()
-			}
-			if n, ok := t.(*types.Named); ok {
-				if pkgs[n.Obj().Pos()] != nil {
-					continue
-				}
-				pkg := res.pkgs[n]
-				pkgs[n.Obj().Pos()] = pkg
-				objs = append(objs, n.Obj())
-			}
-		}
-	}
-
 	var locations []protocol.Location
-	for _, obj := range objs {
-		pkg := pkgs[obj.Pos()]
-		if pkgs[obj.Pos()] == nil || len(pkg.CompiledGoFiles()) == 0 {
+	for _, impl := range impls {
+		if impl.pkg == nil || len(impl.pkg.CompiledGoFiles()) == 0 {
 			continue
 		}
-		file, _, err := i.Snapshot.View().FindPosInPackage(pkgs[obj.Pos()], obj.Pos())
+
+		file, _, err := i.Snapshot.View().FindPosInPackage(impl.pkg, impl.obj.Pos())
 		if err != nil {
 			log.Error(ctx, "Error getting file for object", err)
 			continue
 		}
-		ident, err := findIdentifier(i.Snapshot, pkg, file, obj.Pos())
+
+		ident, err := findIdentifier(i.Snapshot, impl.pkg, file, impl.obj.Pos())
 		if err != nil {
 			log.Error(ctx, "Error getting ident for object", err)
 			continue
 		}
+
 		decRange, err := ident.Declaration.Range()
 		if err != nil {
 			log.Error(ctx, "Error getting range for object", err)
 			continue
 		}
-		// Do not add interface itself to the list.
-		if ident.Declaration.spanRange == i.Declaration.spanRange {
-			continue
-		}
+
 		locations = append(locations, protocol.Location{
 			URI:   protocol.NewURI(ident.Declaration.URI()),
 			Range: decRange,
@@ -102,75 +54,95 @@ func (i *IdentifierInfo) Implementation(ctx context.Context) ([]protocol.Locatio
 	return locations, nil
 }
 
-var ErrNotAMethod = errors.New("this function is not a method")
+var ErrNotAnInterface = errors.New("not an interface or interface method")
 
-func (i *IdentifierInfo) implementations(ctx context.Context) (implementsResult, error) {
-	var T types.Type
-	var method *types.Func
-	if i.Type.Object == nil {
-		// This isn't a type. Is it a method?
-		obj, ok := i.Declaration.obj.(*types.Func)
-		if !ok {
-			return implementsResult{}, fmt.Errorf("no type info object for identifier %q", i.Name)
-		}
-		recv := obj.Type().(*types.Signature).Recv()
-		if recv == nil {
-			return implementsResult{}, ErrNotAMethod
-		}
+func (i *IdentifierInfo) implementations(ctx context.Context) ([]implementation, error) {
+	var (
+		T      *types.Interface
+		method *types.Func
+	)
+
+	switch obj := i.Declaration.obj.(type) {
+	case *types.Func:
 		method = obj
-		T = recv.Type()
-	} else {
-		T = i.Type.Object.Type()
+		if recv := obj.Type().(*types.Signature).Recv(); recv != nil {
+			T, _ = recv.Type().Underlying().(*types.Interface)
+		}
+	case *types.TypeName:
+		T, _ = obj.Type().Underlying().(*types.Interface)
+	}
+
+	if T == nil {
+		return nil, ErrNotAnInterface
+	}
+
+	if T.NumMethods() == 0 {
+		return nil, nil
 	}
 
 	// Find all named types, even local types (which can have methods
-	// due to promotion). We ignore aliases 'type M = N' to avoid
-	// duplicate reporting of the Named type N.
-	var allNamed []*types.Named
-	pkgs := map[*types.Named]Package{}
+	// due to promotion).
+	var (
+		allNamed []*types.Named
+		pkgs     = make(map[*types.Package]Package)
+	)
 	for _, pkg := range i.Snapshot.KnownPackages(ctx) {
+		pkgs[pkg.GetTypes()] = pkg
+
 		info := pkg.GetTypesInfo()
 		for _, obj := range info.Defs {
+			// We ignore aliases 'type M = N' to avoid duplicate reporting
+			// of the Named type N.
 			if obj, ok := obj.(*types.TypeName); ok && !obj.IsAlias() {
+				// We skip interface types since we only want concrete
+				// implementations.
 				if named, ok := obj.Type().(*types.Named); ok && !isInterface(named) {
 					allNamed = append(allNamed, named)
-					pkgs[named] = pkg
 				}
 			}
 		}
 	}
 
-	var msets typeutil.MethodSetCache
+	var (
+		impls []implementation
+		seen  = make(map[types.Object]bool)
+	)
 
-	// Test each named type.
-	var to []types.Type
+	// Find all the named types that implement our interface.
 	for _, U := range allNamed {
-		if isInterface(T) {
-			if msets.MethodSet(T).Len() == 0 {
-				continue // empty interface
-			}
-
-			// T interface, U concrete
-			if types.AssignableTo(U, T) {
-				to = append(to, U)
-			} else if pU := types.NewPointer(U); types.AssignableTo(pU, T) {
-				to = append(to, pU)
+		var concrete types.Type = U
+		if !types.AssignableTo(concrete, T) {
+			// We also accept T if *T implements our interface.
+			concrete = types.NewPointer(concrete)
+			if !types.AssignableTo(concrete, T) {
+				continue
 			}
 		}
-	}
-	var toMethod []*types.Selection // contain nils
-	if method != nil {
-		for _, t := range to {
-			toMethod = append(toMethod,
-				types.NewMethodSet(t).Lookup(method.Pkg(), method.Name()))
+
+		var obj types.Object = U.Obj()
+		if method != nil {
+			obj = types.NewMethodSet(concrete).Lookup(method.Pkg(), method.Name()).Obj()
 		}
+
+		if obj == method || seen[obj] {
+			continue
+		}
+
+		seen[obj] = true
+
+		impls = append(impls, implementation{
+			obj: obj,
+			pkg: pkgs[obj.Pkg()],
+		})
 	}
-	return implementsResult{pkgs, to, toMethod}, nil
+
+	return impls, nil
 }
 
-// implementsResult contains the results of an implements query.
-type implementsResult struct {
-	pkgs     map[*types.Named]Package
-	to       []types.Type // named or ptr-to-named types assignable to interface T
-	toMethod []*types.Selection
+type implementation struct {
+	// obj is the implementation, either a *types.TypeName or *types.Func.
+	obj types.Object
+
+	// pkg is the Package that contains obj's definition.
+	pkg Package
 }

internal/lsp/testdata/implementation/implementation.go

@@ -25,7 +25,7 @@ type Foo struct {
 }
 
 type U interface {
-	U() //TODO: fix flaky @implementations("U", ImpU)
+	U() //@implementations("U", ImpU)
 }
 
 type cryer int

internal/lsp/testdata/summary.txt.golden

@@ -20,5 +20,5 @@ PrepareRenamesCount = 8
 SymbolsCount = 1
 SignaturesCount = 22
 LinksCount = 6
-ImplementationsCount = 4
+ImplementationsCount = 5