When releasing a promise, there was a theoretical race whereby a
promise's refcount could be incremented before Store.promisesMu was
acquired and the promise deleted.
We could fix this by double-checking after acquiring Store.promisesMu,
but it seemed simpler to just guard Promise.refcount with
Store.promisesMu, and skip using atomics. We already lock promisesMu
when acquiring the promise, and so locking when releasing should not
significantly affect our level of contention.
Additionally, make it a panic to call the returned release function more
than once, and document this behavior.
Change-Id: I1135b558b1f13f2b063dcaad129a432c22da0b28
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419504
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
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>
Due to shared caching, the "default" tests can run faster than other
execution modes and still retain most of the test coverage. Update the
test runner to only run the singleton tests if testing.Short() is true,
independent of GOOS.
On the other hand, we lost noticeable coverage when disabling the
Forwarded testing mode. Now that the regtests are lighter weight in
general, reenable it on the longtests builders.
While at it, clean up tests that used the server-side Options hook to
instead use Settings, since clients would configure their server via
Settings and Options can't work with a shared daemon.
Updates golang/go#39384
Change-Id: I33e8b746188d795e88841727e6b7116cd4851dc2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/418774
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
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>
Suppress the parameter hint when it would present redundant
information.
Fixesgolang/go#2361
Change-Id: I4340a903046f212f8a035eab847da665e2692f1a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419497
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Update the markdown documentation for inlay hints and fix a couple
of typos in the examples.
Change-Id: I114502a81999bc5e4f25384ab619888f3e31a731
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419496
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Update all dependencies, except sergi/go-diff.
Also tidy x/tools with -compat=1.16, as it had recently been broken at
1.16.
Change-Id: I2e6c9bf48c6bedb2dff0fa418bf588dd07918866
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419494
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
If a package is being operated on, getPackage returns that package information; otherwise nil.
Change-Id: I881056510b8d6862c274a7532fdfbc840c938468
Reviewed-on: https://go-review.googlesource.com/c/tools/+/418791
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Dylan Le <dungtuanle@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This enables some fill struct code actions for instances of structs with
type parameters.
This additionally adds a filtering mechanism to the suggested fixes in
order to account for multiple suggested fixes in the same location.
Change-Id: I98866b462b026f4c5a4897bc278f704381623f25
Reviewed-on: https://go-review.googlesource.com/c/tools/+/418415
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
token.File.LineStart panics if line < 1, but we were checking line < 0.
Surprising that this was not hit more often: it looks like we
pre-validate input except for parsed positions coming from the Go
command. It is possible that an older version of the Go command returned
invalid positions.
Also report a bug for one error condition in ToOffset: the position
returned by LineStart should always be valid.
Fixesgolang/go#54006
Change-Id: I5965af9c62976b3e00b023512df334a8de943a3d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419109
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This test was originally skipped due to deadline exceeded errors. In the
time since, we've made performance improvements, fixed races, and
altered the regtests to remove arbitrary deadlines. Unskip it to see if
it still flakes.
For golang/go#44098
For golang/go#53878
Change-Id: I06530f2bc9c6883f415dc9147cfcbf260abb2a00
Reviewed-on: https://go-review.googlesource.com/c/tools/+/418898
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
This test ran slowly sometimes. Now that we have improved performance,
elimininated arbitrary timeouts, and improved cacheability of
computed results when running with -short, I suspect this test should no
longer flake.
If it does, we can reduce its cost in other ways rather than turning it
off entirely.
Updates golang/go#48773
Updates golang/go#53878
Change-Id: I878e78117df5a1a25f4ac5f72e02f28fc078ec73
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419106
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The new implementation is based on Myers' paper, and is in the
package diff/lcs.
There is a new option newDiff, that can be set to 'old', 'new',
or 'both'. The default is 'both', although that may not be
the right choice for a release. See gopls/hooks/diff.go.
'both' runs both the old and new diff algorithm and saves some
statistics in a file in os.Tempdir(),
When (or if) the new code becomes the default, this logging (and
some internal checking) will be removed.
The new implementation has internal checking, which currently
panics. The code in gopls/hooks/diff.go tries to save an encrypted
(for privacy) version of the failing input.
The package diff/myers has not been replaced, but it could be.
Fixesgolang/go#52966
Change-Id: Id38d76ed383c4330d9373580561765b5a2412587
Reviewed-on: https://go-review.googlesource.com/c/tools/+/396855
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Peter Weinberger <pjw@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 2009, Google's open-source lawyers asked us to create the AUTHORS
file to define "The Go Authors", and the CONTRIBUTORS file was in
keeping with open source best practices of the time.
Re-reviewing our repos now in 2022, the open-source lawyers are
comfortable with source control history taking the place of the
AUTHORS file, and most open source projects no longer maintain
CONTRIBUTORS files.
To ease maintenance, remove AUTHORS and CONTRIBUTORS from all repos.
For golang/go#53961.
Change-Id: Icaaaf04cc7884b479c7fd1231c53c8bf1f349623
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419105
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Avoid panicking when allocating an array for a nil results list
by returning early.
Change-Id: I26953b5cef7832bad3006bd316d59978a5d94cbd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/418416
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Some interface types could be propagated around type graph. This does
not affect precision as the results are ultimately intersected with the
initial call graph. However, this types of propagation are not
necessary. On very large projects, this can save few seconds.
Change-Id: I73d41c082a52734f50669af19fee940725aed662
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417514
Reviewed-by: Tim King <taking@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
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>
Now that we await ongoing work during shutdown, we are seeing regtest
flakes simply due to outstanding go command invocations.
Allow more time for cleanup. If this is insufficient, we can be more
aggressive about terminating go command processes when context is
cancelled.
For golang/go#53820
Change-Id: I3df3c5510dae34cb14a6efeb02c2963a71e64f3a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417583
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dylan Le <dungtuanle@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
This is a port of CL 288632 to x/tools/go/internal/gcimporter. This
logic was ostensibly unported to avoid breaking build compatibility with
go1.12. Now that gopls no longer support 1.12, It is safe to make this
change.
Fixesgolang/go#53803
Change-Id: Ic9b4d7a60511076a83d8fa72cf7c4a3bdcab3fce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417580
Reviewed-by: Jamal Carvalho <jamal@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
This does not include writing export data in the unified IR format.
Most of this change is an import of code from the Go implementation,
with minor tweaks and gaskets added.
This does not (yet) address the registry issue mentioned in golang/go#52163.
Updates golang/go#52163.
Change-Id: I98030e6c9ff35c6ff678b8a7ce9b653b18e65e17
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412821
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
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>
This analyzer suggests fixes for unused variable errors.
In declarations it will remove the whole statement if the offending
variable is the only one declared in that statement, otherwise it will
just delete the offending variable.
In assignments it will remove the whole statement if the offending
variable is the only one assigned in that statement, otherwise it will
rename the offending variable to `_`. If the assignment RHS contains a
statement that can cause a side effect (a function call or reading from
a channel), the assignment will be removed but RHS will be preserved.
Fixesgolang/go#48975
Change-Id: I3850f1b0340cd5ae63249931df3a5403d8617080
Reviewed-on: https://go-review.googlesource.com/c/tools/+/394934
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@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>
This is used by internal/lsp/analysis/fillreturns and
internal/lsp/analysis/fillstruct. This doesn't need to be in this
analysisinternal package.
This removes the dependency path
go/analysis/internal/checker ->
internal/analysisinternal ->
internal/lsp/fuzzy
Change-Id: I5db674ca30eb06ae6ce7021397cf5530a695af4e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417418
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
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>
Cleaning the regtest sandbox sometimes fails on windows with errors that
may be spurious. Copy logic from the testing package to retry these
errors.
For golang/go#53819
Change-Id: I059fbb5e023af1cd52a5d231cd11a7c2ae72bc92
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417117
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
CL 409936 eliminated cases where we close over a Context during progress
reporting, except in one instance where it wasn't possible: the
WorkDoneWriter that must implement the io.Writer interface.
Unfortunately it contained a glaring bug that the ctx field was never
set, and the regression test for progress reporting during `go generate`
was disabled due to flakiness (golang/go#49901).
Incidentally, the fundamental problem that CL 409936 addressed may also
fix the flakiness of TestGenerateProgress.
Fix the bug, and re-enable the test.
Fixesgolang/go#53781
Change-Id: Ideb99a5525667e45d2e41fcc5078699ba1e0f1a3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417115
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Suppose a call i.Foo(j) and suppose that the initial call graph resolves
this call to just a.Foo(x). Here, i is an interface and a is concrete
type. VTA then creates flows between j and x. However, it also tries to
create flows between i and a.
The latter flows are not needed. The flow from i to a will not be
registered in the type flow graph as it does not make sense. The flow
from a to i would bake in the information from the initial call graph
which would defy the purpose of VTA.
Note that the flow a -> i doesn't occur in practice as that flow is only
created when a and i can alias.
Change-Id: Ia4087651c72a14b94d83d07bb5e6d77603842362
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416517
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Tim King <taking@google.com>
Configuration of LSP settings within the regression test runner had
become a bit of a grab-bag: some were configured via explicit fields on
EditorConfig, some via the catch-all EditorConfig.Settings field, and
others via custom RunOption implementations.
Consolidate these fields as follows:
- Add an EnvVars and Settings field, for configuring environment and
LSP settings.
- Eliminate the EditorConfig RunOption wrapper. RunOptions help build
the config.
- Remove RunOptions that just wrap a key-value settings pair. By
definition settings are user-facing and cannot change without
breaking compatibility. Therefore, our tests can and should set the
exact string keys they are using.
- Eliminate the unused SendPID option.
Also clean up some logic to change configuration.
For golang/go#39384
Change-Id: Id5d1614f139550cbc62db2bab1d1e1f545ad9393
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416876
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@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>
We were already using a token.File-agnostic position for object
positions in our references algorithm, but still relied on token.Pos for
identifying duplicate references. This breaks down when a file may have
multiple parsed representations in different packages.
While we should endeavor to eliminate duplicate parsing, our algorithms
should not rely on this for correctness. Update the reference
de-duplication to use the same position key as object search.
For golang/go#53796
Change-Id: Ic2e6c23380ea4e6b2747e4e5b45d7bfa6e656f0f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416881
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Robert Findley
Hana (Hyang-Ah) Kim
Suzy Mueller
Dylan Le
Abirdcfly
Peter Weinberger
Jamal Carvalho
Russ Cox
aarzilli
Zvonimir Pavlinovic
David Chase
Alan Donovan
Francesco Renzi