The GC-based cache has given us a number of problems. First, memory
leaks driven by reference cycles: the Go runtime cannot collect cycles
involving finalizers, which prevents us from writing natural code in
Bind callbacks. If we screw it up, we get a mysterious leak that takes a
long time to track down. Second, the behavior is generally mysterious;
it's hard to predict how long a value lasts, and harder to tell if a
value being live is a bug. Third, we think that it may be interacting
poorly with the GC, resulting in unnecessary memory usage.
The structure of the values we put in the cache is not actually that
complicated -- there are only 5 significant types: parse, typecheck,
analyze, parse mod, and analyze mod. Managing them manually should not
be conceptually difficult, and in fact we already do most of the work
in (*snapshot).clone.
In this CL the cache adds the concept of "generations", which function
as reference counts on cache entries. Entries are still global and
shared across generations, but will be explicitly deleted once no
generations refer to them. The idea is that each snapshot is a new
generation, and can inherit entries from the previous snapshot or leave
them behind to be deleted.
One obvious risk of this scheme is that we'll leave dangling references
to values without actually inheriting them across generations. To
prevent that, getting a value requires passing in the generation at
which it's being read, and an error will be returned if that generation
is dead.
Change-Id: I4b30891efd7be4e10f2b84f4c067b0dee43dcf9c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/242838
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
FileHandle currently includes LSP-level information about Version and
Session. That's dangerous, because the cache operates in terms of
URIs and content only -- we explicitly want to share results across
sessions and versions if they happen to be the same.
Split the LSP information into separate types, VersionedFileHandle and
VersionedFileIdentity.
Change-Id: I158646b783375b58245468599301e2a29c657e71
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245058
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Continuing the massacre, remove ParseModHandle, and Mod*Handle, from the
source API.
Notably, having the snapshot available means we can simplify the go
command invocation paths a lot.
Change-Id: Ief4ef41e42f93d653f719a230004861e5e1ef70b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244769
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
There were a few merge conflict-related issues in the GC optimization
details CL. Also fixed a few things I noticed after the fact, like
separating out a new mutex.
Staticcheck caught a few things, and I also fixed a bug I noticed
in the cache package.
Change-Id: I3fc519373253418586dca08fdec3114b30a247ea
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245399
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
Due to the runtime's inability to collect cycles involving finalizers,
we can't close over handles in memoize.Functions without causing memory
leaks. Up until now we've dealt with that by closing over all the bits
of the snapshot that we want, but it distorts the design of all the code
used in the Functions.
We can solve the problem another way: instead of closing over the
snapshot/view, we can force the caller to pass it in. This is somewhat
scary: there is no requirement that the argument matches the data that
we're working with. But the reality is that this is not a new problem:
the Function used to calculate a cache value is not necessarily the one
that the caller expects. As long as the cache key fully identifies all
the inputs to the Function, the output should be correct. And since the
caller used the snapshot/view to calculate that cache key, it should
always be safe to pass in that snapshot/view. If it's not, then we
already had a bug.
The Arg type in memoize is clumsy, but I thought it would be nice to
have at least a little bit of type safety. I'm open to suggestions.
Change-Id: I23f546638b0c66a4698620a986949087211f4762
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244019
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
I'm not sure how the regtest didn't catch this - is it possible that
it could unmarshal a single string a slice of string? Either way, I'd
like to get the fix in quicker - I'll try to add more regtests for this
later.
Also, validate the upgrade results more thoroughly.
Change-Id: I812a3fecd9f0642a1408c0a9c0376bb98d50b397
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245065
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This change expands the scope of a workspace to the whole module, if the
user is in module mode. This means that diagnostics will appear and will
be updated for the whole module, even if the user only opens a
subdirectory. Similarly, references and other such queries will always
return consistent results, no matter which directory the user opens.
A new "root" field is added to the view. This is either the view's
folder or its module root. Almost all uses of view.folder have been
changed to view.root.
Updates golang/go#32394
Change-Id: I46f401f7c44b1b8429505aa032e0c15e88c4e2ef
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244117
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This change separates out different functions of mod handles.
Previously, we had ModHandle and ModTidyHandle. ModHandle was used to
parse go.mod files and get the results of `go mod why` and possible
dependency upgrades.
Now, we factor this out into 4 handles: ParseModHandle, ModWhyHandle,
ModUpgradeHandle, and ModTidyHandle. This allows each handle to be
specific to its own functionality. It also simplifies the code a bit,
as the handles can be written in terms of ParseModHandles instead of
FileHandles.
I may have some follow-up CLs to refactor the `go mod tidy` logic out of
the cache package, though I'm no longer certain that that's a good
choice.
Change-Id: I8e12299dfdda7bb61b05903d9aa474461d7f4836
Reviewed-on: https://go-review.googlesource.com/c/tools/+/239117
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Several fixes related to GOPRIVATE handling and links:
+ In Go source, fix links matching GOPRIVATE for external modules.
Previously, in these cases we'd try to match <mod>@v1.2.3/<suffix>,
which wasn't the correct input into the GOPRIVATE matching algorithm.
+ Similarly check GOPRIVATE for go.mod require statement hovers.
+ Likewise, for documentLink requests (both mod and source).
+ Move the existing hover regtest to link_test.go, and expand to cover
all these cases.
Along the way, I encountered a couple apparent bugs, which I fixed:
+ Correctly handle the case where there is only one require in a go.mod
file. This was exercised by the regtest, so took some debugging.
+ Only format links [like](this) if the requested format is actually
markdown.
Fixesgolang/go#36998
Change-Id: I92011821f646f2a7449dcca619483f83bdeb54b0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/238029
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
While working in this code, I was confused by the fact that modHandle
and modData were used for both ModHandle and ModTidyHandle. Separate out
the two so it's a little more clear. This is mostly a copy-paste, with a
few fields renamed to simplify them.
Change-Id: I20ea11c32a624fd250eabf901d09ebb05fbad062
Reviewed-on: https://go-review.googlesource.com/c/tools/+/238337
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Refactor internal/lsp/cache to use a new temporary go.mod file for each
go command invocation. This cleans up the abstraction in the source
package, as we no longer are aware of temporary go.mod files.
This will also fix the raciness of reusing the same temporary go.mod
file for each invocation.
Updates golang/go#37318.
Fixesgolang/go#39504.
Change-Id: I90bc17a678b5df222ab598c8f7dbf6c6fdd393f6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/237517
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Limiting the Config to the view seems reasonable, considering that it is
only used to run the `go` command. I prefer just having the cache run
go commands, so that source doesn't have to deal with the environment.
This also enables CL 237517.
Change-Id: I639c082592de30e9682dc25cdd12c7751ddb4f97
Reviewed-on: https://go-review.googlesource.com/c/tools/+/237600
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
ModHandle races with the initial workspace load if the go.mod file does
not yet exist. We should await for the initial workspace load to
complete before proceeding with update codelenses, etc.
Part of trying to figure out the flakes in golang/go#39504.
Also a few staticcheck fixes, and fix the Windows line endings in
fill_struct.go, because `git gofmt` complains.
Change-Id: Ide21a47137390792d1afb924740cff0bb6f0b764
Reviewed-on: https://go-review.googlesource.com/c/tools/+/237419
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
We use file identities pervasively throughout gopls. Prior to this
change, the identity is the modification date of an unopened file, or
the hash of an opened file. That means that opening a file changes its
identity, which causes unnecessary churn in the cache.
Unfortunately, there isn't an easy way to fix this. Changing the
cache key to something else, such as the modification time, means that
we won't unify cache entries if a change is made and then undone. The
approach here is to read files eagerly in GetFile, so that we know their
hashes immediately. That resolves the churn, but means that we do a ton
of file IO at startup.
Incidental changes:
Remove the FileSystem interface; there was only one implementation and
it added a fair amount of cruft. We have many other places that assume
os.Stat and such work.
Add direct accessors to FileHandle for URI, Kind, and Version. Most uses
of (FileHandle).Identity were for stuff that we derive solely from the
URI, and this helped me disentangle them. It is a *ton* of churn,
though. I can revert it if you want.
Change-Id: Ia2133bc527f71daf81c9d674951726a232ca5bc9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/237037
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
event.Log removed
event.Print -> event.Log
event.Record -> event.Metric
event.StartSpan -> event.Start
In order to support this core now exposes the MakeEvent and Export functions.
Change-Id: Ic7550d88dbf400e32c419adbb61d1546c471841e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/229238
Reviewed-by: Robert Findley <rfindley@google.com>
internal/telemetry/event was renamed to internal/event/core
Some things were partly moved from internal/telemetry/event straight to
internal/event to minimize churn in the following restructuring.
Change-Id: I8511241c68d2d05f64c52dbe04748086dd325158
Reviewed-on: https://go-review.googlesource.com/c/tools/+/229237
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
This change attempts to fix a concurrency error that would cause
textDocument/CodeLens, textDocument/Formatting, textDocument/DocumentLink,
and textDocument/Hover from failing on go.mod files.
The issue was that the go command would return a potential concurrency
error since the ModHandle and the ModTidyHandle are both using the
temporary go.mod file.
Updates golang/go#37824
Change-Id: I6cd63c1f75817c7308e033aec473966536a2a3bd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/224917
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change uses the new unusedparams analyzer to remove any unused parameters from functions inside of internal/lsp/source :)
Change-Id: I220100e832971b07cd80a701cd8b293fe708af3d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225997
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This changes the way keys work, there is still a single internal key
implementation for performance reasons, but the public interface is a set of key
implementations that have type safe Of and Get methods.
This also hides the implemenation of Tag so that we can modify the storage form
and find a more efficient storage if needed.
Change-Id: I6a39cc75c2824c6a92e52d59f16e82e876f16e9c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223137
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Move the lsp specific telemetry package that just declares the labels under the
debug package and call it tag, to make all the usages much more readable.
Change-Id: Ic89b3408dd3b8b3d914cc69d81f41b8919aaf424
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222850
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
This change makes ModHandle and ModTidyHandle return a ModData structure that only contains an error if something goes wrong when parsing the .mod files.
Change-Id: I89661cfefeff666bdf13cf99f4c56137e120de82
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222242
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
When caching file data specific to a session (anything with a Version or
tied to a view), we now need to be more careful about the existence of
multiple sessions.
This change fixes a few places where we appear to cache session data
without explicitly referring to the session. In principal this could
cause data corruption in multi-session gopls instances, but I have not
been able to force this to occur in either manual or automated testing.
Also fix a data race to the unsaved overlays:
https://storage.googleapis.com/go-build-log/588ee798/linux-amd64-race_d0762522.log
Updates golang/go#34111
Change-Id: I47117da1aba1afc2e211785544ad3f8b3416d15d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222059
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
This change attaches a modTidyHandle to the snapshot instance and tries to reuse it as often as possible. It also modifies the modTidyKey to include the files that have not yet been saved. This was necessary because `go mod tidy` only runs using contents on disk.
So, if a user decides to add an import that is not in their modcache, we run diagnostics as normal and the ModTidyKey's imports field gets updated with the new import, we run `go mod tidy` but since the file was not saved yet, nothing changes. Then when the user eventually saves their file, we do not rerun `go mod tidy` because the imports hash has not changed from the time the file was in overlay to the time the file was saved on disk. To be able to account for this, we need the invalidate the ModTidyKey when imports change between saves.
Change-Id: I9e210a15cf009d222cecd7824c2a1a927957483b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/219477
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change implements support for textDocument/hover when it comes to go.mod files.
Updates golang/go#36501
Change-Id: Ie7da0194bb972955b7ab9cf7b9c9972bd9f4b8a9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220359
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change implements support for textDocument/documentLink when it comes to go.mod files.
Updates golang/go#36501
Change-Id: Ic0974e3e858dd1c8df54b7d7abee085bbcb6d4ee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/219938
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Over time we have accumulated a disturbing number of ways to run the Go
command, each of which supported slightly different features and
workarounds. Combine all of them.
This unavoidably introduces some changes in debug logging behavior; I
think we should try to fix them incrementally and consistently.
Updates golang/go#37368.
Change-Id: I664ca8685bf247a226be3cb807789c2fcddf233d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220737
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change reorders the logic within ModTidyHandle and ParseModHandle to parse the modfile first before we copy the contents to the temporary go.mod file. This was causing issues where a go.mod would be in a bad state and then we would try to run "go mod tidy" on the corrupted file.
Change-Id: I1df8fb70f5f3e2bcff306a58b16bc96c32debf2a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/219480
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds a code lens for go.mod files that will let a user know if a module can be upgraded, once it is clicked gopls will run a command to update that module.
Updates golang/go#36501
Change-Id: Id22b8097ede4972cf73bc029ec927544a71b7150
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218557
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
