Skip to content

x/tools/gopls: eliminate dependence on "seen files" #57558

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
Closed
findleyr opened this issue Jan 3, 2023 · 4 comments
Closed

x/tools/gopls: eliminate dependence on "seen files" #57558

findleyr opened this issue Jan 3, 2023 · 4 comments
Assignees
@findleyr
Labels
FrozenDueToAge gopls/metadata Issues related to metadata loading in gopls 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.15.0

Comments

@findleyr
Copy link
Member

findleyr commented Jan 3, 2023

Caveat: this is an abstract issue, but sufficiently fundamental to warrant independent tracking.

In two places inside gopls, there is a notion of "previously seen" files that affects gopls behavior:

  • cache.View.knownFile, which is used to determine if a file is relevant to a given view
  • cache.snapshot.FindFile (and anything that walks the snapshot.files set), which is used to filter to relevant or active files

These notions have no intrinsic definition: by construction they are path dependent. As such, they have been a source of historical and current bugs (for example, changing configuration can invalidate the set of known files, which leads to undefined behavior). They are also a source of test flakes, which is really just a manifestation of underlying bugginess.

We should replace these with well-defined notions, on a case by case basis. For example "is a file in the workspace directory", or "is this file in a loaded package, after loading completes".

CC @adonovan @pjweinb
@findleyr findleyr added this to the gopls/v0.12.0 milestone Jan 3, 2023
@findleyr findleyr self-assigned this Jan 3, 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 3, 2023
@findleyr findleyr mentioned this issue Jan 3, 2023
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/459560 mentions this issue: gopls/internal/regtest: avoid race in TestSwitchFromGOPATHToModuleMode

gopherbot pushed a commit to golang/tools that referenced this issue Jan 3, 2023
@findleyr
gopls/internal/regtest: avoid race in TestSwitchFromGOPATHToModuleMode
1e0dff2 
Fix two bugs in TestSwitchFromGOPATHToModuleMode:
- `go mod init` was run in the wrong directory.
- on-disk change notifications raced with the main.go edit, causing us
  to only encounter the problem of the previous bullet in rare cases
  where the on-disk notification won the race

I've filed golang/go#57558 to track fixing the fundamental raciness of
view changes.

Fixes golang/go#57512

Change-Id: I2b21f944377e0ba45ee7be019a28f18a334f3516
Reviewed-on: https://go-review.googlesource.com/c/tools/+/459560
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
This was referenced Jan 20, 2023
Closed
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/468775 mentions this issue: gopls/internal/lsp/source: use metadata files for workspace symbols

gopherbot pushed a commit to golang/tools that referenced this issue Feb 16, 2023
@findleyr
gopls/internal/lsp/source: use metadata files for workspace symbols
e5b9948 
The workspace symbol handler was walking the files map ("seen files")
rather than the package graph.

In golang/go#57558, we endeavor to avoid any dependency on seen files,
and pragmatically we will soon no longer read the entire workspace
during loading.

For golang/go#57558

Change-Id: Ie95a333842af24ab801bb3e314f5c00e1e5e1b0b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/468775
Reviewed-by: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
@findleyr findleyr modified the milestones: gopls/v0.12.0, gopls/v0.13.0 May 22, 2023
@adonovan adonovan added the gopls/metadata Issues related to metadata loading in gopls label Aug 31, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/525616 mentions this issue: gopls/internal/lsp/cache: simplify tracking of snapshot directories

gopherbot pushed a commit to golang/tools that referenced this issue Sep 5, 2023
@findleyr
gopls/internal/lsp/cache: simplify tracking of snapshot directories
36c4f98 
Great care was taken to track known directories in the snapshot without
blocking in snapshot.Clone, introducing significant complexity.

This complexity can be avoided by instead keeping track of observed
directories as files are set in the snapshot. These directories need
only be reset when files are deleted from the snapshot, which is a
relatively rare event.

Also rename filesMap->fileMap, and move to filemap.go, with a new unit
test.

This reduces some path dependence on seen files, as the set of
directories is well defined and depends only on the files in the
snapshot. Previously, when a file was removed, gopls called Stat to
check if the directory still existed, which leads to path dependence: an
add+remove was not the same as nothing at all.

Updates golang/go#57558

Change-Id: I5fd89ce870fa7d8afd19471d150396b1e4ea8875
Reviewed-on: https://go-review.googlesource.com/c/tools/+/525616
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@findleyr findleyr modified the milestones: gopls/v0.14.0, gopls/v0.15.0 Oct 9, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/551295 mentions this issue: gopls/internal/lsp/cache: switch to new bestViewForURI logic

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

@findleyr findleyr

Labels
FrozenDueToAge gopls/metadata Issues related to metadata loading in gopls 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.15.0
Development

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