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>
Using structured errors in gopls has proven to be difficult to manage:
it's hard to know whether a given error return is expected to be
structured without any type information. We have mostly eliminated
them; finish the job.
I don't intend any semantic changes here.
I considered eliminating CriticalError altogether, but it does seem
useful to have a convenient bundle for return values. So I left it
alone for now.
Change-Id: I4b5f85a8de9712babffbc1383088151b596bd815
Reviewed-on: https://go-review.googlesource.com/c/tools/+/287792
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>
Now that workspace module mode generates a combined go.sum there are
relatively few blockers to enabling -mod=readonly. Fix them and do it.
This CL is a bit of a grab bag, but the fixes are relatively separate. I
can split it into multiple CLs if desired.
- If module A depends on module B at v1.0.0, the go command will want to
upgrade the workspace module from v0.0.0-goplsworkspace to v1.0.0. To
prevent that, use vN.999999.0 as the base pseudoversion, adjusting v0 to
v1 where appropriate. A few test cases needed updating as a result.
- For old Go versions, sort the generated workspace module and
synthesize a go statement from the maximum go version declared in the
workspace.
- Some regtests need go.sum files created.
- matchErrorToModule created incorrect quick fixes: it would try to
download the top-level module mentioned in the error message, not the
one that actually caused the problem. Now it issues quick fixes for the
lowest-level module.
- TestMultiModuleModDiagnostics accidentally included the same module
in the workspace twice. Fix it, and make that an error.
Fixesgolang/go#43346.
Change-Id: I605f762a4d23bedd914241525e64c1b3ecc42150
Reviewed-on: https://go-review.googlesource.com/c/tools/+/287032
Trust: Heschi Kreinick <heschi@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
In CL 271297, I disabled the constantly-running upgrade check, which
removed the upgrade commands for individual dependencies. This seems to
have been a relatively popular feature. Re-introduce it, but requiring
explicit user interaction.
We now run an upgrade check when the user clicks "Check for upgrades".
Those results are stored on the View and used to show diagnostics on
any requires they apply to. Right now we only check the go.mod the user
has open; in multi-module workspaces it might make sense to check all of
them, but I'm not sure.
Fixesgolang/go#42969.
Change-Id: I65205dc99a4fa9daafdb83145b0294b6f3be5336
Reviewed-on: https://go-review.googlesource.com/c/tools/+/286474
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 were not locking while iterating s.files in snapshot.knownFilesInDir.
All other accesses of s.files appear to be locked, so this should fix
golang/go#43972.
Fixesgolang/go#43972
Change-Id: I01184c3992c91f8beb4a3239f70cc4487a528ec0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/287573
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
As part of investigating golang/go#43023, I noticed that it's possible
for us to show context cancellation as a critical error--we should not
do this.
Change-Id: I1f7cc13a151e0a161d5a4ea4d5b55fba15270b32
Reviewed-on: https://go-review.googlesource.com/c/tools/+/284937
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>
This change ensures that packages using exec.LookPath or
exec.Command to find or run binaries do not accidentally run
programs from the current directory when they mean to run programs
from the system PATH instead.
Change-Id: I5907aa630ff64012395a7eb472967a477d90f12e
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/949438
Reviewed-by: Katie Hockman <katiehockman@google.com>
Reviewed-by: Russ Cox <rsc@google.com>
Reviewed-on: https://go-review.googlesource.com/c/tools/+/284773
Run-TryBot: Roland Shoemaker <roland@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Roland Shoemaker <roland@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Our filesystem caching layer uses file modification time to invalidate
file contents. This is an imperfect heuristic, and on certain operating
systems with low resolution filesystem clocks (such as WSL), this can be
broken in practice.
A proper fix would be to just read the file contents directly and rely
on the snapshot to optimize file access, but we don't know that this is
a safe change. Instead, try to reduce the likelihood of false cache hits
by also checking the file size reported by Stat.
For golang/go#43554
Change-Id: I1af384db532725e84fa6f3a2e5469d10b43fee92
Reviewed-on: https://go-review.googlesource.com/c/tools/+/283053
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>
Trust: Robert Findley <rfindley@google.com>
We can't assume that we're in module mode when we rebuild the imports
cache. (This is a long standing bug, but I suspect it was made more
common by my recent CL that refactored this code.)
Fixesgolang/go#43544.
Change-Id: Ie35c43e6b6e496bd2fbf49cf9bf06c28cf1dab80
Reviewed-on: https://go-review.googlesource.com/c/tools/+/282113
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>
The existing test didn't catch it because it doesn't look for specific
warning messages.
Change-Id: I1ec7f7a75c1055c960cdd7545331c2fd655e3aa8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/281860
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Trust: Rebecca Stambler <rstambler@golang.org>
Check that some invariants are met when cloning the snapshot, in an
attempt to catch golang/go#43347
For golang/go#43347
Change-Id: I7404509027a1b0b0085133cba4d21d1006a52a57
Reviewed-on: https://go-review.googlesource.com/c/tools/+/280698
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The fragmentation of the critical error reporting is getting in the way
of small fixes. This change adds a GetCriticalError function that reports
critical errors, while ModTidy and WorkspacePackages no longer report
critical errors. Any function that wants to process critical errors
should call AwaitLoaded.
Some other smaller changes are made to account for these changes. One
change is that we may report multiple *source.Errors, with the
assumption that duplicate diagnostics will be caught by the diagnostic
caching. Also, any `go list` error message that ends with "to add it" is
now considered an error that gets the "add dependency" suggested fix.
Fixesgolang/go#43338Fixesgolang/go#43307
Change-Id: I056b32e0a0495d9a1dcc64f9f5103649a6102645
Reviewed-on: https://go-review.googlesource.com/c/tools/+/280093
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
golang/go#43335 explains the issues with using `go get module@none`,
which will only be resolved in Go 1.17. In the meantime, we use the
go command whenever possible, but if the module is not tidied, we
have to use textual edits instead. This means the go.sum file will not
be accurately updated to remove the dependency, but unfortunately, I
don't believe there is anything that we can do in that case.
Fixesgolang/go#43335
Change-Id: I771f68f34a6136e73e9dd82b692ed4c235c3b293
Reviewed-on: https://go-review.googlesource.com/c/tools/+/279716
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Invalidating the workspace on any change to a go.mod can render a
workspace unusable. Only invalidate when changes are saved.
Additionally, invalidate the workspace if there is a saved change to
tracked go.sum changes.
Fixesgolang/go#42529Fixesgolang/go#42815
Change-Id: I5d903013b33b932eca4998513e3d0a534b2e5a61
Reviewed-on: https://go-review.googlesource.com/c/tools/+/279720
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Rebecca Stambler
Heschi Kreinick
Rob Findley
Jay Conrod
Yuki Ito
Muir Manders
pjw
Roland Shoemaker