1
0
mirror of https://github.com/golang/go synced 2024-11-06 04:16:11 -07:00
Commit Graph

4917 Commits

Author SHA1 Message Date
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
Ian Cottrell
44c82bac18 internal/jsonrpc2: make it an error to fail to call Reply
It is now a programatic error to have a handler registered to a connection that
does not call reply for all messages, including notifications.
This normalizes the flow making the code easier to understand  and fixes a
couple of long standing hard to find bugs.

Change-Id: If41c39ece70e3bc64420abefac75ec647a8f8b37
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226838
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-06 13:49:19 +00:00
Ian Cottrell
7e0acf58eb internal/jsonrpc2: cleanup reply handling
Instead of having a Parallel method, we only have Reply, which must also be used
for Notify messages (with a nil response).
This has no real functional impact but makes it easier to refactor in the next
cl.

Change-Id: Ifd4316dd71706de7913c69e6be539b966800e9dd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226837
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-06 13:49:09 +00:00
Ian Cottrell
6dc6d5718f internal/jsonrpc2: change handler to a function type
Handler is now a function type that mapps to what used to be the Deliver method.
The only handler that used other methods was Canceller, for now that still
exists as LegacyHooks. Once the handlers are fully cleaned up we should be able
to re-implement canceller as handler middleware.
Each connection is now only allowed one handler, and it is passed to the Run
method, but handlers are composable.

Change-Id: I370e0459df851bb9c9c2a679b99cff073b94489e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226479
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-06 13:48:45 +00:00
Rebecca Stambler
44a64ad78b internal/lsp, go/packages: don't log context cancellation errors
Instead of checking the context, check the error. This may expose some
errors that are not wrapped correctly. Replaced all uses of errors
with golang.org/x/xerrors.

Change-Id: Ia40160f8ea352e02618765f2a9415a4ece0dcd94
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227036
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-04-03 19:08:13 +00:00
Michael Matloob
4480df5f16 go/packages: only list current module's directory in determineRootDirs
When determining the root directory of a module, the go list overlay
code would do a go list -m all to get the module information for all
modules. Stop doing that and only list the current module. This fixes
an issue when users had overlays and were using automatic vendoring
because go list -m all doesn't work with automatic vendoring.

It isn't clear if overlays outside of the main module were ever working
and overlays in the module cache don't make sense. There's a chance that
this might break users with overlays in replaced directories, but it's
not clear that that previously worked, and it wasn't tested. If this
change causes breakages, we can use bcmills' suggestion in
golang.org/issue/37629#issuecomment-594179751 point 3 to fix it.

Fixes golang/go#37629

Change-Id: I826a86bbe54311cac72cbcafb2de4863dc805c2d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227117
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-04-03 17:07:48 +00:00
Rebecca Stambler
bcf690261a internal/lsp: use context with timeout in all RunProcessEnvFuncs
The modifications in CL 226559 introduced a new call to
RunProcessEnvFunc that was not using the context with a timeout, so deep
completion tests were hitting the timeout early (I think?).

Also, fix a staticcheck warning that's been bugging me today.

Change-Id: Iac894f630ebfa1cbc5588ae3a5490a93fa53aba2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227057
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-04-02 22:33:21 +00:00
Rob Findley
8cb32c4661 internal/lsp/regtest: clean up module names and include github issues
A couple small improvements for regtests:
 - module names fixed to include a '.'. This was fixed for diagnostic
   tests already.
 - For regtests that were written to exercise specifig github issues,
   include the issue number in the test name.

Change-Id: I67da6d78dc166e854d2543cf63dac382202f9dbc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226844
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-04-02 21:48:35 +00:00
Rebecca Stambler
226fa68e9d internal/lsp: fix references for transitive dependencies
We need to search all transitive dependencies of a package, not just its
immediate imports.

Fixes golang/go#38100

Change-Id: I15b4dbe226ba851691ca0c95460c3648ede32f04
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227030
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
2020-04-02 20:53:30 +00:00
Rebecca Stambler
72cf467e29 internal/lsp: handle non-file:// URIs gracefully
Rather than panicking, we should fail to initialize the server.
Context: https://github.com/microsoft/vscode-go/issues/3081
Change-Id: Ic4622d435dffb77b72dc1e0214f0a1d181a6f767
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227032
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-04-02 20:53:07 +00:00
Rob Findley
8f5be0d382 internal/lsp/regtest: add functions to make diagnostic assertions easier
One of the tricky things about asserting on conditions in regtests is
the asynchronous nature of LSP. For example, as the LSP client we cannot
be sure when we've received all diagnostics for a given file.

Currently, regtests are implemented by awaiting specific diagnostic
expectations.  This means that if gopls generates diagnostics that do
not match those expectations, we can only time out the test.

Ideally, we would want to know that gopls is done generating all diagnostics
for the current file state. This is not possible without knowing the
status of diagnostics for. Barring this, we would want to know that
diagnostics are done for the current file version. Unfortunately, that
also is not possible, because a new version of file B can affect
diagnostics in file A.

So in lieu of this information, this CL exposes a few tools that can be
used to improve the experience of writing new regtests.

 - A new expectation is added: AnyDiagnosticAtCurrentVersion, that is
   satisfied if any diagnostics have been received for the current
   buffer version.
 - ExpectDiagnostics is added to Env, to help check whether the current
   diagnostics matches expectations.

Updates golang/go#38113

Change-Id: I48d2c3db87c13ac3ab424d01d9444cbc285af9e1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226842
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-04-02 20:10:23 +00:00
Rebecca Stambler
9fc00b0a7f internal/lsp: fix panic in builtin completions
Fixes golang/go#38091

Change-Id: I88ac6d3413de3dd9d235e2f2fca9b4a3f7127e0a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227026
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-04-02 17:53:26 +00:00
Heschi Kreinick
0a46fa39b0 internal/lsp/source: propose loaded unimported package names first
When proposing unimported package members, we start with packages loaded
in-memory first, for three reasons: we know that they're fast, they're
fully typed, and they're probably pretty relevant. I hadn't bothered
doing that for package names, because the first two reasons aren't very
relevant. But the third still is -- loaded packages are a pretty good
approximation for in module scope, etc.

With this change we do package names the same as members, so relevance
should be about as good. Not perfect, but nobody's complained much yet.

Fair bit of copy-and-paste, but I don't want to extend the
getAllCandidates abstraction outside of the imports package yet.

Updates #38104.

Change-Id: Ia479181607dff898baee3cd6aa84d1ab61715d19
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226559
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-04-02 17:48:25 +00:00
Rebecca Stambler
3304cfb00f internal/lsp: temporarily disable type error analyzers by default
If we release gopls/v0.4.0 soon, we should keep these new analyzers off
by default. They were just merged, so they haven't been used enough to
be enabled, I think. We'll turn them on by default for gopls/v0.5.0.

Also, ended up creating a helper function to check if analysis has been
abled (which fixed a small bug in FindAnalysisError), and another helper
function to enable all analyses for testing purposes.

Updates golang/go#38212

Change-Id: I5ee94b3582dfc0863978650fc6ce51bfa0606c13
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226962
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
2020-04-02 16:54:32 +00:00
Rebecca Stambler
2d9ba733ec internal/lsp: add a mutex around the view's options
The options can be modified without the view being recreated, so we need
a mutex there.

Change-Id: I87e881835622a941fce98e4a1062aa41acd84fcb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227022
Reviewed-by: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
2020-04-02 16:41:49 +00:00
Rebecca Stambler
69646383af internal/lsp/source: add a replacement for experimentalDisabledAnalyses
Change-Id: I5bff1db1d937ffdcb8bb4e36f05073238fbdeb5f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226961
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
2020-04-02 14:48:50 +00:00
Rohan Challa
099440627f internal/lsp: add goreturns like functionality as quickfix
This change ports the functionality of https://github.com/sqs/goreturns
to be used as code actions on diagnostics that have missing
return values. It improves on the original goreturns functionality by:

- filling out empty return statements
- trying to match existing return values to the required return
  values and then filling in missing parameters

Fixes golang/go#37091

Change-Id: Ifaf9bf571c3bc3c61e672b0a2f725d8d734d432d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/224960
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-04-01 19:27:44 +00:00
Rebecca Stambler
9d5940d493 internal/lsp: refactor and propagate errors from code actions
This change makes sure that all errors in code actions are returned,
instead of ignored. There is also a refactoring of the suggested fixes
for go.mod files so that we don't compute them unless there is already a
matching diagnostic.

Change-Id: I34afda0116f3cc7e47809d980a0171487787e55e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221221
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
2020-03-31 20:20:46 +00:00
Bryan C. Mills
ac2e956812 go.mod: upgrade goldmark dependency to fix js/wasm build
For golang/go#38183
Updates yuin/goldmark#120

Change-Id: I913c3409fe0c774209ce046a85699ef845e7c7b3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226560
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
2020-03-31 19:25:49 +00:00
Rebecca Stambler
a30bf2db82 gopls/integration/govim: revert back to using the latest govim
A new version of govim with the fix for the original issue has been
released.

Change-Id: Iaea2eff7387f352cae1657949c5e364a66337f68
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226557
Reviewed-by: Robert Findley <rfindley@google.com>
2020-03-31 02:57:13 +00:00
Rebecca Stambler
e18c1c42de internal/lsp: add a reg test to test "go mod init"
This change adds a RunGoCommand function to the workspace, which will
allow us to test how gopls responds when a certain go command is
executed. Add a test that checks behavior after "go mod init" runs.

Change-Id: I679249c1654b136d44187397b4196b8a10b5615e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226478
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-03-31 02:48:52 +00:00
Rebecca Stambler
a7c0594f4e internal/lsp: avoid logging context cancellation
This change adds a helper function that checks if the context is
canceled, and if so, doesn't log the error. Tried to use it everywhere
in internal/lsp where it fits, which resulted in changing a few pieces
of error handling throughout.

Updates golang/go#37875

Change-Id: I59cbc6f893e3b70cf84524d9944ff7f4b4febd78
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226371
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-03-31 01:46:33 +00:00
Ian Cottrell
a42d6a358d internal/telemetry: give Event a custom implementation of TagMap
This cuts the allocation cost of preparing an event for export from 4 down to 1.

name                old time/op    new time/op    delta
/LogIgnore-8          5.97µs ± 6%    4.18µs ± 4%  -29.87%  (p=0.000 n=20+19)
/TraceIgnore-8        13.6µs ± 4%    10.8µs ± 6%  -20.92%  (p=0.000 n=18+19)
/StatsIgnore-8        11.3µs ± 6%     7.9µs ± 6%  -30.29%  (p=0.000 n=20+20)
/Log-8                47.8µs ± 6%    43.9µs ± 6%   -8.15%  (p=0.000 n=20+19)
/Trace-8              60.5µs ±12%    54.7µs ± 4%   -9.58%  (p=0.000 n=20+19)
/Stats-8              13.8µs ± 3%    10.6µs ± 5%  -23.57%  (p=0.000 n=20+19)

name                old alloc/op   new alloc/op   delta
/LogIgnore-8          5.12kB ± 0%    3.58kB ± 0%  -30.00%  (p=0.000 n=20+20)
/TraceIgnore-8        14.6kB ± 0%    11.5kB ± 0%  -21.05%  (p=0.000 n=20+20)
/StatsIgnore-8        10.2kB ± 0%     7.2kB ± 0%  -30.00%  (p=0.000 n=20+20)
/Log-8                24.0kB ± 0%    20.9kB ± 0%  -12.81%  (p=0.000 n=18+20)
/Trace-8              31.0kB ± 0%    27.9kB ± 0%   -9.92%  (p=0.000 n=20+20)
/Stats-8              10.2kB ± 0%     7.2kB ± 0%  -30.00%  (p=0.000 n=20+20)

name                old allocs/op  new allocs/op  delta
/LogIgnore-8            64.0 ± 0%      16.0 ± 0%  -75.00%  (p=0.000 n=20+20)
/TraceIgnore-8           160 ± 0%        64 ± 0%  -60.00%  (p=0.000 n=20+20)
/StatsIgnore-8           128 ± 0%        32 ± 0%  -75.00%  (p=0.000 n=20+20)
/Log-8                   382 ± 0%       286 ± 0%  -25.13%  (p=0.000 n=20+20)
/Trace-8                 480 ± 0%       384 ± 0%  -20.00%  (p=0.000 n=20+20)
/Stats-8                 128 ± 0%        32 ± 0%  -75.00%  (p=0.000 n=20+20)

Change-Id: I76ff4ac9658b766daf3929e7cfc0d2cfcab036ed
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226359
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-03-31 00:51:03 +00:00
Ian Cottrell
3b4a2bfdc9 internal/telemetry: removing the IsEmpty method of TagMap
It is not being used, and implementing it efficiently is infeasible without
reducing the efficiency of cases that matter more.

Change-Id: Ie53250f0e63ad08a418724c60a74a52edbdfdb29
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226358
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-03-31 00:50:56 +00:00
Ian Cottrell
4edcf52965 internal/telemetry: add a noop exporter benchmark
This has the exporter registered but doing nothing, to measure the basic cost of
having any exporter without any specific exporter costs.
Specifically at the moment this measures the cost of filling in the time on the
event and building the TagMap that is passed down.

Change-Id: Iaae5659e3de9b871dc281c509fa2ee9c3e1d049a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226357
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-03-31 00:50:42 +00:00