Skip to content

Commit

Permalink
gopls/internal/lsp/source: rewrite hover
Browse files Browse the repository at this point in the history
Significantly rewrite the logic involved in textDocument/hover requests,
eliminate dependence on type checking, and remove the source.Identifier
abstraction.

The existing logichad become very hard to follow, and while I spent
many hours trying to make sure I understood it, some logic seemed
inaccurate and other remained opaque. Notably, it appears much of the
old code was effectively dead, due to impossible combinations of
Node/Object in various execution paths.

Rewrite the essential bits of logic from scratch, preserving only that
which was necessary for hover (the last user of source.Identifier).
After doing this, the intermediate HoverContext and IdentifierInfo data
types became unnecessary.

Along the way, a pair of decisions impacted gopls observable behavior:
- Hovering over a rune was made consistent with other hovering (by way
  of HoverJSON), which had the side effect of introducing newlines to
  hover content.
- Support for hovering over variables with constant initializers was
  removed (#58220). This feature will be revisited in a
  follow-up CL.

This eliminates both posToMappedRange and findFileInDeps helpers, which
were two of the remaining places where we could incidentally type-check.
After this change, the only uses of TypecheckWorkspace are in renaming.

For #57987

Change-Id: I3e0d673b914f6277c3713f74450134ceeb181319
Reviewed-on: https://go-review.googlesource.com/c/tools/+/464415
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
  • Loading branch information
findleyr committed Feb 10, 2023
1 parent 650ae30 commit b15a5bc
Show file tree
Hide file tree
Showing 13 changed files with 564 additions and 1,139 deletions.
2 changes: 1 addition & 1 deletion gopls/internal/lsp/lsp_test.go
Expand Up @@ -976,7 +976,7 @@ func (r *runner) Rename(t *testing.T, spn span.Span, newText string) {
return []byte(err.Error()), nil
}))
if err.Error() != renamed {
t.Errorf("rename failed for %s, expected:\n%v\ngot:\n%v\n", newText, renamed, err)
t.Errorf("%s: rename failed for %s, expected:\n%v\ngot:\n%v\n", spn, newText, renamed, err)
}
return
}
Expand Down
8 changes: 4 additions & 4 deletions gopls/internal/lsp/source/call_hierarchy.go
Expand Up @@ -36,7 +36,7 @@ func PrepareCallHierarchy(ctx context.Context, snapshot Snapshot, fh FileHandle,
return nil, err
}

obj := referencedObject(pkg, pgf, pos)
_, obj, _ := referencedObject(pkg, pgf, pos)
if obj == nil {
return nil, nil
}
Expand Down Expand Up @@ -191,7 +191,7 @@ func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pp pro
return nil, err
}

obj := referencedObject(pkg, pgf, pos)
_, obj, _ := referencedObject(pkg, pgf, pos)
if obj == nil {
return nil, nil
}
Expand Down Expand Up @@ -231,7 +231,7 @@ func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pp pro
return nil, err
}

declNode, _, _ := FindDeclInfo([]*ast.File{declPGF.File}, declPos)
declNode, _, _ := findDeclInfo([]*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
Expand Down Expand Up @@ -266,7 +266,7 @@ func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pp pro

outgoingCalls := map[token.Pos]*protocol.CallHierarchyOutgoingCall{}
for _, callRange := range callRanges {
obj := referencedObject(declPkg, declPGF, callRange.start)
_, obj, _ := referencedObject(declPkg, declPGF, callRange.start)
if obj == nil {
continue
}
Expand Down
51 changes: 30 additions & 21 deletions gopls/internal/lsp/source/definition.go
Expand Up @@ -59,7 +59,7 @@ func Definition(ctx context.Context, snapshot Snapshot, fh FileHandle, position
}

// The general case: the cursor is on an identifier.
obj := referencedObject(pkg, pgf, pos)
_, obj, _ := referencedObject(pkg, pgf, pos)
if obj == nil {
return nil, nil
}
Expand Down Expand Up @@ -102,38 +102,46 @@ func Definition(ctx context.Context, snapshot Snapshot, fh FileHandle, position
return locs, nil
}

// referencedObject returns the object referenced at the specified position,
// which must be within the file pgf, for the purposes of definition/hover/call
// hierarchy operations. It may return nil if no object was found at the given
// position.
// referencedObject returns the identifier and object referenced at the
// specified position, which must be within the file pgf, for the purposes of
// definition/hover/call hierarchy operations. It returns a nil object if no
// object was found at the given position.
//
// It differs from types.Info.ObjectOf in several ways:
// - It adjusts positions to do a better job of finding associated
// identifiers. For example it finds 'foo' from the cursor position _*foo
// - It handles type switch implicits, choosing the first one.
// - For embedded fields, it returns the type name object rather than the var
// (field) object.
// If the returned identifier is a type-switch implicit (i.e. the x in x :=
// e.(type)), the third result will be the type of the expression being
// switched on (the type of e in the example). This facilitates workarounds for
// limitations of the go/types API, which does not report an object for the
// identifier x.
//
// For embedded fields, referencedObject returns the type name object rather
// than the var (field) object.
//
// TODO(rfindley): this function exists to preserve the pre-existing behavior
// of source.Identifier. Eliminate this helper in favor of sharing
// functionality with objectsAt, after choosing suitable primitives.
func referencedObject(pkg Package, pgf *ParsedGoFile, pos token.Pos) types.Object {
func referencedObject(pkg Package, pgf *ParsedGoFile, pos token.Pos) (*ast.Ident, types.Object, types.Type) {
path := pathEnclosingObjNode(pgf.File, pos)
if len(path) == 0 {
return nil
return nil, nil, nil
}
var obj types.Object
info := pkg.GetTypesInfo()
switch n := path[0].(type) {
case *ast.Ident:
// If leaf represents an implicit type switch object or the type
// switch "assign" variable, expand to all of the type switch's
// implicit objects.
if implicits, _ := typeSwitchImplicits(info, path); len(implicits) > 0 {
obj = implicits[0]
} else {
obj = info.ObjectOf(n)
obj = info.ObjectOf(n)
// If n is the var's declaring ident in a type switch
// [i.e. the x in x := foo.(type)], it will not have an object. In this
// case, set obj to the first implicit object (if any), and return the type
// of the expression being switched on.
//
// The type switch may have no case clauses and thus no
// implicit objects; this is a type error ("unused x"),
if obj == nil {
if implicits, typ := typeSwitchImplicits(info, path); len(implicits) > 0 {
return n, implicits[0], typ
}
}

// If the original position was an embedded field, we want to jump
// to the field's type definition, not the field's definition.
if v, ok := obj.(*types.Var); ok && v.Embedded() {
Expand All @@ -142,8 +150,9 @@ func referencedObject(pkg Package, pgf *ParsedGoFile, pos token.Pos) types.Objec
obj = typeName
}
}
return n, obj, nil
}
return obj
return nil, nil, nil
}

// importDefinition returns locations defining a package referenced by the
Expand Down

0 comments on commit b15a5bc

Please sign in to comment.