1
0
mirror of https://github.com/golang/go synced 2024-11-18 22:55:23 -07:00
Commit Graph

4511 Commits

Author SHA1 Message Date
Ian Cottrell
206ec5b82a internal/lsp: migrate telemetry to using the event package
Change-Id: Idc662385ed08bd62593ccd1d54afd3fa8c1a7d29
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222558
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-03-12 03:59:16 +00:00
Ian Cottrell
d780ff7bdd internal/telemetry: unify the event handling to an event package
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>
2020-03-12 03:58:56 +00:00
Muir Manders
3dc7fec788 internal/lsp/cache: remove parseGo semaphore
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>
2020-03-12 03:18:52 +00:00
Tamir Duberstein
c807066ff7 all: run go generate
Change-Id: I2bbce4a336ecfd83836272078af2fa48f8724a83
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223078
Reviewed-by: David Chase <drchase@google.com>
2020-03-11 22:20:14 +00:00
Daisuke Suzuki
0d653b92c5 internal/lsp/tests: fix WorkspaceSymbols tests
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>
2020-03-11 18:46:36 +00:00
Dan Kortschak
aafaee8bce go/analysis/passes/printf: give leeway for fmt.Formatter satisfaction
We have no way of knowing the concrete type of an interface value;
it might be a fmt.Formatter. To avoid false positives, assume that
all interface values are fmt.Formatters.

Updates golang/go#36564

Change-Id: Iaf18ba2794e4d3095d0018502c1c6c459a360b42
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217180
Reviewed-by: Rob Pike <r@golang.org>
2020-03-11 09:07:12 +00:00
Muir Manders
43e3193a9b internal/lsp/source: suppress keyword completions in "case" statement
Now we no longer offer keyword completions inside "case" statements:

    select {
    case fo<>: // don't offer "for"
    }

Updates golang/go#34009.

Change-Id: I908eda32af01fd1912fb587ce59efef1798b6741
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220580
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-03-11 03:54:31 +00:00
Muir Manders
7bb885034f internal/lsp/source: fix completion in empty switch statements
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>
2020-03-11 03:54:16 +00:00
Ian Cottrell
f30ed8521d internal/telemetry: change detach to be an event
deliver detach to the exporter as an event rather than hard coding the
span detach behavior.

Change-Id: I87b6e2a3596fea338908c11ba0b219176b6305bf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222542
Reviewed-by: Robert Findley <rfindley@google.com>
2020-03-11 02:43:08 +00:00
Ian Cottrell
e8dd451b4b internal/telemetry: add a stats benchmark
Change-Id: I9cb1459a8589cec75ae29d0e53f044c6489b1ea2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222537
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-03-11 02:40:42 +00:00
Ian Lance Taylor
71bfc1b943 go/packages: drop pruned packages in tests of import graph
Explicitly listing the pruned packages ties the test to the details of
how the testing package is implemented. That shouldn't matter, and it
changes in CL 219639. This change makes it possible to work whether or
not CL 219639 is committed.

Updates golang/go#34129

Change-Id: I45875dadeaac42a1002e1329a1a762e340c5352c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221657
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Changkun Ou <euryugasaki@gmail.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
2020-03-10 23:16:27 +00:00
Muir Manders
0983143fce internal/lsp/source: avoid type checking in AllImportsFixes
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>
2020-03-10 23:10:53 +00:00
Peter Weinbergr
3e041465cc internal/lsp: change the type of CompletionItem.TextEdit back
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>
2020-03-10 21:12:04 +00:00
Rebecca Stambler
b84001dd64 internal/lsp: suggest filing an issue if a warning is incorrect
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>
2020-03-10 21:11:10 +00:00
Muir Manders
138c814f66 internal/lsp/source: offer completion "if err != nil { return err }"
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>
2020-03-10 21:06:53 +00:00
Peter Weinberger
20ab64c0d9 internal/lsp: bring generated protocol code up to date; stop corrupting logs
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>
2020-03-09 20:21:50 +00:00
Muir Manders
92d517f97b internal/lsp/source: suppress completions in ellipsis
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.

Fixes golang/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>
2020-03-09 18:42:16 +00:00
Rob Findley
aa4048aca1 internal/lsp/lsprpc: don't connect to sockets owned by different users
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>
2020-03-09 18:08:59 +00:00
Hana (Hyang-Ah) Kim
270c59dc40 gopls/integration/govim: update to go1.14 images
Change-Id: I24b6b20ca78867cc973ca0a96c388876f4f40c70
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222663
Reviewed-by: Robert Findley <rfindley@google.com>
2020-03-09 17:26:05 +00:00
Danish Prakash
c94e1fe145 internal/lsp/source: return location(s) for imported packages
Fixes golang/go#32993

Change-Id: Iff2f7ab8e34eea1ce7c1ab99fb2e51589dce95c4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/210321
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-03-09 16:25:02 +00:00
Muir Manders
11ec41452d internal/lsp/source: support completing "range" keyword
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>
2020-03-08 01:35:34 +00:00
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