Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/tools/gopls: variable in third party package is not highlighted #43511

Closed
normal-cock opened this issue Jan 5, 2021 · 5 comments
Closed

x/tools/gopls: variable in third party package is not highlighted #43511

normal-cock opened this issue Jan 5, 2021 · 5 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.6.10

Comments

@normal-cock
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.14 darwin/amd64

$ gopls version
golang.org/x/tools/gopls v0.6.0
    golang.org/x/tools/gopls@v0.6.1 h1:vD171EDBkyUVyp9B45IYXklV/VEfbHgX0FKw0Q0dO30=

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"

What did you do?

nothing

What did you expect to see?

Code in my package:
image

What did you see instead?

Code in third party package:
image

Gopls(server) logs when I move cursor in format variable:

[Trace - 16:51:31.825 PM] Received notification 'window/logMessage'.
Params: {"type":3,"message":"2021/01/05 16:51:31 background refresh finished after 26.412396992s\n"}


[Info  - 4:51:31 PM] 2021/01/05 16:51:31 background refresh finished after 26.412396992s

[Trace - 16:51:35.547 PM] Sending request 'textDocument/hover - (26)'.
Params: {"textDocument":{"uri":"file:///~/.gvm/gos/go1.14/src/fmt/print.go"},"position":{"line":216,"character":15}}


[Trace - 16:51:35.548 PM] Received response 'textDocument/hover - (26)' in 0ms.
Result: {"contents":{"kind":"markdown","value":"```go\nvar format string\n```"},"range":{"start":{"line":216,"character":13},"end":{"line":216,"character":19}}}


[Trace - 16:51:35.749 PM] Sending request 'textDocument/hover - (27)'.
Params: {"textDocument":{"uri":"file:///~/.gvm/gos/go1.14/src/fmt/print.go"},"position":{"line":216,"character":14}}


[Trace - 16:51:35.749 PM] Received response 'textDocument/hover - (27)' in 0ms.
Result: {"contents":{"kind":"markdown","value":"```go\nvar format string\n```"},"range":{"start":{"line":216,"character":13},"end":{"line":216,"character":19}}}


[Trace - 16:51:35.887 PM] Sending request 'textDocument/documentHighlight - (28)'.
Params: {"textDocument":{"uri":"file:///~/.gvm/gos/go1.14/src/fmt/print.go"},"position":{"line":216,"character":15}}


[Trace - 16:51:35.888 PM] Received response 'textDocument/documentHighlight - (28)' in 0ms.
Result: [{"range":{"start":{"line":216,"character":13},"end":{"line":216,"character":19}},"kind":1}]


[Trace - 16:51:36.122 PM] Sending request 'textDocument/codeAction - (29)'.
Params: {"textDocument":{"uri":"file:///~/.gvm/gos/go1.14/src/fmt/print.go"},"range":{"start":{"line":216,"character":15},"end":{"line":216,"character":15}},"context":{"diagnostics":[]}}


[Trace - 16:51:36.126 PM] Received response 'textDocument/codeAction - (29)' in 4ms.
Result: null
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jan 5, 2021
@gopherbot gopherbot added this to the Unreleased milestone Jan 5, 2021
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Jan 5, 2021
@normal-cock
Copy link
Author

Will this issue be fixed in gopls/v1.0.0? @stamblerre

@stamblerre
Copy link
Contributor

We are not yet sure when v1.0.0 will be released, but we will address it before that version. When the issue is resolved, it will be closed and moved to the milestone of the release for which it was actually fixed.

@stamblerre stamblerre mentioned this issue Jan 22, 2021
Closed
@ShoshinNikita
Copy link

I could find the cause of this behavior. Highlight() function calls GetParsedFile() that requests a package with TypecheckWorkspace mode:

// TypecheckFull means to use ParseFull.
TypecheckFull

// TypecheckWorkspace means to use ParseFull for workspace packages, and
// ParseExported for others.
TypecheckWorkspace

...

// ParseExported specifies that the public symbols are needed, but things like
// private symbols and function bodies are not.
// This mode is used for things where a package is being consumed only as a
// dependency.
ParseExported

// ParseFull specifies the full AST is needed.
// This is used for files of direct interest where the entire contents must
// be considered.
ParseFull

So, *ast.File for std and third-party packages doesn't have function bodies. I can see several options here:

  • Use TypecheckFull mode in GetParsedFile(). As far as I can tell, only Highlight() function requires the full AST. So, it is not a good idea
  • Pass a typecheck mode to GetParsedFile. It will give more control, but the function will become less convenient
  • Use a special version of GetParsedFile() with TypecheckFull mode for Highlight()

Related issue: #40809 (cc @heschi)

Side note: ParseExported has a misleading name and a comment because *ast.File still has unexported symbols. trimAST, which is called in this mode, doesn't remove private symbols

@stamblerre
Copy link
Contributor

Thanks for investigating this! I think it would be fine to reuse the content of GetParsedFile directly in the highlight code--it's not much logic, so I don't think it deserves a second helper. What do you think?

ShoshinNikita added a commit to ShoshinNikita/tools that referenced this issue Apr 4, 2021
@ShoshinNikita
internal/lsp/source: fix Highlight for std and 3rd-party packages
d6601a3 
Don't use GetParsedFile because it extracts a package with
TypecheckWorkspace mode, but we want to support std and
3rd-party packages. So, we reuse the content of GetParsedFile
directly in Highlight function with TypecheckFull mode.

Fixes golang/go#43511
@ShoshinNikita ShoshinNikita mentioned this issue Apr 4, 2021
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/307171 mentions this issue: internal/lsp/source: fix Highlight for std and 3rd-party packages

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.6.10 Apr 5, 2021
@golang golang locked and limited conversation to collaborators Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.6.10
Development

Successfully merging a pull request may close this issue.

internal/lsp/source: fix Highlight for std and 3rd-party packages ShoshinNikita/tools
4 participants
@normal-cock @stamblerre @gopherbot @ShoshinNikita