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>
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>
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>
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.
Fixesgolang/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>
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>
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>
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>
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>
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>
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>
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>
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.
Fixesgolang/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>
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.
Fixesgolang/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>
%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>
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.
Fixesgolang/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>
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>
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>
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>
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)
fixesgolang/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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
We need to search all transitive dependencies of a package, not just its
immediate imports.
Fixesgolang/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>
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>
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>
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>
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>
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
Fixesgolang/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>
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>
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>
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>
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>
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>
This allows us to reduce the handler interface and delete the telemetry handler.
It is also safer and faster, and can be easily disabled along with the rest of
the telemetry system.
Change-Id: Ia4961d7f2e374f7dc22360d6a4020a065bfeae6f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225957
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Some things that used to be safe due to the global
lock now need ther own synchronization primitives.
Fixesgolang/go#38102
Change-Id: I03c692682d57620d96fe84b7dc74efae0d3f8f09
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226317
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Seems like os.Environ() is necessary after all. My bad for not testing
my earlier change on Windows. I'm not able to reproduce the behavior
in my test, so it's not really testing the crash correctly.
Fixesgolang/go#38062
Change-Id: Ied45bbf572023a9dcd5d020db49bf3e95c824602
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226370
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This change makes working with tests easier by trimming the folder name
up to "testdata". This will remove any ellipses from the test folder name.
Fixesgolang/go#38103
Change-Id: I33b931e527de63713b8fc370c50b1c382796b2b8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226377
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
https://github.com/microsoft/vscode-go/issues/3076#issuecomment-605062933
inspired me to write a regression test for this case. Turns out we
weren't handling it correctly after all...
This change makes sure that we only rebuild the view once a new go.mod
file is saved, not just created. It also preserves the snapshot ID
number when the view is recreated so that diagnostic caching continues
to work as expected.
Change-Id: I63bd559c3bd33b91828171cd7ddb3d099c31cddb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226017
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This changes span events to return a cheaper end function if there is
no exporter and also to deliver the end event to the same exporter
that the start even was delivered to rather than which was active
when the end event is triggered.
Change-Id: I831283da20e8cc991a0cf7490952ae194d125428
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225737
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
On case-insensitive file systems, the editor may send us a path that
works but doesn't match the actual file's case. Then when we run go
list, we'll get mismatching paths and everything will break.
We can't reliably fix this problem: tracking what case the editor
expects is too difficult to be worth it. Instead, check the workspace
path and bail if it's mismatched.
Possibly we should also check files on DidOpen or such, but we can start
with this.
Updates golang/go#36904.
Change-Id: I7635c8136bf9400db4143a0f2fde25c39b5abc44
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225239
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This was causing crashes by setting the working directory to be empty.
Fixesgolang/go#38062Fixesgolang/go#38101
Change-Id: I6d679ee1d5dcab914df3d565d83aa6de0dd74cb4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225817
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This change adds a quick fix for type errors of the type "no new vars on left side of :=". It will replace the ":=" with an "=".
Updates golang/go#34644
Change-Id: I91af8eb82956104229c3b4f3d0fce60fdfdbb5ea
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225477
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds a quick fix for type errors of the type "no result values expected". It will replace the return statment with an empty return statement.
Updates golang/go#34644
Change-Id: I3885748dfc69a2d19f8e7a2e81f36f6d0a20d25b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223666
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change uses the new unusedparams analyzer to remove any unused parameters from functions inside of internal/lsp/source :)
Change-Id: I220100e832971b07cd80a701cd8b293fe708af3d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225997
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
govim integration tests (and probably some real user sessions) are
broken because telemetry metrics are not threadsafe, resulting in an
index out of range panic.
Fix this by adding a flag (labeled temporary) to disable telemetry
export.
Also temporarily update govim to master to pick up some fixes, and run
only the -short tests to avoid timeouts.
Updates golang/go#38042
Change-Id: I584e5d200c2f732bd4024002ee6253d09623b29f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226057
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change adds a quick fix for diagnostics that have an error message of the form "undeclared name: %s". It provides a quick fix to add a new variable with that name.
Updates golang/go#34644
Change-Id: I6534ee79d1770d1a62bac169c3c7e52e2443f39e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222237
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds support within gopls for analyzers that work with type errors to provide suggested fixes.
Updates golang/go#34644
Change-Id: Ia8929173752fda6bd84a9edaabd310e758f25fe8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222761
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
As part of investigating golang/go#38100, I noticed a few things that I
wanted to clean up. Mostly, for renames, we were calling
qualifiedObjAtProtocolPos twice, so I factored out a shared helper
function. I also added an error return for builtins so that callers
don't have to check.
Change-Id: I28c75c801cbec1611736af931cfa72befd219201
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225777
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
This is one of the simplifications that "gofmt -s" applies.
https://golang.org/cmd/gofmt/#hdr-The_simplify_command
A slice expression of the form:
s[a:len(s)]
will be simplified to:
s[a:]
Updates golang/go#37221
Change-Id: Ibd4dedaadc9b129d5d39524f0c1ccc8a95bf7e0d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223659
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
This is one of the simplifications that "gofmt -s" applies.
https://golang.org/cmd/gofmt/#hdr-The_simplify_command
An array, slice, or map composite literal of the form:
[]T{T{}, T{}}
will be simplified to:
[]T{{}, {}}
Updates golang/go#37221
Change-Id: I2dca46501983c8af3581c9319d973da5cf690388
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223660
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
This change adds a pass that checks for unused parameters inside of a function. It is disabled by default.
Updates golang/go#36602
Change-Id: I9e8de3368f16f27e7816ec4ddb16935e1a05584e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222817
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This is one of the simplifications that "gofmt -s" applies.
https://golang.org/cmd/gofmt/#hdr-The_simplify_command
A range of the form:
for x, _ = range v {...}
will be simplified to:
for x = range v {...}
A range of the form:
for _ = range v {...}
will be simplified to:
for range v {...}
Updates golang/go#37221
Change-Id: Ic6babbd0b8ab961ebb4d0d6df72df52d9acde6e7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223661
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds support for analyzers that have suggested fixes of the kind Source.FixAll. This will allow these fixes to be applied on save if the user desires.
To auto apply these fixes on save, make sure your settings.json looks like:
"[go]": {
"editor.codeActionsOnSave": {
...
"source.fixAll": true,
...
},
...
}
Update golang/go#37221
Change-Id: I534e4f6c8c51ec2848cf2899aab68f587ba68423
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223658
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change removes the disabledAnalyses setting and replaces it with a more general feature named "analyses". This will allow users to opt in as well as opt out of analyzers that they do not find useful. This also updates some documentation to show users what analyzers gopls is using and which are enabled by default.
Change-Id: Id3239b4c4c9667e834f262c889270d14fdba0f93
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223662
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Noticed this in https://github.com/fatih/vim-go/issues/2786. I don't
think that this will fix the problem in this issue, but we should avoid
nil pointers as much as possible. Also, remove a bit of extra whitespace
so that the style closer matches that of the rest of the project.
Change-Id: I94b924ea14e4a296382e3e68c52eeb43f6cd86a6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225523
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Type switch on the key and use it to get the value and decide how
to render it.
Use the same render for all debug tag printing.
Change-Id: Ia305fded7dcf05b57c5805f48bb5c22fa7def71f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225380
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
This uses the strongly typed key to get the value rather than
type switching on the value itself.
Change-Id: I8f72d1d9cac0191b0565c14d8a1108459ee6df36
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225379
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Now that keys are solidly typed, we can use them for the metrics.
This prevents accidentally using the wrong type of key, and
allows us to use the typed accesorrs rather than the raw value.
Change-Id: I553bd8e12128d3f00a3e926dbd3bfd420cd3f135
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225378
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
This changes the benchmarks to be less heavy weight.
It allows the baseline to have no allocations and be
very lightweight. This makes it easier to see
realistic numbers for the costs of the various
operations, making it easier to track and optimize.
Change-Id: I07699881d6976c3ab3432373f057b563752509b2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225337
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
In support of https://golang.org/cl/224960, add utility functions that
create zero value expressions for a given types.Type. There are probably
some bugs here but they pass the basic tests in that CL.
Known problems: it skips through type aliases, and can't handle
anonymous structs/interfaces. Hopefully it's pretty unusual to return
those.
Change-Id: I69a388d9ce9bd60bc230e4a3d2b30f581ec25698
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225177
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
If a package starts out as command-line-arguments, and then becomes
"valid" (i.e., gets a package declaration), we shouldn't continue to try
to diagnose "command-line-arguments". We should remove
"command-line-arguments" from workspace packages any time its metadata
is invalidated (assuming it may get added back if a file= query produces
it again).
Include the relevant regression test.
Fixesgolang/go#37978
Change-Id: I7fc51edeb58007b4b4a163336cbeb752a53da322
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225317
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
There is no reason for it to be public now, it is no longer possible to build an
event
without using the helpers.
This also allows us to delay setting the time until after the nil exporter
check, for
a significant time saving in the no exporter case.
name old time/op new time/op delta
/Baseline-8 588ns ± 4% 575ns ± 4% -2.24% (p=0.000 n=18+18)
/StdLog-8 9.61µs ± 3% 9.58µs ± 3% ~ (p=0.384 n=18+18)
/LogNoExporter-8 3.94µs ± 2% 2.26µs ± 2% -42.50% (p=0.000 n=17+18)
/TraceNoExporter-8 2.77µs ± 4% 1.11µs ± 2% -59.82% (p=0.000 n=18+18)
/StatsNoExporter-8 3.83µs ± 5% 2.15µs ± 3% -43.70% (p=0.000 n=18+17)
/Log-8 23.3µs ± 6% 23.0µs ± 1% ~ (p=0.245 n=18+17)
/Trace-8 26.4µs ± 3% 26.5µs ± 4% ~ (p=0.269 n=18+17)
/Stats-8 5.36µs ± 2% 5.45µs ± 3% +1.68% (p=0.000 n=17+18)
Change-Id: Ibde0e20eaf99d03f786cd1436f05eab7b2a17b20
Reviewed-on: https://go-review.googlesource.com/c/tools/+/224657
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
This separates the concerns of tag collections that have to be iterated
and tag collections that need lookup by key.
Also make it so that events just carry a plain slice of tags.
We pass a TagMap down through the exporters and allow it to be
extended on the way.
We no longer need the event.Query method (or the event type)
We now exclusivley use Key as the identity, and no longer have a
common core implementation but just implement it directly in each
type.
This removes some confusion that was causing the same key through
different paths to end up with a different identity.
Change-Id: I61e47adcb397f4ca83dd90342b021dd8e9571ed3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/224278
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
In an effort to be idiomatic, I made the regtest func signature
func(context.Context, testing.T, *Env), despite the fact that Env
already has a Context and a T.
This just ended up causing more confusion, as it's not clear which
Context or T to use. Remove this and just use the fields on Env.
Change-Id: I54da1fdfe6ce17a67601b2af8640d4d2ea676e8c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225157
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Expressing regtests in terms of textual coordinates is hard to read: the
reader ends up counting lines and characters to understand the text edit
or assertion.
To address, this, add two new functions for fake.Editor: RegexpSearch
and RegexpReplace, as well as a symmetric RegexpSearch function for
workspace files and wrappers for regtext.Env.
This allows expressing edits as well as buffer locations in terms of
easily scannable regexps.
An alternative solution to this problem is to integrate markers ala
packagestest. I tried this, but it ended up being cumbersome to
implement and less usable than regexps, due to the static nature of
markers: after the buffer has been edited all markers must be
updated.
Updates golang/go#36879
Change-Id: Iad087cf0d529737034197beef7b729816a159c69
Reviewed-on: https://go-review.googlesource.com/c/tools/+/224757
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
For debugging purposes, we print the output of `go env` on start. Now,
we print a view-specific `go env`, which will helps us when debugging
multi-project workspaces. Additional information included: the folder of
the view, if the view has a valid build configuration, and the build
flags for the view.
Updates golang/go#37978
Change-Id: Iadffd46f8ac5410971558a304b8bbcb2f9bdbcc3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225137
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This is helpful for users to diagnose issues, but it annoys people who
have workspaces with both non-Go and Go projects. I'm going to spend
some more time thinking about this, particularly about which error
messages we should retry the initial workspace load for.
Updates golang/go#32394
Change-Id: I7d02b385da232914fe7342837dc3a9c4185399fd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225237
Reviewed-by: Heschi Kreinick <heschi@google.com>
I'm not sure if this is entirely the correct approach, but at the very
least we should never pass "command-line-arguments" to packages.Load. We
still need to cache packages with the path "command-line-arguments" to
avoid an excessive number of calls to packages.Load.
Updates golang/go#37978
Change-Id: I1b5263a3dab95aacd797f03f742069fd0c53619c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225117
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This change updates the metrics tutorial to include example code for
exporting traces from go tools.
Change-Id: Ie1d3c373ed4308ef0160f6389b74c642b348bed6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225061
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change updates the metrics tutorial to be compatible with the new event system.
Change-Id: I8b75f6b02d241bc9dede01cfac167a982f1df67c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/225058
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
A common annoying mis-completion is as follows:
type foo struct {
field int
}
func (f foo) Field() int { return f.field }
func (f foo) logic() {
if f.f<>
}
Now at <> we prefer "field" over "Field()". Similarly:
type foo struct {
}
func (f foo) DoSomething() { }
func (f foo) doSomething() { }
func (f foo) logic() {
f.d<>
}
Now at <> we prefer "doSomething()" over "DoSomething()". All else
being equally, you normally want private objects over public objects
when the private objects are available.
The same logic is applied to deep completions so we prefer "c.foo.bar"
over "c.Foo().bar".
Change-Id: Ic91cba7721ddb1f2a30338037693ddcce8c621f7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223877
Run-TryBot: Muir Manders <muir@mnd.rs>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We can re-enable this when we have a version of Go that includes the fix
for golang/go#37971. Then we can continue playing whack-a-mole with go
list corner cases :)
Fixesgolang/go#37958
Change-Id: I2417731bf5c01fbde867fef2ef03b5025ae779c7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/224538
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This allows us to hide the implementation details of how tags are stored on a
context from the normal interface, to allow us to explore more efficient
mechanisms.
The current storage is not intended as the most efficient choice, this cl is
about isolating the API so we can experiment with benchmarks in the future.
Change-Id: Ib101416bccd8ecdee269cee636b1564d51e1da8a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222854
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
The next version of VS Code Go has to be released before this can be
released.
Fixesgolang/go#37966
Change-Id: I6b97f2a3ff9c8400ae85b815d8bf83a9b12b1069
Reviewed-on: https://go-review.googlesource.com/c/tools/+/224518
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
This change updates the metric exporting tutorial to use oragent to spin
up OpenCensus, Prometheus, and Zipkin all at once using docker-compose
rather than manually setting each one up. This allows developers to set
up an environment for testing metrics and traces with minimal effort.
While oragent also spins up Zipkin for traces, the tutorial does not yet
have a section outlining how to export traces from Go tools. A section
for traces will added in a later CL.
Change-Id: I07f49977f7ab49995853ff8ee8eb6ccdf6ef1642
Reviewed-on: https://go-review.googlesource.com/c/tools/+/224258
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
definition.Run requires a nil check of opts before applying to avoid
panic, and test must be run with markdown enabled.
When running 'go test' without -run flag, connection.initialize() was
not called and there was no problem because the default value of
Options.PreferredContentFormat was Markdown.
In addition, currently using the same connection despite different
options, therefore make to use a different connection for different
options.
Change-Id: I6ef773c14cbdcae621da9295d4c618c3aff0beee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222497
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
While experimenting with different static analysis on x/tools, I noticed
that there are many actionable diagnostics found by staticcheck. Fix the
ones that were not false positives.
Change-Id: I0b68cf1f636b57b557db879fad84fff9b7237a89
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222248
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This changes the way keys work, there is still a single internal key
implementation for performance reasons, but the public interface is a set of key
implementations that have type safe Of and Get methods.
This also hides the implemenation of Tag so that we can modify the storage form
and find a more efficient storage if needed.
Change-Id: I6a39cc75c2824c6a92e52d59f16e82e876f16e9c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223137
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
It encourages poor performing log lines, and also reduces the readability of
those lines.
Also delete the Key.With method which was unused, and should never be used
instead of the event.Label function anyway.
Change-Id: I9b55102864ee49a7d03e60af022a2002170c0fb1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222851
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Move the lsp specific telemetry package that just declares the labels under the
debug package and call it tag, to make all the usages much more readable.
Change-Id: Ic89b3408dd3b8b3d914cc69d81f41b8919aaf424
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222850
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Rather than building cusom events and driving the exporter, the test now
registers a custom exporter and then uses the normal higher level methods to
deliver events to it.
This means we are testing the actual information that would arise in a user
program, and also means we no longer have to expose internal features just for
tests.
Metrics are not fully converted yet.
Change-Id: I63248a480fb1b3e6274dce0c2e1d66904d055978
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222849
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
This allows early exporters to adjust the event for later ones.
This is used to lookup key values from the context if needed.
Also add a Query type event which is intended to perform all event
modifications but nothing else, and is used to lookup values from
the context. This cleans up a weirdness where the current lookup
presumes there will be an exporter with a matching mechanism.
Change-Id: I835d1e0b2511553c30f94b7becfe7b7b5462c111
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223657
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
When searching for deep completions, we can end up enumerating struct
types' fields a lot. Optimize fieldSelections to reduce work:
- Wait until we see an embedded field before we create the "seen"
map.
- Use a callback style to iterate over the struct's fields rather than
returning a slice of fields.
- Change "seen" checking strategy back to track struct types rather
than each individual field.
Struct with 5 non-embedded fields:
name old time/op new time/op delta
Fields-16 293ns ± 1% 20ns ± 2% -93.13% (p=0.008 n=5+5)
name old alloc/op new alloc/op delta
Fields-16 120B ± 0% 0B -100.00% (p=0.008 n=5+5)
name old allocs/op new allocs/op delta
Fields-16 4.00 ± 0% 0.00 -100.00% (p=0.008 n=5+5)
Same struct but add an embedded struct with 2 fields:
name old time/op new time/op delta
Fields-16 389ns ± 1% 142ns ± 1% -63.53% (p=0.008 n=5+5)
name old alloc/op new alloc/op delta
Fields-16 120B ± 0% 144B ± 0% +20.00% (p=0.008 n=5+5)
name old allocs/op new allocs/op delta
Fields-16 4.00 ± 0% 2.00 ± 0% -50.00% (p=0.008 n=5+5)
I think the alloc/op went up because the "seen" map is no longer
allocated on the stack. There is more room for more optimization, but
it's probably not worth making things more complicated.
Change-Id: I6f9f2124334a8594ef9d6f9b5ac4b3a8aead5f49
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223419
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This commit will make the 'go generate' CodeLens command check the client
capabilities to use $/progress to report the progress of the command or otherwise
it will fallback to showing the message box already in place. The $/progress will
give you a play by play update while the message box shows only the beginning and
the end. The gopls logs will have all the details in either case.
Note that the $/progress is an LSP 3.15+ feature and not yet supported in all clients.
Updates golang/go#37680
Change-Id: I5ba37448be8e388f728394795e1bb5f1d50cc30d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223739
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change fixes breadcrumbs when the cursor is inside of a method by making the method a top level symbol and removing it from the struct's hierarchy.
Fixesgolang/go#36949
Change-Id: I8da5f453a5c78ade1e7688fc4eeebf79ce96f1e1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221819
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This CL gets code.ts to generate code for $/progress and
window/workDoneProgres/create messages. $/progress uses a
ProgressParams type which contains one of three new data types,
WorkDoneProgressBegin, WorkDonProgressEnd, WorkDoneProgressReport.
In addition, a *TextEdit is now generated for CompletionItem.TextEdit.
The substantive differences in code.ts are around line 451 and line
682. Everything else is whitespace caused by vscode formatting typescript
differently on different OSes.
Change-Id: Ide441e6e0029cbc8401d6476f6a939216cc89634
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223743
Run-TryBot: Peter Weinberger <pjw@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds support for recognizing a //go:generate directive
and offering a CodeLens that will then send a "generate" command to
the server to run "go generate" or "go generate ./...". Because
"go generate" can only be executed per package, there is no need to show
the CodeLens on top of every //go:generate comment. Therefore, only the
top directive will be considered.
The stdout/stderr of the go generate command will be piped to the logger
while stderr will also be sent to the editor as a window/showMessage
The user will only know when the process starts and when it ends so that they wouldn't
get bogged with a large number of message windows popping up. However, they can
check the logs for all the details.
If a user wants to cancel the "go generate" command, they will be able
to do so with a "Cancel" ActionItem that the server will offer to the client
Fixesgolang/go#37680
Change-Id: I89a9617521eab20859cb2215db133f34fda856c7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222247
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This moves the actual trace test into its own file so that it can easily
be seen separately from the functions that support all tests.
Change-Id: I1d30cf8a712377ca79a5de77b85412c881177f43
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223397
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
This CL adds support for sending progress notifications through $/progress
calls as well as being able to cancel them through window/workDoneProgress/cancel.
This feature is only supported in clients running LSP 3.15 and therefore the initialize
request will check for client capabilities for its progress support.
Updates golang/go#37680
Change-Id: Iff8c016694746a9dd553e5cc49444df7afcc21f5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222981
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Now that the feature-set is relatively complete for running gopls in
daemon mode, daemon.md is updated to reflect the new features and to
remove some of the experimental warnings. Similarly the `-remote` flag
is made more welcoming.
The feature still needs more usage, but it shouldn't be considered
experimental anymore.
Fixesgolang/go#34111
Change-Id: I994fc8f9f84bf856f24e1eadabd73c503267e804
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222717
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Previously it was an error to call shutdown while the server was not yet
initialized. This can result in session leak for very short-lived
sessions.
Instead, allow shutdown to proceed and simply log the error. On the
other hand, for consistency make it an error to call initialized without
first calling initialize.
Updates golang/go#34111
Change-Id: I0330d81323ddda6beec0f6ed9eb065f8b717dea0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222671
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Three new flags are added to the serve command, and threaded through to
the LSP forwarder:
-remote.listen.timeout: -listen.timeout for the auto-started daemon
-remote.debug: -debug for the auto-started daemon
-remote.logfile: -logfile for the auto-started daemon
As part of this change, no longer enable debugging the daemon by
default.
Notably none of this configuration affects serving, so modifying this
configuration has been chosen not to change the path to the automatic
daemon. In other words, this configuration has effect only for the
forwarder process that starts the daemon: all others will connect to the
daemon and inherit whatever configuration it had at startup. This should
be OK, because in the common case this configuration should be static
across all clients (e.g., many Vim sessions all sharing the same
.vimrc).
Exposing this configuration made the signature of lsprpc.NewForwarder
a bit hard to understand, so I decided to go ahead and switch to a
variadic options pattern for initializing both the Forwarder and
StreamServer, the latter just for consistency with the Forwarder.
Updates golang/go#34111
Change-Id: Iefb71e337befe08b23e451477d19fd57e69f36c6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222670
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Add a new toplevel `inspect` verb to the gopls command, to expose the
internal state of a running gopls server. For now, this verb exposes a
single subcommand `sessions`, which lists some information about current
server sessions on a gopls daemon.
This can be used even if the debug server is not running, which will
become the default in a later CL.
Updates golang/go#34111
Change-Id: Ib7d654a659fa47280584f9a7301b952cbccc565a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222669
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This makes keys a pointer, which reduces the allocations during logging, and has
a significant improvement in memory and cpu cost when there is no exporter.
Packing a string into an interface requires a 16 byte allocation, packing a pointer
does not.
name old time/op new time/op delta
/LogNoExporter-8 4.39µs ± 2% 3.83µs ± 2% -12.74% (p=0.000 n=18+20)
/Log-8 23.5µs ± 2% 22.6µs ± 1% -4.20% (p=0.000 n=19+18)
name old alloc/op new alloc/op delta
/LogNoExporter-8 1.70kB ± 0% 1.38kB ± 0% -18.78% (p=0.000 n=20+20)
/Log-8 4.91kB ± 0% 4.91kB ± 0% ~ (p=1.000 n=20+20)
name old allocs/op new allocs/op delta
/LogNoExporter-8 83.0 ± 0% 63.0 ± 0% -24.10% (p=0.000 n=20+20)
/Log-8 163 ± 0% 163 ± 0% ~ (all equal)
Change-Id: Iec127f1bff8d5c8f4bd0a6d9f6d8fc4b8bc740b2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222599
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
This is now the only package that is exposed to normal use, and should
be the only thing to appear in libraries.
Change-Id: I90ee47c6519f30db16ff5d5d2910be86e91e5df2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222557
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
The original intention was to limit the number of open file handles,
but the parseLimit semaphore was limiting parser.ParseFile calls that
don't access files, so there was no benefit. The file limiting is
already handled generically by another semaphore in the lsp FileSystem
abstraction.
Change-Id: Idc7f2507af5287e983a7edf3e38a848d26770bbe
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223119
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The tests will not run because there is no workspacesymbol marker.
Therefore, restore testdata used in WorkspaceSymbols tests, and
initialize data so that the tests run even if the target of the marker
is 0.
Change-Id: I051b842183b7e23bb746cc282fc3921a90ca8df1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221617
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Now we properly offer "case" and "default" keyword completion in cases
like:
switch {
<>
}
First I had to add an AST fix for empty switch statements to move the
closing brace down. For example, say the user is completing:
switch {
ca<>
}
This gets parsed as:
switch {
}
Even if we manually recover the "ca" token, "<>" is not positioned
inside the switch statement anymore, so we don't know to offer "case"
and "default" candidates. To work around this, we move the closing
brace down one line yielding:
switch {
}
Second I had to add logic to manually extract the completion prefix
inside empty switch statements, and finally some logic to offer (only)
"case" and "default" candidates in empty switch statements.
Updates golang/go#34009.
Change-Id: I624f17da1c5e73faf91fe5f69e872d86f1cf5482
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220579
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
deliver detach to the exporter as an event rather than hard coding the
span detach behavior.
Change-Id: I87b6e2a3596fea338908c11ba0b219176b6305bf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222542
Reviewed-by: Robert Findley <rfindley@google.com>
Change AllImportsFixes to get the ParseGoHandle directly rather than
using getParsedFile. The latter also type checks the containing
package, which is slow in general and can be really slow in certain
circumstances. This should speed up the organizeImports code action.
Change-Id: Ic505873db41d5d76c398cee42f0323e390d9e034
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222780
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
The recent LPS protocol change made the definition of textEdit into
a union type, so the generated code made TextEdit an interface{},
which broke govim. This CL reverts it to a *TextEdit. The code
generator will be fixed in another CL.
We can revisit this when gopls wants to start using the new
InsertReplaceEdit type.
Change-Id: I4d7a14b3746b747f34b0907f72ecbc3593706a05
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222765
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
It seems that people are running into this warning for reasons other
than ad-hoc packages. Suggest filing an issue, since it may be that they
are opening the wrong directory or something.
Change-Id: I88036d2db9477bbea5099910f4cd8e5ba55e0624
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222597
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Now we offer an error-check-and-return completion candidate when
appropriate:
func myFunc() (int, error) {
f, err := os.Open("foo")
<>
}
offers the candidate:
if err != nil {
return 0, <err>
}
where <> denotes a placeholder so you can easily alter "err".
The completion will only be offered when:
1. The position is in a function that returns an error as final result
value, and
2. The statement preceding position is an assignment whose final LHS
value is an error.
The completion will contain zero values for the non-error return values
as necessary.
Using the above example, the completion will be offered after the user
has typed:
i
if
if err
Basically the candidate will be offered after every keystroke as the
user types "if err".
I call this new type of completion a statement completion - perfect
for when you want to make a statement!
Change-Id: I0a330e1c1fa81a2757d3afc84c24e853f46f26b0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221613
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
1. logging at debug/rpc.go:105 was corrupting logs.
2. DocumentSymbol (a Server method) now returns []interface{}.
3. The latest version of the LSP changed CompletionItem.TextEdit to
possibly be of the new type InsertReplaceEdit, so the generated Go
code now has TextEdit an interface{} rather than a *TextEdit. This
required a change to tests/util.go.
4. The latest version also introduced several other new types,
and new members in some structs.
Tests pass and the I've use the new gopls a little.
Change-Id: Ic44d46f0c923882c9076c2754c1c85e09fbcaa2e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222668
Run-TryBot: Peter Weinberger <pjw@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Editors typically trigger completion automatically after ".". This
pops up annoying, useless completions after "..." variadic param, such
as "foo(bar...<>)". We now suppress completions in or directly after
the ellipsis.
Fixesgolang/go#37358.
Change-Id: I9fc94fbdf69429bd787bcb2c643f4f730e24154d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222200
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
When running gopls as a forwarder it attempts to forward the LSP to a
remote daemon. On posix systems, by default this uses a unix domain
socket at a predictable filesystem location.
As an extra precaution, attempt to verify that the remote socket is in
fact owned by the current user.
Also, change the default TCP listen address used on windows to bind to
localhost.
Updates golang/go#34111
Change-Id: Ib24886d290089a773851c5439586c3ddc9eb797d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222246
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
We now offer "range" keyword in the for loop init statement:
for i := r<> // offer "range" completion candidate
Updates golang/go#34009.
Change-Id: I2e4c1db11c37127406c78191681c39b9dd3439f7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220504
Run-TryBot: Muir Manders <muir@mnd.rs>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Offer "struct", "interface", "map", "chan", and "func" keywords when
we expect a type. For example "var foo i<>" will offer "interface".
Because "struct" and "interface" are more often used when declaring
named types, they get a higher score in type declarations. Otherwise,
"map", "chan" and "func" get a higher score.
I also got rid of the special keyword scoring. Now keywords just use
stdScore and highScore. This makes the interplay with other types of
candidates more predictable. Keywords are offered in pretty limited
contexts, so I don't think they will be annoying.
Finally, keyword candidate score is now to be scaled properly based on
how well they match the prefix. Previously they weren't penalized for
not matching well, so there were probably some situations where
keywords were ranked too high.
Updates golang/go#34009.
Change-Id: I0b659c00a8503cd72da28853dfe54fcb67f734ae
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220503
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change makes ModHandle and ModTidyHandle return a ModData structure that only contains an error if something goes wrong when parsing the .mod files.
Change-Id: I89661cfefeff666bdf13cf99f4c56137e120de82
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222242
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds support when hierarchicalDocumentSymbolSupport is false, this can happen with editors who have not supported textDocument/DocumentSymbol. As a result, these older lsp clients need to recieve []protocol.SymbolInformation rather than []protocol.DocumentSymbol. This change required some changes to internal/lsp/cmd to handle not knowing which type it is receiving, this required manual parsing inside of cmd/symbols.go.
Fixesgolang/go#34893
Change-Id: I944ae24302f155b561047227f65bee34b160def1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221823
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This type was previously guarded implicitly by the global exporter
mutex. With that gone, we've started seeing panics due to races in
(*rpcs).Metric.
Guard it with a mutex.
Change-Id: I2a8c670ecfbaee9422e1f1d282b1fedb86b6a5e0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222300
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change adds support for passing a span to cmd/suggested_fix.go, originally it would just take the filename and apply all the fixes for that file. Now, it can also take just a span within that file and only apply the fixes relevant to that span.
The //@suggestedfix marker now contains an extra parameter to specify which type of codeaction is expected.
Change-Id: I4e94b8dad719f990dc2d0ef3c50816f70f59f737
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222137
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change implements support for textDocument/formatting when it comes to go.mod files. It also adds formatting on save ability.
To enable formatting on save for go.mod files, you need to include the following settings inside of your VSCode settings.json:
...
"[go.mod]": {
"editor.codeActionsOnSave": {
"source.organizeImports": true,
},
"editor.formatOnSave": true,
},
...
Updates golang/go#36501
Change-Id: I60ac14d0e99b2b086fe9a8581770771aafc2173c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221223
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
With #34111, we are forwarding the LSP from one gopls instance to
another. This exposed an asymmetry in our LSP dispatching: for both
ClientDispatcher and ServerDispatcher, we unmarshal to non-nil response
structs. This means that when forwarding the LSP, we translate empty
JSON responses (corresponding to nil values) into the non-nil zero
value.
This causes problems for some editors, as reported in #37570. Fix it by
instead unmarshaling to a pointer.
This is, of course, a somewhat dangerous change. I fixed the one NPE
that occurred in tests, and have done some mild manual testing. I
wouldn't be surprised if we discover more NPEs later on, but I still
think this is the right change to make.
Updates golang/go#34111Fixesgolang/go#37570
Change-Id: Ie69e92d2821c829cdfc4f4ab303679a725f1f859
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222058
Reviewed-by: Peter Weinberger <pjw@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
When the gopls daemon is automatically managed (-remote=auto), it will
be started by one of the forwarder gopls processes that was in turn
started by the editor. By default, this puts it in the same process
group as the forwarder gopls.
Some editors (at least Vim) send SIGTERM to the process groups of
sidecar processes when exiting. This can cause the gopls daemon to
terminate, thereby losing state.
Rather than ignore SIGTERM (which is bound to be editor dependent
anyway), let's just put the gopls daemon in a separate session.
Updates golang/go#34111
Change-Id: I71386fb54b8c2efe1c565f59763f46693a7d48b0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221220
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
When caching file data specific to a session (anything with a Version or
tied to a view), we now need to be more careful about the existence of
multiple sessions.
This change fixes a few places where we appear to cache session data
without explicitly referring to the session. In principal this could
cause data corruption in multi-session gopls instances, but I have not
been able to force this to occur in either manual or automated testing.
Also fix a data race to the unsaved overlays:
https://storage.googleapis.com/go-build-log/588ee798/linux-amd64-race_d0762522.log
Updates golang/go#34111
Change-Id: I47117da1aba1afc2e211785544ad3f8b3416d15d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222059
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
Reading the docstring, it looks like the sense of the type assertion was
accidentally inverted in this function. Fix it to what I believe was
intended.
Fixesgolang/go#37687
Change-Id: Ief44a996f1a262527c2916d6b78b81dd35b2cf9e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222217
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
This means that tags also become cheap if there is no exporter and cleans up the
mess with how spans, tags and logs were related.
Also fixes the currently broken metrics that relied on the span tags.
Change-Id: I8e56b6218a60fd31a1f6c8d329dbb2cab1b9254d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222065
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
We need to use the filename derived from the symbol information, not the
one from CompiledGoFiles, in case of cgo packages.
No tests because workspace symbols are entangled with document symbols,
and the latter doesn't work in cgo packages.
Fixesgolang/go#37659.
Change-Id: Ic32293c542830a49b37c25ebf3b231771c3a4225
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222060
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change attaches a modTidyHandle to the snapshot instance and tries to reuse it as often as possible. It also modifies the modTidyKey to include the files that have not yet been saved. This was necessary because `go mod tidy` only runs using contents on disk.
So, if a user decides to add an import that is not in their modcache, we run diagnostics as normal and the ModTidyKey's imports field gets updated with the new import, we run `go mod tidy` but since the file was not saved yet, nothing changes. Then when the user eventually saves their file, we do not rerun `go mod tidy` because the imports hash has not changed from the time the file was in overlay to the time the file was saved on disk. To be able to account for this, we need the invalidate the ModTidyKey when imports change between saves.
Change-Id: I9e210a15cf009d222cecd7824c2a1a927957483b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/219477
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We no longer use the span as the core type of tracing, instead that is an
artifact of the exporter, and start and end tracing is just event based.
This both makes the interface normalized, and also means the null exporter case
is considerably cheaper in memory and cpu.
See below for benchstat changes
name old time/op new time/op delta
TracingNoExporter-8 4.19µs ±12% 2.71µs ±11% -35.33% (p=0.000 n=20+20)
Tracing-8 24.1µs ± 3% 5.1µs ±17% -78.66% (p=0.000 n=16+20)
name old alloc/op new alloc/op delta
TracingNoExporter-8 2.32kB ± 0% 0.40kB ± 0% -82.76% (p=0.000 n=20+20)
Tracing-8 6.32kB ± 0% 2.32kB ± 0% -63.30% (p=0.000 n=20+20)
name old allocs/op new allocs/op delta
TracingNoExporter-8 35.0 ± 0% 15.0 ± 0% -57.14% (p=0.000 n=20+20)
Tracing-8 215 ± 0% 35 ± 0% -83.72% (p=0.000 n=20+20)
Change-Id: I3cf25871fa49584819504b5c19aa580e5dd03395
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221740
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
These are just like the logging benchmarks but for tracing instead.
Also makes the log writer write out tracing events as well if it is
not in only errors mode
Change-Id: Ie00d7c80f7e2b9433787603261950f70ab1c1e9d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221739
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
This adds a type to events and makes the logging calls use it.
Change-Id: Iaa50fe2e332caae611b1e00424c878a3bc479feb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221741
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
The Tagger interface allowed for specifying a key in a tag list and having the
value be aquired from the context automatically. It was almost never used, and
the alternative (using the key to get the value) is not that much more long
winded, so it was not holding it's own weight from a complexity persective alone
but the performance cost of having to use a list of interface rather than a list
of tags was very large. See the benchstat improvements below for the difference
it makes to both speed and memory usage, especially in the no exporter case.
name old time/op new time/op delta
Baseline-8 341ns ± 2% 342ns ± 1% ~ (p=0.139 n=19+18)
LoggingNoExporter-8 2.46µs ± 5% 1.97µs ± 3% -19.88% (p=0.000 n=19+20)
Logging-8 13.3µs ± 2% 12.8µs ± 2% -3.42% (p=0.000 n=17+20)
LoggingStdlib-8 5.39µs ± 6% 5.34µs ± 3% ~ (p=0.692 n=20+19)
name old alloc/op new alloc/op delta
Baseline-8 80.0B ± 0% 80.0B ± 0% ~ (all equal)
LoggingNoExporter-8 728B ± 0% 440B ± 0% -39.56% (p=0.000 n=20+20)
Logging-8 2.75kB ± 0% 2.46kB ± 0% -10.53% (p=0.000 n=17+20)
LoggingStdlib-8 568B ± 0% 568B ± 0% ~ (all equal)
name old allocs/op new allocs/op delta
Baseline-8 5.00 ± 0% 5.00 ± 0% ~ (all equal)
LoggingNoExporter-8 32.0 ± 0% 23.0 ± 0% -28.12% (p=0.000 n=20+20)
Logging-8 88.0 ± 0% 79.0 ± 0% -10.23% (p=0.000 n=20+20)
LoggingStdlib-8 28.0 ± 0% 28.0 ± 0% ~ (all equal)
Change-Id: Ic203ad0c5de7451348976b999a0d038ac532dc39
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221737
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
We change the main exporter to be stored and fetched using atomics rather
than aquiring a mutex for a mild (but significant in the disabled case) speedup.
Also has the benefit of not holding a global lock over all telemetry operations.
benchstat of logging benchmatks before and after:
name old time/op new time/op delta
Baseline-8 329ns ± 2% 327ns ± 2% ~ (p=0.181 n=19+17)
LoggingNoExporter-8 3.08µs ± 3% 2.42µs ± 2% -21.42% (p=0.000 n=20+19)
Logging-8 13.7µs ± 2% 13.2µs ± 1% -3.49% (p=0.000 n=19+19)
LoggingStdlib-8 5.39µs ± 3% 5.41µs ± 2% ~ (p=0.177 n=19+20)
This is a replacement for https://go-review.googlesource.com/c/tools/+/212244
but built on the single exporter principle rather than the exporter list.
Change-Id: Icc99319c4357e0bcb63386c64372a733e8a76796
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221218
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
This allows us to register a telemetry exporter that works with mulitple active
debug instances.
It also means we don't have to store the debug information in our other objects.
Change-Id: I9a9d5b0407c3352b6eaff80fb2c434ca33f4e397
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221558
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Not sure why I thought it was a good idea to enable extra logging
by default, but this was added in CL 184198.
Fixesgolang/go#37636
Change-Id: I1840a9e53625db99c9097f2c23500ae20d6dac1f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221918
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
This change adjusts where the start and end of the arguments list are when determining which parameter is the active paramter. Rather than use the first and last argument's position, we will use the start and ending parentheses.
Fixesgolang/go#37271
Change-Id: I70bc5c8b4bdb5242fc35a20e63b9a8860cb1d6bd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221817
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change fixes an issue with concurrent map reads and writes that occurred when determining if an import is missing the corresponding dependency in the go.mod file.
Change-Id: Ic5cf3a620b4fd84eb4a377d0e4c22edbc8f37540
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221897
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
In all cases, use a Logf field to configure debug logging. Non-nil means
that logging is enabled through the given function.
Fixes accidental debug spam from goimports, which had a separate Debug
flag that was supposed to guard logging, but wasn't used when creating
the gocommand.Invocation.
Change-Id: I448fa282111db556ac2e49801268d0affc19ae30
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221557
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds a test for diagnostics not disappearing when you create the first .go file for a project and add an import, then save.
Updates golang/go#37195
Change-Id: I871e77d9ab129f13be58931fe009e4297c7f2b38
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221222
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds an upgrade all dependencies codelens on the go.mod file if there are available upgrades.
Updates golang/go#36501
Change-Id: I86c1ae7e7a6dc01b7f5cd7eb18e5a11d96a3acc1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221108
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We were panicking when a LHS assignee's type was nil, such as:
// "foo" has not been declared
foo = 123
A recent refactoring changed (*completer).typeMatches() to no longer
gracefully handle a nil types.Type for its first parameter. That
behavior seems fine, so fix the problematic caller of typeMatches to
check for nil before calling.
Change-Id: Ie11e4a2d374ab1efbf6fd13fbe214e06d359fca0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221020
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Now "implementations" supports finding interfaces implemented by the
specified concrete type. This is the inverse of what "implementations"
normally does. There isn't currently a better place in LSP to put this
functionality. The reverse lookup isn't an important feature for most
languages because types often must explicitly declare what interfaces
they implement.
An argument can be made that this functionality fits better into find-
references, but it is still a stretch. Plus, that would require
find-references to search all packages instead of just transitive
dependents.
Updates golang/go#35550.
Change-Id: I72dc78ef52b5bf501da2f39692e99cd304b5406e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/219678
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Add two new fake editor commands: Formatting and OrganizeImports, which
delegate to textDocument/formatting and textDocument/codeAction
respectively. Use this in simple regtests, as well as on save.
Implementing this required fixing a broken assumption about text edits
in the editor: previously these edits were incrementally mutating the
buffer, but the correct implementation should simultaneously mutate the
buffer (i.e., all positions in an edit set refer to the starting buffer
state). This never mattered before because we were only operating on one
edit at a time.
Updates golang/go#36879
Change-Id: I6dec343c4e202288fa20c26df2fbafe9340a1bce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221539
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
Helper functions on the regtest.Env wrapping workspace or editor
functionality are moved to a new wrappers.go file.
Also, rename 'WriteBuffer' to 'SaveBuffer', for less confusion with
'WriteFile'.
Change-Id: Ide9a5cf919dcee6e4a4fbfdb167eddf751a26eeb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221538
Reviewed-by: Rohan Challa <rohan@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
There are missing nil checks in a few places when using the gopls
debug page. It will result in panics when trying to access a debug
page that no longer exist (for example a closed session).
This change adds nil checks and updates the template to not render
non-existing debug address.
Change-Id: Ic9163f3727fd8c51122cbd4ab4374fc4d55477c3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221699
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
When using the experimental feature where gopls runs as deamon, the
remote is spawned with debug enabled. It currently exposes the debug
interface to all network interfaces. This should be configurable in
the future, but until then we should at least keep it on the
localhost interface.
This change starts the debug on the local interface instead of all.
Updates golang/go#34111
Change-Id: I3184268dd434ae11ff5f8c3c57a229d22c158196
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221697
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
It is never invoked anyway, and it provices no useful benefit while complicating
the implementation, as it is the only non context aware method.
Change-Id: Id5a99439fedafdf4d71285e36103b4854cf3635a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221540
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Darwin's syscall.Dirent is declared as larger than the kernel may
actually return. Don't assume that we can copy the whole thing out of a
buffer.
Fixesgolang/go#37269
Change-Id: I7f2b273f3a14071df8b5ff5bd70e59d784f6d187
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221381
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Originally I decided to use t.Parallel() in hopes of uncovering new
bugs. That may have worked... but manifested as rare flakes that are
difficult to diagnose (golang.org/issues/37318).
Since this level of parallelism is extremely unlikely in normal gopls
workloads, I'm going to remove the t.Parallel() calls in hopes of
eliminating this flakiness. I'd rather be able to continue running these
tests.
Also, don't run in the 'Shared' execution mode by default: normal gopls
execution is either as a sidecar (the Singleton execution mode), or as a
daemon (the Forwarded execution mode).
Un-skip the TestGoToStdlibDefinition test, as hopefully it will no
longer flake.
Updates golang/go#37318.
Change-Id: Id73ee3c8702ab4ab1d039baa038fbce879e38df8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221379
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Related to https://github.com/microsoft/vscode-go/issues/3063.
A small collection of fixes: (1) Don't reload packages with missing
dependencies unless imports change. (2) Show the error message from the
initial workspace load to the user.
Also, a small fix for deletion suggested fixes - upgrading to the latest
version of the VS Code language client revealed that I had made a
mistake there.
Change-Id: I7ab944ed27dc3da24a79d5d311531a1eb2b73102
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221217
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Instead we only have a single exporter, and it must delegate behaviour
to any other exporters it wants to include.
This removes a whole collection of suprises caused by init functions adding
new exporters to a list, as well as generally making things faster, at the small
expense of needing to implement a custom exporter if you want to combine the
features of a few other exporters.
This is essentially the opposite of https://go-review.googlesource.com/c/tools/+/212243
which will now be abandoned in favor of this approach.
Change-Id: Icacb4c1f0f40f99ddd1d82c73d4f25a3486e56ce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220857
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
The highlight tests imported golang.org/x/tools/internal/lsp/protocol
package, which doesn't exist when the testdata is set up in the test
harness. This causes gopls to try to reload the metadata for the
package. I'm not sure why this wasn't an issue before, since we should
re-run go/packages.Load every time we have missing dependencies.
Fixesgolang/go#37365
Change-Id: I8ebcbbf78b7e6fcdac9ab83bef3f5a0c9a50a361
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221107
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
I had meant to continue instead of break this loop. This caused us to
invalidate all IDs (except for those for one file) every time a snapshot
was cloned.
Picked up a staticcheck fix along the way.
Change-Id: I8fb3b2bdd6b58ac21130e01cb0d32fa6a57e6b73
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221103
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
This change implements support for textDocument/hover when it comes to go.mod files.
Updates golang/go#36501
Change-Id: Ie7da0194bb972955b7ab9cf7b9c9972bd9f4b8a9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220359
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change implements support for textDocument/documentLink when it comes to go.mod files.
Updates golang/go#36501
Change-Id: Ic0974e3e858dd1c8df54b7d7abee085bbcb6d4ee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/219938
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We had previously only added to a list of package IDs, rather than
invalidating package IDs along with metadata. This is because we loaded
packages by file, rather than by package path or workspace. Now that we
load by workspace, we can invalidate package IDs. This will avoid the
issue of lingering "command-line-arguments" IDs.
Fixesgolang/go#37213
Change-Id: I21830219efaf0df9531e9d811ccbc3f141c0cbcb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220197
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Searching with an empty string shouldn't return every symbol in the
workspace -- nobody wants that. Limit to 100 results to avoid breaking
editors. (VS Code locks up for like 30 seconds on my workspace.)
Change-Id: I1e0be476e8eeaef9e69767bfa04a89d40bd3a6e5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220939
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We want the go command to get a chance to do its cleanups. Send it
Interrupt before Kill.
Fixesgolang/go#37368.
Change-Id: Id891d2f4f85aae30d2aaa19b9c854d2346ec6e1f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220738
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Over time we have accumulated a disturbing number of ways to run the Go
command, each of which supported slightly different features and
workarounds. Combine all of them.
This unavoidably introduces some changes in debug logging behavior; I
think we should try to fix them incrementally and consistently.
Updates golang/go#37368.
Change-Id: I664ca8685bf247a226be3cb807789c2fcddf233d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220737
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Fix various things related to regtest execution:
+ Check the error from OpenFile in fake.Editor.GoToDefinition.
+ Add an error-checked wrapper to env for CloseBuffer.
+ Use env wrappers in TestDiagnosticClearingOnClose.
+ Use os.Executable to get the test binary path.
+ Add a -listen.timeout to the remote gopls process, so that it is
cleaned up.
Updates golang/go#36879
Change-Id: I056ff50bdb611a78ad78e4f5e2a94a4f155cc9de
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220902
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Various bits of the module code use HasPrefix, which only works if all
paths are clean. Make sure they are.
No test; this is pretty esoteric and it's not easy to test.
Fixesgolang/go#36193.
Change-Id: I8c8e87d1882d149a1a129d8b7123bc95c115b2ad
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220659
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
By running the client connection before the forwarder-remote handshake
completes, we introduce a race in the TestDebugInfoLifecycle: the editor
may connect and initialize before the handshake, and assertions on the
debug state fail.
Requiring that the handshake complete before running the client
connection fixes this flakiness. It also seems like a good thing to
have the handshake complete before proceeding with any LSP.
Fixesgolang/go#37444
Change-Id: I07187797b4759bcaf99f5b0a0b3cd7ac7de34f43
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220903
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
I'm still not sure if we need to handle any other non-standard package
path apart from "command-line-arguments".
Also, a couple of staticcheck fixes.
Change-Id: I0bb3e60f6ffe104ff9027dbebb628020caaa1af4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220138
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Make all the benchmarks cleanly pass around the context to remove a non logging
difference.
Rename the non logging calls benchmark to Baseline
Change-Id: I24a33b0a817df403fb43c53b5f097bc1e9418af4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220520
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
It would be bad behavior if a gopls forwarder process started the gopls
daemon and it stuck around indefinitely. Add an idle timeout by default
for the gopls daemon.
Updates golang/go#34111
Change-Id: I0408a1e6a3b89d862803ae5c439d6aa0d8ed9494
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220521
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Gopls behavior on disconnection is currently somewhat undefined, because
it hasn't mattered when there was a single gopls session per binary
invocation. With golang/go#34111, this changes.
Checks are added to ensure clients and sessions are cleaned up when an LSP
connection closes. Also, normal client disconnection is differentiated
with the jsonrpc2.ErrDisconnected value.
Updates golang/go#34111
Change-Id: I74d48ad6dcfc30d11f7f751dcffb20c18a4cbaa3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220519
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
When running gopls against an automatically started remote instance, we
want the lifecycle of the remote to be detached from that of its
clients, so that it doesn't shut down while clients are still connected.
On the other hand, a gopls process can consume significant resources, so
we don't want it to remain when there are no more connected clients.
The jsonrpc2 package is updated to support the concept of idle timeout:
a duration after which the server is shut down when there are no
connected clients. This is exposed in the gopls serve command via the
-listen.timeout flag.
Update golang/go#34111
Change-Id: Id62b3d4a2fa66de2c9306d130ca431717f01d1e5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220281
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Most users will not want to manage their own gopls instance, but may
still want to benefit from using a shared instance.
This CL adds support for an 'auto' network type that can be encoded in
the -remote flag similarly to UDS (i.e. -remote="auto;uniqueid"). In
this mode, the actual remote address will be resolved automatically
based on the executing environment and unique identifier, and the remote
server will be started if it isn't already running.
Updates golang/go#34111
Change-Id: Ib62159765a108f3645f57709b8ff079b39dd6727
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220137
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
In the ideal future, users will have one or more gopls instances, each
serving potentially many LSP clients. In order to have any hope of
navigating this web, clients and servers must know about eachother.
To allow for such an exchange of information, this CL adds an additional
handler layer to the serving configured in the lsprpc package. For now,
forwarders just use this layer to execute a handshake with the LSP
server, communicating the location of their logs and debug addresses.
Updates golang/go#34111
Change-Id: Ic7432062c01a8bbd52fb4a058a95bbf5dc26baa3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220081
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
For testability, and to support the exchange of debug information across
Forwarder and server, it is helpful to encapsulate all debug information
on the instance object.
This CL moves all state in the debug package into a new 'State' type,
that is added as a field on the debug.Instance. While doing so, common
functionality for object collections is factored out into the objset
helper type.
Also add two new debug object types: Client and Server. These aren't yet
used, but will be in a later CL (and frankly it was easier to leave them
in this CL than to more carefully rewrite history...).
Updates golang/go#34111
Change-Id: Ib809cd14cb957b41a9bcbd94a991f804531a76ea
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220078
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
