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>
also change the return type to be and end function and not an incomplete span
Change-Id: Icd99d93ac98a0f8088f33e905cf1ee3fe410c024
Reviewed-on: https://go-review.googlesource.com/c/tools/+/185349
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
For all uses inside the lsp we use the detatch logic instead
For tests we build it in the test harness instead
This is in preparation for things on the context becomming important
Change-Id: I7e6910e0d3581b82abbeeb09f9c22a99efb73142
Reviewed-on: https://go-review.googlesource.com/c/tools/+/185677
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We err on the side of refreshing package metadata if something goes
wrong getting a file's AST, but we don't want to refresh in the case
of context cancellation. Now we check for that error explicitly.
In particular, I noticed that completions would stop working when
typing quickly. Refreshing the metadata triggers "go list" calls which
can take a long time in certain cases.
Change-Id: I1b0c580e5541b1536a69ccaef241d7e8c5720d60
GitHub-Last-Rev: 6a82bfb586f93ef5e8e5996b11e06ffc7808f529
GitHub-Pull-Request: golang/tools#130
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184977
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This adds the ability to tie a background context to the context that created it
in traces, and also cleans up and annotates the context used in type checking.
This gives us detailed connected traces of all the type checking and parsing
logic.
Change-Id: I32721220a50ecb9b4404a4e9354343389d7a5219
Reviewed-on: https://go-review.googlesource.com/c/tools/+/183757
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This uses the new opencensus compatability layer to add telementry to some of
the functions in the lsp, in order to allow us to understand their costs and
call patterns.
Change-Id: I7df820cd4eace7a4840ac6397d5df402369bf0a7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/183419
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The first deadlock involved differing mutex acquisition order in two
code paths:
1. loadParseTypecheck() holds the "mcache" mutex then eventually
acquires the "handleMu" file mutex.
2. (*goFile).invalidateContent() acquires the "handleMu" mutex first and
then the "mcache" mutex.
Fix by changing the acquisition order in invalidateContent().
The second deadlock involved the file watcher. The two code paths
involved were:
1. (*goFile).GetPackages() holds the view mutex and eventually calls
(*WatchMap).Watch, which acquires the watcher mutex.
2. (*session).openOverlay acquires the watcher mutex as it triggers a
file's callbacks, and then the callback
"(*goFile).invalidateContent" acquires the view mutex.
Fix by not holding the watcher mutex as we invoke the callbacks.
Fixesgolang/go#32910
Change-Id: I9d060e0d80fd86a317a1d6c7aaa736a8ce10bd07
GitHub-Last-Rev: 04944fa0249c0e6f1022a415787e23abce21bc2e
GitHub-Pull-Request: golang/tools#129
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184880
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change modifies gopls to use the internal goimports library, which
allows us to manually configure the ProcessEnv. We also add a logger to
the ProcessEnv to allow this change not to conflict with gopls's logging
mechanism.
Fixesgolang/go#32585
Change-Id: Ic9aae69c7cfbc9b1f2e66aa8d812175dbc0065ce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184198
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>
As per discussion on golang/go#32810, to avoid the `go list` storm caused by many
files being opened, we check if the file content opened is equivalent to
the content on disk. If so, we mark this file as "on disk" so that we
don't send it as an overlay to go/packages.
Updates golang/go#32810
Change-Id: I0a520cf91bbe933c9afb76d0842f5556ac4e5b28
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184257
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
There was a situation where we were trying to re-acquire a lock that was
already held. This change solves this issue.
Change-Id: I97cf6bad7e7c219a267e3ca5d174a2573f70ebe2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184217
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change moves from marking a package with `go list` errors as
missing, to marking a package with no files as missing.
Change-Id: Ibad1e67518d8a7f4c4bde416c53ab8132ae534e3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184039
Reviewed-by: Ian Cottrell <iancottrell@google.com>
There has been a race condition that occasionally appears in test runs
on TryBots. Multiple threads perform type-checking, so they may race on
setting the fields of the *goFiles. Add a mutex to synchronize this.
Change-Id: If52c9d792c6504fc89044964998b06de7dfbd19c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/183978
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
typeCheck() was swallowing context.Canceled errors and leaving the
cached package in a bad state. In particular, after two rapid changes
to imports I was left in the "no package for file" error mode until I
change my imports again. The second change canceled the first change
which ended up sticking a skeleton *pkg in the package cache instead
of propagating the canceled error.
Change-Id: I15b072188c3359d9cd1812bd49e72548ba214250
GitHub-Last-Rev: 240f61718fbb5bfc787bbfaaaae1d38925d7c405
GitHub-Pull-Request: golang/tools#126
Reviewed-on: https://go-review.googlesource.com/c/tools/+/183940
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change adds supports for a package belonging to multiple files.
It requires additional packages.Loads for all of the packages to which a
file belongs (for example, if a non-test file also belongs to a package's
test variant).
For now, we re-run go/packages.Load for each file we open, regardless of
whether or not we already know about it.
This solves the issue of packages randomly belonging to a test or not.
Follow-up work needs to be done to support multiple packages in
references, rename, and diagnostics.
Fixesgolang/go#32791Fixesgolang/go#30100
Change-Id: I0a5870a05825fc16cc46d405ef50c775094b0fbb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/183628
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change just separates minor changes made along the course of the
memoization CL out into their own change. This will clean up the diffs
in the memoization CL.
Change-Id: I7d59e05ba6472af5f1bf516b1e5b879a5815b9a5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/183250
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
So we can surface their code actions later.
Change-Id: I735e5d025a1250861d49db227f5a79453f599140
Reviewed-on: https://go-review.googlesource.com/c/tools/+/182837
Run-TryBot: Michael Matloob <matloob@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
The `if err :=` block creates a shadow err value that is then discarded
after the block.
Change-Id: I78f6a7298ac5d3d86ece056a9e328bcee9fdc683
Change-Id: I78f6a7298ac5d3d86ece056a9e328bcee9fdc683
GitHub-Last-Rev: 4bac8fb982fa3a0724206422b1a4dbc5e41c34a9
GitHub-Pull-Request: golang/tools#114
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181777
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Without this accessing gof.ast.file.Pos() will panic if gof.ast.file is nil
Change-Id: I829f3667d201f026fcf0475f4fdabce0aced58f8
Change-Id: I829f3667d201f026fcf0475f4fdabce0aced58f8
GitHub-Last-Rev: 777c015d691bf0d14cab5bb0bca6749cc9bdf605
GitHub-Pull-Request: golang/tools#119
Reviewed-on: https://go-review.googlesource.com/c/tools/+/182420
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This changes the packageErrorSpan function into the listErrorSpan.
Previously, this was causing the gopls-generated errors to get parsed,
which would result in attempts to send diagnostics for invalid filenames.
Fixesgolang/go#32603
Change-Id: I7a54ed8884b78beb3f894598f18a24ed232f7412
Reviewed-on: https://go-review.googlesource.com/c/tools/+/182460
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change does not actually use the token handle for GetToken right
now, but implements the approach for memoizing *token.Files.
Change-Id: I75919f4e97abd6893b202c021adecd2c9dbfc2be
Reviewed-on: https://go-review.googlesource.com/c/tools/+/182277
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change ensures that we always populate a new set of missing imports
when we re-load a package. Otherwise, we might keep stale missing
imports around.
Change-Id: Id9b6b8201e3ed093a7b9ece97a81e298de612a5e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/182459
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This adds an IDs map to the metadata cache, which maps package paths to
IDs. This is only ever used by the Import function in the type checker.
Change-Id: I8677d9439895bc6cbca5072e3fa9fddad4e165d5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181683
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Previously when you added a new file to an existing package, the new
file would get stuck with the "no package for file" error until you
saved the file and then made changed a different file in the
package. There were two changes required to fix the errors:
First, we need to invalidate the package cache when a new file is
added to a package so that the package will actually re-parse and
re-type check. We now notice if file names changed when updating a
package's metadata and invalidate the package cache accordingly.
Second, when dealing with overlay (unsaved) files, we need to map
the *goFile to the package even if we fail to parse the
file (e.g. the new file fails to parse when it is empty). If we don't
map it to the package, the package won't get refreshed as the file is
changed.
Fixesgolang/go#32341
Change-Id: I1a728fbedc79da7d5fe69554a5893efcd1e1d902
GitHub-Last-Rev: e7c3d4c1f8f73b12c87ee76d868cc04893e55808
GitHub-Pull-Request: golang/tools#111
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181417
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change replaces the strings that were previously used for both the
ID and package path fields. This is a precursor to the change that will
replace the uses of package path with package ID.
Change-Id: I353e98aedede9b85c7a183fdd49048ff43b1e26d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181757
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Previously these were only applied from inside parseFiles, which also made it
harder to refactor the remaining parse logic.
This theoretically means fixup is now called in more places than it was before,
but should cause no change in behaviour.
Change-Id: Ic6d006c1d36daca7514626653aaedf90d76e1d0f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181544
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
If the context is canceled (or times out) during parsing, we were
previously caching the package with no *ast.Files. Any further LSP
queries against that package would fail because the package is already
loaded, but none of the files are mapped to the package. Fix by
propagating non-parse errors as "fatal" errors in
parseFiles. typeCheck will propagate these errors and not cache the
package.
I also fixed the package cache to not cache errors loading
packages. If you get an error like "context canceled" then none of the
package's files are mapped to the package. This prevents the package
from ever getting unmapped when its files are updated. I also added a
retry mechanism where if the current request is not canceled but the
package failed to load due to a previous request being canceled, this
request can try loading the package again.
Updates golang/go#32354, golang/go#32360
Change-Id: I466ddb8d336aeecf6e50f9f6d040787a86a60ca0
GitHub-Last-Rev: 5f1e7ef9c883b76a9c1b3636936d91ec0821d922
GitHub-Pull-Request: golang/tools#110
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181317
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We weren't comparing the right values when checking to see if a file's
import statements had changed. This was causing us to not refetch a
package's metadata when needed in certain cases. In particular, if you
typed out an import path by hand you would get stuck with "no metadata
for package" until you added or deleted another import line to trigger
metadata refresh.
Updates golang/go#32516, golang/go#32232
Change-Id: I1c99e9ee56d18f02c44c749d33679aa4e561c620
GitHub-Last-Rev: 20d46b30b520572b05e3228d9e43ba46dfd1be3d
GitHub-Pull-Request: golang/tools#112
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181578
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
We panic if the uri was not a valid file uri instead
They always are a valid file URI, and we would fail miserably to cope if they were
not anyway, and there are lots of places where we need to be able to get the filename
and don't want to cope with an error that cannot occur.
If we ever have not file uri's, you will have to check if it is a file before calling
.Filename, which seems reasonable anyway.
Change-Id: Ifb26a165bd43c2d310378314550b5749b09e2ebd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181017
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change marks any unresolved packages discovered as imports by
go/packages.Load. It re-runs go/packages.Load for any package with
missing imports, which will result in the LSP registering changes in
dependencies.
However, this means that we re-run go/packages.Load on
package's with unresolved imports much more than we normally would, so
it may result in a slowdown or unexpected behavior. I'm not sure if this
is necessarily the correct approach here.
Updates golang/go#32232
Change-Id: Id611fa1876e42c88ca2c3e4db30da66dc66945fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/180537
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change enables the trimmed ASTs behavior and fixes the broken
tests by caching parse errors along with the ASTs that are being reused.
Change-Id: Ia8e7cdb138c62a8be88659f801506e03f213fe8c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181357
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This change marks the overlay for a saved file as "on disk".
This will reduce the number of overlays we provide to go/packages, which
can be expensive.
Updates golang/go#31796, golang/go#32457
Change-Id: I8e69503ab80bba29caf4e42491d87e643bf17f1a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181123
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Peter Weinberger <pjw@google.com>
This change correctly invalidates the cache when we
have to go from a trimmed to untrimmed AST.
The "ignoreFuncBodies" behavior is still disabled due to a racy test.
Updates golang/go#30309
Change-Id: I6b89d1d2140d77517616cb3956721a157c25ab71
Reviewed-on: https://go-review.googlesource.com/c/tools/+/180857
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
On FileHandle Read now just returns the data hash and error
This makes it more obvious that you should handle the error, rather than hiding
it all in a struct.
We also change the way we get and return content, the main source.File
constructs now hold a FileHandle that then updates on invalidation
Change-Id: I20be1b995355e948244342130eafec056df10081
Reviewed-on: https://go-review.googlesource.com/c/tools/+/180417
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
