Tools version of https://go.dev/cl/435336
I believe this helped the compiler (though I don't see the benchmark).
We probably need benchmarks for tools, too.
Change-Id: I3450ecb82fcef7050f28a4cffcbc224972680f36
Reviewed-on: https://go-review.googlesource.com/c/tools/+/450678
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
original CL was https://go.dev/cl/433037
"cmd/compile: introduce "temporary" readers for more storage reuse"
Change-Id: I8989ae45b17a84451d29c7dc95b8ab2a06b6f153
Reviewed-on: https://go-review.googlesource.com/c/tools/+/449499
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: David Chase <drchase@google.com>
flattenImports used to traverse the transitive closure of all imports by
recursively visiting all imported packages. This is a lot of redundant
work, because flattening happens for every package, the recursion is not
necessary and this change removes the recursion.
Change-Id: Id5a1b3b15ef3ce8e0fc6c945b5484b3dd06bd6b6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/450755
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Florian Zenker <floriank@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The only expected use of the AfterChange helper is in the pattern
env.Await(env.AfterChange(...)). Simplify this pattern by having
AfterChange do the awaiting.
Change-Id: I832d619e8d44daae952f250e7aef69eba4e6715a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/450676
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Diagnostic suppression used the view-relative snapshot ID to avoid
publishing stale diagnostics. When the layout of views changes due to a
didChangeWorkspaceFolders notification, this suppression is broken as
snapshot IDs reset to 0. Fix this (hopefully temporarily) by introducing
a globally monotonic snapshot ID.
Fixesgolang/go#56731
Change-Id: Ib108b1436e800cf5a45fbba298c9975a2cf1d735
Reviewed-on: https://go-review.googlesource.com/c/tools/+/450275
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
TestInput is fiendishly slow and memory-hungry. Breaking it into
subtests reveals that the vast majority of that cost can be attributed
to a single test case: "testdata/a_test.go".
Update the address-space-based skip to skip only that one input, skip
it under the race detector (it is timing out on the new
"linux-amd64-longtest-race" builder), and run the remaining inputs in
parallel (to reduce test latency now that we've better identified the
culprit).
Updates golang/go#14113.
Updates golang/go#54630.
Change-Id: I105cfa173edc74692eaa41efd50ae08eeacf0d7d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/449518
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This change adds a FileSet field to Package, and uses it in
preference to Snapshot.FileSet wherever that is appropriate:
all but a handful of places.
For now, they must continue to refer to the same instance,
but once we do away with the Snapshot's cache of parsed files,
there will be no need for a global FileSet and each Package
will be able to create its own. (Some care will be required
to make sure it is always clear which package owns each
each token.Pos/ast.Node/types.Object when there are several
in play.)
Updates golang/go#56291
Change-Id: I017eb794ffb737550da6ae88462d23a8f5c1e1e7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448377
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>
This changes uses the string types to enforce better hygiene
of conversions. Fishy conversions were flagged with TODO comments.
Perhaps some of these contribute to bugs in our support for vendoring.
Also, the function formerly known as ImportPath is now UnquoteImportPath.
Change-Id: Ia6bf8749908d881fb0a62cb6c98f7dd00563a69e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/449497
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>
Classify the unaffecting/affecting vulnerabilities as we
iterate over the vulnerabilities to generate info/warning
level diagnostics, instead of iterating over the list twice.
Change-Id: Icfac41c3897d443c368f0292279ca936daffe24e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448978
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
golang.org/x/vuln/exp/govulncheck API was significantly
changed. The previous Main is gone, and we need to use
Source. The returned data types changed significantly
too.
Change-Id: Ic702ffe9a94a8ddd1867a0f2766bb49e2133d3a3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448975
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
...since we do our own parsing.
Change-Id: Id762cca408692b9535b8bb36017d6719180e5bb1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/449498
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Currently compilebench relies on installed .a files for std and
cmd, as it runs the compiler and linker directly. For the upcoming
Go 1.20, compiled .a files will not be installed. Don't rely on
them. Instead, build importcfg file and pass it to the compiler
and the linker.
Verified that this approach still works with previous versions of
Go (1.19 and 1.18).
For golang/go#47257.
Change-Id: Ie0eb9541fb995649e5b68d4481a5acfbdfe8f2a7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448118
Reviewed-by: Michael Pratt <mpratt@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Update golang.org/x dependencies to their latest tagged versions.
Once this CL is submitted, and post-submit testing succeeds on all
first-class ports across all supported Go versions, this repository
will be tagged with its next minor version.
Change-Id: Ie52140f20343bd6dd2b73662fce64c8065f5a80b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/449096
Auto-Submit: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Gopher Robot <gobot@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
The only explanation I can think of for the failure in
https://go.dev/issue/49387#issuecomment-1303979877 is that maybe the
idle timeout started as soon as conn1 was closed, without waiting for
conn2 to be closed. That might be possible if the connection returned
by Dial was still in the server's accept queue, but never actually
accepted.
To eliminate that possibility, we can send an RPC on that connection
and wait for a response, as we already do with conn1. Since the conn1
RPC succeeded, we know that the connection is non-idle, conn2 should
be accepted, and the request on conn2 should succeed unconditionally.
Fixesgolang/go#49387 (hopefully for real this time).
Change-Id: Ie3e74f91d322223d82c000fdf1f3a0ed08afd20d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448096
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Add [Lapce](https://lapce.dev) Go plugin to `gopls` documentation
Change-Id: I58ec42d69708b519cfba3de1cdee269ffecdbbc4
GitHub-Last-Rev: 37762df491e6e7a5797606025357fcfed28be56d
GitHub-Pull-Request: golang/tools#413
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448235
Auto-Submit: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Fix the test failure demonstrated in the following failed builder:
https://build.golang.org/log/d0511c583201e8701e72066985ebf950d9f5511d
It should be OK to set multiple keys in the validated map. Support this
by keeping track of seen and deletion clock time. There are still
potential problems with this analysis (specifically, if a map is
constructed via SetAll), but we ignore those problems for now.
Change-Id: I5940d25f18afe31e13bc71f74d4eea7d737d593d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448696
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
I felt a bit confused on my first reading of the docs for using `go work`.
It wasn't clear to me if the `tools` argument in `go work use tools tools/gopls` was an alias or a directory name, so I thought this might make it very clear to understand for first time users.
Change-Id: I9c5a04a8928207b53acfb36ce7add8ca5f033d46
GitHub-Last-Rev: 49e125d83e40f06239f3a24c92f16258a25305c3
GitHub-Pull-Request: golang/tools#409
Reviewed-on: https://go-review.googlesource.com/c/tools/+/441415
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
I meant to do this in the first CL, but was prevented by
a bug which I have since reported and linked to from the code.
Change-Id: I651e728c535cdeb0885eae4d510fda3c24518dcf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448376
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 change will make it possible to do semantic token regtests.
Change-Id: I9963c60f61af30f973a2ee4cd32aaa5545bdc4ec
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448296
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Peter Weinberger <pjw@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Coverage.go computes the test coverage from running all the gopls tests.
This CL accounts for the changed source tree (internal/lsp is gone)
and new actions returned by go test -json ('pause' and 'cont').
Change-Id: I970b3ec107746ce02e3dcdcad9f8c19cffad8d11
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448295
Run-TryBot: Peter Weinberger <pjw@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This test is flaky, likely due to some race in the
experimentalWorkspaceModule logic. Since we're about to delete support
for that experimental feature, simply skip the test to stop the flakes.
Leave the test as an artifact, as it will be deleted as part of the
clean up of experimentalWorkspaceModule. No need to delete it before
then.
Updates golang/go#55331Fixesgolang/go#55923
Change-Id: Ic17485e42e335459df462af00a2088812ecfb5f4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448116
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Robert Findley <rfindley@google.com>
When awaiting diagnostics, we should almost always wrap expectations in
a OnceMet(precondition, ...), so that tests do not hang indefinitely if
the diagnostic pass completes and the expectations are still not met.
Before this change, the user must be careful to pass in the correct
precondition (or combination of preconditions), else they may be
susceptible to races.
This change adds an AllOf combinator, and uses it to define a new
DoneDiagnosingChanges expectation that waits for all anticipated
diagnostic passes to complete. This should fix the race in
TestUnknownRevision.
We should apply a similar transformation throughout the regression test
suites. To make this easier, add a shorter AfterChange helper that
implements the common pattern.
Fixesgolang/go#55070
Change-Id: Ie0e3c4701fba7b1d10de6b43d776562d198ffac9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448115
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The old code based on packages.Visit traversed in a
deterministic order, and didn't stop when it found its
target (the 'return false' only prunes that subtree).
This CL replaces it with a precomputation of the
PkgPath-to-*Package mapping.
The performance difference is small for this test but
it nearly dominates on a larger input (e.g. k8s).
Example code shouldn't steer users into asymptotic traps.
Change-Id: I19f4fc2c25da3d2ae00090704df30a54d8516bf5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/447958
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
While checking for changes to go.mod files in the importsState, there is
no reason for special handling based on workspace mode: we can simply
hash all active modfiles. This moves us towards an improved API for the
workspace: it should simply be responsible for tracking active modfiles.
This also incidentally avoids the panic reported in golang/go#55837.
Fixesgolang/go#55837
Change-Id: I8cb345d1689be12382683186afe3f9addb19d467
Reviewed-on: https://go-review.googlesource.com/c/tools/+/447956
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Update selected dependencies following the v0.10.0 release, excluding
sergi/go-diff and x/vuln.
Gofumpt@v0.4.0 requires go1.18, so link it selectively following the
pattern of staticcheck. While at it, clean up some things related to the
wiring of staticcheck and gofumpt support. Notably, in VS Code error
messages do not support formatting such as newlines or tabs.
Add a test for the conditional Gofumpt support.
For golang/go#56211
Change-Id: Id09fdcc30ad83c0ace11b0dea9a5556a6461d552
Reviewed-on: https://go-review.googlesource.com/c/tools/+/446736
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change adds an internal API for marshalling and
unmarshalling a types.Package to "shallow" export data,
which does not index packages other than the main one.
The import function accepts a function that loads symbols
on demand (e.g. by recursively reading export data for
indirect dependencies).
The CL includes a test that the entire standard
library can be type-checked using shallow data.
Also:
- break dependency on go/ast.
- narrow the name and type of qualifiedObject.
- add (test) dependency on errgroup, and tidy go.mod.
Change-Id: I92d31efd343cf5dd6fca6d7b918a23749e2d1e83
Reviewed-on: https://go-review.googlesource.com/c/tools/+/447737
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
We plan to add experimental features to this package for
use by gopls, but the directory structure makes this
tricky using the "internal directory" mechanism.
Change-Id: Ib842c0b100b167f6978c6ff783ea0e5d0704b4a7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/447955
Reviewed-by: Robert Findley <rfindley@google.com>
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>
Clean up some broken links and stale documentation in gopls/README.md,
and add new documentation for the gopls release policy.
Fixesgolang/go#55267
Change-Id: I9c7ed1f1d3949025f3c02edb69b475cf34f214eb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/446863
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Remove the internal guard preventing the loopclosure analyzer from
checking parallel subtests.
Also, improve the accuracy of the parallel subtest check:
- only consider statements after the final labeled statement in the
subtest body
- verify that the *testing.T value for which T.Parallel() is invoked
matches the argument to the subtest literal
Fixesgolang/go#55972
Change-Id: Ia2d9e08dfa88b5e31a9151872025272560d4b5e8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/447256
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Tim King <taking@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Prior to this CL we already shut down a jsonrpc2_v2.Conn when its
Reader breaks, which we expect to be the common shutdown path.
However, with certain kinds of connections (notably those over
stdin+stdout), it is possible for the Writer side to fail while
the Reader remains working.
If the Writer has failed, we have no way to return the required
Response messages for incoming calls, nor to write new Request
messages of our own. Since we have no way to return a response,
we will now mark those incoming calls as canceled.
However, even if the Writer has failed we may still be able to read
the responses for any outgoing calls that are already in flight. When
our in-flight calls complete, we could in theory even continue to
process Notification messages from the Reader; however, those are
unlikely to be useful with half the connection broken. It seems more
helpful — and less surprising — to go ahead and shut down the
connection completely when it becomes idle.
This is a redo of CL 446315, with additional fixes for bugs exposed on
the -race builders and some extra code cleanup from the process of
diagnosing those bugs.
Updates golang/go#46520.
Updates golang/go#49387.
Change-Id: I746409a7aa2c22d5651448ed0135b5ac21a9808e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/447035
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This CL includes some clarifications while trying to
understand the performance of the initial workspace load
and analysis. No significant behavior changes.
Server.diagnose:
- Factor the four copies of the logic for dealing
with diagnostics and errors.
- Make the ActivePackages blocking step explicit.
Previously mod.Diagnostics would do this implicitly,
making it look more expensive than it is.
Server.addFolders:
- eliminate TODO. The logic is not in fact fishy.
- use informative names and comments for WaitGroups.
- use a channel in place of a non-counting WaitGroup.
Also, give pkg a String method.
Change-Id: Ia3eff4e784fc04796b636a4635abdfe8ca4e7b5a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/445897
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Rename (*connection).AddFile to openFile, which is more accurate, and
fix a deadlock resulting from holding a Client lock while issuing a
Server request.
Fixesgolang/go#56450
Change-Id: Ie6f34613e1e10e3274c3e6728b12f77e3a523b89
Reviewed-on: https://go-review.googlesource.com/c/tools/+/446856
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Fix a panic during completion on variables of type *error. As a
predeclared type, the error type has nil package. Fix the crash
resulting from this oversight, as well as a related crash in the tests
analyzer, from which the new completion code was adapted.
Fixesgolang/go#56505
Change-Id: I0707924d0666b238821fd14b6fc34639cc7a9c53
Reviewed-on: https://go-review.googlesource.com/c/tools/+/446815
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Simplify the api-diff implementation to use `go run` and cmp.Diff. The
latter is more maintainable and produces more readable output, due to
supporting line diffs for multi-line strings.
For golang/go#54459
Change-Id: I11c00e9728ce241aef8f9828f3840b4202294a9a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/444799
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Prior to this CL we already shut down a jsonrpc2_v2.Conn when its
Reader breaks, which we expect to be the common shutdown path.
However, with certain kinds of connections (notably those over
stdin+stdout), it is possible for the Writer side to fail while
the Reader remains working.
If the Writer has failed, we have no way to return the required
Response messages for incoming calls, nor to write new Request
messages of our own. Since we have no way to return a response,
we will now mark those incoming calls as canceled.
However, even if the Writer has failed we may still be able to read
the responses for any outgoing calls that are already in flight. When
our in-flight calls complete, we could in theory even continue to
process Notification messages from the Reader; however, those are
unlikely to be useful with half the connection broken. It seems more
helpful — and less surprising — to go ahead and shut down the
connection completely when it becomes idle.
Updates golang/go#46520.
Updates golang/go#49387.
Change-Id: I713f172ca7031f4211da321560fe7eae57960a48
Reviewed-on: https://go-review.googlesource.com/c/tools/+/446315
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Memory profiles show heavy allocation for the stack
and the function closure of FindDeclAndField.
This change moves both outside the loop, reducing
this function's fraction of allocation from 6.7%
before to 5.0% after, and reducing running time
by 3-7% on these benchmarks.
before
BenchmarkStructCompletion/completion-8 100 10432280 ns/op
BenchmarkImportCompletion/completion-8 1350 921785 ns/op
BenchmarkSliceCompletion/completion-8 100 10876852 ns/op
BenchmarkFuncDeepCompletion/completion-8 142 7136768 ns/op
BenchmarkCompletionFollowingEdit/completion-8 63 21267031 ns/op
After
BenchmarkStructCompletion/completion-8 100 10030458 ns/op
BenchmarkImportCompletion/completion-8 1311 918306 ns/op
BenchmarkSliceCompletion/completion-8 100 10179937 ns/op
BenchmarkFuncDeepCompletion/completion-8 150 6986303 ns/op
BenchmarkCompletionFollowingEdit/completion-8 63 20575987 ns/op
Change-Id: Ia459e41ecf20851ff4544f76ad7b415a24606cd1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/446185
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Serve had a misleading name and signature: it did not actually block
on serving the connection, and never returned a non-nil error.
Updates golang/go#56281.
Change-Id: Ia6df0ba20066811b0551df3b3267dff2fffd7881
Reviewed-on: https://go-review.googlesource.com/c/tools/+/443678
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Also make (*Connection).Close safe to call from Bind.
A jsonrpc2_v2.Server has no good way to report an error from Bind.
If the Server saves the error to return from its own Wait method, that
might not ever actually happen: Wait waits for in-flight connections
to complete, but if some existing connection stays up then Wait will
not return.
If the Server goes ahead with establishing the connection and installs
its own Handler, that Handler needs to decide whether to serve the
error from Bind or something more opaque, and at that point Bind may
as well return a handler that makes that choice more precisely.
If the Server merely logs the error and closes the Connection, then
the Bind method itself may as well do that directly too.
It seems to me that the only winning move is not to play. Only Bind is
in a position to decide how to handle its errors appropriately, so it
should not return them to the Server.
Updates golang/go#56281.
Change-Id: I07dc43ddf31253ce23da21a92d2b6c0f8d4b3afe
Reviewed-on: https://go-review.googlesource.com/c/tools/+/443677
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Also implement and use the Shutdown method, which was mentioned in a
doc comment in CL 292169 but not actually present at that time.
With proper synchronization, we don't need heuristics to determine
whether an error is due to a connection or listener being closed.
We know whether we have called Close (and why), and we can assume
that if we have called Close then that is probably the reason for
any returned error.
Fixesgolang/go#56281.
Change-Id: I5e0ed7db0f736ca8df8cd8cf556b674e7a863a69
Reviewed-on: https://go-review.googlesource.com/c/tools/+/443675
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Prior to this CL, (*Server).run only filters out inactive connections
when it accepts a new connection. If existing connections complete,
their associated resources can't be garbage-collected until either the
next connection is accepted or the Listener is closed.
This change moves the open-connection accounting to an explicit hook
passed to newConnection, eliminating the need to call Wait entirely.
For golang/go#46047
Change-Id: I3732cb463fcea0c142f17f2b1510fdfd2dbc81da
Reviewed-on: https://go-review.googlesource.com/c/tools/+/388774
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change fixes the semantics of Close to actually wait for
in-flight requests before closing the ReadWriteCloser. (Previously,
the Close method closed the ReadWriteCloser immediately, which I
suspect is what led to many of the failures observed in
golang/go#49387 and golang/go#46520.)
It achieves this by explicitly tracking the number of in-flight
requests, including requests with pending async responses, and
explicitly rejecting new Call requests (while keeping the read loop
open!) once Close has begun.
To make it easier for me to reason about the request lifetimes, I
reduced the number of long-lived goroutines from three to just one
(the Read loop), with an additional Handler goroutine that runs only
while the Handler queue is non-empty. Now, it is clearer (I hope!)
that the number of in-flight async requests strictly decreases
after Close has begun, even though the Read goroutine continues
to read requests (and, importantly, responses) and to forward
Notifications to the preempter.
For golang/go#49387
For golang/go#46520
Change-Id: Idf5960f848108a7ced78c5382099c8692e9b181e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/388134
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
In a type such as
type X interface { m() []*interface { X } }
the traversal never encounters the named type X in the result of m
since Interface.Methods expands it to a set of methods, one that
includes m, causing the traversal to get stuck.
This change uses an alternative, shallow hash function on the types
of interface methods to avoid the possibility of getting stuck in
such a cycle.
(An earlier draft used a stack of interface types to detect cycles,
but the logic of caching made this approach quite tricky.)
Fixesgolang/go#56048Fixesgolang/go#26863
Change-Id: I28a604e6affae5dfdd05a62e405d49a3efc8d709
Reviewed-on: https://go-review.googlesource.com/c/tools/+/439117
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Tim King <taking@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
When the default filter glob changed to **/node_modules,
the performance of BenchmarkWorkspaceSymbols was observed
to degrade by an astonishing 20%.
(CPU profiles of the benchmark reported that the Disallow
functions percentage had increased only slightly, but these
measures are misleading since the benchmark has a very
CPU-intensive set-up step, so all the percentages are
quotients of this figure, masking their relative importance
to the small region during which the benchmark timer is
running.)
This change removes the unnecessary ^.* prefix from the
generated regular expression. Really the regexp package
ought to do this.
Also, minor cleanups and tweaks to the surrounding code.
Change-Id: I806aad810ce2e7bbfb2c9b04009d8db752a3b10d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/446177
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
David Chase
Florian Zenker
Robert Findley
Bryan C. Mills
Alan Donovan
Sasha Melentyev
Hana (Hyang-Ah) Kim
Cherry Mui
Gopher Robot
Arsen Musayelyan
Angus Dippenaar
pjw