gopls/internal/golang: Hover: show field info for embedded fields too
This CL removes the logic that caused Hover over an embedded field
to report on the type, not the field itself, which was unhelpful
when (e.g.) inspecting field size/align information.
The logic was originally common to Hover and Definition (where it
is still wanted), but was moved up into Hover itself in the
preceding CL of the stack of cleanups attached to this issue.
+ test
Fixes golang/go#75975
Change-Id: Ice74a430638f48349e300cf442685137a45a09ef
Reviewed-on: https://go-review.googlesource.com/c/tools/+/714701
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/golang/hover.go b/gopls/internal/golang/hover.go
index 6368f72..5a63244 100644
--- a/gopls/internal/golang/hover.go
+++ b/gopls/internal/golang/hover.go
@@ -348,17 +348,6 @@
hoverRange = &rng
}
- // If the original position was an embedded field, we want to show
- // the field's type definition, not the field's definition.
- // (This was the fix to #42254 in Definition, but it is the cause of #75975 in Hover.
- // TODO(adonovan): a follow-up CL will delete this logic from Hover.)
- if v, ok := obj.(*types.Var); ok && v.Embedded() {
- // types.Info.Uses contains the embedded field's *types.TypeName.
- if typeName := pkg.TypesInfo().Uses[ident]; typeName != nil {
- obj = typeName
- }
- }
-
// Handle type switch identifiers as a special case,
// since they don't have a regular object.
// There's not much useful information to provide.
@@ -511,12 +500,8 @@
//
var sizeOffset string
- // As painfully learned in golang/go#69362, Defs can contain nil entries.
- if def, _ := pkg.TypesInfo().Defs[ident]; def != nil && ident.Pos() == def.Pos() {
+ if ident.Pos() == obj.Pos() {
// This is the declaring identifier.
- // (We can't simply use ident.Pos() == obj.Pos() because
- // objectAt prefers the TypeName for an embedded field).
- // TODO(adonovan): fix as part of #75975.
// format returns the decimal and hex representation of x.
format := func(x int64) string {
diff --git a/gopls/internal/test/marker/doc.go b/gopls/internal/test/marker/doc.go
index 3df8f93..c4aaefc 100644
--- a/gopls/internal/test/marker/doc.go
+++ b/gopls/internal/test/marker/doc.go
@@ -207,9 +207,11 @@
textDocument/highlight request at the given src location, which should
highlight the provided dst locations and kinds.
- - hover(src, dst location, sm stringMatcher): performs a textDocument/hover at
- the src location, and checks that the result is the dst location, with
- matching hover content.
+ - hover(src, dst location, sm stringMatcher): performs a textDocument/hover
+ at the src location, and checks that the result is the dst location, with
+ matching hover content. Be careful to avoid self-satisfying hover markers:
+ if the hover content literally includes the marker comment itself,
+ it will always contain the expected string! A backslash escape may help.
- hovererr(src, sm stringMatcher): performs a textDocument/hover at the src
location, and checks that the error matches the given stringMatcher.
diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go
index 84a470f..fe8277b 100644
--- a/gopls/internal/test/marker/marker_test.go
+++ b/gopls/internal/test/marker/marker_test.go
@@ -377,14 +377,6 @@
return mapper
}
-// error reports an error with a prefix indicating the position of the marker
-// note.
-func (mark marker) error(args ...any) {
- mark.T().Helper()
- msg := fmt.Sprint(args...)
- mark.T().Errorf("%s: %s", mark.run.fmtPos(mark.note.Pos), msg)
-}
-
// errorf reports a formatted error with a prefix indicating the position of
// the marker note.
//
@@ -442,7 +434,7 @@
args := append([]any{mark}, mark.note.Args[1:]...)
argValues, err := convertArgs(mark, ftype, args)
if err != nil {
- mark.error(err)
+ mark.errorf("%v", err)
return
}
results := reflect.ValueOf(fn).Call(argValues)
@@ -485,7 +477,7 @@
args := append([]any{mark}, mark.note.Args...)
argValues, err := convertArgs(mark, ftype, args)
if err != nil {
- mark.error(err)
+ mark.errorf("%v", err)
return
}
reflect.ValueOf(fn).Call(argValues)
diff --git a/gopls/internal/test/marker/testdata/definition/embed.txt b/gopls/internal/test/marker/testdata/definition/embed.txt
index da55dbc..ab3d4fa 100644
--- a/gopls/internal/test/marker/testdata/definition/embed.txt
+++ b/gopls/internal/test/marker/testdata/definition/embed.txt
@@ -247,22 +247,17 @@
[`(b.S1).S2` on pkg.go.dev](https://pkg.go.dev/mod.com/b#S1.S2)
-- @S2 --
```go
-type S2 struct { // size=32 (0x20)
- F1 string //@loc(S2F1, "F1")
- F2 int //@loc(S2F2, "F2")
- *a.A //@def("A", AString),def("a",AImport)
-}
+field S2 S2 // size=32 (0x20), offset=8
```
---
-```go
-func (a.A) Hi()
-```
+@loc(S1S2, "S2"),def("S2", S2),hover("S2", "S2", S2)
+
---
-[`b.S2` on pkg.go.dev](https://pkg.go.dev/mod.com/b#S2)
+[`(b.S1).S2` on pkg.go.dev](https://pkg.go.dev/mod.com/b#S1.S2)
-- @S2F1 --
```go
field F1 string
@@ -304,33 +299,13 @@
[`(a.S).Field` on pkg.go.dev](https://pkg.go.dev/mod.com/a#S.Field)
-- @aA --
```go
-type A string // size=16 (0x10)
+field A a.A // size=16 (0x10), offset=40 (0x28)
```
-
----
-
-@loc(AString, "A")
-
-
-```go
-func (a.A) Hi()
-```
-
----
-
-[`a.A` on pkg.go.dev](https://pkg.go.dev/mod.com/a#A)
-- @aAlias --
```go
-type aAlias = a.A // size=16 (0x10)
-
-type A string
+field aAlias aAlias // size=16 (0x10), offset=56 (0x38)
```
---
-@loc(aAlias, "aAlias")
-
-
-```go
-func (a.A) Hi()
-```
+@def("a", aAlias),hover("a", "aAlias", aAlias)
diff --git a/gopls/internal/test/marker/testdata/hover/issue75975.txt b/gopls/internal/test/marker/testdata/hover/issue75975.txt
new file mode 100644
index 0000000..8d4ef74
--- /dev/null
+++ b/gopls/internal/test/marker/testdata/hover/issue75975.txt
@@ -0,0 +1,34 @@
+This test checks that hovering over an embedded field
+reports information about the field, not its type.
+
+We put backslash escapes in the hover expectation as the @hover marker
+appears on the same line as the declaration. Otherwise the hover doc
+content, which includes the comment containing the marker itself,
+would trivially satisfy itself no matter what text we expect!
+
+-- flags --
+-skip_goarch=386,arm
+
+-- go.mod --
+module mod.com
+go 1.18
+
+-- a/a.go --
+package a
+
+type A struct {
+ byte
+ S string //@ hover("S", "S", "field S string // size=16 (0x10), offset=8\n")
+}
+
+type B struct {
+ byte
+ S //@ hover("S", "S", "field S S // size=1, class=8, offset=1\n")
+}
+
+type C struct {
+ byte
+ *S // hover("S", "S", "field S S // size=1, class=8, offset=1\n")
+}
+
+type S byte
