1
0
mirror of https://github.com/golang/go synced 2024-11-18 13:34:41 -07:00
Commit Graph

4791 Commits

Author SHA1 Message Date
Rob Findley
405595e0b5 internal/lsp/fake: be more careful when closing the workspace
Closing the workspace has frequently been failing on Windows, due to
file locks held by the go command.

This change makes several tests more careful to check errors when
closing resources, and defers closing the regtest workspaces until the
entire test suite completes, at which point it is much more likely that
closing the workspace will succeed.

If this change results in test flakes on Windows, we should temporarily
demote errors in regtest.Runner.Close to a t.Log.

Updates golang/go#38490

Change-Id: Ibd2f7dd0e0e2faecfa0ca8c60237fc72e64f6719
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228231
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-04-16 19:25:41 +00:00
pjw
ea51688515 go/packages: fix misbehavior when an overlay changes the package name
The new code checks for inconsistencies between the package name on disk
and the package name in overlays (as might happen when editing).

This Cl is meant to fix https://github.com/golang/go/issues/38328

Change-Id: If56b542775c216b4e0ed8ce91d6721c672bff792
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228236
Run-TryBot: Peter Weinberger <pjw@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
2020-04-16 18:54:13 +00:00
Rebecca Stambler
5744cfde56 internal/lsp: fix imports issue with duplicate package decl
We shouldn't apply formatting fixes when we are
organizing imports.

Fixes golang/go#38412

Change-Id: I082f4d9a7d1d1dd571304733c287b0d5e90ea448
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228264
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-16 06:17:24 +00:00
Ian Cottrell
9c19d0db36 internal/telemetry: delete the unit package
It is not currently used at all, and is probably not the right approach.
Now we can easily add type safe keys we can add ones for types that know
their unit rather than storing it separately.

Change-Id: If393ad1df76033cff571c46f98cde8decb722b9c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228265
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-16 04:05:50 +00:00
Ian Cottrell
18395615f2 internal/telemetry: pack strings efficiently into tags
TagOfString() deconstructs a string into length and data pointer using the
unsafe package, and then stores the length into Tag.packed and the pointer into
Tag.untyped, which it can do without allocations.
Tag.UnpackString can reconstruct a string on access also using the unsafe
package to rebuild the string header.

This makes tag significantly smaller, which in turn makes things a faster

name                old time/op    new time/op    delta
/Baseline-8            160ns ± 6%     158ns ± 8%     ~
/StdLog-8             7.50µs ± 8%    7.47µs ±20%     ~
/LogNoExporter-8      1.13µs ± 7%    1.04µs ± 2%   -8.08%
/TraceNoExporter-8    3.61µs ±15%    2.96µs ± 5%  -18.08%
/StatsNoExporter-8    1.65µs ± 7%    1.39µs ± 7%  -16.14%
/LogNoop-8            4.43µs ±14%    4.05µs ± 7%     ~
/TraceNoop-8          10.9µs ± 5%    10.1µs ± 8%     ~
/StatsNoop-8          8.08µs ± 3%    7.42µs ±13%     ~
/Log-8                16.2µs ±14%    13.4µs ± 3%  -17.10%
/Trace-8              61.7µs ±22%    53.6µs ± 7%     ~
/Stats-8              11.3µs ±10%     9.5µs ± 4%  -15.56%

name                old alloc/op   new alloc/op   delta
/Baseline-8            0.00B          0.00B          ~
/StdLog-8               552B ± 0%      552B ± 0%     ~
/LogNoExporter-8       0.00B          0.00B          ~
/TraceNoExporter-8    3.58kB ± 0%    2.82kB ± 0%  -21.43%
/StatsNoExporter-8     0.00B          0.00B          ~
/LogNoop-8            3.58kB ± 0%    2.82kB ± 0%  -21.43%
/TraceNoop-8          11.5kB ± 0%     9.2kB ± 0%  -20.00%
/StatsNoop-8          7.17kB ± 0%    5.63kB ± 0%  -21.43%
/Log-8                3.58kB ± 0%    2.82kB ± 0%  -21.43%
/Trace-8              27.9kB ± 0%    23.6kB ± 0%  -15.60%
/Stats-8              7.17kB ± 0%    5.63kB ± 0%  -21.43%

name                old allocs/op  new allocs/op  delta
/Baseline-8             0.00           0.00          ~
/StdLog-8               30.0 ± 0%      30.0 ± 0%     ~
/LogNoExporter-8        0.00           0.00          ~
/TraceNoExporter-8      16.0 ± 0%      16.0 ± 0%     ~
/StatsNoExporter-8      0.00           0.00          ~
/LogNoop-8              16.0 ± 0%      16.0 ± 0%     ~
/TraceNoop-8            64.0 ± 0%      64.0 ± 0%     ~
/StatsNoop-8            32.0 ± 0%      32.0 ± 0%     ~
/Log-8                  16.0 ± 0%      16.0 ± 0%     ~
/Trace-8                 384 ± 0%       384 ± 0%     ~
/Stats-8                32.0 ± 0%      32.0 ± 0%     ~

Change-Id: If5c369f138b60435f1aa74120aa3c1b68baae402
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228234
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
2020-04-16 04:05:44 +00:00
Ian Cottrell
be55493b88 internal/telemetry: add tag value formatting to the Key
This means that format functions don't need an exhaustive list of the
key types to work correctly.

Change-Id: Iee17b225a0ecf82eb3f50d6341baf677655efc0d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228233
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
2020-04-16 04:05:32 +00:00
Ian Cottrell
bd061c738e internal/telemetry: expose the TagOf* and Unpack* methods
These allow tags to be constructed and used from outside
the event package.
This makes it easy for users of these APIs to write their own
implementations of Key.

Change-Id: Ic3320a80f297bbe1d4cd6d9beafbe13ebbace398
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228232
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
2020-04-16 04:05:21 +00:00
Rebecca Stambler
5d8e1897c7 internal/lsp: fix erroneous documentation for struct fields
The fix is just a missing return.
Also clean up a staticcheck thing, regenerate the golden files.

Fixes golang/go#38417

Change-Id: I290b63df9d97211c59d6399fda7a273bc700fbdb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228297
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-15 03:45:06 +00:00
Dominik Honnef
92398ad77b go/analysis/analysistest: print unified diff for test failures
Instead of printing the golden file and actual output using %#v, print
a unified diff. That way, instead of a possibly long, hard to decipher
error like this

        analysistest.go:134: suggested fixes failed for /home/dominikh/prj/src/honnef.co/go/tools/simple/testdata/src/CheckDeclareAssign/LintDeclareAssign.go, expected:
            "package pkg\n\nfunc fn() {\n\tvar x int = 1\n\t_ = x\n\n\tvar y interface{} = 1\n\t_ = y\n\n\tif true {\n\t\tvar x string = \"a\"\n\t\t_ = x\n\t}\n\n\tvar z []string\n\tz = append(z, \"\")\n\t_ = z\n\n\tvar f func()\n\tf = func() { f() }\n\t_ = f\n\n\tvar a int\n\ta = 1\n\ta = 2\n\t_ = a\n\n\tvar b int\n\tb = 1\n\t// do stuff\n\tb = 2\n\t_ = b\n}\n"
            got:
            "package pkg\n\nfunc fn() {\n\tvar x int = 1\n\t_ = x\n\n\tvar y interface{} = 1\n\t_ = y\n\n\tif true {\n\t\tvar x string = \"\"\n\t\t_ = x\n\t}\n\n\tvar z []string\n\tz = append(z, \"\")\n\t_ = z\n\n\tvar f func()\n\tf = func() { f() }\n\t_ = f\n\n\tvar a int\n\ta = 1\n\ta = 2\n\t_ = a\n\n\tvar b int\n\tb = 1\n\t// do stuff\n\tb = 2\n\t_ = b\n}\n"

we get a much more concise and readable diff:

        analysistest.go:133: suggested fixes failed for /home/dominikh/prj/src/honnef.co/go/tools/simple/testdata/src/CheckDeclareAssign/LintDeclareAssign.go:
            --- /home/dominikh/prj/src/honnef.co/go/tools/simple/testdata/src/CheckDeclareAssign/LintDeclareAssign.go.golden
            +++ actual
            @@ -8,7 +8,7 @@
             	_ = y

             	if true {
            -		var x string = "a"
            +		var x string = ""
             		_ = x
             	}

One downside of this approach is that unprintable characters won't be
visible in the diff. However, the vast majority of Go code does not
contain unprintable characters, and an even smaller amount of
suggested fixes affect unprintable characters. It is worth optimizing
readability for the common case.

Change-Id: I857aa6b6ee719f0fb018d5007eb162882e79cc25
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228118
Run-TryBot: Dominik Honnef <dominik@honnef.co>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
2020-04-15 00:09:39 +00:00
Ian Cottrell
33e937220d internal/jsonrpc2: simplify request even futher
It does not need to expose wire level fields, especially not by embedding.
It also no longer needs to know what Conn it came from.

Change-Id: I1aede2baaa2daa40da4e7a1278b2eaada25b2310
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227919
Reviewed-by: Jonathan Amsterdam <jba@google.com>
2020-04-14 21:18:25 +00:00
Ian Cottrell
ce2627f346 internal/jsonrpc2: remove the OnReply callbacks
The same behavior can now be achieved by wrapping the Replier as we traverse the
handler stack instead.
Request is no longer mutated during handler invocation.

Change-Id: I2213de500f39e048f3f80161e234f3ae30464d70
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227918
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-14 21:18:03 +00:00
Rob Findley
6a72e3782c internal/lsp/fake: don't lock while calling formatting
While trying to be pragmatic, I simply locked the fake.Editor while
calling textDocument/formatting. This did indeed manifest later on as a
deadlock, so instead we release the lock but fail if the buffer version
changes while awaiting the formatting call.

Change-Id: I0ca24a502f3118e76de306a016449bbe665e70e4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228230
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-04-14 20:55:25 +00:00
Rob Findley
3bb7580c69 internal/lsp/fake: correctly configure the fake editor
Configuration was not being set correctly, because the configuration
response was not honoring the configuration section order.

Change-Id: I8418535b45e6a24fd8f0605d58cb370e22664f17
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228257
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-04-14 20:27:33 +00:00
Rob Findley
912958979a internal/lsp/regtest: put testing.T back in the test func signature
In https://golang.org/cl/225157, I removed the redundant T and Context
parameters from the regtest func signature in an effort to reduce
confusion. In hindsight, removing the testing.T did not reduce
confusion, because there is a T in the test signature anyway (and now
it's the wrong T!).

Change-Id: I2e84009e9cb33cd51055012cfef9fe4987e1b9bd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228229
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Paul Jolly <paul@myitcv.org.uk>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-04-14 18:42:46 +00:00
Yasuhiro Matsumoto
0037cb7812 all: Fix spelling of Marshaling.
The single "l" form is used throughout the adjacent tools package
and the Go standard library.

Change-Id: I88c3530ef9d3b1354895d342e39403fa20ccd4d1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228237
Reviewed-by: Ian Cottrell <iancottrell@google.com>
2020-04-14 13:15:30 +00:00
Dominik Honnef
332987a829 gopls/internal/hooks: don't run staticcheck's SA5011
SA5011 relies on facts from dependencies to avoid false positives.
However, gopls currently only loads export data for dependencies, it
does not compute facts.

SA5011 is unlike other analyzers in staticcheck, which may produce
false negatives if facts are missing, but no false positives.

Change-Id: I5063b701bbedca7b09d1894997f8c574fa497939
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228119
Run-TryBot: Dominik Honnef <dominik@honnef.co>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-04-14 03:22:29 +00:00
Rob Findley
a4a177c7d7 internal/lsp/diff/difftest: ignore for GOOS=illumos
illumos has a different copy of the `diff` executable, causing this test
to fail. Ignore it for this GOOS.

Updates golang/go#38414

Change-Id: I45ec5fcd9fd332977349bb6ba33d9ed09417a023
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228199
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-04-14 02:36:50 +00:00
Dominik Honnef
ae52e4b557 go/analysis/analysistest: give up if we can't find golden file
Change-Id: I6b3b5aa573edc4bcab50a262306b47cf0665f957
Reviewed-on: https://go-review.googlesource.com/c/tools/+/228117
Reviewed-by: Michael Matloob <matloob@golang.org>
2020-04-14 00:10:08 +00:00
Rob Findley
07bb9fb2f9 internal/proxydir: add an internal package for file-based proxies
Both packagestest and the gopls regtests need to write module data to
the filesystem in proxy structure. Since this seems like a common and
self-contained concerned, factor this out into a shared package.

Change-Id: I5275dbc0cd7b13290061e8bb559d6dd287fbb275
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227841
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-04-13 22:35:07 +00:00
Michael Matloob
250b2131eb go/loader: remove check on ioutil.TestTempFile in TestStdlib
The check was there to test that the loader worked with a cycle between the
three augmented packages compress/bzip2, io/ioutil, and regexp, because of
dependencies between each of the packages' tests and the next package.

The test in io/ioutil that had the dependency that created the cycle no longer
exists in that package (it's been moved out into the ioutil_test xtest).
Remove the check for that package.

Unfortunately this means that the cycle that was being checked for before is no
longer being checked. That could be fixed in a future change by creating three
fake packages in testdata that have this relationship.

Fixes golang/go#38318
Updates golang/go#19152

Change-Id: I8ce88102a5505d8edf8d54d2098c85a8d3cd622f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227772
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
2020-04-13 16:19:37 +00:00
Ian Cottrell
1f08ef6002 internal/jsonrpc2: split reply from request
reply is now passed to handlers separately from request, which allows it to be
substituted by handlers.
This also makes the handler signature much closer to http (which has
ResponseWriter)

Change-Id: I12be2e8e8b9bd508982ba43c9092709429284eaf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227839
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-13 01:58:12 +00:00
Ian Cottrell
b854406a88 internal/jsonrpc2: make the wire structures private
The wire structures do not need to be public, and making them so might make it
harder to keep the package correct without breaking changes if the protocol
changes in the future.

Change-Id: I03a5618c63c9f7691183d4285f88a177ccdd3b35
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227838
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-13 01:57:29 +00:00
Ian Cottrell
2595e72532 internal/jsonrpc2: replace NewErrorf with fmt.Errorf
We utilize error wrapping to recover the error codes when needed.
The code constants are also replaced by fully declared errors with
human readable messages.

Change-Id: I8edeb05f5028e99966e4ca28151f644f008d4e7d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227837
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-13 01:57:14 +00:00
Rebecca Stambler
79a7a3126e internal/lsp: add an extra bounds check to avoid nil pointers
A nil pointer was reported to the golang-tools group (see
https://groups.google.com/g/golang-tools/c/JrNTz8I6ifo/m/tcJRpek-AAAJ).
I think this bounds check should address it.

Updates golang/go#34433

Change-Id: I87352c269c65c844c86ebe9ee3fd2d041cc49ee9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227770
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-10 19:49:07 +00:00
Joe Tsai
e3f0bd94ad go/ast/inspector: impose maximum capacity
Consider a pathological file that contains a single string that
is 1GiB large. The current heuristic for allocating the
events would create a slice that is 330M elements large, when
only a small number of events would ever be needed. This may easily
cause the analysis to OOM on the pathological file.

Alleviate the consequences of a pathological file by imposing
a maximum number of events.

Change-Id: I937bb8a3f479866631c4895ac1e729458f6e5e34
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227861
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
2020-04-10 18:16:43 +00:00
Ian Cottrell
ae9902aceb internal/lsp: remove the CallCanceller
This required changing the jsonrpc.Conn.Call signature to also return the
request ID so it can be cancelled.
The protocol package now declares the Call function which wrapps up
Conn.Call and then sends a cancel message if the context was
cancelled during the call.
There is a small chance that a context can be cancelled on a
request that has already completed, but it is safe to do so.

Change-Id: Ic8040c193e1dd4ef376ad21194b1d0ea82145976
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227558
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-10 13:26:12 +00:00
Ian Cottrell
c92aeb7438 internal/lsp: improve ID formatting
Replace the String method with a Format method so we can use it for extra
formats.
Add some tests to make sure it is all correct

Change-Id: I39f361ffba036fad99c93f8c0944164f7cf199ec
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227486
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-10 13:25:36 +00:00
Rebecca Stambler
3bd20875a2 internal/lsp/cache: hide type errors if we fix up the AST
I was curious about why were logging errors during type-checking in
tests, and the answer turned out to be a bit more sinister than I
expected. We were getting type error messages without filepaths, so I
tried to reproduce it in the playground and wasn't able to. I realized
that these errors were coming from were coming from the "fixed" version
of the AST that we pass to the type checker.

Adding fake positions to our fake Cond statements trivially fixes the
logging issue, but it does nothing to handle the fact that the error
makes no sense to the user - because it applies to something that's not
in the source code. I figured we have two options: (1) skip type errors
for all packages with "fixed" ASTs, or (2) add something to the error
messages to indicate that the source code may not match. Starting with
(1) here, and if it becomes a problem, we can move to 2. All ASTs that
we fix have *ast.BadExpr in them, meaning that, by definition they have
parse errors which we will preferentially show those errors to users in
diagnostics (so I'm not sure how to test this).

Change-Id: I17733968aa15f989cdd3e4e7261c4f4fe9b97495
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227557
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-04-10 04:07:51 +00:00
Rebecca Stambler
c08edad6b3 internal/lsp: linkify IP addresses in textDocument/documentLink
Found golang/go#18824, which helped me understand what to do to fix this
issue. Also, changed some of the structure of the code to propagate
errors up to the top-level functions. I think this will help us deal
with actual errors vs. ignorable errors.

Didn't add tests because of golang/go#35880. Just prioritized that issue
for gopls/v0.5.0 - it's been biting us a lot lately.

Fixes golang/go#38285

Change-Id: I6962462818becb1bcedde4a5be3a2060e71c9389
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227559
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-04-10 03:56:59 +00:00
pjw
700752c244 internal/lsp/regtest: add test for issue 32149 (wrong package)
Gopls gets confused about the package if the package name is edited.
(See golang.org/issues/32149) This skipped test will succeed as long
as the bug is present.

Change-Id: Ic99ceda133f92f306baf94e2f8ad0381ed565814
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227760
Run-TryBot: Peter Weinberger <pjw@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-04-09 21:04:53 +00:00
Rob Findley
bc09a2cac9 internal/lsp/regtest: output gopls logs on test failure
Wrap the regtest test servers to capture jsonrpc2 logs, so that they can
be printed on test failure.

There was a little bit of complication to implement this in the 'Shared'
execution mode, and since we're not really using this mode I just
deleted it.

Updates golang/go#36897
Updates golang/go#37318

Change-Id: Ic0107c3f317850ae3beb760fc94ae474e647cb78
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226957
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
2020-04-09 19:58:30 +00:00
Rob Findley
97c4fbe514 internal/lsp/protocol: make loggingStream log writes concurrency-safe
Per the documentation for jsonrpc2.Stream Write must be safe for
concurrent use, but this isn't the case for the loggingStream.

Guard it with a mutex.

Change-Id: I384892b90cef950d518089421d05cf8040c6b233
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227487
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
2020-04-09 19:31:31 +00:00
Dmitri Shuralyov
77362c5149 go/packages: replace darwin with netbsd in TestSizes
The darwin/386 port has been removed per golang.org/issue/37610.
TestSizes needs an operating system that has both amd64 and 386 ports,
so darwin no longer qualifies. NetBSD has had its 386 port restored
recently in golang/go#31726, so it can be used instead.

Fixes golang/go#38319.
For golang/go#37610.

Change-Id: I37ce6d86ca3ddad43e9294e0de66f36091cba54a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227552
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
2020-04-09 17:04:54 +00:00
Ian Cottrell
9ee5ef7a2c internal/jsonrpc2: remove Direction
It is no longer used after the changes to the logging system.

Change-Id: I7b96fb8297eb66f2ebad67c74c82fa7ed96c3139
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227485
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-08 13:21:56 +00:00
Ian Cottrell
888a00fb34 internal/telemetry: a faster logging exporter
Moved printing directly inside the exporter,
which yields a massive reduction in allocations.

name                old time/op    new time/op    delta
/Log-8                41.7µs ± 1%    13.6µs ± 2%  -67.48%

name                old alloc/op   new alloc/op   delta
/Log-8                20.9kB ± 0%     3.6kB ± 0%  -82.86%

name                old allocs/op  new allocs/op  delta
/Log-8                   286 ± 0%        16 ± 0%  -94.41%

Change-Id: Ieafd644683d98d24978c8be061e6632dd8ef113e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227302
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
2020-04-08 13:21:36 +00:00
Ian Cottrell
6a75126720 internal/lsp: make tag iteration allocation-free
Change the tag iteration api to something less flexible
that allows for event iteration without allocation.

Change-Id: I212d45ebceea0183d1a61e6b611e0558649be60a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227301
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
2020-04-08 13:20:38 +00:00
Rohan Challa
46bd65c853 internal/lsp/cache: add concurrency error check for go cmds
This change attempts to fix a concurrency error that would cause
textDocument/CodeLens, textDocument/Formatting, textDocument/DocumentLink,
and textDocument/Hover from failing on go.mod files.

The issue was that the go command would return a potential concurrency
error since the ModHandle and the ModTidyHandle are both using the
temporary go.mod file.

Updates golang/go#37824

Change-Id: I6cd63c1f75817c7308e033aec473966536a2a3bd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/224917
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-04-08 03:22:09 +00:00
Rebecca Stambler
4d14fc9c00 internal/lsp: add type error fixes to existing diagnostics
This change is the first step in handling golang/go#38136. Instead of
creating multiple diagnostic reports for type error analyzers, we add
suggested fixes to the existing reports. To match the analyzers for
FindAnalysisError, we add an ErrorMatch function to source.Analyzer.

This is not an ideal solution, but it was the best one I could come up
with without modifying the go/analysis API. analysisinternal could be
used for this purpose, but it seemed to complicated to be worth it, and
this is fairly simple. I think that go/analysis itself might need to be
extended for type error analyzers, but these temporary measures will
help us understand the kinds of features we need for type error
analyzers.

A follow-up CL might be to not add reports for type error analyzers
until the end of source.Diagnostic, which would remove the need for the
look-up.

Fixes golang/go#38136

Change-Id: I25bc6396b09d49facecd918bf5591d2d5bdf1b3a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226777
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-04-08 01:45:16 +00:00
Ellison Leão
cd5a53e07f gopls/docs: adding nvim-lsp option in gopls README file
Change-Id: Ic44210e227559c8e1cd5cf1350cde69e2817f5e3
GitHub-Last-Rev: c020218392160d03a9448eda62e098fe0c91a063
GitHub-Pull-Request: golang/tools#221
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227442
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-04-07 19:18:07 +00:00
Rebecca Stambler
a3568bac92 internal/lsp: disable unimported completions for snippet tests
Turns out that these were already disabled in the
golang.org/x/tools/internal/lsp tests, which is why we only ever save
the failures in golang.org/x/tools/internal/lsp/source tests. Discussing
the reasons that this is necessary on the issue, but I think completion
scoring in general needs to be centralized and documented, so hopefully
we can handle this case as part of that larger fix.

Fixes golang/go#38269

Change-Id: Ie1b615315e417ab8211a3580cf8f27cbdc3b74e4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227417
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-07 14:37:52 +00:00
Hana (Hyang-Ah) Kim
066fd1390e internal/telemetry/event: fix error/value key type tag formatting
%v behaves better with nil values.

Change-Id: I4041f5460173c8f420553996d64d9463ef5a0370
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227355
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-06 21:38:09 +00:00
Rebecca Stambler
1fd976651f internal/lsp: make sure that gofmt -s analyses don't modify AST
The code for `gofmt -s` directly modifies the AST, since the ASTs are
not long-lived. Some of this code made it into our analysis
implementations, causing very strange bugs to manifest. Added a
regression test for this specific case.

Fixes golang/go#38267

Change-Id: I235620adcbf2bbc7027c6d83ff2c7fe74729062e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227299
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-04-06 21:01:14 +00:00
Ian Cottrell
903869a827 internal/lsp: make event directly implement TagMap
Makes Event implement TagMap directly, instead of
having to build an entirely new object.

Change-Id: I0c1e8638de3dc3347f60fd93af3df6b7f8387751
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227300
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
2020-04-06 17:24:01 +00:00
Ian Cottrell
ff0df58207 internal/telemetry: add an example of using the logging behaviour
And also fix that the output was not actually correct!

Change-Id: If81e22e586b1c2a71c69843b49d5dd8d5d2dfde6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227298
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
2020-04-06 17:23:38 +00:00
Rebecca Stambler
ee2abff5cf internal/lsp: use one context throughout completion
I originally made this change to see if it would help with the timeouts.
Based on the TryBot results, it doesn't -- but I still think it's more
correct to have the contexts this way. It was my mistake to put the
context on the completer in the first place, I think.

Change-Id: Ib77c8f0ac0b0d0922b82db4120820fb96cb664f4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227303
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-04-06 16:51:37 +00:00
Ian Cottrell
7db14c95bf internal/lsp: rewrite the rpc debug page
Now it only uses the telemetry messages directly rather than the metric system.
This is much faster and more direct, and removes a blocker on improving the
metrics support.
Also fixes the fact that recieced and sent were the wrong way round before,
probably as an artifact of the old protocol logging code, and also removes
the bytes histogram which was a lie (it was a histogram of write sizes that
was presented as a histogram of message sizes)

fixes golang/go#38168

Change-Id: Ib1c3459c0ff1cf0c6087a828981e80c1ce1c5c1b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227139
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-06 14:44:18 +00:00
Ian Cottrell
ccaaa5c26f internal/jsonrpc2: dont add any handlers by default
This pushes the handler construction out to the user, allowing flexability of
use, and is the final stage of the switch to the new handler API.

Change-Id: Id2e61813a817df0d6e4d20dd47ce8c92b0ae87db
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227024
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-06 14:44:07 +00:00
Ian Cottrell
17cc17e0bb internal/jsonrpc2: remove the legacy interface
We can do cancelling at the top level handler now, it can drop the cancel
messages themselves before they enter the queue stage, and also track
all the events as they flow through it.
The ugly part is the OnCancelled interface, which is a bit clunky.

Change-Id: I3fa972198625fb3517fdecc740d1a3fdb19a188a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226959
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-06 14:43:58 +00:00
Ian Cottrell
3eebf4bf9d internal/jsonrpc2: break Run up into composable handlers
Change-Id: I9aff86f7ab06e61849495b9c73553147e29343f1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226840
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-06 14:43:41 +00:00
Ian Cottrell
5c4bdbc02c internal/jsonrpc2: remove request state
This removes the state machine from the request.
It adds a done handler and uses that to manage the unlock channel instead
This also allows us to remove the nextRequest channel from request.
This is the last major piece that allows us to split up the run method into
composable handlers.

Change-Id: I5517ed5a51e30534754522a58453c27b5178ffa8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226839
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-06 14:40:45 +00:00