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>
snapshot.View().Session().Cache().FileSet() has been driving me crazy
for a while. Add it to snapshot. Along the way, discover that the Cache
interface is now totally unused and delete it.
I also changed a bunch of View arguments to Snapshot while I was in the
area.
Change-Id: I1064d0020b1567c2ed28d2d55e0f4649eb94c060
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245324
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
To manually collect cache entries, we need to know when a snapshot is
idle. Add a reference count in the form of a WaitGroup and keep track of
its uses. The pattern is that any time a snapshot is returned, it comes
with a release function that decrements the ref count.
Almost all uses of a snapshot originate in a user-facing request,
handled in beginFileRequest. There it's mostly an exercise in passing
Snapshots around instead of Views.
In the other places I took the path of least resistance. For file
modifications I tried to minimize the amount of code that needed to deal
with snapshots. For diagnostics I just acquired the snapshot at the
diagnostics call.
Change-Id: Id48a2df3acdd97f27d905e2c2be23072f28f196b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/241837
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Just like ParseGoHandle, PackageHandle isn't very useful as part of the
public API. Remove it.
Having PackagesForFile take a URI rather than a FileHandle seems
reasonable, and made me wonder if that logic applies to other calls like
ParseGo. For now I'm going to stop here. I could also revert that part
of the change.
Change-Id: Idba8e9fdba0b0c48e841a698eb97e47fd5f23cf5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244637
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
ParseGoHandles serve two purposes: they pin cache entries so that
redundant calculations are cached, and they allow users to obtain the
actual parsed AST. The former is an implementation detail, and the
latter turns out to just be an annoyance.
Parsed Go files are obtained from two places. By far the most common is
from a type checked package. But a type checked package must by
definition have already parsed all the files it contains, so the PGH
is already computed and cannot have failed. Type checked packages can
simply return the parsed file without requiring a separate Check
operation. We do want to pin the cache entries in this case, which I've
done by holding on to the PGH in cache.pkg.
There are some cases where we directly parse a file, such as for the
FoldingRange LSP call, which doesn't need type information. Those parses
can actually fail, so we do need an error check. But we don't need the
PGH; in all cases we are immediately using and discarding it.
So it turns out we don't actually need the PGH type at all, at least not
in the public API. Instead, we can pass around a concrete struct that
has the various pieces of data directly available.
This uncovered a bug in typeCheck: it should fail if it encounters any
real errors.
Change-Id: I203bf2dd79d5d65c01392d69c2cf4f7744fde7fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244021
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
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>
Add a symbolStyle configuration option, and use it to parameterize the
following behavior when computing workspace symbols:
+ package (default): include package name in the workspace symbol.
+ full: fully qualify the symbol by import path
+ dynamic: use as the symbol the shortest suffix of the full path that
contains the match.
To implement this, expose package name in the source.Package interface.
To be consistent with other handling in the cache package, define a new
cache.packageName named string type, to avoid confusion with packageID
or packagePath (if confusing those two identifiers was a problem, surely
it is a potential problem for package name as well).
Change-Id: Ic8ed6ba5473b0523b97e677878e5e6bddfff10a7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/236842
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Paul Jolly <paul@myitcv.org.uk>
Fully qualify (using a package's import path) symbols when matching
them. This is a minimal change in preparation for later changes to add
more advanced query functionality to the workspace Symbol method.
Add more comprehensive regtest tests (covering case sensitive, case
insensitive and fuzzy matchers) in addition to updating lsp tests.
Change-Id: I675cde2f7b492158988cf9c3206a0a55fe29622a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228123
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
In preparation for later changes to the workspace Symbol method, we add
a separate configuration option keyed by "symbolMatcher" that specifies
the type of matcher to use for workspace symbol requests. We also define
a new type SymbolMatcher, the type of this new option. We require
SymbolMatcher to be a separate type from Matcher because a later CL adds
a type of symbol matcher that does not make sense in the context of
other uses of Matcher, e.g. completion.
Change-Id: Icde7d270b9efb64444f35675a8d0b48ad3b8b3dd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228122
Reviewed-by: Robert Findley <rfindley@google.com>
WorkspaceSymbols matches symbols across views using the given query,
according to the matcher Matcher.
The workspace symbol method is defined in the spec as follows:
> The workspace symbol request is sent from the client to the server to
> list project-wide symbols matching the query string.
It is unclear what "project-wide" means here, but given the parameters
of workspace/symbol do not include any workspace identifier, then it has
to be assumed that "project-wide" means "across all workspaces". Hence
why WorkspaceSymbols receives the views []View.
However, it then becomes unclear what it would mean to call
WorkspaceSymbols with a different configured Matcher per View.
Therefore we assume that Session level configuration will define the
Matcher to be used for the WorkspaceSymbols method.
As part of this change we also tidy up lsp_test.go and source_test.go to
remove some repetition.
Change-Id: I444f9a78303ac9d2c8d8ac6496603b5758e4aafd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228763
Run-TryBot: Paul Jolly <paul@myitcv.org.uk>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
In VS Code, a workspace symbol query with an empty query parameter
is issued as soon as users open the symbol search box. There are many
symbols in a reasonably sized project and the chance that a user finds
a result in the randomly chosen 100 items out of those many symbols is
low. Thus, this first query is often useless.
Ignore this query and return an empty result immediately.
Change-Id: Idc7703c8e460c9115ecbcf198907acc9c82add4d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/232986
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>
We need to use the filename derived from the symbol information, not the
one from CompiledGoFiles, in case of cgo packages.
No tests because workspace symbols are entangled with document symbols,
and the latter doesn't work in cgo packages.
Fixesgolang/go#37659.
Change-Id: Ic32293c542830a49b37c25ebf3b231771c3a4225
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222060
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Searching with an empty string shouldn't return every symbol in the
workspace -- nobody wants that. Limit to 100 results to avoid breaking
editors. (VS Code locks up for like 30 seconds on my workspace.)
Change-Id: I1e0be476e8eeaef9e69767bfa04a89d40bd3a6e5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220939
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Create a real type for protocol.DocumentURIs. Remove span.NewURI in
favor of path/URI-specific constructors. Remove span.Parse's ability to
parse URI-based spans, which appears to be totally unused.
As a consequence, we no longer mangle non-file URIs to start with
file://, and crash all over the place when one is opened.
Updates golang/go#33699.
Change-Id: Ic7347c9768e38002b4ad9c84471329d0af7d2e05
Reviewed-on: https://go-review.googlesource.com/c/tools/+/219482
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change allows to use fuzzy or case-sensitive matchers in addition
to case-insensitive when searching for symbols.
Matcher is specified by UserOptions.Matcher just like Completion.
Updates golang/go#33844
Change-Id: I4000fb7984c75f0f41c38d740dbe164398032312
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218737
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Currently there is no need for this because the file contents are part
of the file handle. This change is in preparation for an impending
improvement that tweaks the source code during the parse stage to fix
certain kind of terminal parse errors. Any code that wants to use
an *ast.File or *token.File in conjunction with the file contents
needs access to the doctored source code so things line up.
Change-Id: I59d83d3d6150aa1264761aa2c1f6c1269075a2ce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218979
Run-TryBot: Muir Manders <muir@mnd.rs>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds support for the LSP workspace/symbol. Unlike
documentSymbol, the target is symbols that exist not only in a specific
file, but also in the current or imported packages. It returns symbols
whose name contains the query string of the request(case-insensitive),
or all symbols if the query string is empty.
However, the following is not implemented:
- Setting of deprecated and containerName fields in SymbolInformation
- Consideration of WorkspaceClientCapabilities
- Progress support
- CLI support
Updates golang/go#33844
Change-Id: Id2a8d3c468084b9d44228cc6ed2ad37c4b52c405
Reviewed-on: https://go-review.googlesource.com/c/tools/+/213317
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
