The Check for upgrades codelens was only looking for upgrades for
the current module, but was applying diagnostics to all go.mod
files in the workspace. This change makes sure to only apply the
diagnostics in the same selected go.mod.
Fixesgolang/go#54556
Change-Id: I1eacbc8af2e9dcfe1e1a67516f047bcb94099872
Reviewed-on: https://go-review.googlesource.com/c/tools/+/425195
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
This change ensures that the End position provided to span.NewRange
in suggestedAnalysisFixes is valid even when the diagnostic has
only a valid start position. This seems to be the cause of
some panics observed in the ARM builders in the attached issue.
Also, reduce the scope of the recover operation to just the analyzer's
run method: we don't want to hide further bugs (or discard stack traces)
in the setup or postprocessing logic.
Also:
- split a single assertion in span.NewRange into two.
- Add information to various error messages to help identify causes.
- Add TODO comments about inconsistent treatment of token.File
in span.FileSpan, and temporarily remove bug.Errorf that
is obviously reachable from valid inputs.
- Add TODO to fix another panic in an analyzer that is covered
by our tests but was hitherto suppressed.
- Add TODO to use bug.Errorf after recover to prevent recurrences.
We can't do that until the previous panic is fixed.
Updates https://github.com/golang/go/issues/54655
Change-Id: I0576d03fcfffe0c8df157cf6c6520c9d402f8803
Reviewed-on: https://go-review.googlesource.com/c/tools/+/425356
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
Go 1.19 introduced new formatting for doc comments, and a new package
for processing them. This change uses the new package when gopls is
compiled with go 1.19 or later.
The difficulty is with the hover tests, which have to work both when
gopls is compiled with earlier versions of go, and with go 1.19.
Fortunately the changes in formatting the test cases are easily checked.
Fixesgolang/go#54260
Change-Id: I9e8e7f0cf3392afa0865b5d3f4e5fcdd88dfe75f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/421502
Run-TryBot: Peter Weinberger <pjw@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
CL 417576 externalized shouldLoad tracking into a map, which was used to
trigger a reload and cleared once reload completes. Unfortunately, it
overlooked the fact that we may also reload the entire workspace (via
reinitialization). In this case, we should clear newly loaded IDs from
the shouldLoad map, so that they are not subsequently loaded again.
Fixesgolang/go#54473
Change-Id: I26f49552cae502644142dc4a4e946294db37f6f7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/424074
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
- ignore error from buildPackageHandle, with rationale.
- remove unnessary optimization in call to Clone(updates={}):
Clone does the same check internally.
- more comments.
Change-Id: I4551cf560aea722d972fb6da404aed71a79f4037
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416217
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Diagnostic Tags add another hint for the client for how to
display diagnostics. The Unnecessary tags allows clients to
fade out the diagnostic instead of adding a squiggle. This adds
this tag to unused import errors as well as unused vars.
For golang/vscode-go#2285
Change-Id: I0c93b28a6a2ef4eed314dcf30a37c27dd65940ac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415499
Reviewed-by: Robert Findley <rfindley@google.com>
When the go.work file is set by the GOWORK environment variable, we must
create an additional file watching pattern.
Fixesgolang/go#53631
Change-Id: I2d78c5a9ee8a71551d5274db7eb4e6c623d8db74
Reviewed-on: https://go-review.googlesource.com/c/tools/+/421501
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
In order to correctly process changes to the go.work file, the workspace
must know about GOWORK settings configured in the users environment.
Compute this when initializing the view, and thread this through to the
workspace.
At this point, workspace information is spread around in a few places.
Add some TODOs to clean this up.
Also remove some module data that was not used in
TestBrokenWorkspace_DuplicateModules.
Updates golang/go#53631
Change-Id: Ie0577d702c8a229304387bc7fe53a8befb544acb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/421500
Reviewed-by: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
For users of Go >= 1.18, update the error message presented to users
with broken workspaces to suggest using a go.work. For older Go
versions, suggest updating to 1.18+.
Also:
- add support for changing workspace folders from the fake editor
- update the test to confirm that changing workspace folders resolves
the error message
- inline validBuildConfiguration, and make a few TODOs for how the code
could be further cleaned up
Fixesgolang/go#53882
Change-Id: Ia03dcfec59569b1a3ac941dc40d079b9c2593825
Reviewed-on: https://go-review.googlesource.com/c/tools/+/421499
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Dylan Le <dungtuanle@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
It is no longer needed, now that we don't consider invalid packages to
be workspace packages.
Change-Id: I6155a2609ab07046b9507dc04717eea7b974f1b6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/421257
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
To support the experimentalUseInvalidMetadata mode, we keep around
invalid metadata. This can help gopls features work when, for example,
the go.mod file is broken. It is debatable whether this feature is worth
supporting (see golang/go#54180), but in the meantime there is a very
negative side-effect when module paths are changed in the go.mod file:
we keep around a bunch of workspace packages with a stale module path.
As a result we can be left with a lots of extra type-checked packages
in memory, and many inaccurate diagnostics.
Fix this by skipping packages with invalid metadata when computing
workspace packages. While we may want to use invalid metadata when
finding the best package for a file, it does not make sense to re-
type-check and diagnose all those stale packages.
Fixesgolang/go#43186
Change-Id: Id73b47ea138ec80a9de63b03dae41d4e509b8d5a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420956
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Based on dle8's CL 414454, wire in directoryFilters into the goimports
ModuleResolver scan. A few changes/fixes/additions from that CL:
- Fix a bug in filter validation that was causing -**/a not to validate.
- Fix a bug in regex building where -a was not treated as an excluded
prefix (it was matching 'a' anywhere).
- Use absolute paths in the SkipPathInScan, so that we can evaluate
directory filters relative to the workspace folder.
- Add several regression tests.
- Consolidate directoryFilters regression tests under a new
directoryfilters_test.go file.
- Add several TODOs.
Fixesgolang/go#46438
Change-Id: I55cd3d6410905216cc8cfbdf528f301d67c2bbac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420959
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Dylan Le <dungtuanle@google.com>
Fixgolang/go#50267 by ensuring that diagnostics are re-sent following
didOpen or didClose events. Additionally, introduce a new hidden
'chattyDiagnostics' option that causes diagnostics to be resent on
*every* file change event. This latter option is for LSP clients that
get confused when diagnostics are not re-sent for later file versions.
For now, be conservative and only force diagnostic publication on
didOpen and didClose.
Update tests whose 'NoDiagnostics' assertions were broken by the new
behavior.
Fixesgolang/go#50267
Change-Id: I6332d66a1851e0d8261599d37020a03b4c598f7d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420539
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Fix a tricky race in gopls/internal/regtest/diagnostics.Test_Issue38211:
When reloading the workspace, we can encounter context cancellation if
the snapshot is cancelled, and can write this cancellation as a critical
error *before* the context is cloned, leading to a state where there is
a critical error that won't go away.
This should resolve test flakes reported in golang/go#44098.
For golang/go#44098
Change-Id: I41c0f49b2fe999131f4c31166e69b2cde85470b7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419502
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Per the explanation at golang/go#54069, allow network access whenever
loading the workspace. Enable one test that exercises this behavior.
More tests will be added in subsequent CLs.
Updates golang/go#47540
Updates golang/go#53676
Updates golang/go#54069
Change-Id: I9c3bb19d36702bc6b8051bee6b7cddaec5b97c0c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419500
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
The logic to detect whether the snapshot should be reinitialized is
complicated, and had at least a couple latent bugs:
- It depends on workspace.Clone to learn whether relevant mod/work
files have been saved.
- It naively checks for changes in vendor directories, and
reinitializes if there was an initialization failure.
- It copies the initializeOnce field. Pragmatically this may not matter
(due to context cancellation, for example), but as a matter of best
practices we should not copy this field. It could lead to missing an
initialization on the new snapshot.
- Writing initializeOnce was technically racy.
Furthermore, due to vendored file handling being interleaved with other
changes, it was possible that we detect that the snapshot should be
reinitialied only after we have processed other changes, and that
processing of other changes depended on knowing whether the snapshot
will be reinitialized.
Simplify this by
- Replacing 'initializeOnce' with an 'initialized' bool, which is
guarded with snapshot.mu. We were relying on the view initialization
semaphore to prevent races anyway, so there is no need for a sync.Once
to prevent multiple in-flight initializations.
- Removing the 'changed' result from workspace.Clone: it shouldn't
actualy matter for snapshot.Clone (only reinit matters), and in any
case can be derived by comparing the new workspace with the old.
- Detect changes to vendored files before processing the rest of the
changes.
Change-Id: I3624a46d4be9c054a6f719f20549599986b64cbd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419499
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
When a go.work file fails to validate, the workspace is left in an
invalid state: we will detect that the workspace is defined by the
go.work, but will not actually parse any active modules. This should be
a critical error.
Fix this by adding allowing the workspace to surface critical errors via
a new cache.workspace.criticalError method.
Additionally:
- only build the workspace mod file in workspace.build if the mode is
fileSystemWorkspace (in all other modes the modfile is already
determined)
- rename workspace.invalidate to workspace.Clone, to be consistent with
other data structures
- rename CriticalError.DiagList to CriticalError.Diagnostics
- add several TODOs for observations while reading the code
- create a new file for regtests related to broken workspaces
- make the regtest sandbox panic when duplicate paths are present in
the sandbox file set (an error I made while writing the test)
Updates golang/go#53933
Change-Id: If8625ab190129bc9c57e784314bc9cc92644c955
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417593
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
added
Add logic to invalidate any packages with missing dependencies when a
file is added.
Also fix a latent bug overwriting 'anyFileOpenenedOrClosed' for each
loop iteration.
Fixesgolang/go#54073
Change-Id: I000ceb354885bd4863a1dfdda09e4d5f0e5481f3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419501
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Each regtest does a significant amount of extra work re-doing things
like parsing and type-checking the runtime package. We can share this
work across regtests by using a shared cache, significantly speeding
them up at the cost of potentially hiding bugs related to timing.
Sharing this work still retains most of the benefit of the regtests, so
implement this in the default mode (formerly called "singleton" and now
renamed to "default"). In a subsequent CL, modes will be cleaned up so
that "default" is the only mode that runs with -short.
Making this change actually revealed a caching bug: our cached package
stores error messages extracted from go/packages errors, but does not
include these errors in the cache key. Fix this by hashing all metadata
errors into the package cache key.
Updates golang/go#39384
Change-Id: I37ab9604149d34c9a79fc02b0e1bc23fcb17c454
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417587
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
The fundamental bug causing TestChangePackageName to fail has been
fixed, yet unskipping it revealed a new bug: tracking whether or not a
package should be loaded requires that we actually store that package in
s.meta. In cases where we drop metadata, we also lose the information
that a package path needs to be reloaded.
Fix this by significantly reworking the tracking of pending loads, to
simplify the code and separate the reloading logic from the logic of
tracking metadata. As a nice side-effect, this eliminates the needless
work necessary to mark/unmark packages as needing loading, since this is
no longer tracked by the immutable metadata graph.
Additionally, eliminate the "shouldLoad" guard inside of snapshot.load.
We should never ask for loads that we do not want, and the shouldLoad
guard either masks bugs or leads to bugs. For example, we would
repeatedly call load from reloadOrphanedFiles for files that are part of
a package that needs loading, because we skip loading the file scope.
Lift the responsibility for determining if we should load to the callers
of load.
Along the way, make a few additional minor improvements:
- simplify the code where possible
- leave TODOs for likely bugs or things that should be simplified in
the future
- reduce the overly granular locking in getOrLoadIDsForURI, which could
lead to strange races
- remove a stale comment for a test that is no longer flaky.
Updates golang/go#53878
Change-Id: I6d9084806f1fdebc43002c7cc75dc1b94f8514b9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417576
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
In FilterDisallow, change filter to regex form to match with file paths. Add a unit regtest for FilterDisallow.
For golang/go#46438
Change-Id: I7de1986c1cb1b65844828fa618b72b1e6b76b5b9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414317
Run-TryBot: Dylan Le <dungtuanle@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This should have been in CL 417116.
Also:
- (related to CL 417415), rename packageHandle.check to await
to indicate its blocking nature.
- rename typeCheck to typeCheckImpl, following the pattern.
- move "prefetch" parallel loop into typeCheckImpl.
- add some comments.
Change-Id: Iea2c8e1f1f74fb65afd0759b493509147d87a4bb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417581
Run-TryBot: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Also:
- add test of NewHandle
- update package doc and other doc comments
- factor Store.Handle with NewHandle
- declare Handle before Store
Change-Id: I4bcea2c9debf1e77f973ef7ea9dbe2fd7a373996
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417417
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The trimming optimization deletes parts of the syntax tree
that don't affect the type checking of package-level declarations.
It used to remove unexported struct fields, but this had
observable consequences: it would affect the offset of later
fields, and the size and aligment of structs, causing the
'fieldalignment' analyzer to report incorrect findings.
Also, it required a complex workaround in the UI element
for hovering over a type to account for the missing parts.
This change restores unexported fields.
The logic of recordFieldsUses has been inlined and specialized
for each case (params+results, struct fields, interface
methods) as they are more different than alike.
BenchmarkMemStats on k8s shows +4% HeapAlloc:
a lot, but a small part of the 32% saving of the trimming
optimization as a whole.
Also:
- trimAST: delete func bodies without visiting them.
- minor clarifications.
Updates golang/go#51016
Change-Id: Ifae15564a8fb86af3ea186af351a2a92eb9deb22
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415503
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
All Get/Set operations on the maps now happen within a single
function (buildPackageKey, actionHandle).
No behavior change.
Change-Id: I347dfda578c28657a28538e228ecfb6f0871b94b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417415
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
As with mod tidy in CL 417116, we can rely on invalidation
rather than cache keys to avoid reusing stale results, and
there's no need to save these handles in the global store.
Change-Id: I3763c01fa21c6114248c1d541e3c168fc6a128c9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417416
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Previously, the modtidy operation used a persistent map
of handles in the central store that cached the result
of a parsing the go.mod file after running 'go mod tidy'.
The key was complex, including the session, view, imports
of all dependencies, and the names of all unsaved overlays.
The fine-grained key prevented spurious cache hits for
invalid inputs by (we suspect) preventing nearly all cache hits.
The existing snapshot invalidation mechanism should be
sufficient to solve this problem, as the map entry is evicted
whenever the metadata or overlays change. So, this change
avoids keeping handles in the central store, so they are
never shared across views.
Also, modtidy exploited the fact that a packageHandle
used to include a copy of all the Go source files
of each package, to avoid having to read the files
itself. As a result it would entail lots of unnecessary
work building package handles and reading dependencies
when it has no business even thinking about type checking.
This change:
- extracts the logic to read Metadata.{GoFiles,CompiledGo}Files
so that it can be shared by modtidy and buildPackageHandle.
- packageHandle.imports has moved into mod_tidy.
One call (to compute the hash key) has gone away,
as have various other hashing operations.
- removes the packagesMap typed persistent.Map wrapper.
- analysis: check cache before calling buildPackageHandle.
- decouple Handle from Store so that unstored handles may
be used.
- adds various TODO comments for further simplification.
Change-Id: Ibdc086ca76d6483b094ef48aac5b1dd0cdd04973
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417116
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Before, these methods of the Source interface used to use
a cache of buildASTCache, which built a Pos-keyed map for
a whole file, but the necessary algorithm is essentially
a binary search which is plenty fast enough to avoid the
need for cache.
This change implements that algorithm and moves
both methods out of the interface into a single function,
source.FindDeclAndField.
--
I measured the duration of all calls to astCacheData (before)
and FindDeclAndField (after) occurring within this command:
$ go test -bench=TestBenchmarkConfiguredCompletion -v ./gopls/internal/regtest/bench -completion_workdir=$HOME/w/kubernetes -completion_file=../kubernetes/pkg/generated/openapi/zz_generated.openapi.go -completion_regexp=Get
(The numbers reported by this benchmark are problematic,
which is why I measured call times directly; see
https://github.com/golang/go/issues/53798.)
Results:
before (n=4727) max = 21ms, 90% = 4.4ms, median = 19us
after (n=6282) max = 2.3ms, 90% = 25us, median = 14us
The increased number of calls to the function after the change
is due to a longstanding bug in the benchmark: each iteration of
the b.N loop doesn't do a fixed amount of work, it does as much
as it can in 10s. Thus making the code faster simply causes
the benchmark to spend the same amount of time on other parts of
the program--such as the loop that calls FindDeclAndField.
See https://go-review.googlesource.com/c/tools/+/221021 for
background on the previous implementation.
Change-Id: I745ecc4e65378fbe97f456228cafba84105b7e49
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416880
Auto-Submit: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Add a new WaitGroup to the View that allows waiting for all snapshot
destroy operations to complete. This helps ensure that the server
properly cleans up resources when shutting down, and lets us remove
work-arounds in the gopls regtests intended to avoid races during
shutdown.
Also:
- re-enable postfix completion tests that had to be disabled due to
being too racy
- rename the inlayHints regtest package to follow lower-cased naming
conventions
- add several TODOs
Fixesgolang/go#50707Fixesgolang/go#53735
Change-Id: If216763fb7a32f487f6116459e3dc45f4c903b8a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416594
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
CL 416874 changed the logic of populateProcessEnv and incorrectly
removed a write of ProcessEnv.ModFlag. We should explicitly set
-mod=readonly.
Change-Id: Ibacf3d4b4c0c978d65fde345741945d6136db159
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416877
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
This reverts CL 416879, which itself was a revert of CL 416874.
Reason for revert: failure is understood now, as described in
https://github.com/golang/go/issues/53796#issuecomment-1181157704 and fixed in CL 416881.
Change-Id: I1d6a4ae46fbb1bf78e2f23656de7885b439f43fb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416882
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Previously, snapshots were born with a reference count of zero.
Although leases were created and returned by functions that
return a snapshot, and correctly disposed of by the caller, the
view did not hold a lease for its snapshot. The view's lease
on the snapshot was implicit, and for this reason the view
made explicit calls to Destroy when these implicit leases ended,
which would then wait for all explicit leases to end.
Now that the view holds an explicit lease to its snapshot, it is
safe (as a follow-up change) to move the destroy logic into the
call of release() that brings the refcount to zero.
Also:
- clarify that release functions should be called when
and only when err is nil.
- in View: remove unused View.cancel field.
- in tempModFile: clarify tricky return + defer + named-result-var statement.
- in DidModifyFiles: simplify type of releases from []func() to func().
- in Server.addFolders: fix reference leak (looks minor).
Change-Id: Ibf61f4ce109e91060e2ccd854c32214b119f2f0a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416875
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Whenever a view.importsState value is created, we use
snapshot.goCommandInvocation to extract information used to run the go
command in the imports.ProcessEnv. Before this CL, that process acquired
the current snapshot (whatever it was), and held onto it for the purpose
of sharing a workspace directory. As a consequence all the memory in
that snapshot was pinned for the lifecycle of the importsState, which
can be the entire editing session. This results in a memory leak as
information in the session is invalidated.
Fix this by creating a copy of the workspace directory to be owned by
the importsState.
Also:
- Add some TODOs
- Clean up some stale comments
Fixesgolang/go#53780
Change-Id: I2c55cc26b2d46c9320c6c7cd86b3e24971cd5073
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416874
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
...now that I have understood why they are not
in fact inessential.
Change-Id: I1ab881a7d24cd71ee183bc8c6a1a058dbda641e2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416077
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Now that the lifetime of all handles in the store is
determined by reference counting, we no longer need
the generation feature.
The Arg interface, renamed RefCounted, is now optional,
and causes the lifetime of the argument to be extended
for the duration of the Function call. This is important
when the Get(ctx) context is cancelled, causing the
function call to outlive Get: if Get's reference to
the argument was borrowed, it needs to increase the
refcount to prevent premature destruction.
Also:
- add missing snapshot.release() call in
importsState.populateProcessEnv.
- remove the --memoize_panic_on_destroyed flag.
Change-Id: I0b3d37c16f8b3f550bb10120c066b628c3db244b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416076
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This change simplifes the ParseGo interface to
make it consistent with the other handle+map operations:
- ParseGoImpl is the basic parser.
- The 'fixed' bool result is a field of ParsedGoFile.
- ParseGo is the caching wrapper.
The map accessors have been inlined into it.
- goFiles (renamed parsedGoFiles) is now just a bare
persistent.Map.
- parseGoHandle is replaced by *memoize.Handle
- the operations of "make a handle" and "wait for it"
are no longer separate (since clients never want
one without the other).
- cachedPGF and peekOrParse have been combined into
peekParseGoLocked.
Change-Id: If01a6aaa7e6a8d78cb89c305e5279738e8e7bb55
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416223
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
If metadata is refreshed during the execution of buildPackageHandle, we
should not store the resulting package handle in the snapshot, as it
breaks the invariant that computed packages match the currently loaded
metadata.
This strictness revealed another bug: because of our fine-grained
locking in snapshot.load, it is possible that we set valid metadata
multiple times, leading to unnecessary invalidation and potential
further races to package handles. Fix this by building all metadata when
processing the go/packages result, and only filtering updates after
grabbing the lock.
For golang/go#53733
Change-Id: Ib63ae4fbd97d0d25d45fe04f9bcd835996db41da
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416224
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This change replaces the 5 remaining calls to Bind (generational
lifetime) with GetHandle (reference counting). The handles
are now stored in persistent.Maps, which simplifies the
invalidation logic.
All 5 have span.URIs as keys:
symbolizeHandles
parse{Mod,Work}Handles
mod{Tidy,Why}Handles
Also, factor the functions that use these maps to have a common form:
- a fooImpl function that returns an R result and an error;
- a foo wrapper that decorates it with caching.
- a local fooResult type, defined struct{R; error} that is the cache entry.
The functions for getting/setting map entries are all inlined.
The fooHandle types are all replaced by *memoize.Handle, now that
their use is local.
No behavior change is intended.
The other uses of Bind are deleted in these CLs:
https://go-review.googlesource.com/c/tools/+/415975 (astCacheData)
https://go-review.googlesource.com/c/tools/+/415504 (actions)
Change-Id: I77cc4e828936fe171152ca13a12f7a639299e9e5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415976
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This change replaces Bind (generational lifetime) with GetHandle
(reference counting) for the cache of buildASTCache calls, as
Bind is deprecated.
Also:
- add missing commentary, particularly on the question of why
this cache is needed at all.
- remove unused field astCacheData.err
- simplify SignatureHelp to avoid unnecessary use of Declaration.
- minor simplifications to logic surrounding FindPackageFromPos
and PosTo{Decl,Field}.
Change-Id: I2b7a798b84f23856037797fa6e9ccc5595422e7c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415975
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This change uses a persistent.Map for actions, just like packages.
Actions are now reference counted rather than generational.
Also:
- note optimization opportunities.
- minor cleanups.
Change-Id: Ibbac8848a3beb3fe19056a7b160d2185155e7021
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415504
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
span.NewRange now accepts a *token.File and two token.Pos.
It is the caller's responsibility to look up the File
in the FileSet, if necessary (it usually isn't), and to
ensure the Pos values are valid. Ditto NewMappedRange.
This reduces the creep of Snapshot into functions that
have no need to know about it. Also the bug.Report call
in NewRange has been pushed up into the caller and
logically eliminated in all but one case.
I think we should aim for the invariant that functions that
operate on a single file should accept a *token.File, not
a FileSet; only functions that operate on sets of files
(e.g. type checking, analysis) should use a FileSet.
This is not always possible: some public functions
accept a FileSet only to re-lookup a single file already
known to the caller; if necessary we could provide token.File
variants of these.
This may ultimately allow us to create a new FileSet per
call to the parser, so that Files and FileSets are in
1:1 correspondance and there is no global FileSet.
(It currently grows without bound, on the order of kilobytes
per keystroke.) FileSets containing multiple files,
needed when we interact with the type checker and analysis,
may be temporarily synthesized on demand from a set of Files,
analogous to mmap'ing a few files into a blank address space.
Also:
- replace File.Position(pos).Line by File.Line(pos)
- replace pos == token.NoPos by pos.IsValid()
- avoid fishy token.Pos conversions in link.go
- other minor simplifications
Change-Id: Ia3119e0ac7e193801fbafa81c8f48acfa14e9ae4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/409935
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This speeds up workspace initialization time in DegradeClosed memory
mode from 3m to 1m by avoiding unnecessary recomputation of results.
Change-Id: Ie5df82952d50ab42125defd148136329f0d50a48
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413658
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: David Chase <drchase@google.com>
This on average reduces latency from 12ms to 4ms on internal codebase.
Updates golang/go#45686
Change-Id: Id376fcd97ce375210f2ad8b88e42f6ca283d29d3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413657
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
...so that each client doesn't have to.
Change-Id: I039c493031c5c90c4479741cf6f7572dad480808
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415502
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Before, buildPackageHandle and buildKey were mutually recursive.
Together they performed a sequential recursion over Metadata.Deps,
calling GetFile and parseGoHandle for every file, and then
finally (in postorder) binding a Handle for the type checking step.
This change inlines buildKey to make the recursion more obvious,
performs the recursion over dependencies first, followed by
the reading of Go source files for this package, in parallel.
(The IWL benchmark reports improvement but its variance is so
high I'm not sure I trust it.) Other opportunities for parallelism
are pointed out in new comments.
The Bind operation for typechecking calls dep.check for each
dependency in a separate goroutine. It no longer waits for
each one since it is only prefetching the information
that will be required during import processing, which will
block until the information becomes available.
Before, both reading and parsing appear to occur twice:
once in buildPackageHandle and once in doTypeCheck.
(Perhaps the second was a cache hits, but there's no need
to rely on a cache.)
Now, only file reading (GetFile) occurs in buildPackageHandle,
since that's all that's needed for the packageKey.
And parsing only occurs in doTypeCheck. The source.FileHandles
are plumbed through as parameters.
Also:
- move parseGoHandles to a local function, since it exists only
for buildPackageKey. It no longer parses, it only reads.
- lots of TODO comments for possible optimizations,
and typical measured times of various operations.
- remove obsolete comment re: Bind and finalizers.
Change-Id: Iad049884607b73eaa6701bdf7771f96b042142d5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411913
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Now that the workspace directory uses Snapshot.Destroy
to clean up (see https://go-review.googlesource.com/c/tools/+/414499)
there is no need for this feature.
Change-Id: Id5782273ce5030b4fb8f3b66a8d16a45a831ed91
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414500
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
This change causes (*snapshot).getWorkspaceDir to create a temporary
directory directly, rather than via the Store/Generation/Handle
mechanism. The work is done at most once per snapshot, and the
directory is deleted in Snapshot.Destroy.
This removes the last remaining use of Handle's cleanup mechanism,
which will be deleted in a follow-up.
Change-Id: I32f09a67846d9b5577cb8849b226427f86443303
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414499
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Suzy Mueller
Alan Donovan
pjw
Robert Findley
Dylan Le
Ruslan Nigmatullin