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

Jump to bottom

x/tools/gopls: filter intermediate test variants when resolving package for file #57795

Closed
findleyr opened this issue Jan 13, 2023 · 4 comments
Closed

x/tools/gopls: filter intermediate test variants when resolving package for file #57795

findleyr opened this issue Jan 13, 2023 · 4 comments
Assignees
@adonovan
Labels
FrozenDueToAge gopls/incremental gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.12.0

Comments

@findleyr
Copy link
Contributor

https://go.dev/cl/455615 simplified our package APIs, but dropped the explicit filtering of intermediate test variants from snapshot.PackageForFile.

I believe as a result of this change, we may get an intermediate test variant at the call sites where we previously would not. This can cause needless type-checking.

Not yet released, but we must address this before v0.12.0

CC @adonovan
@findleyr findleyr added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 13, 2023
@findleyr findleyr added this to the gopls/v0.12.0 milestone Jan 13, 2023
@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 13, 2023
@gopherbot
Copy link

Change https://go.dev/cl/462497 mentions this issue: gopls/internal/lsp/source: include ITVs in global references

gopherbot pushed a commit to golang/tools that referenced this issue Jan 18, 2023
@adonovan
gopls/internal/lsp/source: include ITVs in global references
f46e418 
The global search for references should include intermediate
test variants (ITVs), because the rdeps of every variant are disjoint.
The existing comment was mistaken.

The local search does not need them for the valid reason given in
the existing comment.

The previous call to RemoveIntermediateTestVariants had not
quite the intended effect because we forgot to use its result. (D'oh.)
The effect of the change in the test was to expose the bug
by creating an ordinary test variant.

Updates golang/go#57795

Change-Id: I1852aebf81ae0d169191834eb9a880d0ba90bc6e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/462497
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
@findleyr
Copy link
Contributor Author

This is important to fix, and should be straightforward.

@adonovan
Copy link
Member

adonovan commented Apr 7, 2023

FWIW I just audited all calls to MetadataForFile and found that all but one of them either:

  • discard ITVs from the result;
  • compute rdeps and then discard ITVs (see typeCheckReverseDependencies in rename.go);
  • use only metas[0], the narrowest variant; or
  • use the widest variant but don't type-check it (see packageReferences in references.go).

The only caller that does not is implementations, which acknowledges in a comment that it wants test variants, though it is not explicit about whether it means ordinary or intermediate.

I'm struggling to remember why we would ever want to care about ITVs, if we're happy to disregard the possible cross-package effects such as changes to selectors caused by method declarations in the ordinary test variant.

@gopherbot
Copy link

Change https://go.dev/cl/487095 mentions this issue: gopls/internal/lsp/source: filter intermediate test variants

@golang golang locked and limited conversation to collaborators Apr 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@adonovan adonovan

Labels
FrozenDueToAge gopls/incremental gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. release-blocker Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.12.0
Development

No branches or pull requests
3 participants
@adonovan @gopherbot @findleyr