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.

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

x/tools/gopls: lsp features not working in std lib package #40809

Closed
muirdm opened this issue Aug 15, 2020 · 6 comments
Closed

x/tools/gopls: lsp features not working in std lib package #40809

muirdm opened this issue Aug 15, 2020 · 6 comments
Labels
Milestone

Comments

@muirdm
Copy link

@muirdm muirdm commented Aug 15, 2020

On master (9882f1d1823da58283ff91ebbec3f7c0cc27cddc), after go-to-definitioning into a standard library package, code navigation features no longer work inside the standard library package. Attached is a log where I jumped to "fmt.Println" and then tried to jump to the definition of "newPrinter".
vscode.log

@gopherbot gopherbot added this to the Unreleased milestone Aug 15, 2020
@muirdm
Copy link
Author

@muirdm muirdm commented Aug 15, 2020

@heschik I think this was an unintended consequence of https://go-review.googlesource.com/c/tools/+/248380

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v.0.4.5 Aug 15, 2020
@heschik
Copy link
Contributor

@heschik heschik commented Aug 17, 2020

I agree I broke this, but I don't know how to fix it.

There are good reasons we don't want to type check every package in full mode: it uses a ton of memory and CPU for code that the user is probably not working on. So our strategy of checking workspace packages in full mode, and others in export-only mode, is useful. But it puts us in a difficult position when dealing with non-workspace packages.

Let's consider Find References on fmt.Printf, as mentioned in the description of 248380. If we use the export-only version of the fmt package, we will successfully find references from the user's code to Println. We won't find references outside the workspace, and we won't even realize an unexported symbol exists. If we use the full version, we can deal with unexported identifiers, and we can find references internal to the package itself. Nothing else will be connected to it.

Fundamentally, we cannot fully support non-workspace packages without fully type checking them, and we can't afford to do that. So the question is what tradeoffs we want to make. Some options:

  • Very specifically for Go To Definition, add a fallback to or force full type checking. Since we're only interested in forward references, that will work fine.
  • Continue with the current state, where ~no features work outside of the workspace.
  • Attempt to work in both modes for all requests, merging the results. In the Printf example, this would allow you to find references in the fmt package and the user's workspace, but not the stdlib generally. I think that might be too confusing.

I think maybe I'm leaning toward a special case for Go To Definition? Possibly it can choose the type checking mode based on the exported-ness of the identifier.

@muirdm
Copy link
Author

@muirdm muirdm commented Aug 17, 2020

In order of importance to me:

  1. If I open a non-workspace file, gopls should type check a package it belongs to so go-to-definition works.
  2. Find-references should work within that non-workspace package since that is useful when poking around.
  3. Find-references from within the non-workspace package should find references in my workspace.

I think 1) correspond to your first option, and 3) corresponds to your third option.

We sort of already handle same-package-checked-multiple-times for find-references and find-implementations due to test variant packages. We seed the find-references search with all packages the starting file belongs to (e.g. "foo" and "foo [foo.test]" or w/e), and we dedupe results by token.Position. It should work to also throw full and partial versions into the mix.

In conclusion I would like at least 1) and to consider doing 3). I don't find the lack of references across non-workspace packages confusing because I already don't expect that to work. Others may have different expectations.

@heschik
Copy link
Contributor

@heschik heschik commented Aug 18, 2020

Test variants aren't totally comparable. The big problem is incoming references, where the type objects won't line up, and test variants don't have any of them.

I'll play around with it.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 18, 2020

Change https://golang.org/cl/249120 mentions this issue: internal/lsp/cache: don't always type check in default mode

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 20, 2020

Change https://golang.org/cl/249637 mentions this issue: internal/lsp/source: fix unexported references of non-workspace packages

gopherbot pushed a commit to golang/tools that referenced this issue Aug 20, 2020
qualifiedObjsAtProtocolPos returned too early. Have it keep looking in
the rest of the candidate packages.

This changes the returned error slightly but AFAICT nobody cares.

Updates golang/go#40809.

Change-Id: Ic8199a484f0abcaa48cb6a3bcdd782195802d670
Reviewed-on: https://go-review.googlesource.com/c/tools/+/249637
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.