gopls/internal/lsp: separate some requests from source.Identifier

Start to unwind source.Identifier by unrolling definition, type
definition, and call hierarchy handlers.

Along the way, introduce a couple primitive helper functions, which may
be made obsolete in the future but allowed preserving source.Identifier
behavior:
- referencedObject returns the object referenced by the cursor position,
  as defined by source.Identifier.
- mapPosition is a helper to map token.Pos to MappedRange in the narrow
  context of a package fileset.

After this change, the only remaining use of source.Identifier is for
Hover, but that is a sizeable refactoring and therefore left to a
subsequent CL.

Updates golang/go#57987

Change-Id: Iba4b0a574e6a6d3d54253f3b4bff8fe6e13a1b15
Reviewed-on: https://go-review.googlesource.com/c/tools/+/463955
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: findleyr

gopls/internal/lsp/definition.go

@@ -14,41 +14,33 @@ import (
 )
 
 func (s *Server) definition(ctx context.Context, params *protocol.DefinitionParams) ([]protocol.Location, error) {
+	// TODO(rfindley): definition requests should be multiplexed across all views.
 	snapshot, fh, ok, release, err := s.beginFileRequest(ctx, params.TextDocument.URI, source.UnknownKind)
 	defer release()
 	if !ok {
 		return nil, err
 	}
-	if snapshot.View().FileKind(fh) == source.Tmpl {
+	switch kind := snapshot.View().FileKind(fh); kind {
+	case source.Tmpl:
 		return template.Definition(snapshot, fh, params.Position)
+	case source.Go:
+		return source.Definition(ctx, snapshot, fh, params.Position)
+	default:
+		return nil, fmt.Errorf("can't find definitions for file type %s", kind)
 	}
-	ident, err := source.Identifier(ctx, snapshot, fh, params.Position)
-	if err != nil {
-		return nil, err
-	}
-	if ident.IsImport() && !snapshot.View().Options().ImportShortcut.ShowDefinition() {
-		return nil, nil
-	}
-	var locations []protocol.Location
-	for _, ref := range ident.Declaration.MappedRange {
-		locations = append(locations, ref.Location())
-	}
-
-	return locations, nil
 }
 
 func (s *Server) typeDefinition(ctx context.Context, params *protocol.TypeDefinitionParams) ([]protocol.Location, error) {
+	// TODO(rfindley): type definition requests should be multiplexed across all views.
 	snapshot, fh, ok, release, err := s.beginFileRequest(ctx, params.TextDocument.URI, source.Go)
 	defer release()
 	if !ok {
 		return nil, err
 	}
-	ident, err := source.Identifier(ctx, snapshot, fh, params.Position)
-	if err != nil {
-		return nil, err
-	}
-	if ident.Type.Object == nil {
-		return nil, fmt.Errorf("no type definition for %s", ident.Name)
+	switch kind := snapshot.View().FileKind(fh); kind {
+	case source.Go:
+		return source.TypeDefinition(ctx, snapshot, fh, params.Position)
+	default:
+		return nil, fmt.Errorf("can't find type definitions for file type %s", kind)
 	}
-	return []protocol.Location{ident.Type.MappedRange.Location()}, nil
 }

gopls/internal/lsp/lsp_test.go

@@ -666,7 +666,9 @@ func (r *runner) MethodExtraction(t *testing.T, start span.Span, end span.Span)
 	}
 }
 
-func (r *runner) Definition(t *testing.T, spn span.Span, d tests.Definition) {
+// TODO(rfindley): This handler needs more work. The output is still a bit hard
+// to read (range diffs do not format nicely), and it is too entangled with hover.
+func (r *runner) Definition(t *testing.T, _ span.Span, d tests.Definition) {
 	sm, err := r.data.Mapper(d.Src.URI())
 	if err != nil {
 		t.Fatal(err)
@@ -676,18 +678,18 @@ func (r *runner) Definition(t *testing.T, spn span.Span, d tests.Definition) {
 		t.Fatalf("failed for %v: %v", d.Src, err)
 	}
 	tdpp := protocol.LocationTextDocumentPositionParams(loc)
-	var locs []protocol.Location
+	var got []protocol.Location
 	var hover *protocol.Hover
 	if d.IsType {
 		params := &protocol.TypeDefinitionParams{
 			TextDocumentPositionParams: tdpp,
 		}
-		locs, err = r.server.TypeDefinition(r.ctx, params)
+		got, err = r.server.TypeDefinition(r.ctx, params)
 	} else {
 		params := &protocol.DefinitionParams{
 			TextDocumentPositionParams: tdpp,
 		}
-		locs, err = r.server.Definition(r.ctx, params)
+		got, err = r.server.Definition(r.ctx, params)
 		if err != nil {
 			t.Fatalf("failed for %v: %+v", d.Src, err)
 		}
@@ -699,8 +701,19 @@ func (r *runner) Definition(t *testing.T, spn span.Span, d tests.Definition) {
 	if err != nil {
 		t.Fatalf("failed for %v: %v", d.Src, err)
 	}
-	if len(locs) != 1 {
-		t.Errorf("got %d locations for definition, expected 1", len(locs))
+	dm, err := r.data.Mapper(d.Def.URI())
+	if err != nil {
+		t.Fatal(err)
+	}
+	def, err := dm.SpanLocation(d.Def)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !d.OnlyHover {
+		want := []protocol.Location{def}
+		if diff := cmp.Diff(want, got); diff != "" {
+			t.Fatalf("Definition(%s) mismatch (-want +got):\n%s", d.Src, diff)
+		}
 	}
 	didSomething := false
 	if hover != nil {
@@ -717,13 +730,13 @@ func (r *runner) Definition(t *testing.T, spn span.Span, d tests.Definition) {
 	}
 	if !d.OnlyHover {
 		didSomething = true
-		locURI := locs[0].URI.SpanURI()
+		locURI := got[0].URI.SpanURI()
 		lm, err := r.data.Mapper(locURI)
 		if err != nil {
 			t.Fatal(err)
 		}
-		if def, err := lm.LocationSpan(locs[0]); err != nil {
-			t.Fatalf("failed for %v: %v", locs[0], err)
+		if def, err := lm.LocationSpan(got[0]); err != nil {
+			t.Fatalf("failed for %v: %v", got[0], err)
 		} else if def != d.Def {
 			t.Errorf("for %v got %v want %v", d.Src, def, d.Def)
 		}

gopls/internal/lsp/source/call_hierarchy.go

@@ -15,44 +15,48 @@ import (
 
 	"golang.org/x/tools/go/ast/astutil"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
+	"golang.org/x/tools/gopls/internal/lsp/safetoken"
+	"golang.org/x/tools/gopls/internal/span"
+	"golang.org/x/tools/internal/bug"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/event/tag"
 )
 
 // PrepareCallHierarchy returns an array of CallHierarchyItem for a file and the position within the file.
-func PrepareCallHierarchy(ctx context.Context, snapshot Snapshot, fh FileHandle, pos protocol.Position) ([]protocol.CallHierarchyItem, error) {
+func PrepareCallHierarchy(ctx context.Context, snapshot Snapshot, fh FileHandle, pp protocol.Position) ([]protocol.CallHierarchyItem, error) {
 	ctx, done := event.Start(ctx, "source.PrepareCallHierarchy")
 	defer done()
 
-	identifier, err := Identifier(ctx, snapshot, fh, pos)
+	pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckFull, NarrowestPackage)
+	if err != nil {
+		return nil, err
+	}
+	pos, err := pgf.PositionPos(pp)
 	if err != nil {
-		if errors.Is(err, ErrNoIdentFound) || errors.Is(err, errNoObjectFound) {
-			return nil, nil
-		}
 		return nil, err
 	}
 
-	// The identifier can be nil if it is an import spec.
-	if identifier == nil || identifier.Declaration.obj == nil {
+	obj := referencedObject(pkg, pgf, pos)
+	if obj == nil {
 		return nil, nil
 	}
 
-	if _, ok := identifier.Declaration.obj.Type().Underlying().(*types.Signature); !ok {
+	if _, ok := obj.Type().Underlying().(*types.Signature); !ok {
 		return nil, nil
 	}
 
-	if len(identifier.Declaration.MappedRange) == 0 {
-		return nil, nil
+	declLoc, err := mapPosition(ctx, pkg.FileSet(), snapshot, obj.Pos(), adjustedObjEnd(obj))
+	if err != nil {
+		return nil, err
 	}
-	declMappedRange := identifier.Declaration.MappedRange[0]
-	rng := declMappedRange.Range()
+	rng := declLoc.Range
 
 	callHierarchyItem := protocol.CallHierarchyItem{
-		Name:           identifier.Name,
+		Name:           obj.Name(),
 		Kind:           protocol.Function,
 		Tags:           []protocol.SymbolTag{},
-		Detail:         fmt.Sprintf("%s • %s", identifier.Declaration.obj.Pkg().Path(), filepath.Base(declMappedRange.URI().Filename())),
-		URI:            protocol.DocumentURI(declMappedRange.URI()),
+		Detail:         fmt.Sprintf("%s • %s", obj.Pkg().Path(), filepath.Base(declLoc.URI.SpanURI().Filename())),
+		URI:            declLoc.URI,
 		Range:          rng,
 		SelectionRange: rng,
 	}
@@ -174,41 +178,71 @@ outer:
 }
 
 // OutgoingCalls returns an array of CallHierarchyOutgoingCall for a file and the position within the file.
-func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pos protocol.Position) ([]protocol.CallHierarchyOutgoingCall, error) {
+func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pp protocol.Position) ([]protocol.CallHierarchyOutgoingCall, error) {
 	ctx, done := event.Start(ctx, "source.OutgoingCalls")
 	defer done()
 
-	identifier, err := Identifier(ctx, snapshot, fh, pos)
+	pkg, pgf, err := PackageForFile(ctx, snapshot, fh.URI(), TypecheckFull, NarrowestPackage)
+	if err != nil {
+		return nil, err
+	}
+	pos, err := pgf.PositionPos(pp)
 	if err != nil {
-		if errors.Is(err, ErrNoIdentFound) || errors.Is(err, errNoObjectFound) {
-			return nil, nil
-		}
 		return nil, err
 	}
 
-	if _, ok := identifier.Declaration.obj.Type().Underlying().(*types.Signature); !ok {
+	obj := referencedObject(pkg, pgf, pos)
+	if obj == nil {
 		return nil, nil
 	}
-	node := identifier.Declaration.node
-	if node == nil {
+
+	if _, ok := obj.Type().Underlying().(*types.Signature); !ok {
 		return nil, nil
 	}
-	callExprs, err := collectCallExpressions(identifier.Declaration.nodeFile, node)
+
+	// Skip builtins.
+	if obj.Pkg() == nil {
+		return nil, nil
+	}
+
+	if !obj.Pos().IsValid() {
+		return nil, bug.Errorf("internal error: object %s.%s missing position", obj.Pkg().Path(), obj.Name())
+	}
+
+	declFile := pkg.FileSet().File(obj.Pos())
+	if declFile == nil {
+		return nil, bug.Errorf("file not found for %d", obj.Pos())
+	}
+
+	uri := span.URIFromPath(declFile.Name())
+	offset, err := safetoken.Offset(declFile, obj.Pos())
 	if err != nil {
 		return nil, err
 	}
 
-	return toProtocolOutgoingCalls(ctx, snapshot, fh, callExprs)
-}
+	// Use TypecheckFull as we want to inspect the body of the function declaration.
+	declPkg, declPGF, err := PackageForFile(ctx, snapshot, uri, TypecheckFull, NarrowestPackage)
+	if err != nil {
+		return nil, err
+	}
 
-// collectCallExpressions collects call expression ranges inside a function.
-func collectCallExpressions(pgf *ParsedGoFile, node ast.Node) ([]protocol.Range, error) {
-	type callPos struct {
-		start, end token.Pos
+	declPos, err := safetoken.Pos(declPGF.Tok, offset)
+	if err != nil {
+		return nil, err
 	}
-	callPositions := []callPos{}
 
-	ast.Inspect(node, func(n ast.Node) bool {
+	declNode, _ := FindDeclAndField([]*ast.File{declPGF.File}, declPos)
+	if declNode == nil {
+		// TODO(rfindley): why don't we return an error here, or even bug.Errorf?
+		return nil, nil
+		// return nil, bug.Errorf("failed to find declaration for object %s.%s", obj.Pkg().Path(), obj.Name())
+	}
+
+	type callRange struct {
+		start, end token.Pos
+	}
+	callRanges := []callRange{}
+	ast.Inspect(declNode, func(n ast.Node) bool {
 		if call, ok := n.(*ast.CallExpr); ok {
 			var start, end token.Pos
 			switch n := call.Fun.(type) {
@@ -225,70 +259,48 @@ func collectCallExpressions(pgf *ParsedGoFile, node ast.Node) ([]protocol.Range,
 				// for ex: direct function literal calls since that's not an 'outgoing' call
 				return false
 			}
-			callPositions = append(callPositions, callPos{start: start, end: end})
+			callRanges = append(callRanges, callRange{start: start, end: end})
 		}
 		return true
 	})
 
-	callRanges := []protocol.Range{}
-	for _, call := range callPositions {
-		callRange, err := pgf.PosRange(call.start, call.end)
-		if err != nil {
-			return nil, err
-		}
-		callRanges = append(callRanges, callRange)
-	}
-	return callRanges, nil
-}
-
-// toProtocolOutgoingCalls returns an array of protocol.CallHierarchyOutgoingCall for ast call expressions.
-// Calls to the same function are assigned to the same declaration.
-func toProtocolOutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, callRanges []protocol.Range) ([]protocol.CallHierarchyOutgoingCall, error) {
-	// Multiple calls could be made to the same function, defined by "same declaration
-	// AST node & same identifier name" to provide a unique identifier key even when
-	// the func is declared in a struct or interface.
-	type key struct {
-		decl ast.Node
-		name string
-	}
-	outgoingCalls := map[key]*protocol.CallHierarchyOutgoingCall{}
+	outgoingCalls := map[token.Pos]*protocol.CallHierarchyOutgoingCall{}
 	for _, callRange := range callRanges {
-		identifier, err := Identifier(ctx, snapshot, fh, callRange.Start)
-		if err != nil {
-			if errors.Is(err, ErrNoIdentFound) || errors.Is(err, errNoObjectFound) {
-				continue
-			}
-			return nil, err
+		obj := referencedObject(declPkg, declPGF, callRange.start)
+		if obj == nil {
+			continue
 		}
 
 		// ignore calls to builtin functions
-		if identifier.Declaration.obj.Pkg() == nil {
+		if obj.Pkg() == nil {
 			continue
 		}
 
-		if outgoingCall, ok := outgoingCalls[key{identifier.Declaration.node, identifier.Name}]; ok {
-			outgoingCall.FromRanges = append(outgoingCall.FromRanges, callRange)
-			continue
+		outgoingCall, ok := outgoingCalls[obj.Pos()]
+		if !ok {
+			loc, err := mapPosition(ctx, declPkg.FileSet(), snapshot, obj.Pos(), obj.Pos()+token.Pos(len(obj.Name())))
+			if err != nil {
+				return nil, err
+			}
+			outgoingCall = &protocol.CallHierarchyOutgoingCall{
+				To: protocol.CallHierarchyItem{
+					Name:           obj.Name(),
+					Kind:           protocol.Function,
+					Tags:           []protocol.SymbolTag{},
+					Detail:         fmt.Sprintf("%s • %s", obj.Pkg().Path(), filepath.Base(loc.URI.SpanURI().Filename())),
+					URI:            loc.URI,
+					Range:          loc.Range,
+					SelectionRange: loc.Range,
+				},
+			}
+			outgoingCalls[obj.Pos()] = outgoingCall
 		}
 
-		if len(identifier.Declaration.MappedRange) == 0 {
-			continue
-		}
-		declMappedRange := identifier.Declaration.MappedRange[0]
-		rng := declMappedRange.Range()
-
-		outgoingCalls[key{identifier.Declaration.node, identifier.Name}] = &protocol.CallHierarchyOutgoingCall{
-			To: protocol.CallHierarchyItem{
-				Name:           identifier.Name,
-				Kind:           protocol.Function,
-				Tags:           []protocol.SymbolTag{},
-				Detail:         fmt.Sprintf("%s • %s", identifier.Declaration.obj.Pkg().Path(), filepath.Base(declMappedRange.URI().Filename())),
-				URI:            protocol.DocumentURI(declMappedRange.URI()),
-				Range:          rng,
-				SelectionRange: rng,
-			},
-			FromRanges: []protocol.Range{callRange},
+		rng, err := declPGF.PosRange(callRange.start, callRange.end)
+		if err != nil {
+			return nil, err
 		}
+		outgoingCall.FromRanges = append(outgoingCall.FromRanges, rng)
 	}
 
 	outgoingCallItems := make([]protocol.CallHierarchyOutgoingCall, 0, len(outgoingCalls))