Previously, we would reload if a user's import list decreased or simply
changed order. This is not necessary. Now, we only re-run if a new import
needs to be loaded.
Updates golang/go#35388
Change-Id: I47874afe773dddb835ac27b18895e7a082950dc7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209057
Reviewed-by: Heschi Kreinick <heschi@google.com>
When searching for implementations we look at all packages in the
workspace. We do a full parse since we need to look for non-exported
types and look in functions for type declarations. However, we always
type check a package's dependencies in export-only mode to save work.
This leads to what I call the "two world" syndrome where you have both
the export-only and full-parse versions of a package in play at once.
This is problematic because mirror objects in each version do not
compare equal.
For example:
-- a/a.go --
package a
type Breed int
const Mutt Breed = 0
type Dog interface{ Breed() Breed }
-- b/b.go --
package b
import "a"
type dog struct{}
func (dog) Breed() a.Breed { return a.Mutt }
---
In this situation, the problem is "b" loads its dependency "a" in
export only mode so it gets one version of the "a.Breed" type. The
user opens package "a" directly so it gets fully type checked and has
a second version of "a.Breed". The user searches for "a.Dog"
implementations, but "b.dog" does not implement the fully-loaded
"a.Dog" because it returns the export-only version of the "a.Breed"
type.
Fix it by always loading in-workspace dependencies in full parse mode.
We need to load them in full parse mode anyway if the user does find
references or find implementations.
In writing a test I fixed an incorrect import in the testdata. This
uncovered an unrelated bug which made a different implementation test
very flaky. I disabled it for now since I couldn't see a fix simple
enough to slip into this commit.
Fixesgolang/go#35857.
Change-Id: I01509f57d54d593e62c895c7ecb93eb5f780bec7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209759
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Lack of context in error messages is making my life difficult. Add
context to a few, refactoring out some duplicate code along the way.
Change-Id: I3a940b12ec7c82b1ae1fc477694a2b8b45f6ff71
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209860
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
As far as I can tell, the code I removed in from load did roughly
nothing -- returning nil metadata didn't suppress type checking as I
think was intended. Throwing away the metadata also created the race in
Pull the check for missing import changes up to PackageHandles, where it
is non-racy and can cause type checking to be skipped. Simplify and
refactor.
Fixesgolang/go#35951.
Change-Id: Id4b32b86569afb36863aaf982616b2b3727b0e83
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209737
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sometimes the prefix of the thing you want to complete is a keyword.
For example:
variance := 123
fmt.Println(var<>)
In this case the parser produces an *ast.BadExpr which breaks
completion. We now repair this BadExpr by replacing it with
an *ast.Ident named "var".
We also repair empty decls using a similar approach. This fixes cases
like:
var typeName string
type<> // want to complete to "typeName"
We also fix accidental keywords in selectors, such as:
foo.var<>
The parser produces a phantom "_" in place of the keyword, so we swap
it back for an *ast.Ident named "var".
In general, though, accidental keywords wreak havoc on the AST so we
can only do so much. There are still many cases where a keyword prefix
breaks completion. Perhaps in the future the parser can be
cursor/in-progress-edit aware and turn accidental keywords into
identifiers.
Fixesgolang/go#34332.
PS I tweaked nodeContains() to include n.End() to fix a test failure
against tip related to a change to go/parser. When a syntax error is
present, an *ast.BlockStmt's End() is now set to the block's final
statement's End() (earlier than what it used to be). In order for the
cursor pos to test "inside" the block in this case I had to relax the
End() comparison.
Change-Id: Ib45952cf086cc974f1578298df3dd12829344faa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209438
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We were previously returning errors when we failed to load/check a
user's workspace folder, but now we suppress all errors. We shouldn't
disable gopls functionality if something is broken in a user's workspace
folder, rather, we should fall back to the file= queries that will run
when a user edits a file.
Change-Id: Iae05174ca80d2573c0222ac42f29e5556bda0134
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209420
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Running staticcheck on the entire workspace causes a slowdown, and most
likely users don't want to see staticcheck reports for every
subdirectory of their workspace. Only run staticcheck on open files.
Also, fixed a staticcheck warning that showed up along the way. Filed
golang/go#35718 to remind ourselves to fix all of the staticcheck warnings
that showed up when we ran gopls with staticcheck on x/tools.
Finally, made sure that we don't send empty diagnostics when diagnosing
the snapshot on start-up, as that is not necessary.
Change-Id: Ic51d1abfc80b1b53397057f06a4cfd7e2dc930f9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208098
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
The early return logic for didOpen events in
(*snapshot).invalidateContent was preventing the creation of a new
snapshot, which was in turn stopping the versions from being updated.
This exposed a fundamental issue in the way we were calculating
workspace diagnostics. Since we weren't waiting for diagnostics to be
completed for an entire snapshot before replying that the server had
been initialized, snapshots were being cloned without any type
information. For quickfix code actions, we assume that we have all
information cached (since we need to have sent the diagnostics that the
quickfix is mapped to), so we were not finding the cached analysis
results.
To handle this in the short-term, we key analyses by their names, and
then regenerate results as-needed for code actions. This is technically
more correct than simply assuming that we have the analyses cached. In a
follow-up CL, I will send a follow-up that will make sure that
snapshots "wait" on each other to be fully constructed before being
cloned.
Change-Id: Ie89fcdb438b6b8b675f87335561bf47b768641ac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208265
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
When packages.Load'ing cgo packages, the authored files show up in
GoFiles, and the generated files show up in CompiledGoFiles. We need the
AST and type information for the latter, since they're the only thing we
can type check. But we also need the contents (and column mapper) for
the authored file so that we can navigate into it.
Store GoFiles in package metadata and checked Packages. Parse the extra
files, just for their mappers. Refactor the View functions a little bit,
since there's only one place that actually needs to find the mapper for
a file.
Updates golang/go#35720.
Change-Id: I9f96872a9a592bf0e11da27ebd8976c6db8752c9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208502
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
I'm not sure why this was being managed by the view, but delete the code
that handles tracking a file's first change. It is only used to avoid
spamming the user with error messages.
Change-Id: Id95089478ffb7e189d38cbc147e3dde6a1c55c5e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208274
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Right now, we request analyses for files in ParseExported mode, which
doesn't actually produce any meaningful facts. Disable it until we
resolvegolang/go#35089, since right now, all this is doing is wasting
memory and CPU.
Change-Id: I6ffb7bdf6c915159b55753b51289cef4bd937603
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208270
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This change modifies the behavior of the GetReverseDependencies function
used for diagnostics. Since we now return diagnostics for the entire
workspace, we don't have to worry if a file is open to show errors in
it. This change requires the addition of a new (*snapshot).PackageHandle
function that gets a CheckPackageHandle for a given package ID. This
function does not cause a re-load of the package metadata, though if we
feel that this is something we need in the future we can add it.
Change-Id: I863bdf284d15f2317d8fae395928a90b9455146b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208102
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This change uses the FileIdentity when reporting an error message, so
that the version number can be propagated to through the
publishDiagnostics notification.
Change-Id: I6a2103e304717ca09895008ea40336e3ace3c66d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208260
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This change cleans up internal/lsp/source/view.go to have a more logical
ordering and deletes the view.CheckPackageHandle function. Now, the only
way to get a CheckPackageHandle is through a snapshot (so all of the
corresponding edits).
Also, renamed fuzzy tests to fuzzymatch. Noticed this weird error when
debugging - I had golang.org/x/tools/internal/lsp/fuzzy in my module
cache and it conflicted with the test version.
Change-Id: Ib87836796a8e76e6b6ed1306c2a93e9a5db91cce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208099
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
As we improve support for cgo we'll need to reference GoFiles, not just
CompiledGoFiles. "Files" is right out.
I think I got everything that needs renaming but please let me know if
not.
Updates golang/go#35720.
Change-Id: I97a6ebf5b395535de0d5f4f8b3f84b46ca34643f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208101
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
When line directives are in use, we want the logical file name, not the
one we found the bytes in. This matters most for cgo, where the file we
parsed is not the one the user wants to see.
Updates golang/go#35720.
Change-Id: I495328071d8865e6895cb731467f1601f11e93db
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208100
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change runs diagnostics on all packages in the workspace, instead
of just open files. We also want to avoid invalidating the type
information for a newly-opened file (since we should have it be default
now), so handle that case.
This causes a large increase in memory usage in the
internal/lsp/cmd tests, so to handle that, share an app between all of
the tests, rather than creating one per-test type.
Change-Id: Ifba18d77a700cda79ec79f66174de0e7f13fe319
Reviewed-on: https://go-review.googlesource.com/c/tools/+/207353
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
textDocument/didChange events need to indicate if the change includes
the full file content or just a diff. Previously, the
contentChange.Range field was a pointer, so if it was nil, then we would
conclude that the file change was for the whole file. Now, the best we
can do is compare it to an empty range, but this still doesn't work if
you are at the beginning of a file. I think that the range needs to be a
pointer for this to work correctly.
Also, some minor changes that came up along the way while debugging:
(1) Don't close over the *cache variable for fear of pinning anything in
memory
(2) Improve the error message when the token.File is nil
(3) Check for a nil token.File earlier
Change-Id: If9f310e92b7fb740b45e6cd3f9ca678a6fb52ff6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/207906
Reviewed-by: Heschi Kreinick <heschi@google.com>
Rather than panicking when we have not created any views for the packages,
we should show a reasonable error to the user. This change propagates the
errors to the user.
Updates golang/go#35599
Change-Id: I49789d8ce18e154f111bc3584488f468a129e30c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/207344
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
When looking for references, look in the entire workspace rather than
the same package. This makes the references query more expensive because
it needs to look at every package in the workspace, but hopefully
it shouln't be user-noticable. This can be made more efficient by only
checking packages that are transitive reverse dependencies. I don't think a
mechanism to get all transitive reverse dependencies exists yet.
One of the references test have been changed: it looked up references
of the builtin int type, but now there are so many refererences that
the test too slow and doesn't make sense any more. Instead look up
references of the type "i" in that file.
Change-Id: I93b3bd3795386f06ce488e76e6c7c8c1b1074e22
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206883
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change is the first step in centralizing control of modifications
to different files, either within the workspace or outside of it. We add
a source.FileAction type to pass into the internal/lsp/cache package and
handle the difference between opening and creating a file.
Now that we load all packages in a workspace by default, we no longer
need to re-load a file on open. This CL should enable CL 206883 to work
correctly.
Change-Id: I2ddb21ca2dd33720d668066e73283f5629d02867
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206888
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change propagates the versions sent by the client to the overlay
so that they can be used when sending text edits for code actions and
renames.
Fixesgolang/go#35243
Change-Id: I8d1eb86fe9f666f7aa287be5026b176b46712c97
Reviewed-on: https://go-review.googlesource.com/c/tools/+/205863
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change makes sure that we only return files that contain the given
position. There are a few instances of needing to look up files by URI
in the internal/lsp/cache package, so use an unexported package for
that. This allows us to remove some code in the implementations code.
Change-Id: Ifa7a62c67271826e6c632e4c88667d60f8b760c4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206880
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
This change adds support for returning versions along with file URIs, so
that the client can know when to apply changes. The version is not yet
propagated along to the internal/lsp/cache package, so this change will
have no effect (VS Code ignores a version of 0 and still applies the
changes).
A few minor changes made in the rename code (to remove the view
parameter). Some minor staticcheck fixes.
Updates golang/go#35243
Change-Id: Icc26bd9d9e5703c699f555424b94034c97b01d6f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206882
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
If a user is typing fast, they will quickly invalidate many snapshots.
We don't want to stack up a bunch of stale type check and analysis
operations, so we should propagate cancellation through the cache.
Handles are long-lived, so we may cancel an operation only to
restart it again later. Also, there may be multiple operations waiting on
the same computation, and just because one is cancelled doesn't mean we
should necessarily stop. The easiest way to support all that was to add
an explicit state to each handle, and track the number of waiters.
See the code for more details on Handle life cycles.
As far as I can tell, the rest of gopls is prepared for this behavior.
I added an explicit check to the type checking code, where I was worried
it might get overly confused. But long-term it would probably be good to
return an error from Get.
Change-Id: I3ea6e141b52b94300a41248d3f2e039b023709d0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206879
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Look in all packages the snapshot knows of (through a new method on snapshot called
KnownPackages) and see if any of those packages contain implementations. Before,
the Implementation call only looked in the current package.
Much of the new complexity in implementation.go is routing through the Type to
Package data in the implementsResult.pkg field so the identifier can be looked up
in its correct package.
Fixesgolang/go#32973
Change-Id: Ifa7115b300f52fb4fb55cc00db2e7f339e8c2582
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206518
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
It attempts to detect changes that would invalidate the view and replace itself
with a new view when that happens
Change-Id: I0f1a8cd3bd6ddcef115fedc6c57ae0398b16d3b6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206147
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We don't want to return an error for the whole package when we are just
building out error positions.
Change-Id: I56b5b88ff2b4b44da8a372ade81cd9b1534235c4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206597
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
Even if the packages.Load of the directory the NewView is being created for
fails, create and add the view. But also return the error from NewView, just
after the new view has been added.
Fixesgolang/go#35468
Change-Id: I76c2d3cbe1a508ad0794a6fcd3bc67cd48c97e22
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206497
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Treat it as okay if no packages are found when loading all the packages in a
workspace. Users may open workspaces that don't have any Go files, either because
they are workspaces for other languages, or because no Go files have been created
yet.
Fixesgolang/go#35455
Change-Id: I60912472ec8930649996edc150d1d19cd74a0a2e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206140
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Add a source.Scope type that can be used to refer to directories or
files, and modify (*snapshot).load to take source.Scope.
Then call load in NewView.
Change-Id: I8f03c7b271d700b162100d2890d23219ef9578c2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204822
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Add a new "verboseOutput" config flag (defaults to "false") to enable
verbose go/packages and imports output. Previously this output was
always present.
The go/packages output would dump out the entire (humongous) "go list"
JSON response which would lock up my editor for a second whenever
something triggered a go/packages call.
The imports output would produce a bunch of "gopathwalk" debug
messages that aren't useful in general and in particular add noisy
output to tests.
Change-Id: Ie4693d074cb84f1397e0e51d7346dc9391bd1278
Reviewed-on: https://go-review.googlesource.com/c/tools/+/205138
Reviewed-by: Koichi Shiraishi <zchee.io@gmail.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Unimported completions now try to pull Packages from everywhere, not
just the transitive dependencies of the current package. That confused
the import formatting code, which only looked at deps. Pass the Package
along with the import suggestion, and use it when it's present.
Also change some error messages to be different for diagnostic purposes.
Fixesgolang/go#35359.
Change-Id: Ia8ca923e46723e855ddd2da7611e6eb13c02bb4f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/205501
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Loading completion suggestions can be slow, especially in GOPATH mode
where basically anything can change at any time. As a compromise, cache
everything for 30 seconds. Specifically, after a completion operation
finishes, if the cache is more than 30 seconds old, refresh it
asynchronously. That keeps user-facing latency consistent, without
chewing up CPU when the editor isn't in use. It does mean that if you
walk away for an hour and come back, the first completion may be stale.
In module mode this is relatively benign. The only things the
longer caching affects are the main module and replace targets, and
relevant packages in those will generally be loaded by gopls, so they'll
have full, up-to-date type information regardless.
In GOPATH mode this may be more troublesome, since it affects
everything. In particular, go get -u of a package that isn't imported
yet won't be reflected until the cache period expires. I think that's a
rare enough case not to worry about.
Change-Id: Iaadfd0ff647cda2b1dcdead9254b5492b397e86e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/205163
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Packages that aren't imported in the current file will often have been
used elsewhere, which means that gopls will have their type information
available. Expose loaded packages in the Snapshot, and try to use that
information when possible for unimported packages.
Change-Id: Icb672618a9f9ec31b9796f0c5da56ed3d2b38aa7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204824
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The metadata was being added to the cache before it was fully computed.
Change-Id: I6931476a715f0383f7739fa4e950dcaa6cbec4fe
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204562
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Some analyses may panic, so we should be careful to recover.
Change-Id: Ie13df07aca1a21f4e6a66648cd4890b6a31966cc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204817
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
The approach of using ASTs to determine error ranges had more
complications than I anticipated. Revert it for now to go back to the
old user experience, while we consider a better approach.
Change-Id: I5b23f0147c26089330f8a4bbf7b6914ae66cf59a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204561
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change stops the memory leak from happening. It does not stop the
large number of allocations done by analysis, however, so these still
need to be prevented through correct cancellation.
Change-Id: I1cf7fbf6f85ed413af1f2ea55f91862a6d7f2db7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204558
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Closing over the checkPackageHandle creates a cycle that forces the
checkPackageHandle not to be garbage collected until the value is
created. If a value is never created, the handle will not be collected.
Change-Id: I0f94557da917330ebe307a0e843b16ca7382e210
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204079
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change encompasses the refactorings needed to correctly implement
CL 204079. The goal of this CL is to make the actual relevant diffs more
clear.
Change-Id: I38acfd436e2380be790910e01b6e37d8280e9100
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204139
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change eliminates the need for the importer struct. We should no
longer need the "seen" map for cycle detection. This is because
go/packages will not return import maps with cycles, and we fail in the
Import function if we see an import we do not recognize.
Change-Id: I06922c74e07eb47ce63b56fa2ac2099e7fc8bd8a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/202299
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Also, address minor comments that I ignored from Heschi because I am
obnoxious.
Change-Id: I99dcac38578585af2cdd951dd2b9755732ef945f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/203281
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Now that we are using the memoize package to cache analysis results, we
can use that cache for suggested fixes.
Change-Id: I42905a6fe575f49d38979d53d58ea8ec59210ae0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/203278
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
There is no need to cache the pass on the actionHandle, as it does not
need to be reused and does not live outside the exec function.
Change-Id: I1737271383776b35718df3475b4f888232d57ae4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/203177
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
We don't need to worry about a package's errors unless it is the
top-level package. Also fix some fallback logic in the type error range
computation.
Change-Id: Ib26b5e25bd70193ea24ec4a197811eedf69b0e2c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/202622
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
This change effectively reverts CL 202039. This CL was a mistake, as it
creates a cycle. Snapshots hold CheckPackageHandles, which in turn hold
pkgs.
Change-Id: I944304cb365f0ef98b5e54ea38edea6cece40453
Reviewed-on: https://go-review.googlesource.com/c/tools/+/202740
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
*ast.ArrayTypes are type expressions like "[]foo" or "[2]int". They
show up as standalone types (e.g. "var foo []int") and as part of
composite literals (e.g. "[]int{}"). I made the following
improvements:
- Always expect a type name for array types.
- Add a "type modifier" for array types so completions can be smart
when we know the expected type. For example:
var foo []int
foo = []i<>
we know we want a type name, but we also know the expected type is
"[]int". When evaluating type names such as "int" we turn the type
into a slice type "[]int" to match against the expected type.
- Tweak the AST fixing to add a phantom selector "_" after a naked
"[]" so you can complete directly after the right bracket.
I split out the type name related type inference bits into a separate
typeNameInference struct. It had become confusing and complicated,
especially now that you can have an expected type and expect a type
name at the same time.
Change-Id: I00878532187ee5366ab8d681346532e36fa58e5f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/197438
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change modifies the invalidContent function to take a file change
type. This allows us to eliminate the separate invalidateMetadata
function. The logic of watching changed files is then further pushed
into the caching layer.
Updates golang/go#34218
Change-Id: Id31b3931c45ec408b6e7b4a362e00f9091ba4f70
Reviewed-on: https://go-review.googlesource.com/c/tools/+/201221
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
A continuation of CL 202298, only for analysis errors.
Change-Id: I957d52cef31938ef66be73463e92695a5b56869c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/202540
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Since a user's module cache is generally going to be much bigger than
their main module, one would expect that caching just information about
the module cache would be sufficient. It turns out that's not correct.
When we discover something in the module cache, we have to make sure
that a different version of it isn't already in scope. Doing that can
require information about the main module or replace targets, so that
needs to be cached too.
Concretely, when I'm working in x/tools, if a scan discovers a version
of x/tools in the module cache, it should usually ignore that version.
But that might not be true in more complicated cases, especially those
involving nested modules whose boundaries change.
So, cache everything except GOROOT. Since the new data is mutable,
we store it separately from the module cache data so that it can be
discarded easily between runs.
Change-Id: I47364f6c0270fee03af8898fec6c85d1b9c8d780
Reviewed-on: https://go-review.googlesource.com/c/tools/+/202045
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Scan most sources, including GOPATH, the module cache, the main module,
and replace targets as appropriate. Use the cached stdlib instead of
scanning GOROOT.
We heavily cache the contents of the module cache, so performance is
decent. But we have to look at all the modules not in the module cache
too to get the right versions of modules (see
(*ModuleResolver).canonicalize), which currently isn't cached at all,
even just for a single run. That ends up being pretty expensive.
The implementation changes are relatively small; add package name
loading to scan(), cache that result, and allow callers to control what
directories are scanned so that it can skip GOROOT.
I also cleared out most of the stdlib from the unimported completion
test and added a simple external completion to it for safety's sake.
Change-Id: Id50fd4703b1126be35a000fe90719e19c3ab84bf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/199178
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds a source.Error type which is used to collect the error
information that comes out of the loading, parsing, and type checking
stages. We also add specific sources per-error, rather than having them
all be labeled as "LSP".
This change will enable follow-ups that do a better job of extracting
error ranges.
Change-Id: I3fbb5e42d66aa2c5bb1b2f41d1eadfc45f3a749b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/202298
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
We were caching the package before setting the handle, so a
checkPackageHandle might be in use before it was fully created.
Change-Id: Ic98e7c351cbed5e4caa87098e95ad04d4f54f8df
Reviewed-on: https://go-review.googlesource.com/c/tools/+/202040
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
A package really should always be associated with its snapshot rather
than its view. This eliminates some extra parameters in a few utility
functions.
Change-Id: I60f9b7286e0072d3268602f6bd32052a3d2e5559
Reviewed-on: https://go-review.googlesource.com/c/tools/+/202039
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Currently array and slice literals don't work very well for
completion. When go/parser is not expecting a type, it often turns
array types (e.g. "[]int") into *ast.BadExpr, which messes up
completion because we can't figure out the prefix from *ast.BadExpr,
and *ast.BadExprs don't get type checked.
This change addresses the first problem of not being able to figure
out the prefix. If we see an *ast.BadExpr, we now blindly try to
reparse it as a composite literal by adding on "{}". If we end up with
an *ast.CompositeLit with an *ast.ArrayType "Type", we swap
the *ast.BadExpr for the *ast.ArrayType. This approach is dumb but
simple, and fixes lexical completions in array types.
Change-Id: Ifa42e646bcbf2a30170d73e6dd11982384d40b43
Reviewed-on: https://go-review.googlesource.com/c/tools/+/197437
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
There was a regression where gopls would not type-check any package with
a bad import. This change fixes the regression and adds a test to make
sure it doesn't happen again.
Change-Id: I3acf0917d46e9444c20135559f057f0ecd20e15b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/201539
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Recently been noticing errors where we don't have full metadata for a
given package. It seems to me that, since we added the context to the
packages.Config, there have been cases where the context is canceled on
the first load, and then we type-check with incomplete data. I'm still
not sure if allowing go/packages to be canceled is the correct approach.
Change-Id: I6767ce763538bd579458c8f8db07f15c9eec7b4a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/201518
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Muir Manders <muir@mnd.rs>
Reviewed-by: Michael Matloob <matloob@golang.org>
This span resulted in misleading information, since it would appear any
time you called the Check function. This function often returns cached
results, so it's not particularly useful.
Change-Id: I26be652a99b906fb2e8703ff9af86fbe2ef5fc5d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/201219
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Analyzer names are not guaranteed to be unique.
Change-Id: I4d4cc9fa746cbfbea9926f2cae0eb5dfc41027a5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/201217
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
This change makes sure that actionHandles are cached within the
snapshot, to minimize the cost of re-computing action handles on each
run of go/analysis.
Change-Id: If6191c5224285eb207aebb997787ea551a47fbaa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/201098
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
This change moves to the approach of caching the analysis using the
memoize package. This means that we will do less work, as we no longer
need to recompute results that are unchanged. The cache key for an
analysis is simply the key of the CheckPackageHandle, along with the
name of the analyzer.
Change-Id: I0e589ccf088ff1de5670401b7207ffa77a254ceb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/200817
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Include the context when invoking go/packages.Load so it is
cancelable. Otherwise if the user is manually typing an import or
otherwise messing around with imports, a big queue of potentially very
slow go/packages.Load calls will build up.
Fixesgolang/go#34414.
Change-Id: I80732086b478b908c720739708dd773e81fe2b81
Reviewed-on: https://go-review.googlesource.com/c/tools/+/200058
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This will prevent us from panicking in cases with errors.
Fixesgolang/go#34824
Change-Id: I02c20655f6926ec00c1591a905ff5a107cc44192
Reviewed-on: https://go-review.googlesource.com/c/tools/+/200300
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change includes dependencies in the cache keys for
CheckPackageHandles. This should fix the issue with propagating results
to reverse dependencies.
Updates golang/go#34410
Change-Id: I025b7f0d6b0dcaa89c3461ebd94eadd35de4955f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/198317
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change does not complete the work to handle snapshots correctly,
but it does implement the behavior of re-building the snapshot on each
file invalidation.
It also moves to the approach of caching the FileHandles on the snapshot,
rather than in the goFile object, which is now not necessary.
Finally, this change shifts the logic of metadata invalidation into the
content invalidation step, so there is less logic to decide if we should
re-load a package or not.
Change-Id: I18387c385fb070da4db1302bf97035ce6328b5c3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/197799
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change uses the missing imports detection to return diagnostics and
warning messages to the user.
Updates golang/go#34484
Change-Id: If7bb7e702b8bdbd7e1ad5e26f93acc50d16209b0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196985
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Race condition: I deleted the code that acquired the mutex when checking
if a file's imports have changed.
Merge conflict: I submitted a change without rebasing and re-running TryBots.
Change-Id: Iae351f2fff893cfd94db79d9f79578d9827a8c21
Reviewed-on: https://go-review.googlesource.com/c/tools/+/197297
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change moves from caching package information the file object to
caching in a map that gets invalidated when content changes.
This simplifies cache invalidation and reduces the number of fields
guarded by the (*goFile).mu lock.
Change-Id: I33fef6e0b18badb97e49052d9d6e3c15047c4b63
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196984
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Instead of relying on the diagnostics cached on the package, use the
diagnostics sent by the code action when computing suggested fixes.
Change-Id: I77f7fd468b34b824c6c5000a51edbe0f8cc6f637
Reviewed-on: https://go-review.googlesource.com/c/tools/+/197097
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Currently, we cache source.CheckPackageHandles for each file and package
that we are aware of, as well as dependencies. This is not necessary,
since the active packages pin their imports in memory.
Change-Id: Ia0101f4d4a2d36d5baeb890af3d7c8baec297847
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196982
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change shifts our approach to make sure that a top-level package
only ever imports "trimmed" packages.
Change-Id: I63c35791ef6efad7dac248a9ff877835f46df9ed
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196523
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This will allow view configuration to modify the set of analyzers being applied, and also allow the main gopls to inject new analyzers
Change-Id: Ic2a76118c3e29b059e19b31bd1fb54b1d9e15e54
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196320
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This changes adds basic support for running `go mod tidy` as a code
action when a user opens a go.mod file. When we have a command
available like `go mod tidy -check`, we will be able to return edits as
part of the codeAction. For now, we execute the command directly.
This change also required a few modifications to our handling of file
kinds so that we could distinguish between a Go file and a go.mod file.
Change-Id: I343079b8886724b67f90a314e45639545a34f21e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196322
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change encodes an invariant that, a dependency package will only
ever be parsed with trimmed ASTs.
Updates golang/go#34410
Change-Id: I2ceab3672c0bae0b98cec2a8e60b92a0c01a900f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196537
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
We were recursing infinitely in cases like this:
switch true {
case true:
go foo.F<>
}
There were three things that came together to cause this:
1. We recently starting recursively fixing broken go/defer statements.
2. In this case we were failing to swap in the correct ast.Node in for
the *ast.BadStmt because we were only looking
for *ast.BlockStmt (and *ast.CaseStmt has no block).
3. After 2), we weren't returning an error so the fix() code thought
it should recurse.
Fix 2) by using reflection to swap AST nodes in a generic way. Perhaps
a bit overkill in this case, but I happened to have already written
this for an upcoming change, so I just pulled it in to fix this bug.
Fix 3) by returning an error if we fail to swap the AST nodes.
Fixesgolang/go#34353.
Change-Id: I17ff1afd52ae165c0ba9de5820dcec4cb7d756cb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196137
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Parse errors need to be treated separately from actual errors when
parsing a file. Parse errors are treated more like values, whereas
actual errors should not be propagated to the user. This enables us to
delete some of the special handling for context.Canceled errors.
Change-Id: I93a02f22b3f54beccbd6bcf26f04bb8da0202c25
Reviewed-on: https://go-review.googlesource.com/c/tools/+/195997
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
We had too many options for functions to use to get type information for
a package. Now we stick with having one option to get the check package
handles, and then the caller can refine the results as needed.
Change-Id: I81f69a670e1539854ee23b6f364159a7de9b782f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194457
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This is to remove the confusion around having only handles that have had Get
called pin the value into memory.
Instead now there is a single handle per key, and it is the handle that is
weakly held not the value.
Change-Id: I9e813a0dfe2adf4cb651af9b5cfc8878fa71c041
Reviewed-on: https://go-review.googlesource.com/c/tools/+/186839
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This function incorrectly used cached packages to get ASTs and type
information that should have been directly found from the origin
package. Shift to using pkg.FindFile instead.
Change-Id: I9f73209bb1a1343f53b430150e78ffd180e14a44
Reviewed-on: https://go-review.googlesource.com/c/tools/+/195797
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change allows to remove some of the special handling for the
builtin package.
Change-Id: I105fcefd8812af2d42ff42edca954824c98db429
Reviewed-on: https://go-review.googlesource.com/c/tools/+/195758
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
A mapper is always uniquely tied to a file at a specific version, so
just build it when we get a new *ast.File. We build the mapper using the
*token.File associated with the particular *ast.File, which is why there
is one per ParseGoHandle instead of FileHandle.
Change-Id: Ida40981ef91f6133cdd07e9793337fcd67510fba
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194517
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Previously we would type check all package handles as we fetched them.
This meant if you only cared about a file's primary package you would
still have to wait for all its packages to be type checked. For
example, when completing in foo.go, you would wait for [foo.go] and
[foo.go, foo_test.go] to be checked (the latter being the test
variant).
Now we don't type check packages as we put together the package
handles.
Change-Id: Ibca0c6b34cf4f0a07bcdeb62959d60158f4ccf17
Reviewed-on: https://go-review.googlesource.com/c/tools/+/195417
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Previous changes to the config mechanism made the config options
per-view, not per-session. We should now make sure to obey config
changes per-view. This does not fix the configuration handling for
"watchChangedFile" however. This should be done in a future CL.
Change-Id: I73f6236386c36d2587fdb9c0601670833a4366c3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194818
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This fixes the issue of config options not being applied.
Also, handle config errors and deprecation by showing a message to the
user.
Change-Id: I850d5303a7a1e301c97324060a87929710ee6700
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194682
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Our original caching plan was to use only the file ParseGoHandles as
cache keys to define a given package. However, because of package test
variants, we cannot rely on files alone. A package may have the exact
same set of files, but be a test variant. Add the ID to the key to avoid
clobbering entries in the cache.
Also, remove the unused metadata ID cache.
Change-Id: I4b33de1f83f6c769d23441e98a2a7324ff0fa1b0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194571
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Now when a file is deleted we force the file's packages to refresh
go/packages metadata, and kick off diagnostics.
I made a couple other changes to watched file handling:
- Kick off diagnostics in a goroutine to match how DidChange works.
This will allow us to work through big sets of file changes faster,
and will save duplicated work once type checking can be canceled.
- Don't assume a watched file is only part of one view.
Two interesting cases we don't handle yet:
- If the deleted file was the only file in the package, we don't
currently update diagnostics for dependent packages. This requires
rejiggering how diagnostics are invoked a bit.
- If the deleted file is still open in the editor and then later
closed, we don't trigger metadata/diagnostics refresh on DidClose.
Updates golang/go#31553
Change-Id: I65768614c24d9800ffea149ccdbdbd3cb7b2f3d8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193121
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Now when trying to fix *ast.BadStmt, we parse the manually extracted
expression using parser.ParseFile instead of parser.ParseExpr.
ParseFile will yield *ast.BadStmt nodes for any bad statements nested
in our first bad statement, allowing us to fix them recursively.
To turn our expression into a "valid" file we can pass to
parser.ParseFile, I wrapped it thusly:
package fake
func _() {
<our expression>
}
Change-Id: I0d4fd4ebce6450021da8e03caa11d0ae5152ea8d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194342
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We now wait to build views until we have the options for that view,
and pass the options in to the view constructor.
The environment and build flags are now part of the view options.
Change-Id: I303c8ba1eefd01b66962ba9cadb4847d3d2e1d3b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194278
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
In the case of documentation items for completion items, we should make
sure to use the ASTs and type information for the originating package.
To do this while avoiding race conditions, we have to do this by
breadth-first searching the top-level package and its dependencies.
Change-Id: Id657be969ca3e400bb2bbd769a82d88e91865764
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194477
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Remove the wantSuggestedFixes flag, and run the flagged code
by default.
Add test cases for suggested fixes.
Generate a suggested fix to the assign analysis that suggests removing redundant assignments.
Fix the propagation of suggested fixes (using rstambler's code).
Change-Id: I342c8e0b75790518f228b00ebd2979d24338be3b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193265
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Improve the existing fix-the-AST code to better identify the
expression following the "go" or "defer" keywords:
- Don't slurp the expression start outside the loop since the
expression might only have a single token.
- Set expression end to the position after the final token, not the
position of the final token.
- Track curly brace nesting to properly capture an entire "func() {}"
expression.
- Fix parent node detection to work when BadStmt isn't first statement
of block.
- Add special case to detect dangling period, e.g. "defer fmt.". We
insert phantom "_" selectors like go/parser does to prevent the
dangling "." from messing up the AST.
- Use reflect in offsetPositions so it updates positions in all node
types. This code shouldn't be called often, so I don't think
performance is a concern.
I also tweaked the function snippet code so it properly expands
"defer" and "go" expressions to function calls. It thought it didn't
have to expand since there was already a *ast.CallExpr, but the
CallExpr was faked by us and the source doesn't actually contain the
"()" calling parens.
Note that this does not work for nested go/defer statements. For
example, completions won't work properly in cases like this:
go func() {
defer fmt.<>
}
I think we can fix this as well with some more work.
Change-Id: I8f9753fda76909b0e3a83489cdea69ad04ee237a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193997
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
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>
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>
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>
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>
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>
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 will just make it a bit easier to debug the context
cancellation errors.
Change-Id: I580751ac04e3357031678eb31914828029c96e4b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/190412
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This is a straight move of some code with no changes.
It splits the part of the telemetry code that will become a standalone library from the bit that belongs in the lsp.
Change-Id: Icedb6bf1f3711da9251450531729984df6df7787
Reviewed-on: https://go-review.googlesource.com/c/tools/+/190403
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
We need to limit the concurrent file reads to prevent `too many open
file` errors. The first attempt to solve this was done in change 189437,
but this change turned out to be incorrect.
So this change reverts the changes made in change 189437 and instead
adds an new semaphore around the `nativeFileHandle.Read` method.
The limit is currently set to half of the lowest open file system limit
of *nix, Windows and MacOS system (their respective limits are 1024, 16384,
and 256).
Change-Id: I27d741e3a3c36eb11bfa7457575f5d5831c06ad7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/190417
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change fixes a race condition in the metadata caching logic.
Also, some minor fixes to comments and invalidation logic (it's not
necessary to invalidate ASTs when a package is invalidated).
Change-Id: I927bf6fbc661a86ef0ba99b29a9ed979cd1eb95d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/190317
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change eliminates the need for the package cache map, and instead
stores package type information in the store. We still have to maintain
invalidation logic because the key is not computed correctly.
Change-Id: I1c2a7502b99491ef0ff68d68c9f439503d531ff1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/185438
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change fixes documentation for completion items by using cached
package and AST information to derive the documentation. We also add
testing for documentation in completion items.
Change-Id: I911fb80f5cef88640fc06a9fe474e5da403657e3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/189237
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
On MacOS the default open file limit is 256 files per process. For big
projects this could cause issues when reading all files (more or less)
at the same time.
By moving up the `parseLimit` so the reading of the file is only done
when it is allowed to start parsing the file, we stay well below the 256
files per process (as the `parseLimit` is currently set to 20).
Since `parseLimit` is actually only used to limit IO access, let's also
rename the const to `parseLimit`.
Change-Id: Ie8744030875d84d0d6095ee4ec2d9d553911bed1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/189437
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
The `files` slice is used twice. First it's used to get all results from `ph.Parse`, and then it's reused to filter all `nil` values (which may have been returned by the `ph.Parse` method).
After the loop to "filter" out all the `nil` values, we also need to strip the remaining values.
I also changed the ordering so that we first check the errors and
only then perform this loop. That way the code will return earlier
when the context was canceled.
Partially fixes#33531 by prevention the panic reported in that issue.
Change-Id: I09478e765adcd0384ec4745921eb5c5aea405ef2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/189397
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This is probably a better approach than showing an extra diagnostic,
since a user cannot dismiss a diagnostic.
Fixesgolang/go#33397
Change-Id: I92b9a00f51a463673993793abfd4cfb99ce69a91
Reviewed-on: https://go-review.googlesource.com/c/tools/+/188766
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This relates to https://github.com/golang/go/issues/31374 and should switch all instances within `gopls` to use `x/errors` instead of `fmt` to create new errors.
Change-Id: I18339b75d12418d852e0dcc2ba0ed6c2970783b3
GitHub-Last-Rev: f4a55d9b79e7458ef1f1e06cb5eabbabd884f321
GitHub-Pull-Request: golang/tools#108
Reviewed-on: https://go-review.googlesource.com/c/tools/+/179880
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
switch the ioLimit channel from a bool to a struct{} as the values
stuffed into the channel have no meaning.
A mix of consistency and comment cleanups in *importer.parseFiles.
sameFile is reported as unused, so removed, removing some now unused
imports.
The assignment to lit in *view.parseDeferOrGoStmt's for{} loop was
reported as ineffective.
Update the comment of fix a little for clarity.
Change-Id: I50f0442bfd7e4d0cc0e6fdadbf1f6272366a716c
Change-Id: I50f0442bfd7e4d0cc0e6fdadbf1f6272366a716c
GitHub-Last-Rev: 74d8a8bdb768a827db3c1fd1723a528818e222ee
GitHub-Pull-Request: golang/tools#109
Reviewed-on: https://go-review.googlesource.com/c/tools/+/179957
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The results of running 'go list -m' are only valid as long as the
current module and the modules in its replace directives do not
change their go.mod files. Store the 'go.mod' versions that are
used in the imports call, and reinitialize the module resolver if
they change.
Change-Id: Idb73c92b9e4dc243a276885e5333fafd2315134d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/186597
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
A detatched context ends up attributing all background work to the initialize
function.
Change-Id: I81206462752228d5ac81408fb1e3fb86ab36796e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/186457
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds a Logf field to the packages.Config.
Change-Id: I144a9a1e1181585bbe621898c4a3e6a007a38322
Reviewed-on: https://go-review.googlesource.com/c/tools/+/185993
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
The imports ProcessEnv contains cached module and filesystem state. This change
allows gopls to use the same ProcessEnv and resolver across multiple calls to the
internal/imports library.
A ProcessEnv belongs to a view, because the cached module state depends
on the module that is open in the workspace.
Since we do not yet track whether the 'go.mod' file has changed, we
conservatively reset the cached state in the module resolver before
every call to imports.Process.
Change-Id: I27c8e212cb0b477ff425d5ed98a544b27b7d92ee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184921
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change removes the need for the ast and token fields on the *goFile
object. We switch to using source.ParseGoHandles on the package, which
means that we can easily access both the AST and token via the package,
which is already cached.
Change-Id: I5f78bbe09362f4d95eb15556617bdbd809a7a55d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/185878
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
And purge the loggers from the view and session.
Change-Id: I262958f340e9a5ac9cc9b3db9e9910381e457478
Reviewed-on: https://go-review.googlesource.com/c/tools/+/185989
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change merely modifies session.DidOpen to accept the document's
language ID. It does not actually add any handling of the language ID.
Change-Id: I2582ae307d1ca062f37b4683907cdbcfdfc61809
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184160
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
If typeCheck() returned an error, we could get into a state where a
package had an entry in the pcache, but the package's files had an
empty "pkgs" map. When we got a DidChange event for one of the files,
no packages would get invalidated since the file's "pkgs" was
empty. This resulted in the cached typeCheck() error persisting
indefinitely. Fix by never caching pcache entries on error.
An easy way to reproduce the problem was to delete the package name
from a file. For example, edit "package foo" to be just
"package". This caused the package to get stuck with an "AST for %s
has an invalid position" error.
Change-Id: I330bf9e419852dffa0f2dee94b56226367488dd1
GitHub-Last-Rev: 18be7078521b942694c76f799a2d520eee47167d
GitHub-Pull-Request: golang/tools#135
Reviewed-on: https://go-review.googlesource.com/c/tools/+/185839
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
