ParseGoHandles serve two purposes: they pin cache entries so that
redundant calculations are cached, and they allow users to obtain the
actual parsed AST. The former is an implementation detail, and the
latter turns out to just be an annoyance.
Parsed Go files are obtained from two places. By far the most common is
from a type checked package. But a type checked package must by
definition have already parsed all the files it contains, so the PGH
is already computed and cannot have failed. Type checked packages can
simply return the parsed file without requiring a separate Check
operation. We do want to pin the cache entries in this case, which I've
done by holding on to the PGH in cache.pkg.
There are some cases where we directly parse a file, such as for the
FoldingRange LSP call, which doesn't need type information. Those parses
can actually fail, so we do need an error check. But we don't need the
PGH; in all cases we are immediately using and discarding it.
So it turns out we don't actually need the PGH type at all, at least not
in the public API. Instead, we can pass around a concrete struct that
has the various pieces of data directly available.
This uncovered a bug in typeCheck: it should fail if it encounters any
real errors.
Change-Id: I203bf2dd79d5d65c01392d69c2cf4f7744fde7fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244021
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Due to the runtime's inability to collect cycles involving finalizers,
we can't close over handles in memoize.Functions without causing memory
leaks. Up until now we've dealt with that by closing over all the bits
of the snapshot that we want, but it distorts the design of all the code
used in the Functions.
We can solve the problem another way: instead of closing over the
snapshot/view, we can force the caller to pass it in. This is somewhat
scary: there is no requirement that the argument matches the data that
we're working with. But the reality is that this is not a new problem:
the Function used to calculate a cache value is not necessarily the one
that the caller expects. As long as the cache key fully identifies all
the inputs to the Function, the output should be correct. And since the
caller used the snapshot/view to calculate that cache key, it should
always be safe to pass in that snapshot/view. If it's not, then we
already had a bug.
The Arg type in memoize is clumsy, but I thought it would be nice to
have at least a little bit of type safety. I'm open to suggestions.
Change-Id: I23f546638b0c66a4698620a986949087211f4762
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244019
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The PackageHandle interface is fairly redundant with the Package
interface. As much as convenient, move users to Package and
weaken/remove methods from PackageHandle.
I would like to get rid of CompiledGoFiles too but
NarrowestPackageHandle is a little annoying. I think this is
unambiguously a step forward so I figured we can get it in and keep
iterating.
Change-Id: I6c5a3f462b1f19cbca6a267fedc36ce54613b6fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244018
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
When a package consists of files that only fail to parse, we fail to
associate parse errors with the package, and therefore return no
diagnostics. To address this, we need to associate the errors with the
package. This involves adding a *token.File to the parseGoData so that
error messages and positions can be properly extracted, even when there
is no associated AST.
Fixesgolang/go#39763
Change-Id: I5620821b9bcbeb499c498f9061dcf487d159bed5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/243579
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Currently, diagnostics for modules that are missing from the go.mod
appear in the Go files in which those modules are imported. As a result,
the diagnostics were previously calculated as part of the Go file
diagnostic calculations. This is convoluted and required passing around
an extra map.
This CL puts that logic in the ModTidyHandle where it belongs.
The diagnostics for the Go files are combined from the multiple sources.
Also, added a skipped test for golang/go#39784, since this CL was
originally intended to be a fix for that issue...
Change-Id: Ic0f9aa235dcd56ea131a2339de9801346f715415
Reviewed-on: https://go-review.googlesource.com/c/tools/+/242579
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Completion could be slow due to calls to astutil.PathEnclosingInterval
for every candidate during formatting. There were two reasons we
called PEI:
1. To properly render type alias names, we must refer to the AST
because the alias name is not available in the typed world.
Previously we would call PEI to find the *type.Var's
corresponding *ast.Field, but now we have a PosToField cache that
lets us jump straight from the types.Object's token.Pos to the
corresponding *ast.Field.
2. To display an object's documentation we must refer to the AST. We
need the object's declaring node and any containing ast.Decl. We
now maintain a special PosToDecl cache so we can avoid the PEI call
in this case as well.
We can't use a single cache for both because the *ast.Field's position
is present in both caches (but points to different nodes). The caches
are memoized to defer generation until they are needed and to save
work creating them if the *ast.Files haven't changed.
These changes speed up completing the fields of
github.com/aws/aws-sdk-go/service/ec2 from 18.5s to 45ms on my laptop.
Fixesgolang/go#37450.
Change-Id: I25cc5ea39551db728a2348f346342ebebeddd049
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221021
Run-TryBot: Muir Manders <muir@mnd.rs>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Add a symbolStyle configuration option, and use it to parameterize the
following behavior when computing workspace symbols:
+ package (default): include package name in the workspace symbol.
+ full: fully qualify the symbol by import path
+ dynamic: use as the symbol the shortest suffix of the full path that
contains the match.
To implement this, expose package name in the source.Package interface.
To be consistent with other handling in the cache package, define a new
cache.packageName named string type, to avoid confusion with packageID
or packagePath (if confusing those two identifiers was a problem, surely
it is a potential problem for package name as well).
Change-Id: Ic8ed6ba5473b0523b97e677878e5e6bddfff10a7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/236842
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Paul Jolly <paul@myitcv.org.uk>
In GOPATH mode, you can import example.com/foo even if it has a go.mod
that says its path is example.com/foo/v2. Adjust our import path
resolution to support that case.
Fixesgolang/go#39560.
Change-Id: I758a2220c579c4374084365c3a78a3a2bbd14b01
Reviewed-on: https://go-review.googlesource.com/c/tools/+/238260
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We use file identities pervasively throughout gopls. Prior to this
change, the identity is the modification date of an unopened file, or
the hash of an opened file. That means that opening a file changes its
identity, which causes unnecessary churn in the cache.
Unfortunately, there isn't an easy way to fix this. Changing the
cache key to something else, such as the modification time, means that
we won't unify cache entries if a change is made and then undone. The
approach here is to read files eagerly in GetFile, so that we know their
hashes immediately. That resolves the churn, but means that we do a ton
of file IO at startup.
Incidental changes:
Remove the FileSystem interface; there was only one implementation and
it added a fair amount of cruft. We have many other places that assume
os.Stat and such work.
Add direct accessors to FileHandle for URI, Kind, and Version. Most uses
of (FileHandle).Identity were for stuff that we derive solely from the
URI, and this helped me disentangle them. It is a *ton* of churn,
though. I can revert it if you want.
Change-Id: Ia2133bc527f71daf81c9d674951726a232ca5bc9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/237037
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
In CL 229779 I enabled Cgo type checking for go/packages, but we don't
actually type check there. We need to enable it in our own type checking
too.
No test updates because the negative effects are relatively subtle and
caught by an upcoming regtest.
Change-Id: I31691d69eb104cdabfd4fbe0a14b1f3c9741eabb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/234102
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
In (*snapshot).addPackage, we return early if the package handle is
already cached, but we continue building the dependency graph with a
handle passed into addPackage.
This seems fine since both handles should have the same cache key,
but if we clone the snapshot, we will end up dropping the handle that
had the type information on it. It will then have to be recomputed,
causing the skew in the types.Package.
Fixesgolang/go#38403
Change-Id: I0e360447a428123fcac444fbea3c2a3232ef941a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/232817
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
LoadMode and ParseMode are currently hashed with a string(int)
conversion, but this conversion is now discouraged (see the vet check
'stringintconv' for more information). Since the hash uses the code
point, this change replaces these instances with string(rune(int)).
Updates golang/go#32479.
Change-Id: I0d8e91d073fc34ac9faafe75a0d86cf71a5327d4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/232679
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
event.Log removed
event.Print -> event.Log
event.Record -> event.Metric
event.StartSpan -> event.Start
In order to support this core now exposes the MakeEvent and Export functions.
Change-Id: Ic7550d88dbf400e32c419adbb61d1546c471841e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/229238
Reviewed-by: Robert Findley <rfindley@google.com>
internal/telemetry/event was renamed to internal/event/core
Some things were partly moved from internal/telemetry/event straight to
internal/event to minimize churn in the following restructuring.
Change-Id: I8511241c68d2d05f64c52dbe04748086dd325158
Reviewed-on: https://go-review.googlesource.com/c/tools/+/229237
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
I was curious about why were logging errors during type-checking in
tests, and the answer turned out to be a bit more sinister than I
expected. We were getting type error messages without filepaths, so I
tried to reproduce it in the playground and wasn't able to. I realized
that these errors were coming from were coming from the "fixed" version
of the AST that we pass to the type checker.
Adding fake positions to our fake Cond statements trivially fixes the
logging issue, but it does nothing to handle the fact that the error
makes no sense to the user - because it applies to something that's not
in the source code. I figured we have two options: (1) skip type errors
for all packages with "fixed" ASTs, or (2) add something to the error
messages to indicate that the source code may not match. Starting with
(1) here, and if it becomes a problem, we can move to 2. All ASTs that
we fix have *ast.BadExpr in them, meaning that, by definition they have
parse errors which we will preferentially show those errors to users in
diagnostics (so I'm not sure how to test this).
Change-Id: I17733968aa15f989cdd3e4e7261c4f4fe9b97495
Reviewed-on: https://go-review.googlesource.com/c/tools/+/227557
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This change adds a helper function that checks if the context is
canceled, and if so, doesn't log the error. Tried to use it everywhere
in internal/lsp where it fits, which resulted in changing a few pieces
of error handling throughout.
Updates golang/go#37875
Change-Id: I59cbc6f893e3b70cf84524d9944ff7f4b4febd78
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226371
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>
This change adds support within gopls for analyzers that work with type errors to provide suggested fixes.
Updates golang/go#34644
Change-Id: Ia8929173752fda6bd84a9edaabd310e758f25fe8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222761
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This changes the way keys work, there is still a single internal key
implementation for performance reasons, but the public interface is a set of key
implementations that have type safe Of and Get methods.
This also hides the implemenation of Tag so that we can modify the storage form
and find a more efficient storage if needed.
Change-Id: I6a39cc75c2824c6a92e52d59f16e82e876f16e9c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/223137
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Move the lsp specific telemetry package that just declares the labels under the
debug package and call it tag, to make all the usages much more readable.
Change-Id: Ic89b3408dd3b8b3d914cc69d81f41b8919aaf424
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222850
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
I'm still not sure if we need to handle any other non-standard package
path apart from "command-line-arguments".
Also, a couple of staticcheck fixes.
Change-Id: I0bb3e60f6ffe104ff9027dbebb628020caaa1af4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220138
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
An error should be reported if an "internal" package is imported
into code that is outside of the tree rooted at the parent
of the "internal" directory.
Fixes#35937
Change-Id: If5ff3dd79b462087381d575dddb20b78c10f0a83
GitHub-Last-Rev: f5d19960046da7f9701325afc36b5bd0b9663ab6
GitHub-Pull-Request: golang/tools#207
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218977
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change appends to the pkg.go.dev link the version of the module that is being used. To get this functionality, go/packages.Package now contains a module field which gets populated from the "go list" call. This module field is then used to get the version of the module that we are linking to.
Updates golang/go#36501
Change-Id: I9668a6da0fd3ec8f4cde017986419c8d28196765
Reviewed-on: https://go-review.googlesource.com/c/tools/+/219079
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Currently there is no need for this because the file contents are part
of the file handle. This change is in preparation for an impending
improvement that tweaks the source code during the parse stage to fix
certain kind of terminal parse errors. Any code that wants to use
an *ast.File or *token.File in conjunction with the file contents
needs access to the doctored source code so things line up.
Change-Id: I59d83d3d6150aa1264761aa2c1f6c1269075a2ce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218979
Run-TryBot: Muir Manders <muir@mnd.rs>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change largely reverts CL 217139, which attempted to guess a
package's parse mode based on whether or not it was in the user's
workspace. This ignored the fact that a user may jump to the definition
of a file outside of their workspace.
Fixesgolang/go#37045
Change-Id: Icb6b9d055bd1f260013227db1a6a34873c45b680
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218499
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
This change alters the key that is used to cache go.mod diagnostic changes, in particular it replaces using the snapshot ID with a string of all the imports used in the module and the hashed contents of the go.mod file. This reduces the number of times that we run "go mod tidy" to only when we detect import changes or the go.mod file is changed.
Change-Id: Icf49db34f44a4ae4772fff6dfb8b9a6955a8e2d6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218238
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This metadata was hardly being used, and it's not really necessary now
that we are creating package handles in load. There are still a number
of cases that can simplified because of this fact, but those will be
done in follow-ups.
Also, fix a stray staticcheck warning.
Change-Id: I12d1b4d568a8fd145d08397a926e7ba6f3428604
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217138
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Our current invariant is that all workspace packages are parsed in full
mode, and all dependencies are parsed in exported mode. We can rely on
this, as well as the fact that workspace packages are set during
metadata loads, to reduce the amount of plumbing the mode requires.W
Change-Id: Ib9406ca3c0dc2c81c7ee3158407f28022924d4d0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217139
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
We seem to be leaking cache entries. A simple status page will help us
confirm that.
Change-Id: I485bfff6ebfb5d30655554487583e15a3f49f9a4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217597
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
source.Identifier previously was used for references and rename, so it
needed to take a package policy. Now, it's only used for definition and
hover, so it should always be the narrowest package handle. We can use
this fact to determine if the identifier is located in its declaring
package, and if that package is a test variant, we don't link to the
documentation on pkg.go.dev, since it doesn't exist.
Change-Id: I5686828858a3feafb8ff2e4c5964b562f66db9fa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217137
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This change adds a check inside of the types.ImporterFunc to see if the context has been cancelled.
Updates golang/go#34683
Change-Id: I0f12da0f8158ecda0eec00150ed6ff772c2f89c3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217257
Run-TryBot: Rohan Challa <rohan@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
We treat package IDs and import paths as semi-interchangeable, which is
wrong when GOPATH vendoring is in use. The only place it hurts us is
during import resolution, which is fixed here. We should always have the
package loaded, so it's just a matter of finding it by searching each
possible vendor location.
Fixesgolang/go#36155.
Change-Id: If789092d16fa3d3294b6d8a2bcb980264506c161
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215904
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
In golang.org/cl/209419, CheckPackageHandle was renamed to
PackageHandle, but a number of references to CheckPackageHandle remained
in function names and comments.
This CL cleans up most of these, though there was at least one case
(internal/lsp/cache.checkPackageKey) where the obvious renaming
conflicted with another function, so I skipped it.
Change-Id: I517324279ff05bd5b1cab4eeb212a0090ca3e3ad
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214800
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reloading metadata on demand fails for some our test packages,
because I don't understand how to construct arguments to commands.
Change-Id: Ib18454a09772e854a612528af898d06ce14133c2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214717
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
There is no reason for these functions to live on the view. They make
more sense as unexported functions in internal/lsp/source.
Initially, I had to propagate contexts through a lot of functions in
internal/lsp/source, but instead I removed the unused contexts forom
snapshot.GetFile.
Change-Id: I8323419d0356feb2010091fe8d3ed35e511f801a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214384
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
A test variant for a package can only be reloaded by running go/packages
on the non-test variants import path with the -test flag. We need to
cache this import path in order to be able to reload a test package
on-demand.
Also, always ignore test main packages by detecting them in the
metadata.
Fixesgolang/go#36473
Change-Id: I6e5a61c8e73689212303ae09fa3aed4a2ee8762a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/213660
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
The initial workspace load was happening when a view was created, in serial.
It should really just be kicked off in a separate goroutine once we create a
new view. Implementing this change required some other significant changes,
particularly the additional work being done by the WorkspacePackageIDs
method.
Some other changes had to be made while debugging. In particular, the
modification to the circular dependencies test was a consequence of
golang/go#36265.
Change-Id: I97586c9574f6c4106172d7983e4c6fad412e6aa1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212102
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
The metadata for the workspace packages may not be available when we
need it, so we should allow loading a single package ID. This can be
improved in follow-up CLs by consolidating the individual IDs into one
call to packages.Load. Some adjustments from CL 212102 were split out
into this CL.
Change-Id: I173a79a3cb136530bc99d093f1c2be189eac8ce2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212628
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
In one of my previous refactoring changes, I lost the fact that the
context should be detached before invalidating a file's contents. If
this function is canceled, we will be in a bad state.
Also, small change to return ctx.Err() instead of a custom error message
from (*packageHandle).check.
Change-Id: I19e513e09e438feee105fdd89cb7364a0c3c5e7f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212104
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Now that `go list` errors are sufficient for us to determine a circular
dependency, we don't need to cache errors on dependency packages.
Change-Id: I0633aeb356f93d21afed3371d61d7eae7de255ac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212197
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rohan Challa <rohan@golang.org>
Reviewed-by: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
This change eliminates the extra step of calling GetFile on the view and
getting the FileHandle from the snapshot. It also eliminiates the
redundant source.File type. Follow up changes will clean up the file
kind handling, since it still exists on the fileBase type.
Change-Id: I635ab8632821b36e062be5151eaab425a5698f60
Reviewed-on: https://go-review.googlesource.com/c/tools/+/211778
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
After the addition of golang/go#35964, the import cycle error now
has the import stack attached in the message. This CL parses that
stack and attached the import cycle diagnostic to the import versus
just adding it to the first character of the .go file.
Fixesgolang/go#33085
Change-Id: I6f5f067c338879b898829951236f816aa63d9dfa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/210942
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change will provide a more useful error when you
are self importing a package. It has TODOs in place to propagate the
"import cycle not allowed" error from go list to the user.
Updates golang/go#33085
Change-Id: Ia868a7c688b0f0a7a9689cfda5ea8cea8ae1faff
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209857
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
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>
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>
