Skip to content

Commit b15a5bc

Browse files
committed
gopls/internal/lsp/source: rewrite hover
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 (golang/go#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 golang/go#57987 Change-Id: I3e0d673b914f6277c3713f74450134ceeb181319 Reviewed-on: https://go-review.googlesource.com/c/tools/+/464415 Run-TryBot: Robert Findley <[email protected]> gopls-CI: kokoro <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
1 parent 650ae30 commit b15a5bc

File tree

13 files changed

+564
-1139
lines changed

13 files changed

+564
-1139
lines changed

gopls/internal/lsp/lsp_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -976,7 +976,7 @@ func (r *runner) Rename(t *testing.T, spn span.Span, newText string) {
976976
return []byte(err.Error()), nil
977977
}))
978978
if err.Error() != renamed {
979-
t.Errorf("rename failed for %s, expected:\n%v\ngot:\n%v\n", newText, renamed, err)
979+
t.Errorf("%s: rename failed for %s, expected:\n%v\ngot:\n%v\n", spn, newText, renamed, err)
980980
}
981981
return
982982
}

gopls/internal/lsp/source/call_hierarchy.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ func PrepareCallHierarchy(ctx context.Context, snapshot Snapshot, fh FileHandle,
3636
return nil, err
3737
}
3838

39-
obj := referencedObject(pkg, pgf, pos)
39+
_, obj, _ := referencedObject(pkg, pgf, pos)
4040
if obj == nil {
4141
return nil, nil
4242
}
@@ -191,7 +191,7 @@ func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pp pro
191191
return nil, err
192192
}
193193

194-
obj := referencedObject(pkg, pgf, pos)
194+
_, obj, _ := referencedObject(pkg, pgf, pos)
195195
if obj == nil {
196196
return nil, nil
197197
}
@@ -231,7 +231,7 @@ func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pp pro
231231
return nil, err
232232
}
233233

234-
declNode, _, _ := FindDeclInfo([]*ast.File{declPGF.File}, declPos)
234+
declNode, _, _ := findDeclInfo([]*ast.File{declPGF.File}, declPos)
235235
if declNode == nil {
236236
// TODO(rfindley): why don't we return an error here, or even bug.Errorf?
237237
return nil, nil
@@ -266,7 +266,7 @@ func OutgoingCalls(ctx context.Context, snapshot Snapshot, fh FileHandle, pp pro
266266

267267
outgoingCalls := map[token.Pos]*protocol.CallHierarchyOutgoingCall{}
268268
for _, callRange := range callRanges {
269-
obj := referencedObject(declPkg, declPGF, callRange.start)
269+
_, obj, _ := referencedObject(declPkg, declPGF, callRange.start)
270270
if obj == nil {
271271
continue
272272
}

gopls/internal/lsp/source/definition.go

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ func Definition(ctx context.Context, snapshot Snapshot, fh FileHandle, position
5959
}
6060

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

105-
// referencedObject returns the object referenced at the specified position,
106-
// which must be within the file pgf, for the purposes of definition/hover/call
107-
// hierarchy operations. It may return nil if no object was found at the given
108-
// position.
105+
// referencedObject returns the identifier and object referenced at the
106+
// specified position, which must be within the file pgf, for the purposes of
107+
// definition/hover/call hierarchy operations. It returns a nil object if no
108+
// object was found at the given position.
109109
//
110-
// It differs from types.Info.ObjectOf in several ways:
111-
// - It adjusts positions to do a better job of finding associated
112-
// identifiers. For example it finds 'foo' from the cursor position _*foo
113-
// - It handles type switch implicits, choosing the first one.
114-
// - For embedded fields, it returns the type name object rather than the var
115-
// (field) object.
110+
// If the returned identifier is a type-switch implicit (i.e. the x in x :=
111+
// e.(type)), the third result will be the type of the expression being
112+
// switched on (the type of e in the example). This facilitates workarounds for
113+
// limitations of the go/types API, which does not report an object for the
114+
// identifier x.
115+
//
116+
// For embedded fields, referencedObject returns the type name object rather
117+
// than the var (field) object.
116118
//
117119
// TODO(rfindley): this function exists to preserve the pre-existing behavior
118120
// of source.Identifier. Eliminate this helper in favor of sharing
119121
// functionality with objectsAt, after choosing suitable primitives.
120-
func referencedObject(pkg Package, pgf *ParsedGoFile, pos token.Pos) types.Object {
122+
func referencedObject(pkg Package, pgf *ParsedGoFile, pos token.Pos) (*ast.Ident, types.Object, types.Type) {
121123
path := pathEnclosingObjNode(pgf.File, pos)
122124
if len(path) == 0 {
123-
return nil
125+
return nil, nil, nil
124126
}
125127
var obj types.Object
126128
info := pkg.GetTypesInfo()
127129
switch n := path[0].(type) {
128130
case *ast.Ident:
129-
// If leaf represents an implicit type switch object or the type
130-
// switch "assign" variable, expand to all of the type switch's
131-
// implicit objects.
132-
if implicits, _ := typeSwitchImplicits(info, path); len(implicits) > 0 {
133-
obj = implicits[0]
134-
} else {
135-
obj = info.ObjectOf(n)
131+
obj = info.ObjectOf(n)
132+
// If n is the var's declaring ident in a type switch
133+
// [i.e. the x in x := foo.(type)], it will not have an object. In this
134+
// case, set obj to the first implicit object (if any), and return the type
135+
// of the expression being switched on.
136+
//
137+
// The type switch may have no case clauses and thus no
138+
// implicit objects; this is a type error ("unused x"),
139+
if obj == nil {
140+
if implicits, typ := typeSwitchImplicits(info, path); len(implicits) > 0 {
141+
return n, implicits[0], typ
142+
}
136143
}
144+
137145
// If the original position was an embedded field, we want to jump
138146
// to the field's type definition, not the field's definition.
139147
if v, ok := obj.(*types.Var); ok && v.Embedded() {
@@ -142,8 +150,9 @@ func referencedObject(pkg Package, pgf *ParsedGoFile, pos token.Pos) types.Objec
142150
obj = typeName
143151
}
144152
}
153+
return n, obj, nil
145154
}
146-
return obj
155+
return nil, nil, nil
147156
}
148157

149158
// importDefinition returns locations defining a package referenced by the

0 commit comments

Comments
 (0)