Downranking builtins causes weird interplay with other completion
candidates due to fuzzy matching. For example:
notNil := 123
var foo *int = nil<>
ranks "notNil" before "nil" in the builtin list, which is counter
productive.
Change it to not downrank builtins. In my testing with this change,
builtins never were ranked above lexical items with similar names. I
think this is because the "natural" order of completion items puts
builtins last, and we stable sort items by score, so their relative
order is preserved.
Change-Id: Ifbad02be205e3cb26c1d4ce500b77690e7ac5b04
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193897
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The next in the sequence of CLs to convert to using protocol positions.
Change-Id: Ib3421bfc73af1b546b60c328ca66528cb9031e19
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193719
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This cl is the first in a set that change the configuration behaviour.
This one should have no behaviour differences, but makes a lot of preparatory changes.
The same options are set to the same values in the same places.
The options are now stored on the Session instead of the Server
The View supports options, but does not have any yet.
Change-Id: Ie966cceca6878861686a1766d63bb8a78021259b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193726
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The existing code uses maps to associate requests with responses. This
change adds locking to avoid simultaneous and illegal reads and writes.
Change-Id: I7bfb21cad6b37ac25e4f6946cb660d82f23c2b80
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193058
Run-TryBot: Peter Weinberger <pjw@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Folding ranges need to be computed to present folding ranges that make
sense when lineFoldingOnly is true. This change computes the folding
ranges to include the lines that are contained within the start and end
parenthesis/braces when applicable.
Folding ranges are not returned when the contained nodes begin or end on
the same lines as the parenthesis/brace. This is to avoid misleading
folding ranges like the following in unformatted code:
if true {
fmt.Println("true") } else {
fmt.Println("false")
}
---folding "if true {}"--->
if true {
fmt.Println("false")
}
Change-Id: I2931d02837ad5f2dd96cc93da5ede59afd6bcdce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/192678
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
In shouldRunGopackages we would reset a goFile's metadata and pkgs in
advance of re-running go/packages. However, if we did not end up
running go/packages for whatever reason (read: we got canceled), the
goFile gets stuck in the unfortunate state of not belonging to any
packages because "pkgs" is empty. I think this leads to "no
CheckPackageHandle" errors, at least in relation to GetCachedPackage()
calls.
Fix by deferring the reset of goFile's metadata and pkgs until after
the go/packages call has succeeded.
Change-Id: I95aace85c026e1232b42cadee9e7772951c817d0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193601
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change propagates the file handle through the type-checking
process, ensuring that the same handle is used throughout. It also
removes the ordering constraint that f.mu needs to be acquired before
f.handleMu. To make this more correct, we should associate a cached
package only with a FileHandle, but this relies on correct cache
invalidation, so that will be addressed in future changes.
Updates golang/go#34052
Change-Id: I6645046bfd15c882619a7f01f9b48c838de42a30
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193718
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
We had previously been returning the metadata map in a few of the
loading functions. We don't actually need the map; we only need the
actual metadata. The race was caused by the return of the f.meta field
in a few functions, unprotected by the f.mu lock. This was likely a
result of the f.mu lock being added after the fact.
Fixesgolang/go#33978
Change-Id: Ice5778d9d6dea23304237baf321b55d4fee6599c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193717
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
If someone puts something silly in their module cache, ignore it instead
of crashing.
Fixesgolang/go#34027.
Change-Id: I114e10010bd6bc483f865a628dc2b331c3a34a11
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193268
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
There are issues with contexts being propagated through the calls to
type-checking, and I think that a lot of these were related to us using
the importer's context. Instead, we should propagate the context from
the store as much as possible - only using the importer's context when
absolutely necessary (in the call to Import). This change propagates the
correct context where possible.
Updates golang/go#34103
Change-Id: I4bdc37d014ee1f775b720c9e7ad8abffffcf6ba3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193480
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This feature has been in an experimental state for a long enough time
that I think we can enable it by default at master.
Change-Id: I9bbb8b41377719f0e97f608f6e5163a883a176b3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/192259
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
The memoize store passes a detached context to the value getter
function. This is important since if the value getter experiences a
context cancellation it will end up caching context.Canceled, which
you never want. When type checking, we were ignoring the detached
context and using the "real" request context. This would cause the
context.Canceled error to get cached and continue popping up in
various situations.
Fix by swapping the importer's context to the detached context. It is
a little messy since the importer stores the context as a field. I
added a defer to restore the original context since it doesn't seem
correct to let the detached context escape the memoize function.
Updates golang/go#33678
Change-Id: I20dd466b0072ac2e856adbe993364f77e93ab054
Reviewed-on: https://go-review.googlesource.com/c/tools/+/192719
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
If the client registers with foldingRange.lineFoldingOnly = true, only
return folding ranges that span multiple lines. Do this as they are
computed, so that if other filtering is applied later, we do not include
ranges that would go unused by the client anyway.
Change-Id: I27ea24428d25f180e26892de0f6d16c211225bf7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/192477
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
FoldingRanges may be nested. Test nested folding ranges by separating
out the folding ranges by nested level and checking each level.
Change-Id: I12c72daa3e6c6b9d4959209b3a41b27e2b59866f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/192398
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The existing implementation did not suggest struct field names
when running completion from within a slice literal of
pointers. Now, struct field names are suggested in that case.
Fixesgolang/go#33211
Change-Id: I6028420a9a789846b070fcc6e45ec89dc4d898d4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/192277
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Invert "useDeepCompletions" config flag to "disableDeepCompletion" and
separate out "disableFuzzyMatching" which reverts to the previous
prefix matching behavior.
I separated fuzzy matching tests out to a separate file so they aren't
entangled with deep completion tests. In coming up with representative
test cases I found a couple issues which I fixed:
- We were treating a fuzzy matcher score of 0 as no match, but the
matcher returns 0 for candidates that match but have no bonuses. I
changed the matcher interface so that a score of 0 counts as a
match. For example, this was preventing a pattern of "o" from
matching "foo".
- When we lower a candidate's score based on its depth, we were
subtracting a static multiplier which could result in the score
going negative. A negative score messes up future score weighting
because multiplying it by a value in the range [0, 1) makes it
bigger instead of smaller. Fix by scaling a candidate's score based
on its depth rather than subtracting a constant factor.
Updates golang/go#32754
Change-Id: Ie6f9111f1696b0d067d08f7eed7b0a338ad9cd67
Reviewed-on: https://go-review.googlesource.com/c/tools/+/192137
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The "Organize imports" code action uses internal/imports that needs a
valid GOPATH set. Since Go 1.8 setting GOPATH manually is not required,
and if it isn't set gopls will sometimes fail to properly import
packages.
This CL sets GOPATH to the default if the env var GOPATH isn't set.
Fixesgolang/go#33918.
Change-Id: Ib63a26a801e15af730197999de4d1d4901694a30
Reviewed-on: https://go-review.googlesource.com/c/tools/+/191600
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Packages found in the module cache do not change. When we encounter a
directory we have already processed in the module cache, skip that
directory and add the packages that have already been computed.
Change-Id: Ib1bf0bf22727110b8073b415b145034acceb6787
Reviewed-on: https://go-review.googlesource.com/c/tools/+/186921
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
To check if a package is in a module that is in scope, the module
resolver checks if there are Go files that would be included in a
package in the directory matching the import path in scope.
If this directory is in the module cache and we have saved it as a
package, we know this directory contains Go files, and do not have to
read the directory.
Change-Id: I7c9365ce42c760ab95bc68b036212120895c89fb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/186922
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
The root of the module containing a package in the module cache can be
determined by looking at the directory path. Use this instead of
scanning up the file tree to find the mod file of a package from a
module cache. The go command prunes nested modules before populating
the module cache, so there is only one go.mod within each module.
Change-Id: I434a04350ef3ca2f44b7ffd08ccc5afe4209654f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/190906
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Prepare rename gets the range of the identifier to rename. Returns an
error when there is no identifier to rename.
Change-Id: I5e5865bc9ff97e6a95ac4f0c48edddcfd0f9ed67
Reviewed-on: https://go-review.googlesource.com/c/tools/+/191170
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Over time the existing implementation became buggy. This implementation
logs close to where data is read or written from the stream connected
to the client. As is required, the log records are from the point of view
of the client.
Fixesgolang/go#33755
Change-Id: I91150c697dc2cdb6d3eecbbed7a8d1805a7c476d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/191963
Run-TryBot: Peter Weinberger <pjw@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Send the code action kinds that we support, if codeActionLiteralSupport
is specified. Editors may use the CodeActionKinds that we support to
determine UI layout for example.
Change-Id: Iee368aa02c26f4395bb2894593ef38d84d3283b7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/191620
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Deep completions can take a long time (500ms+) if there are many
large, deeply nested structs in scope. To make sure we return
completion results in a timely manner we now notice if we have spent
"too long" searching for deep completions and reduce the search scope.
In particular, our overall completion budget is 100ms. This value is
often cited as the longest latency that still feels instantaneous to
most people. As we spend 25%, 50%, and 75% of our budget we limit our
deep completion candidate search depth to 4, 3, and 2,
respectively. If we hit 90% of our budget, we disable deep completions
entirely.
In my testing, limiting the search scope to 4 normally makes even
enormous searches finish in a few milliseconds. Of course, you can
have arbitrarily many objects in scope with arbitrarily many fields,
so to cover our bases we continue to dial down the search depth as
needed.
I replaced the "enabled" field with a "maxDepth" field that disables
deep search when set to 0.
Change-Id: I9b5a07de70709895c065503ae6082d1ea615d1af
Reviewed-on: https://go-review.googlesource.com/c/tools/+/190978
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
this makes sure that any diff implementation obeys the semantics we expect
at higher layers
Change-Id: Iae8842cfb9fece94ea71c04ec146d825eff0cbeb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/191017
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Now we register for and handle didChangeWatchedFiles "change"
events. We don't handle "create" or "delete" yet.
When a file changes on disk, there are two basic cases. If the editor
has the file open, we want to ignore the change since we need to
respect the file contents in the editor. If the file isn't open in the
editor then we need to re-type check (and re-diagnose) any packages it
belongs to.
We will need special handling of go.mod changes, but start with
just *.go files for now.
I'm putting the new behavior behind an initialization flag while it is
under development.
Updates golang/go#31553
Change-Id: I81a767ebe12f5f82657752dcdfb069c5820cbaa0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/190857
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change allows renamings of the name of an import spec.
Since there is not always explicit identifier available to select and
rename, allow renaming packages from positions within the import spec.
Change-Id: I0a8aaa92c26e1795ddb9c31a1165b2f2ee89aa34
Reviewed-on: https://go-review.googlesource.com/c/tools/+/191165
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
When there is an explicit name for an import spec, treat it as its own
identifier, separate from the import path.
Example:
import h "hello"
The name h is defined in that import spec, not in the package hello
it contains its own references. If asked about a position within the
import path, continue treating that as referencing the imported package.
If the position is within the name, use the identifier there that is
local to that file.
This change allows for go to definition of the explicit name to point to
itself, find all references from the import spec, and rename the
explicit name from the import spec.
Change-Id: Ia1d927a26e73f2dc450d256d71909c006bdf4c37
Reviewed-on: https://go-review.googlesource.com/c/tools/+/191164
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Since renaming an identifier within an import spec is not yet supported,
return an error when this is encountered. These idents from the import
spec have a nil declaration object.
Import paths that contain '.' or '/' are caught by the valid identifier check
avoiding the crash, but import paths such as "fmt" are not as fmt is a
valid identifier. This change checks if i.decl.obj is nil and returns an error
if it is to avoid the crash.
Fixesgolang/go#33768
Change-Id: I4e757b42bedffd648fc821590e4a383826200dc3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/191163
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This will allow us to configure the connection at need.
It will also allow us to intercept the content for tests.
Change-Id: Id7d34f2d56f233eae112bea97cccab1f2a88de55
Reviewed-on: https://go-review.googlesource.com/c/tools/+/190798
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
The module cache can only be added to, so any information discovered
about directories that are within a module in the module cache will
not change. Store the information we have discovered about the module
cache.
Updates #32750
Change-Id: I56c88f03f6a364221198fb032b139208497cd0e5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/188762
Reviewed-by: Heschi Kreinick <heschi@google.com>
Optimize a few things to speed up deep completions:
- item() is slow, so don't call it unless the candidate's name matches
the input.
- We only end up returning the top 3 deep candidates, so skip deep
candidates early if they are not in the top 3 scores we have seen so
far. This greatly reduces calls to item(), but also avoids a
humongous sort in lsp/completion.go.
- Get rid of error return value from found(). Nothing checked for this
error, and we spent a lot of time allocating the only possible error
"this candidate is not accessible", which is not unexpected to begin
with.
- Cache the call to types.NewMethodSet in methodsAndFields(). This is
relatively expensive and can be called many times for the same type
when searching for deep completions.
- Avoid calling deepState.chainString() twice by calling it once and
storing the result on the candidate.
These optimizations sped up my slow completion from 1.5s to
0.5s. There were around 200k deep candidates examined for this one
completion. The remaining time is dominated by the fuzzy
matcher. Obviously 500ms is still unacceptable under any
circumstances, so there will be subsequent improvements to limit the
deep completion search scope to make sure we always return completions
in a reasonable amount of time.
I also made it so there is always a "matcher" set on the
completer. This makes the matching logic a bit simpler.
Change-Id: Id48ef7031ee1d4ea04515c828277384562b988a8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/190522
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
