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>
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>
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>
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 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>
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>
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>
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>
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>
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>
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>
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>
The code to check if a candidate object matches our candidate
inference had become complicated, messy, and in some cases incorrect.
The main source of the complexity is the "derived" expected and
candidate types. When considering a candidate object "foo", we also
consider "&foo", "foo()", and "*foo", as appropriate. On the expected
side of things, when completing the a variadic function parameter we
expect either the variadic slice type and the scalar element type.
The code had grown organically to handle the expanding concerns, but
that resulted in confused code that didn't handle the interplay
between the various facets of candidate inference.
For example, we were inappropriately invoking func candidates when
completing variadic args:
func foo(...func())
func bar() {}
foo(bar<>) // oops - expanded to "bar()"
and we weren't type matching functions properly as builtin args:
func myMap() map[string]int { ... }
delete(myM<>) // we weren't preferring (or invoking) "myMap()"
We also had methods like "typeMatches" which took both a "candidate"
object and a "candType" type, which doesn't make sense because the
candidate contains the type already.
Now instead we explicitly iterate over all the derived candidate and
expected types so they are treated the same. There are still some
warts left but I think this is a step in the right direction.
Change-Id: If84a84b34a8fb771a32231f7ab64ca192f611b3d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218877
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
This change updates Staticcheck to the newly released 2020.1.2.
Change-Id: I80606b9c993de2f504c0ca3ee68f695ec8bd50e9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220477
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change improves the temporary go.mod file name to include the base directory of the folder that is opened.
Change-Id: Ide759222e268bdc24f82b29625ac18f4f285aa64
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220361
Run-TryBot: Rohan Challa <rohan@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This test is flaking on the Trybots. Skip it until this is understood.
Updates golang/go#37318
Change-Id: Ie4c1db47797b88d5eb201a73c4ddfb5481f362ea
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220360
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
