1
0
mirror of https://github.com/golang/go synced 2024-11-05 20:36:10 -07:00
Commit Graph

1811 Commits

Author SHA1 Message Date
Muir Manders
41a9f6dc66 internal/lsp/source: fix function completion ranking
I noticed an annoying completion ranking issue:

   // ranks "HandlerFunc" over "HandleFunc"
   http.HandleFunc<>

This was due to us downranking function calls to prefer fields/vars. I
tweaked the logic to only downrank methods (with a receiver).

Change-Id: Ia4040dc8a35f641be2d7d934bf555090831219ac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246357
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-06 22:03:01 +00:00
tennashi
9099c90ca5 internal/lsp: fix the first change check
In the current implementation, the return value of wasFirstChanges() is
reversed and did not record the URI of the changed file.  In addition,
the snapshot was not stored in the snapshotByURI.

So I fixed when didChange() is executed, the URI of the edited file is
registered and wasFirstChanges() returns true if the URI is not
registered.  Also the snapshot is now stored in snapshotByURI.

Fixes golang/go#40531

Change-Id: I0adbf7459593d70660beb3b37900ffc88f707917
Reviewed-on: https://go-review.googlesource.com/c/tools/+/247118
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-06 20:04:45 +00:00
Josh Baum
110bd3ba6b internal/lsp: make function extraction smarter
In the previous implementation, the extracted function would
sometimes include superfluous parameters and return values. It
might also  unnecessarily initialize variables. This CL introduces
3 rules to limit this behavior. (1) a variable is not passed as a
parameter to the extracted function if its first use within the
function is its own redefinition. (2) a variable is not returned
from the extracted function if its first use after the function is its
own redefinition. (3) when possible, we redefine variables in the call
expression to the extracted function.

Change-Id: Ideb5a7eff8a1bf462c83271a2f043116ff5d8b76
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244770
Run-TryBot: Josh Baum <joshbaum@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-06 19:37:29 +00:00
Rebecca Stambler
ee49e490f2 internal/lsp/source: fix bug introduced by CL 246757
CL 246757 resulted in an infinite loop because the value of "o" is
never updated.

Change-Id: I79cf265349838de19089c4468128c565a9a3cda3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/247182
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Baum <joshbaum@google.com>
2020-08-06 18:32:31 +00:00
Muir Manders
90696ccdc6 internal/lsp/source: simplify variadic logic in completion
In the example:

    append([]T{}, <>)

We used to track the "objType" as "[]T", and "variadicType" separately
as "T". However, most things are more interested in "T" vs "[]T", so
they had to fiddle around with swapping types. Now instead we track
objType as T, and add a "variadic=true" flag indicating that "[]T" is
also an acceptable type.

Change-Id: I8ee3ef840917378c8406368cb5c660a377498dfd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246698
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-06 02:28:45 +00:00
Muir Manders
6fe4996ff4 internal/lsp/source: fix multi return value func completion
Fix a minor completion ranking issue:

    foo := func(int, int) {}
    foo(123, <>)

Previously we were preferring "foo()" at "<>" even though it can't be
used. We mistakenly thought we were completing the first param because
the *ast.CallExpr appears to only have a single param.

Change-Id: Iedbbb1870a4b9eb5d5be4ed266b8bb3e313b496b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246697
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-06 02:28:35 +00:00
Peter Weinbergr
0b898c9289 internal/lsp: add options to control which details gc_details shows
The gc_details command, which shows the gc compiler's decisions, can
produce thousands of diagnostics for a package. New gopls options
'noBounds', 'noEscape', 'noInline', 'noNilcheck' will suppress diagnostics
of less interest to the user. These are in a new 'annotations' section
parallel to 'codelens' or 'analyses'.

Change-Id: Ica75de25b14f38b67ddfa9f997f581674f45221d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246477
Run-TryBot: Peter Weinberger <pjw@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-05 22:37:31 +00:00
Heschi Kreinick
f29cbc7105 internal/lsp: remove source.Cache
snapshot.View().Session().Cache().FileSet() has been driving me crazy
for a while. Add it to snapshot. Along the way, discover that the Cache
interface is now totally unused and delete it.

I also changed a bunch of View arguments to Snapshot while I was in the
area.

Change-Id: I1064d0020b1567c2ed28d2d55e0f4649eb94c060
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245324
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-05 22:08:24 +00:00
Rebecca Stambler
b4d825fe35 internal/lsp: fix applyTextDocumentEdits and delete extra function
Follow-up from CL24645 246458.

Change-Id: I77872154e2aa2d6028add84dadc5d8aad05b59ee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246759
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Baum <joshbaum@google.com>
2020-08-05 20:02:55 +00:00
Rebecca Stambler
d55f2eddcb internal/lsp/source: fix nil pointer in extract function
Ran into this while debugging another issue.

Change-Id: I154493418c7676a24457a4e11431ad4f0311c07a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246757
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Baum <joshbaum@google.com>
2020-08-05 15:47:06 +00:00
Josh Baum
e5d681aac7 internal/lsp: remove excess 'zero values' in return statements
In the previous implementation, fillreturns only altered return
statements that contained too few values. Now, fillreturns also examines
return statements with too many return values. In these situations,
we remove any value that is a "zero value" and does not match a type
in the return signature.

Change-Id: I7548307234ff4b16534b72a8aead95a322eb535a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246520
Run-TryBot: Josh Baum <joshbaum@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-05 14:29:31 +00:00
Danish Dua
fec4f28ebb internal/lsp/source: fix completion prefix for comment completion
This CL addresses completion prefix not being overwritten for completion
in comments for exported variables/functions/types etc. Instead of
setting the surrounding range as cursor position, we expand out from
cursor instead to replace the word we're currently on.

Fixes golang/go#39262

Change-Id: I90c28562e3ef285ce6848598f8d7bd7545d5c957
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246237
Run-TryBot: Danish Dua <danishdua@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-08-04 23:49:16 +00:00
smasher164
25c5b132c9 internal/imports: update stdlib index for 1.15
$ go run mkstdlib.go

Updates golang/go#38706.

Change-Id: I9d1bba54c4f9a0369b1d088c00a2f0c3e4409806
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246581
Run-TryBot: Akhil Indurti <aindurti@gmail.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-08-04 21:03:08 +00:00
Josh Baum
5c72ddda61 internal/lsp: fix bug in extract variable edit positions
Previously, the suggested fix tests did not properly handle the case
in which one fix contained at least two edits. We also prevent
the server from panicing when we cannot extract the selection.

Change-Id: I38f7b6d871b2f2741349a3fd94fd95b396f5fd33
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246458
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-08-04 20:55:55 +00:00
Josh Baum
b2519014df internal/lsp: fail test if command cannot be applied
In the previous implementation, a test would pass if the given
command could not be applied to the given range.

Change-Id: I2e63972472cbd146cb5f27a3e27c878387222cb6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/246517
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-04 20:25:19 +00:00
Rebecca Stambler
d73700863c internal/lsp: get the correct declaration node when finding references
We were returning the AST node for the identifier that the rename was
called from, not the actual declaration, so the doc comments weren't
getting updated.

Fixes golang/go#40463

Change-Id: Id8ba0b0aeb8f42d2aaa561e7a964edcca5202916
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245817
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-08-04 17:27:45 +00:00
Heschi Kreinick
6c149bb5ef internal/lsp/cache: store parseGoHandles
parseGoHandles have lifetimes separate from the packages they belong to.
For example, a package may be invalidated by a change to one of its
files, but we still want to retain the parse results for all the rest.
Track them explicitly.

Change-Id: I03a4ffe283bf2b252d2d838bdb2cf332cd981075
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245059
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-04 01:15:35 +00:00
Danish Dua
5a22b632c5 internal/lsp: refactor line folding range code
This CL removes duplicate code in lineFoldingRange function under
lsp/source/folding_range.go and generally improves code quality.

Fixes bug with composite literal folding where gopls was folding literals
with braces on the same line as end token (paranthesis/braces).

Change-Id: I742f285d866d72a243129c0aef0935fe2a1ad0dd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245757
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-08-03 22:55:02 +00:00
Heschi Kreinick
c9619e8fac internal/lsp: separate LSP files from FS files
FileHandle currently includes LSP-level information about Version and
Session. That's dangerous, because the cache operates in terms of
URIs and content only -- we explicitly want to share results across
sessions and versions if they happen to be the same.

Split the LSP information into separate types, VersionedFileHandle and
VersionedFileIdentity.

Change-Id: I158646b783375b58245468599301e2a29c657e71
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245058
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-08-03 22:16:06 +00:00
Heschi Kreinick
39fdd541e6 internal/lsp: move builtin package to Snapshot
The builtin package was the one special case where we parsed Go outside
the context of a Snapshot. Move it up.

Change-Id: I1f4bb536adb40019e0ea9c5c89f38b15737abb8c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245057
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-03 22:09:14 +00:00
Heschi Kreinick
412b8bda49 internal/lsp/cache: ref-count snapshots
To manually collect cache entries, we need to know when a snapshot is
idle. Add a reference count in the form of a WaitGroup and keep track of
its uses. The pattern is that any time a snapshot is returned, it comes
with a release function that decrements the ref count.

Almost all uses of a snapshot originate in a user-facing request,
handled in beginFileRequest. There it's mostly an exercise in passing
Snapshots around instead of Views.

In the other places I took the path of least resistance. For file
modifications I tried to minimize the amount of code that needed to deal
with snapshots. For diagnostics I just acquired the snapshot at the
diagnostics call.

Change-Id: Id48a2df3acdd97f27d905e2c2be23072f28f196b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/241837
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-08-03 22:08:54 +00:00
Rebecca Stambler
b5fad4ed8d internal/lsp: invalidate all packages on go.mod file changes
Previously, we were only invalidating workspace packages when the go.mod
changed, but we really need to be invalidating all known packages.

Fixes golang/go#40456

Change-Id: I9ad353a26ab40c74c7760ed7a1c5de517640cfab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245779
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-07-31 06:09:45 +00:00
Danish Dua
1ac65761fe internal/lsp: fix go.mod.golden test file to have sorted mod names
This change is generated whenever you run go test . --golden to generate
updated golden files. Commiting it so it doesn't show up in other CLs.
The module names are now sorted and hence this change shouldn't happen again.

Change-Id: I5d6c44d9a4f3ca24e336afad301136af7123eef7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245997
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Danish Dua <danishdua@google.com>
2020-07-30 22:19:56 +00:00
Pontus Leitzler
fe6bb45d21 internal/lsp: use URI as key in gc optimization details map
Packages that have gc_details enabled is stored internally in a
map[span.URI]struct{}, but when toggling on/off via ExecuteCommand, the
scheme is lost. The mismatch lead to no diagnostics from gc_details.

This fix uses span.URIFromPath() to parse the file path, so that the
internal keys are found when diagnostics are calculated.

Change-Id: I53b4e869625251c366b12b570eb03212319d2770
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245858
Run-TryBot: Pontus Leitzler <leitzler@gmail.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-07-30 20:01:20 +00:00
Danish Dua
644d416712 internal/lsp: add folding range for select case statements
This CL adds folding ranges for CommClause nodes (which represent select
statement cases).

Fixes golang/go#40146

Change-Id: Ifcc91cc43714d52346445fc02d9eee8b8364af20
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245697
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-07-30 18:32:39 +00:00
Heschi Kreinick
6467de6f59 internal/lsp: remove Mod handles
Continuing the massacre, remove ParseModHandle, and Mod*Handle, from the
source API.

Notably, having the snapshot available means we can simplify the go
command invocation paths a lot.

Change-Id: Ief4ef41e42f93d653f719a230004861e5e1ef70b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244769
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-07-29 19:44:36 +00:00
Rebecca Stambler
b47602a87a internal/lsp: handle nil pointer in go.mod links
Fixes golang/go#40433

Change-Id: I9e85f6d28886f8addf1d9142bc87f22eee1a4a36
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244840
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-07-29 19:22:10 +00:00
Rebecca Stambler
6b78e25f47 internal/lsp: fix a few staticcheck suggestions, some cleanup
There were a few merge conflict-related issues in the GC optimization
details CL. Also fixed a few things I noticed after the fact, like
separating out a new mutex.

Staticcheck caught a few things, and I also fixed a bug I noticed
in the cache package.

Change-Id: I3fc519373253418586dca08fdec3114b30a247ea
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245399
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
2020-07-29 17:11:59 +00:00
Rebecca Stambler
8afe7e0893 internal/lsp: don't show diagnostic in go.mod for direct dependencies
Fixes golang/go#40470.

Change-Id: I4831b837e6db5cc344c20480abde1e688ddab0dc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245537
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
2020-07-29 17:04:04 +00:00
Rebecca Stambler
449c585133 internal/lsp: properly check for nil on i.enclosing
Fixes golang/go#40479

Change-Id: If827a86fd603574d3adde295c1165bb6267732eb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245486
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-07-29 16:35:35 +00:00
Josh Baum
1c1ec420e5 internal/lsp/source: move initial extract function logic to lsp/command
In the previous implementation, the initial verification in lsp/command
for whether extract function was relavant to the given range did not
contain much of the initial logic for extract function. This meant
that "canExtractFunction" produced many false positives (i.e. the
lightbulb would appear when it should not have in VSCode). This CL
moves more of the verification process from "extractFunction"
(lsp/source) to "canExtractFunction" (lsp/command).

Change-Id: If2683dc9ac3f4bfa8c3444418cf88edd8cbe73e6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245398
Run-TryBot: Josh Baum <joshbaum@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-07-29 16:09:07 +00:00
Rebecca Stambler
df70183b18 internal/lsp/source: add extra nil check in searchForEnclosing
I still keep seeing this crash too, even after CL 244841.

Fixes golang/go#40464

Change-Id: Ic587045e65f34c24bd6df452e24517fd90e36bbe
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245440
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-07-29 04:18:21 +00:00
Peter Weinbergr
e8769ccb43 internal/lsp: fix logged message for gc_details
Removes leftover debugging string from the message logged when
gc_details executes.

Change-Id: I802b276d1590e0b1e9793190990a1e576dc893ed
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245417
Run-TryBot: Peter Weinberger <pjw@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-07-28 23:52:36 +00:00
Rebecca Stambler
0dd562f374 internal/lsp: remove uses of crypto/sha1 in gopls
SHA1 can have collisions. Replaced with crypto/sha256.

Change-Id: I54e8c042ae1a4eb41760ccbe26a7c59f68309477
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245326
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-07-28 22:26:17 +00:00
Peter Weinbergr
95780ea8b3 internal/lsp: show compiler optimization decisions
The gc compiler will report its decisions about inlining, escapes, etc.
This can be turned on and off with a new optional code lens gc_details.
When enabled, the code lens will be displayed above the package
statement. The compiler's decisions are shown as information diagnostics.
(Other diagnostics have been errors and warnings.)

Change-Id: I7d1d5b5b5cf8acd7ff08f683e537ea618e269547
Reviewed-on: https://go-review.googlesource.com/c/tools/+/243119
Run-TryBot: Peter Weinberger <pjw@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-07-28 21:59:21 +00:00
Rebecca Stambler
edd3c8e9e2 internal/lsp: de-duplicate logic for canExtractVariable
We'd like to call canExtractVariable in extractVariable without
duplicating logic. The same needs to be done for canExtractFunction.

Change-Id: Ia99befabbafffcf13dd3bc12355f9ddb81a71002
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245135
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Josh Baum <joshbaum@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-07-28 19:08:22 +00:00
Rebecca Stambler
342ee1054f internal/lsp: check URIs of all workspace folders
We can prevent crashing on non-file URIs by checking the URIs of the
workspace folders, as well as the root URI.

Updates golang/go#40272

Change-Id: Ieddc6d6053fbb3d61e4c26fc8831c092328f6f33
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244602
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-07-28 19:06:24 +00:00
Rob Findley
e7a7e3a8a0 internal/lsp/source: add a unit test for searchForEnclosing
The logic to resolve the enclosing type for an identifier is somewhat
tricky. Add a unit test to exercise a few edge cases.

This would probably be easier to read and write using a hybrid approach
that extracts markers from the source.

This test uncovered a bug, that on the SelectorExpr branch we were
accidentally returning a nil *Named types.Type, rather than a nil
types.Type.

Change-Id: I43e096f51999b2a6e109c09d3805ad70a4780398
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244841
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-07-28 18:51:24 +00:00
Heschi Kreinick
b6476686b7 internal/lsp: remove PackageHandle
Just like ParseGoHandle, PackageHandle isn't very useful as part of the
public API. Remove it.

Having PackagesForFile take a URI rather than a FileHandle seems
reasonable, and made me wonder if that logic applies to other calls like
ParseGo. For now I'm going to stop here. I could also revert that part
of the change.

Change-Id: Idba8e9fdba0b0c48e841a698eb97e47fd5f23cf5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244637
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-07-28 17:35:25 +00:00
Heschi Kreinick
a9439ae9c1 internal/lsp: replace ParseGoHandle with concrete data
ParseGoHandles serve two purposes: they pin cache entries so that
redundant calculations are cached, and they allow users to obtain the
actual parsed AST. The former is an implementation detail, and the
latter turns out to just be an annoyance.

Parsed Go files are obtained from two places. By far the most common is
from a type checked package. But a type checked package must by
definition have already parsed all the files it contains, so the PGH
is already computed and cannot have failed. Type checked packages can
simply return the parsed file without requiring a separate Check
operation. We do want to pin the cache entries in this case, which I've
done by holding on to the PGH in cache.pkg.

There are some cases where we directly parse a file, such as for the
FoldingRange LSP call, which doesn't need type information. Those parses
can actually fail, so we do need an error check. But we don't need the
PGH; in all cases we are immediately using and discarding it.

So it turns out we don't actually need the PGH type at all, at least not
in the public API. Instead, we can pass around a concrete struct that
has the various pieces of data directly available.

This uncovered a bug in typeCheck: it should fail if it encounters any
real errors.

Change-Id: I203bf2dd79d5d65c01392d69c2cf4f7744fde7fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244021
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-07-28 17:35:11 +00:00
Heschi Kreinick
4d1d9acccf internal/lsp/cache: fix parseKey
The FileIdentity struct mixes information about the file itself
(filename, hash) with information about the LSP references to that file
(session ID, version). When we create a cache key using it, we only want
the former, as returned by the String method. Otherwise we split the
cache whenever those irrelevant fields are different.

We also use FileIdentity as an element of diagnosticsKey, but I believe
that use is appropriate.

Change-Id: I094e00d2700e05778da635effbb69d0ebcb6726e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244020
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-07-28 17:34:57 +00:00
Heschi Kreinick
72051f7961 internal/lsp: pass snapshot/view to memoize.Functions
Due to the runtime's inability to collect cycles involving finalizers,
we can't close over handles in memoize.Functions without causing memory
leaks. Up until now we've dealt with that by closing over all the bits
of the snapshot that we want, but it distorts the design of all the code
used in the Functions.

We can solve the problem another way: instead of closing over the
snapshot/view, we can force the caller to pass it in. This is somewhat
scary: there is no requirement that the argument matches the data that
we're working with. But the reality is that this is not a new problem:
the Function used to calculate a cache value is not necessarily the one
that the caller expects. As long as the cache key fully identifies all
the inputs to the Function, the output should be correct. And since the
caller used the snapshot/view to calculate that cache key, it should
always be safe to pass in that snapshot/view. If it's not, then we
already had a bug.

The Arg type in memoize is clumsy, but I thought it would be nice to
have at least a little bit of type safety. I'm open to suggestions.

Change-Id: I23f546638b0c66a4698620a986949087211f4762
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244019
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-07-28 17:34:46 +00:00
Heschi Kreinick
60da08ac03 internal/lsp/cache: remove files from the memoize.Store
The memoize cache buys us little for files: the cache value is not
really a function of the inputs, but rather the filesystem state. It's
pretty much just as easy to manage them explicitly, and it's a start at
simplifying our caching strategy.

We do lose one small feature: if we try to read the same file
concurrently, reads will not be deduplicated. I suspect that doesn't
matter.

Change-Id: I75e219467fb7a512d9cfdf87443d012c85f03df9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/243197
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
2020-07-28 17:34:34 +00:00
Heschi Kreinick
051e64e62c internal/lsp: minimize PackageHandle interface
The PackageHandle interface is fairly redundant with the Package
interface. As much as convenient, move users to Package and
weaken/remove methods from PackageHandle.

I would like to get rid of CompiledGoFiles too but
NarrowestPackageHandle is a little annoying. I think this is
unambiguously a step forward so I figured we can get it in and keep
iterating.

Change-Id: I6c5a3f462b1f19cbca6a267fedc36ce54613b6fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244018
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-07-28 17:34:24 +00:00
Rebecca Stambler
2ad651e9e2 internal/lsp: handle bad formatting with CRLF line endings
The importPrefix logic is complicated by Windows line endings, since
go/ast isn't aware of different line endings in comment text. I made a
few changes to the way that import prefixes are computed to handle this.

Specifically, for comments, we try to make sure the range ends on a full
line as much as possible, because that addresses the line ending issue.

Fixes golang/go#40355

Change-Id: I84c1cfa0d0bae532e52ed181e8a5383157feef24
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244897
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-07-28 16:05:17 +00:00
Josh Baum
3c048e20c6 internal/lsp: support return statements in extract function
Previously, users could not extract code that contained a return
statement. Now, users can extract code with return statements, as long
as the statements are nested within an if, case, or other control
flow statement.

Updates golang/go#37170

Change-Id: I2df52d0241517472decabce3666a32392ff257bd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/243650
Run-TryBot: Josh Baum <joshbaum@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-07-28 16:04:03 +00:00
Peter Weinbergr
cd83430bb0 internal/lsp: update code for LSP protocol
1. Change code.ts so it generates []json.RawMessage in place of []interface{}
for Command-related arguments. As usual, vscode introduces a lot of
whitespace-only changes.
2. Generate code based on the July 28 version of vscode-languageserver.
The changes are mostly related to SemanticToken, and didn't require
any changes to gopls, other than in the generated code.

Change-Id: I673e29e2fbc097409683dfe7af911d8f66e25c5c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245134
Run-TryBot: Peter Weinberger <pjw@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2020-07-28 15:42:44 +00:00
Pontus Leitzler
55644ead90 internal/lsp: allow narrower scope for convenience CodeActions
Code actions that apply convenience fixes were filtered by the start
line so it wasn't possible to narrow the scope to a specific range.

This change allows clients to send a specific range (or cursor position)
to filter all fixes where the range doesn't intersect with the provided
range. It also widens the diagnostic returned by fillstruct analysis.

The idea is to provide a way to narrow the scope without breaking
clients that do want to ask for code actions using the entire line.

Updates golang/go#40438

Change-Id: Ifd984a092a4a3bf0b3a2a5426d3e65023ba4eebc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244519
Run-TryBot: Pontus Leitzler <leitzler@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
2020-07-27 23:36:28 +00:00
Rebecca Stambler
7b4c4ad3dc internal/lsp: correctly marshal arguments for upgrade code lens
I'm not sure how the regtest didn't catch this - is it possible that
it could unmarshal a single string a slice of string? Either way, I'd
like to get the fix in quicker - I'll try to add more regtests for this
later.

Also, validate the upgrade results more thoroughly.

Change-Id: I812a3fecd9f0642a1408c0a9c0376bb98d50b397
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245065
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-07-27 21:52:59 +00:00
Rebecca Stambler
59c6fc0b54 internal/lsp: correctly invalidate metadata for batched changes
I finally spent the time to understand why branch changes were causing
unexpected errors. There may be other bugs, but this is the first I
spotted. For batched invalidations, we were overriding the value of
invalidateMetadata for each file, so the results depended on the order
of files in the didChangeWatchedFiles notification.

Change-Id: Id3ca7a758af0115c46dcd74ede590a0be3f8307d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244606
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
2020-07-27 19:55:46 +00:00