Despite the name, ParseExported only hollowed out declarations -- it
didn't actually drop any from the AST. This leaves a fair amount of
unexported crud behind. Unfortunately, there are a *lot* of ways to
expose an unexported declaration from an exported one, and it can be
done across files. Because of that, discarding unexported declarations
requires a lot of work.
This CL implements a decent attempt at pruning as much as possible from
the AST in ParseExported mode.
First, we analyze the AST of all the files in the package for exported
uses of unexported identifiers, iterating to a fixed point. Then, we
type check those ASTs. If there are missing identifiers (probably due to
a bug in the dependency analysis) we use those errors to re-parse. After
that we give up and fall back to the older, less effective trimming. The
pkg type changes slightly to accomodate the new control flow.
We have to analyze all the files at once because an unexported type
might be exposed in another file. Unfortunately, that means we can't
parse a single file at a time any more -- the result of parsing a file
depends on the result of parsing its siblings. To avoid cache
corruption, we have to do the parsing directly in type checking,
uncached.
This, in turn, required changes to the PosTo* functions. Previously,
they operated just on files, but a file name is no longer sufficient to
get a ParseExported AST. Change them to work on Packages instead. I
squeezed in a bit of refactoring while I was touching them.
Change-Id: I61249144ffa43ad645ed38d79e873e3998b0f38d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/312471
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Provide some support for template files, implementing most of
https://docs.google.com/document/d/1clKAywucZVBXvL_v4mMhLQXso59lmQPMk1gtSpkV-Xw
Template support is controlled by the option 'experimentalTemplateSupport'
which defaults to false.
Most of the code is in a new 'template' package. Implemented are
semantic tokens, diagnostics, definitions, hover, and references,
and there is a stub for completions.
This code treats all the template files of a package together, so as
to follow cross-references.
Change-Id: I793606d8a0c9e96a0c015162d68f56b5d8599294
Reviewed-on: https://go-review.googlesource.com/c/tools/+/297871
Run-TryBot: Peter Weinberger <pjw@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Trust: Peter Weinberger <pjw@google.com>
ast.NewPackage mutates the input files, making it difficult to avoid
races with our caching model. I had avoided a race resulting from cache
handle cancellation, just to run into another race in multi-session
servers.
But there's no reason to use ast.NewPackage when we only have a single
file. We can just interrogate the file scope wherever needed.
Fixesgolang/go#45868
Change-Id: I521475b51ee3b1c3e408916affecafbc629b0191
Reviewed-on: https://go-review.googlesource.com/c/tools/+/315629
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Calling PackagesForFile on builtin.go loads is at a
command-line-arguments package, which has many type checking errors.
Add a new snapshot method IsBuiltin, which is used to avoid calling
PackagesForFile on builtin.go when diagnosing changed files or checking
for orphaned files. There may be other places where this should be used,
but this functionality can't reasonably be pushed down, as
PackagesForFile should always return something.
This exacerbated an existing race to building the builtin, because
ast.NewPackage unfortunately mutates the ast.File. Fix this by just
building the builtin package directly when building the handle. It
should be very fast.
Fixesgolang/go#44866
Change-Id: Ie6c07478493fa011e92e6966289c2fa822d87b35
Reviewed-on: https://go-review.googlesource.com/c/tools/+/314290
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
The existing implementation uses a lot of URI.Filename() calls,
which are pretty expensive. Moreover, these calls are not necessary,
as long as all the actions could be done with the raw URI string.
This patch removes such calls and uses simple string casts.
Updates golang/go#45686
Change-Id: Ibe11735969eaf0cfe33024f08418e14bf71e7fc4
GitHub-Last-Rev: 67a3ccdf30a6a99bc1b5a8e9cd2a7c0865d894d0
GitHub-Pull-Request: golang/tools#306
Reviewed-on: https://go-review.googlesource.com/c/tools/+/312809
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Rebecca Stambler <rstambler@golang.org>
Trust: Suzy Mueller <suzmue@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
For large codebases, the cost of copying these maps can be fairly high,
especially when it needs to repeatedly grow the map's underlying storage.
Preallocate these to the size of the original snapshot maps to prevent
the need to grow the storage during the clone.
Updates golang/go#45686
Change-Id: I4cfcd5b7cba8110e4f7e706fd9ea968aaeb6ff0c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/312689
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We still hear from users for whom gopls uses too much memory. My efforts
to reduce memory usage while maintaining functionality are proving
fruitless, so perhaps it's time to accept some functionality loss.
DegradeClosed MemoryMode typechecks all packages in ParseExported mode
unless they have a file open. This should dramatically reduce memory
usage in monorepo-style scenarious, where a ton of packages are in the
workspace and the user might plausibly want to edit any of them.
(Otherwise they should consider using directory filters.)
The cost is that features that work across multiple packages...won't.
Find references, for example, will only find uses in open packages or in
the exported declarations of closed packages.
The current implementation is a bit leaky; we keep the ParseFull
packages in memory even once all their files are closed. This is related
to a general failure on our part to drop unused packages from the
snapshot, so I'm not going to try to fix it here.
Updates golang/go#45457, golang/go#45363.
Change-Id: I38b2aeeff81a1118024aed16a3b75e18f17893e2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/310170
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
This broke staticcheck and x/tools/refactor, most notably used for our
rename support. Doesn't look like a winner. Roll it back :(
Updates golang/go#45457.
Change-Id: I30d5aa160fd9319329d36b2a534ee3c756090726
Reviewed-on: https://go-review.googlesource.com/c/tools/+/311549
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
As predicted in CL 308730, there are some analyzers that need this: all
of staticcheck. (I probably should've seen that coming.) For now, don't
do the trimming when staticcheck is on. Since staticcheck ~doubles
memory footprint, those users probably don't care much.
Change-Id: I7cfd96b3654e3617d28d62015928a45a85407d13
Reviewed-on: https://go-review.googlesource.com/c/tools/+/311376
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
The type checker emits a TypeAndValue entry for (among other things)
every constant in a Go file. Normally, that cost is moderate. However,
in the case of a large slice literal, it can get out of control very
quickly. Imagine a code generator that creates a 2KB byte slice literal;
that's 2K TypeAndValue entries, each of which is considerably larger
than the 1-3 bytes for the source text.
Unfortunately, there are a number of such code generators. Notably,
there are such slice literals in proto code, e.g.
https://github.com/grpc/grpc-go/blob/master/examples/route_guide/routeguide/route_guide.pb.go#L360
This CL changes the type checking code to remove the TypeAndValue
entries for slice literals of basic types after the checker returns.
In the extreme case of https://github.com/googleapis/go-genproto, which
is nothing but protos, I see a ~40% drop in heap usage.
I believe this change is generally safe, but there's no way to guarantee
it. I don't think any editor features need to know the type or value of
an arbitrary slice element, but it is just barely possible that an
analyzer does.
Change-Id: Iee1af2369f994597a42fd1dcbf8af20faa43410e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/308730
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Links in the new LSP diagnostic CodeDescription for go/types errors are
missing a scheme (unclear why they work in some clients). Fix this by
delegating to the existing source.BuildLink helper (this also adds the
utm_source query parameter for pkg.go.dev).
Fixesgolang/go#44360
Change-Id: Ife8969f32bb11840c0fdd31765b6051b3d997a93
Reviewed-on: https://go-review.googlesource.com/c/tools/+/293509
Trust: Rebecca Stambler <rstambler@golang.org>
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Now that https://github.com/bazelbuild/rules_go has a working prototype of a `GOPACKAGESDRIVER`, it may be time to revert that commit.
The draft implementation is at https://github.com/bazelbuild/rules_go/pull/2858.
Change-Id: Ia738e8be448d936f8a3b2b421d0a765f94bbff52
GitHub-Last-Rev: 0df6c91074febdddb5703a39591090acb5f42c05
GitHub-Pull-Request: golang/tools#297
Reviewed-on: https://go-review.googlesource.com/c/tools/+/307169
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Some of the refactoring changed the way that we label code action kinds,
and we need to add quickfix and fixall kinds for each diagnostic type.
Support a per-kind suggested fix, and fix a small issue in setting the
analyzer for a fixall code action.
Fixesgolang/go#45111
Change-Id: I6bb32c9aa7427b690f42910672d3139579e84478
Reviewed-on: https://go-review.googlesource.com/c/tools/+/303209
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
According to its comment, reloadOrphanedFiles is intended to work around
overlay bugs. But as of 1.16, there are no overlay bugs that we know of.
So what is it still doing?
Apparently, quite a bit, not much of it useful. Clean up as much as
possible.
- Files with no valid package declaration are ignored by the go command.
There's no point trying to reload them; stop.
- During metadata invalidation, we clear out all IDs for a file, even if
only one of its IDs is invalidated, e.g. when a test package is removed.
That leaves valid metadata for the non-test, so we don't refresh it in
the workspace reload, and only catch it as an orphan. It seems to me we
should only remove the invalidated ID.
- If the client incorrectly sends us a didOpen for a non-Go file, we
will attempt to load it as an orphaned file. Fix the regtest that did
that.
- TestEmptyGOPATHXTest_40825 set up an invalid GOPATH: you can't work in
GOPATH/src. However, it exists to test code that no longer exists, so
just delete it.
After this change, almost none of the regression tests trigger orphaned
file reloading. It's difficult/impractical to rule it out entirely
because some of them only appear racily. Since I intend to remove the
code path, I'm not too worried about more creeping in before I'm done.
The only useful case is multiple ad-hoc packages. Because we only
allow one "command-line-arguments" package in the workspace, if you
switch between two the old one becomes orphaned. I hope to work on that
soon.
Change-Id: Ia355cf104280ce51f6189c6638e8da8f4aef2ace
Reviewed-on: https://go-review.googlesource.com/c/tools/+/302089
Trust: Heschi Kreinick <heschi@google.com>
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Locating the workspace module by convention has multiple problems:
+ gopls's view of $TMPDIR might be different from the client
+ there might be multiple views
+ there might be multiple gopls sessions per pid
Instead, assign a temp workspace directory for each workspace folder,
and provide a command to access this information.
Cleaning up all these temp directories was overcomplicated. Instead,
create a temp directory for the gopls server to nest them under, that
can be removed up on server shutdown.
Also fix a bug where the snapshot was not acquired before copying its
workspace.
Updates golang/go#42252
Change-Id: I0641cebe09cd376dfa27373cac30397711c64a8f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/300409
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
In debugging the metadata CLs, I noticed that when a view's only go.mod
file became unparseable, we would fall back into reloading the entire
view. In such cases, we should just not reinitialize.
Change-Id: I1b552158da8855bf80e9ded8b29c346c67564239
Reviewed-on: https://go-review.googlesource.com/c/tools/+/300674
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Generating go get quick fixes in a single place only made it harder to
get them right. Do it during diagnostics generation, as is the new
norm.
Also improve the user experience. When we fail to import a package
because one of its dependencies is missing, it makes more sense to run
go get on the package we tried to import, not the one that's missing:
that will download all of its missing dependencies if there happen to be
more.
Change-Id: Ib6a8140bccfafcb9f966d25639799dd4c7347c3d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/300072
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
This change fixes x/tools tests before CL 298650 lands. After that CL,
the go command reports a different error message when GO111MODULE=on
and it is invoked without a go.mod file (with a command that requires
a go.mod file). We plan to backport that change to 1.16.
For golang/go#44745
Change-Id: Idb3e146828703c89f788ae660ffc95aef16433e4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/298792
Trust: Jay Conrod <jayconrod@google.com>
Reviewed-by: Peter Weinberger <pjw@google.com>
Now that we're generating quick fixes at analysis time, we can use those
in code action requests and delete a fair amount of redundancy. The
codeAction function is a little cluttered, but I want to get it all in
one place before I decide how to split it up.
Change-Id: Icd91e2547542cce0a05c18c02a088833f71232a4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/297532
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Type error analyzers can be viewed as enhancing type errors, rather
than analyzers in their own right. Create a source.DiagnosePackage
function that combines the list/parse/typecheck diagnostics with type
error analyzers. This allows us to remove some special cases from the
analysis path, and is a first step in removing all the special
handling for analysis quick fixes.
Along the way:
Pass pointers to source.Analyzer after I spent half an hour chasing a
loop capture bug. Spend a further 2-3 hours chasing slowdown in the
command tests as a result.
Move Unnecessary tag generation into diagnostic creation rather than
as a mutating post-processing step that required cloning diagnostics.
Change-Id: Id246667a9dcf484dc79516f92d5524261c435794
Reviewed-on: https://go-review.googlesource.com/c/tools/+/297879
Trust: Heschi Kreinick <heschi@google.com>
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Most callers of source.Package.GetDiagnostics do it via
GetTypeCheckDiagnostics. Push its logic up or down as appropriate and
delete it.
Rather than requiring fully populated maps of diagnostics, which was
rather subtle, call storeDiagnostics for every Go file in the package.
Change-Id: If43b0cc922af1013e80f969362246538df14985b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/297878
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
In CL 295413 we fixed the handling of related type checker diagnostics
to correctly identify the primary and secondary errors at a position.
However, on clients that don't support diagnostic related information,
this can lead to confusing primary diagnostics.
Add handling for clients that don't support related information, to
embed the secondary error in the primary error.
Fixesgolang/go#44735
Change-Id: I3d2470d2a4044661e6ed31ac9ffd2f9ff27f7d4b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/297875
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
CL 295414 suppressed all type checking errors in the presence of parser
errors, but this was a bit overzealous. go/types attempts to suppress
follow-on errors from bad syntax, so as long as we haven't had to modify
the AST and the current file parses, type checking errors can still
provide a decent signal to the user
Fixesgolang/go#44736
Change-Id: Icd89404fbae663b8f934a38908eaaa87c561b64a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/297872
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
In practice, the only code shared among the various switch cases was the
call to spanToRange, which definitely doesn't justify the giant
function. Split it out into per-type functions.
I removed the unused case for types.Error, and a bit of error checking I
believe to be redundant. I don't intend any functional changes.
At this point it might be worth considering moving the functions into
other files, but I don't think it matters that much.
Change-Id: I05b4d86dd37a9ff1887a116183c915c225faf3a7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/297129
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Hopefully improve some of the details around parsing that have always
confused me.
- parser.ParseFile will never return an error other than
scanner.ErrorList. Encode that in ParsedGoFile.
- parser.ParseFile will never return a nil file. Eliminate the code
path that handled that.
- Explain why we might fail to find a token.File.
- Trying to fix errors appears quite expensive even if there aren't any
to fix. Don't waste the time.
Change-Id: I87e082ed52c98a438bc7fd3b29e1a486f32fb347
Reviewed-on: https://go-review.googlesource.com/c/tools/+/297069
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
When a //go:embed or //go:build (//+build) line changes, we need to
invalidate metadata. Do so. It might be preferable to only invalidate on
save, but we don't currently have an approach for doing that. So for now
we'll load on each keystroke in a magic comment.
Fixesgolang/go#38732, golang/go#44342.
Change-Id: Id05fb84f44215ea6242a7cf8b2bca4e85f74680e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/296549
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
go/packages.Error has filenames relative to the go command's working
directory. We need to interpret them as such.
This would perhaps be better done in go/packages but with no release
process in place I'm leery of making changes to it.
Updates golang/go#44342.
Change-Id: I95bcdff0368efe09ec7059394e59a39bf195310b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/295412
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
In typeCheckDiagnostics, we have logic to suppress go list and type
checking errors based on the presence of other errors. Nobody seems to
know why the logic is exactly what it is, and suppressing list errors
means that bad //go:embed patterns don't show up.
Intuitively, it makes sense to me that we don't want to type check if
listing or parsing fails: list errors mean that whole files may be
missing, and parsing errors may wipe out arbitrary chunks of files.
There's little point reporting errors from type checking that. However,
list errors and parse errors should be mostly orthogonal: go list
parses very little of the file and in practice only reports errors
parsing the package statement. So, at least for now, we report both
parse and list errors, and stop there if there are any.
Finally, move the suppression logic to the actual typeCheck function
that generates the diagnostics. typeCheckDiagnostics is the primary
consumer, but I think it's better to do the suppression at the source.
Because we are now showing list errors, and they are prone to getting
stuck due to bad overlay support, a couple of tests now require 1.16.
Change-Id: I7801e44c4d3da30bda61e0c1710a2f52de6215f5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/295414
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Due to misuse of the expandErrors function, when we encountered a
continuing error, we would group all the related errors into a single
one, then return both it and all its secondaries individually. That
makes for a strange user experience where you get one complete error and
N-1 incomplete ones.
Instead, create a copy of the complete error for each secondary error,
moving the copy to the location of the secondary error and adding a bit
to the text to (hopefully) clarify what's going on.
Change-Id: I87cf0e3b2cea98317f650d16a78476edf108a934
Reviewed-on: https://go-review.googlesource.com/c/tools/+/295413
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
### gopls
- Add `https` scheme to `CodeDescription.href`
According to the [LSP specification](https://microsoft.github.io/language-server-protocol/specification#diagnostic), `CodeDescription.href` must be [URI type](https://microsoft.github.io/language-server-protocol/specification#uri). As described in the [RFC](https://tools.ietf.org/html/rfc3986#section-3), the scheme is required for URI:
> The scheme and path components are required, though the path may be empty (no characters).
Current `gopls` does not add the scheme to `CodeDescription.href`, and this results in some LSP clients ([at least this client](https://github.com/autozimu/LanguageClient-neovim)) which are strictly validating the URI format to be failed to populate diagnostics.
Change-Id: I73f01c2e97ed1adb62fbed451a7c9b0c9794b66a
GitHub-Last-Rev: 6985bfe60e182ee788082d8fcb9515275d9612fa
GitHub-Pull-Request: golang/tools#277
Reviewed-on: https://go-review.googlesource.com/c/tools/+/294569
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Rebecca Stambler <rstambler@golang.org>
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Our handling of go command errors was cobbled together, leading to
unexpected gaps and duplication. Refactor it to be more coherent.
Our goal is to turn every go command error into a diagnostic in the
relevant location. The errors don't contain error positions, so we have
to guess where they belong using the module names mentioned in the
error. If we can't find any reference to those modules, we are forced to
add diagnostics to all go.mod files.
I may have destroyed the intent of TestMultiModule_OneBrokenModule but
I'm not sure what to do about it.
Some cleanup along the way:
- Stop parsing modfile.Parse error text: it returns structured errors
and we can just use them.
- Return CriticalErrors from awaitLoadedAllErrors, and do error
extraction lower in the stack. This prevents a ridiculous situation
where initialize formed a CriticalError, then awaitLoadedAllErrors
returned just its MainError, and then GetCriticalError parsed out
a new CriticalError from the MainError we got from a CriticalError.
- In initialize, return modDiagnostics even if load succeeds: we are
missing packages and should not silently fail, I think?
- During testing I tripped over ApplyQuickFixes' willingness to not
actually do anything, so I made that an error.
Fixesgolang/go#44132.
I may also have fixedgolang/go#44204 but I haven't checked.
Change-Id: Ibf819d0f044d4f99795978a28b18915893e50c88
Reviewed-on: https://go-review.googlesource.com/c/tools/+/291192
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
findFile has a case that returns early if the file does not exist.
Handle this error in getFile to avoid inconsistently returning errors
when getting file handles for files that don't exist.
Unskip the test, since it is no longer flaky.
Fixesgolang/go#44227
Change-Id: I07a4f860cfc9f852728c31706bd924e419bd54e2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/291391
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Iterating through the map means that the ordering changes, which may
result in us creating different workspace module files for the same
workspace. We should be careful to always create the same file.
Change-Id: I4ccd3f9ebbbe81bb062285fe9c3ad675bdf2e53a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/291493
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
We currently write directly to go.mod/sum via the go command, expecting
that editors will pick up the changes. While that's true for VS Code,
vim doesn't necessarily reload unchanged buffers. Change to send
explicit edits instead, but only if the file is open. Behavior when
using Go versions that don't support -modfile is unchanged.
Fixesgolang/go#44035.
Change-Id: Ie4e5cf60673b860f5dfcbdb726bee0efe6aae405
Reviewed-on: https://go-review.googlesource.com/c/tools/+/290189
Trust: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
When go.sum updates are needed in experimental workspace module mode, we
don't necessarily know which module needs the correction. As a fix,
apply all of these fixes to each module in the multi-module workspace.
The "add dependency" quick fix also seems to be broken, but I'll fix
that in a separate CL.
Fixesgolang/go#44097
Change-Id: Id4a6013f2aceb6b902dff3118b027f6cb99eb3c1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/289772
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Fully switch to the new generated command API, and remove the old
dynamic command configuration.
This involved several steps:
+ Switch the command dispatch in internal/lsp/command.go to go through
the command package. This means that all commands must now use the new
signature.
+ Update commandHandler to use the new command signatures.
+ Fix some errors discovered in the command interface now that we're
actually using it.
+ Regenerate bindings.
+ Update all code lens and suggested fixes to new the new command
constructors.
+ Generate values in the command package to hold command names and the
full set of commands, so that they may be referenced by name.
+ Update any references to command names to use the command package.
+ Delete command metadata from the source package. Rename command.go to
fix.go.
+ Update lsp tests to execute commands directly rather than use an
internal API. This involved a bit of hackery to collect the edits.
+ Update document generation to use command metadata. Documenting the
arguments is left to a later CL.
+ Various small fixes related to the above.
This change is intended to be invisible to users. We have changed the
command signatures, but have not (previously) committed to backwards
compatibility for commands. Notably, the gopls.test and gopls.gc_details
signatures are preserved, as these are the two cases where we are aware
of LSP clients calling them directly, not from a code lens or
diagnostic.
For golang/go#40438
Change-Id: Ie1b92c95d6ce7e2fc25fc029d1f85b942f40e851
Reviewed-on: https://go-review.googlesource.com/c/tools/+/290111
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Allow fixSrc to run multiple times on a file. For example:
func main() {
var s S
if s.<>
}
type S struct {
i int
}
To properly complete at <>, we need to perform two fixes. We must
insert a phantom underscore after "s." to deal with the dangling
selector, and then we must insert phantom "{}" for the "if" statement
to deal with the incomplete "if" block. Previously we stopped at one
fix, but now we allow for up to 10. I added a limit because I am
afraid there are cases where we could get stuck trying to apply the
same fix again and again.
Also, drive-by set isIncomplete=true when we return early in
lsp/completion.go. I have found my editor incorrectly caches this zero
result in certain cases because it thinks results are "complete".
Fixesgolang/go#43471.
Change-Id: Idd34cc164d579fa12a27920dc3afb372799abf26
Reviewed-on: https://go-review.googlesource.com/c/tools/+/289271
Run-TryBot: Muir Manders <muir@mnd.rs>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Trust: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The only thing that the mod tidy diagnostics use the network for is
adding dependencies, and we already have quick fixes for those. The one
exception is the case covered by TestBadlyVersionedModule, a dependency
that fails to declare one of its own dependencies and therefore requires
an indirect dependency in the workspace module. That only triggers an
error on the dependency's import statement, which the user will never
see.
Fortunately, the go command does expose these problems in the DepsErrors
field of the list response. Add an internal API to access that, and turn
it into diagnostics on both the file and the controlling go.mod.
Refactor the go get diagnostic generation so that it applies to both
modules and packages.
Fixesgolang/go#38462.
Change-Id: Ie2af940087654682a40de9ecfccd46f404a88b60
Reviewed-on: https://go-review.googlesource.com/c/tools/+/289309
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
We still need to pass -mod=mod when AllowModfileModifications is enabled
on 1.16.
Change-Id: I9cecd30914eb52c9faa877cece25d5722b36df79
Reviewed-on: https://go-review.googlesource.com/c/tools/+/289695
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Collapse Diagnostic.Kind, Source, and Category into just Source. Remove
code that converted from Diagnostic to Diagnostic. Notes on the changes
I had to make along the way:
- We used to use Kind to determine Severity. Set Severity when the
Diagnostic is created instead.
- Use constants for Source as much as possible -- we still need to use
Analyzer.Name for analysis diagnostics. It would be nice to break that
dependency so that Source was totally opaque, but that's a separate
issue.
- Introduce a new Source for gc_details, "optimizer details". It was "go
compiler" previously.
- Some of the assignments are a little arbitrary. Is inconsistent
vendoring really a "go list" error?
- GetTypeCheckDiagnostics had code to cope with diagnostics that had no
URI associated with them. We now spread such diagnostics to all files
when they are generated.
- Analyze modifies Diagnostics by adding a Tag to them. That means it
has to own them, so I had it clone them. I would like to push that logic
down to the diagnostics, per the TODO, but that's another CL.
And some observations:
- It's obviously tempting to combine DiagnosticSource and
diagnosticSource, but they mean very different things. I'm open to a
better name for one or the other.
Change-Id: If2e861d6fe16bfd2e5ba216cf7e29cf338d0fd25
Reviewed-on: https://go-review.googlesource.com/c/tools/+/288215
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
source.Error and source.Diagnostic are almost identical types, used
arbitrarily in different parts of the code. This CL is the first step in
cleaning up that redundancy: it deletes the source.Error type.
To do that, I added the fields from source.Error to source.Diagnostic,
and made absolutely no other semantic code changes -- I just renamed
things that were named Error to Diagnostic. With only aesthetic concerns
in play, I hope this CL will be easy to review. The next CL will clean
up all the stupid-looking code that converts a Diagnostic to a
Diagnostic, etc.
Change-Id: I1298cc8144c686b8a37fc2cc106930105e511353
Reviewed-on: https://go-review.googlesource.com/c/tools/+/288214
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The Typescript source is still at version 3.16, but there are new
requests, more detailed client capabilities, and an attempt to be
more specific about ranges of number in the Typescript code.
Vscode defines types integer and uinteger (32-bit signed and unsigned),
so the Go code now uses int32 and uint32.
They've changed the use of TextDocument, so version information is sometimes
missing. cache/session.go:625 was changed correspondingly.
This CL also make CodeAction.Disabled into a pointer.
New requests or notifications:
DidCreateFiles, DidRenameFiles, DidDeleteFiles (notifications)
ShowDocument, WillCreateFiles,WillRenameFiles, WillDeleteFiles (request)
It's a lot of code; I've probably missed something.
Change-Id: I8449ad8473ac00947d0344c5f6133f9bd73b9e10
Reviewed-on: https://go-review.googlesource.com/c/tools/+/286192
Run-TryBot: Peter Weinberger <pjw@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Peter Weinberger <pjw@google.com>
Heschi Kreinick
Rebecca Stambler
Peter Weinbergr
Rob Findley
Anton Kuklin
William Langford
Steeve Morin
Jay Conrod
Yuki Ito
Muir Manders