1
0
mirror of https://github.com/golang/go synced 2024-10-01 01:18:32 -06:00
Commit Graph

4640 Commits

Author SHA1 Message Date
Muir Manders
2047c2d578 internal/lsp/source: complete keywords as types
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>
2020-03-08 01:33:38 +00:00
Rohan Challa
51e69f7192 internal/lsp/cache/mod: return errors only within ModHandle and ModTidyHandle
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>
2020-03-06 19:16:17 +00:00
Rohan Challa
e4e22f76d0 internal/lsp: support when hierarchicalDocumentSymbolSupport is false
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.

Fixes golang/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>
2020-03-06 18:17:37 +00:00
Ian Cottrell
ca9608b5cd internal/telemetry: refactor the benchmark framework
Change-Id: I566ed7cd3a4d755cf06504e749ba54d0a309e53f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221921
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-03-06 18:16:20 +00:00
Rob Findley
8287d625a0 internal/lsp/debug: guard rpc stats with a Mutex
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>
2020-03-06 16:15:13 +00:00
Rohan Challa
a0897bacdd internal/lsp/cmd: improve flexibility of suggested fixes
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>
2020-03-06 14:31:35 +00:00
Rohan Challa
136c3617b6 internal/lsp: support textDocument/formatting for .mod extension
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>
2020-03-06 13:51:27 +00:00
Rob Findley
de023d59a5 internal/lsp/protocol: unmarshal to pointers when dispatching requests
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#34111
Fixes golang/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>
2020-03-05 22:45:36 +00:00
Rob Findley
d1d1f200c6 internal/lsp/lsprpc: use Setsid on POSIX GOOSes, to avoid SIGTERMs
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>
2020-03-05 21:44:44 +00:00
Rob Findley
bc073721ad internal/lsp/cache: include session IDs in some cache keys
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>
2020-03-05 20:50:14 +00:00
Rob Findley
6a641547f5 internal/lsp/cache: fix NPE in fileWasSaved
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.

Fixes golang/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>
2020-03-05 18:53:22 +00:00
Dan Kortschak
d7d4448666 internal/tool: avoid editorialization
Change-Id: Ic274b13ce7b621e2baf4c2659fb4e7facd6b19b4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212581
Reviewed-by: Ian Cottrell <iancottrell@google.com>
2020-03-05 14:01:59 +00:00
Ian Cottrell
32f14692fc internal/lsp: use standardised events for tagging
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>
2020-03-05 14:00:10 +00:00
Heschi Kreinick
95d2e580d8 internal/lsp/source: use logical filenames for workspace symbols
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.

Fixes golang/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>
2020-03-04 19:39:43 +00:00
Rohan Challa
d6a4d55695 internal/lsp/cache: attach ModTidyHandle to snapshot instance
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>
2020-03-04 14:31:13 +00:00
Ian Cottrell
c4206d458c internal/telemetry: change tracing to be event based
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>
2020-03-04 02:41:40 +00:00
Ian Cottrell
c5a1414753 internal/telemetry: add simple tracing benchmarks
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>
2020-03-03 22:57:24 +00:00
Ian Cottrell
3e346efd93 internal/telemetry: move the benchmarks to the main package
Change-Id: I9aabed798951ffba775c2255c8baafd56b009636
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221738
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>
2020-03-03 22:57:05 +00:00
Ian Cottrell
046aa1cdaf internal/telemetry: moving towards a unified event based exporter
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>
2020-03-03 22:56:52 +00:00
Ian Cottrell
a1c56757aa internal/telemetry: remove the concept of a Tagger
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>
2020-03-03 22:56:24 +00:00
Ian Cottrell
ae19ec7143 internal/telemetry: use atomics to get the exporter
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>
2020-03-03 22:56:03 +00:00
Ian Cottrell
4183ba16a9 internal/lsp: move the debug.Instance onto the Context
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>
2020-03-03 22:54:53 +00:00
Rebecca Stambler
2b0b585e22 internal/imports: don't set a logger unless the user has provided it
Not sure why I thought it was a good idea to enable extra logging
by default, but this was added in CL 184198.

Fixes golang/go#37636

Change-Id: I1840a9e53625db99c9097f2c23500ae20d6dac1f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221918
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
2020-03-03 21:46:25 +00:00
Rebecca Stambler
658b03bcd3 all: upgrade outdated dependencies
Used Rohan's new code lens feature to upgrade some of our outdated
dependencies.

Change-Id: Ifddd5c00f36e5a7e32aca3113e03cd3caeb4cdb9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221102
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
2020-03-03 20:20:40 +00:00
Rohan Challa
55b754b4e0 internal/lsp: fix active param in signature help
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.

Fixes golang/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>
2020-03-03 19:47:17 +00:00
Rohan Challa
115860e962 internal/lsp: fix concurrent map read/write for missingmodules
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>
2020-03-03 19:46:16 +00:00
Jonathan Amsterdam
4fc520f0d3 go/analysis/passes/errorsas: clarify message
Make it clear that the second argument must be a non-nil pointer.

The new message text matches the phrasing used in the errors.As doc.

Updates golang/go#37625.

Change-Id: I69dc2e34a5f3a5573030ba0f63f20e0821be1816
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221877
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
2020-03-03 19:44:43 +00:00
Heschi Kreinick
5bcca83a78 internal: rationalize debug logging
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>
2020-03-03 16:59:18 +00:00
Rohan Challa
9b52d559c6 internal/lsp/regtest: add a test for diagnostics on first file
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>
2020-03-02 22:55:59 +00:00
Rohan Challa
c4f5635f10 internal/lsp: add an upgrade all dependencies codelens
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>
2020-03-02 21:30:18 +00:00
Muir Manders
8d77031968 internal/lsp/source: fix completion crash with untyped assignee
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>
2020-03-02 21:29:26 +00:00
Muir Manders
ae0473a2ca internal/lsp/source: support inverse "implementations"
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>
2020-03-02 21:28:54 +00:00
Rob Findley
49e4010bbf internal/lsp/regtest: implement formatting and organizeImports
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>
2020-03-02 19:16:53 +00:00
Rob Findley
9aa23abf06 internal/lsp/regtest: consolidate Env wrappers
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>
2020-03-02 19:06:06 +00:00
Stephen Searles
b1e4e04173 go/analysis/passes/nilness: detecting panic with provably nil values
Calling `panic` with a nil value is not possible to detect using
recover. The existing SSA-based "nilness" analyzer already inspects
relationships with simple comparisons to nil and the surrounding code,
pointing out mistakes where provably nil values are used incorrectly.
This change adds calls to `panic` with similarly provable nil values to
the circumstances to report.

The tests included cover common forms of the mistake. In particular, the
case where the value passed to panic was of type `error` (which is to
say, an interface type), the nil was not properly detected.
Incorporating unpacking the `ChangeInterface` SSA transformation allow
these cases to be detected. This change may have an impact on the cases
caught by existing nilness circumstances, but those only in that they
detect additional true problems. The difference is considered safe
because the ChangeInterface operation does not change the kind of value
or its degree of nilness.

In discussion on this change, it was also pointed out that type
assertions using the CommaOk form do not cause panics when asserting on
nil values. The analyzer previously reported a false positive "nil
dereference" error in those cases and that is now fixed.

Updates golang/go#25448

Change-Id: Ia0c62e84a841c91837ff4b155f66dd2a2739c267
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220777
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-03-02 15:56:37 +00:00
Pontus Leitzler
7ae42e3a8b internal/lsp/debug: add nil checks in debug page
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>
2020-03-02 15:51:55 +00:00
Pontus Leitzler
066e0c0245 internal/lsp/lsprpc: use localhost for remote gopls debug interface
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>
2020-03-01 22:23:51 +00:00
Ian Cottrell
71482053b8 internal/telemetry: remove Flush method from exporter
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>
2020-02-28 22:46:39 +00:00
Ian Lance Taylor
a628ca32eb go/packages: drop imports of reflect in tests of import graph
In CL 219639 the generated testing package gets a dependency on reflect.
Trim that dependency in the go/packages tests, just as we trim
other dependencies of the generated testing package.

Updates golang/go#34129

Change-Id: I5e2578fc61cafb4d524d0a58525cf78a402c1143
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221477
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
2020-02-28 19:43:28 +00:00
Marwan Sulaiman
5c7c66ced5 gopls/doc: update godoc.org to pkg.go.dev
Now that it's the default link target.

Change-Id: Ic90700168711a16fc0c98f980c6d449ca1328669
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221498
Reviewed-by: Ian Cottrell <iancottrell@google.com>
2020-02-28 13:56:38 +00:00
Rob Findley
706bc42d1f gopls/doc: add documentation for running gopls as a daemon
Some initial documentation is added describing how to use a shared gopls
daemon, following the work in golang.org/issues/34111.

Updates golang/go#34111

Change-Id: I8ee07d7415ae799bb867973c34bda53f9a35126d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221138
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-02-27 22:23:43 +00:00
Heschi Kreinick
b3f10971cb internal/fastwalk: fix checkptr failure on Darwin
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.

Fixes golang/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>
2020-02-27 19:33:42 +00:00
Rob Findley
afe1c6fc1b internal/lsp/regtest: remove calls to t.Parallel()
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>
2020-02-27 18:46:34 +00:00
Rebecca Stambler
204d844ad4 internal/lsp: mitigate possiibility of a slow code action
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>
2020-02-26 22:45:02 +00:00
Ian Cottrell
2e887e3d13 internal/telemetry: removing the concept of exporter lists
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>
2020-02-26 21:51:01 +00:00
Rebecca Stambler
eb7c56241b internal/lsp: remove unknown dependency from highlight tests
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.

Fixes golang/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>
2020-02-26 20:52:01 +00:00
Anthony Fok
26f6a1b680 internal/lsp/source: fix typo: identifer → identifier
Change-Id: I37da513eb4a28df5054066b94ff1d915b9dce239
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221022
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-02-26 18:09:45 +00:00
Rebecca Stambler
020676185e internal/lsp: replace mistaken "break" with "continue"
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>
2020-02-26 17:12:34 +00:00
Rohan Challa
abb57c682a internal/lsp: support textDocument/hover for .mod extension
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>
2020-02-26 15:59:49 +00:00
Rohan Challa
48cfad2f5e internal/lsp: support textDocument/documentLink for .mod extension
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>
2020-02-26 15:45:04 +00:00