x/tools/gopls: reconsider var hover behavior with constant initializer #58220
Assignees
Labels
FrozenDueToAge
gopls
Issues related to the Go language server, gopls.
NeedsFix
The path to resolution is known, but the work has not been done.
Tools
This label describes issues relating to any tools in the x/tools repository.
Milestone
Comments
findleyr
added
the
NeedsFix
gopherbot
added
Tools
|
Ideally we would display the variable's value if (and only if) no other writes occur in the workspace. (But that's a much harder analysis.) |
|
I agree it's harder. For unexported vars it only requires looking at a single type-checked package but that's still a lot more work than hover needs to do in the common case, after Rob's recent work on it. |
|
Change https://go.dev/cl/464415 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Feb 10, 2023
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]>
|
Change https://go.dev/cl/482676 mentions this issue: |
|
Change https://go.dev/cl/496885 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
May 22, 2023
This test is updated to exercise hover over literals, not vars, as was decided in golang/go#58220. Updates golang/go#53878 Change-Id: Ic70d3492f28580ebfea24ec08dc47b1ad385c2ff Reviewed-on: https://go-review.googlesource.com/c/tools/+/496885 Run-TryBot: Robert Findley <[email protected]> Reviewed-by: Alan Donovan <[email protected]> gopls-CI: kokoro <[email protected]> Auto-Submit: Robert Findley <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
In #45802, we added hover content for variables whose initializer is an int literal (e.g.
var foo = 123).As I rewrite the hover logic, I think this is a misfeature: it is misleading to make it look like the variable has a known value.
Instead, we should support hovering over hex/octal/binary literals.
The text was updated successfully, but these errors were encountered: