This is one of our most frequent and oldest open crash bugs.
Updates golang/go#60890
Change-Id: I97bdf339ec355aaf23fb81ee8fed11b142d28409
Reviewed-on: https://go-review.googlesource.com/c/tools/+/581175
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The predicate that determines whether the type checker
creates types.Alias nodes is complex: it depends on an
environment variable (GODEBUG) that is somewhat tricky
to parse correctly, since it is a rightmost-wins list.
Critically, however, its default value is a function of
the go directive in the application's go.mod file, which
is inaccessible to logic in x/tools (per-file build tags
can detect the toolchain version, but not the go.mod version).
Equally critically, the current effective value of the
gotypesalias variable changes when os.Setenv(GODEBUG) is
called, which happens in tests, and there is no way to
detect those events; therefore any attempt to cache the
value is wrong.
This change exposes a simplified version of the Enabled
function from aliases, which always computes the ground
truth by invoking the type checker, regardless of cost.
It also adds an 'enabled' parameter to NewAlias so that
callers can amortize this cost.
Also, various minor cleanups related to aliases.
Updates golang/go#64581
Change-Id: If926edefb8e1c1f63c17e4fad0a808e27bac6d5b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/580455
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Use ReportRangef to replace the original Reportf
to try to implement two TODOs.
Change-Id: I9dfc0f47881d7638e460164a9dc5573394b92eee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/579615
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Change-Id: I5d5f6cc4a6984b8577e8178082bc60dfe08107b6
GitHub-Last-Rev: 16ede6bf75
GitHub-Pull-Request: golang/tools#490
Reviewed-on: https://go-review.googlesource.com/c/tools/+/580156
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Change-Id: If300720c92bfb1ac63b395efc2419f1f1c8bed8f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/580595
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This reverts commit 618670db50.
Reason for revert: The test does not in fact pass, but this was obscured by a bug (?) in the godebug machinery, which uses a default value poked into the executable at link-time of "...gotypesalias=0...",
which means the default value is not "on", as we thought when we changed the interpretation of unset to mean "on".
Change-Id: Ib45b230a80d8b52a9d69db56025dae34add29fb8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/579757
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This test reenables the staticcheck test, which now passes
with gotypesalias={,0,1}. (Staticcheck may yet need work, but
that isn't evident to our tests.)
Updates golang/go#64581
Change-Id: I1c8acca2c6b7d16e551a5c8a68be564254713d0d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/579756
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Fix two places where gopls fails to clean up temporary files:
- In telemetry tests, the deferred cleanup was not run after os.Exit.
- In the GC details codelens, a persistent GC details directory is
assumed; just use a temp directory instead.
Change-Id: Icef5a4b612ac1727fee7d2c65e99a90f73123081
Reviewed-on: https://go-review.googlesource.com/c/tools/+/579755
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This setting changes the behavior of gopls in fundamental low-level
ways, and therefore is severely under-tested and now worth maintaining.
Gopls should make accurate decisions about whether to allow network
access for commands.
Fixesgolang/go#66860
Change-Id: Ia2f58bf146fc6bd37d769b65fc227cb2d0027032
Reviewed-on: https://go-review.googlesource.com/c/tools/+/579437
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
That no tests fail when this setting is removed is evidence enough that
it is unsupported. Additionally, this was one of the final pins holding
together some of the unnecessary complexity of the gocommand package,
which subsequent CLs will untangle.
Fixesgolang/go#65546
Change-Id: I8efececfd743d898c5f72c6a2d2a416e023ee3bc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/579435
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Whether or not semantic tokens are enabled should be a client-side
setting. Clients that don't want semantic tokens should not ask for
them, and if clients send textDocument/semanticTokens requests, gopls
should handle them by default. Unfortunately, as described in
golang/vscode-go#3338, this is not how semantic tokens were configured
in the past.
This CL changes the default value of the "semanticTokens" setting to
true. We should not release a gopls version with this change until
addressing golang/vscode-go#3338, but by making this change in master
now it is easier to test the fix in vscode-go.
For golang/vscode-go#3338
Change-Id: I05d7084436cd4dfe312460cfe08e3b1777f190ed
Reviewed-on: https://go-review.googlesource.com/c/tools/+/579337
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Remove the concept of "experiments" from gopls settings and testing. In
the past, this was a way for us to selectively enable features in the VS
Code nightly build, but we have since switched to a model where we cut
earlier prereleases and endeavor to have more prerelease testing.
Fixesgolang/go#65548
Change-Id: I64d739dd4b4899c19ce5cd0c3da45e498e9ae417
Reviewed-on: https://go-review.googlesource.com/c/tools/+/579395
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Previously, all CodeAction were assumed to be fixes or refactorings,
but GoTest and GoDoc are pure queries, and should be permitted even
in generated files. Unfortunately there's no systematic way to
distinguish, so for now we use an ad-hoc allow-list.
+ Test
Also:
- unexport golang.TestsAndBenchmarks
- clarify "want kind" logic in server.CodeAction
Change-Id: I78b991c68252505c962d22275a9e47bb1433ee40
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577257
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This CL causes the fourth and subsequent parameters of functions
and methods in the index section to be elided and replaced by
"...", following pkg.go.dev.
Also, unnamed parameters in the navigation selector are treated
as blanks (_).
Added a test of both features.
Change-Id: I542eac61a5ab8b93e5ac02b80d3562a111a0514b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578675
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The docstring for the "symbolScope" setting is a relic from when we
attempted to make "workspace" the default value, but the default was
subsequently reverted to "all". Update the docstring to not assume a
default.
Change-Id: Ib97b9e7f40227067dedb87700ea7c19bb2758660
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578935
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Later, we can switch to using aliases, which produces
nicer output.
Updates golang/go#65294
Change-Id: I1d807651cccf52961779cf66e057f859f2bb3a05
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578324
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
This is required temporarily as we flip the default.
Updates golang/go#65294
Change-Id: I552e40475cc48b949e2307af347ca98a428c55ea
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578041
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Simplify some of the legacy indirection that used to be required when
the bulk of gopls' implementation lived in the x/tools module. We no
longer need to set hooks on the settings.Options; hooked-in
implementations can simply be statically linked.
Remove the settings.Hooks type, and pull the thread as far as it will
go, cleaning up a bunch of unnecessary indirection. As a result, we no
longer need the hooks package.
Change-Id: Ifd71da13174af4b7bc733f97774830ec1d98a2bc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578039
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
In CL 538796, options were made immutable on the View, meaning any
change to options required a new View. As a result, the
minorOptionsChange heuristic, which avoided recreating views on certain
options changes, was removed.
Unfortunately, in golang/go#66647, it appears that certain clients may
send frequent didChangeConfiguration notifications. Presumably the
configuration is unchanged, and yet gopls still reinitializes the view.
This may be a cause of significant performance regression for these
clients.
Fix this by making didChangeConfiguration a no op if nothing changed,
using reflect.DeepEqual to compare Options. Since Hooks are not
comparable due to the GofumptFormat func value, they are excluded from
comparison. A subsequent CL will remove hooks altogether.
For golang/go#66647
Change-Id: I280059953d6b128461bef1001da3034f89ba3226
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578037
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
The check command previously ignored Diagnostic.RelatedInformation,
which confused me as I was trying to run an experiment related
to golang/go#64547. This change causes it print the related errors.
+ test
(Aside: there's a lot of weirdness in the way that
check.typeErrorsToDiagnostics constructs the response,
as you will see if you run the test on c.go not c2.go,
but that's not the test's concern.)
Change-Id: I14efbf8f2e47ac00c2e7d6eceeacbaaecab88213
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576656
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
It is currently failing on CI on pre-go1.20 toolchains
due to the too-new go.mod file.
Change-Id: I4cabd40bd50c8bdf084dac113062c04b34d53946
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578038
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
We could do more, but this is a minimal change addressing
golang/go#61543.
Fixesgolang/go#61543
Change-Id: I1535ffcf9246d2d8abad384cab4a29c24696a220
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578455
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Now gopls has already support highlight doc links, and it would be
useful to support hover and defition over doc links.
Fixesgolang/go#64648
Change-Id: I53d4e41ec7328fca6cf4988b576b2893d6a02434
Reviewed-on: https://go-review.googlesource.com/c/tools/+/554915
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Fixesgolang/go#66733
Change-Id: Idbd92f92bad0fcb9286fcb68c6109b42a9d84045
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578035
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
When a program refers to 'rand.Float64()' without an import statement,
import math/rand/v2 is the better answer.
Fixes: golang/go#66407
Change-Id: I92709c335b61aa10dd49ce2e5336ea233d8f9af8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/573075
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
With gopls@v0.15.0, zero config gopls made it much more likely that
sessions would have multiple Views. Additionally, with improved build
tag support, it made it more likely that these Views would share files.
As a result, we encountered (and fixed) this latent bug:
1. User changes file x.go, invalidating view A and B. A and B are
scheduled for diagnosis.
2. User changes file y.go, invalidating only view B. Step (1) is
cancelled and view B is scheduled for diagnosis.
3. View A never gets rediagnosed.
The fix was naive: just mark view A and B as dirty, and schedule a
goroutine to diagnose all dirty views after each step. As before, step
(2) would cancel the context from step (1).
But there's a problem: diagnoses were happening on the *Snapshot*
context, not the operation context. Therefore, if the goroutines of step
(1) and (2) both find the same snapshots, the diagnostics of step (1)
would *not* be cancelled, and would be performed in addition to the
diagnostics of (2). In other words, following a sequence of
invalidations, we could theoretically be collecting diagnostics N times
rather than 1 time.
In practice, this is not so much of a problem for smaller repositories.
Most of the time, changes are arriving at the speed of keystrokes, and
diagnostics are being scheduled faster than we can type. However, on
slower machines, or when there is more overall work being scheduled, or
when changes arrive simultaneously (such as with a 'save all' command or
branch switch), it is quite possible in practice to cause gopls to do
more work than necessary, including redundant loads. I'm not sure if
this is what conspires to cause the regressions described in
golang/go#66647, but it certainly is a real regression.
Fix this by threading the correct context into diagnoseSnapshot.
Additionally, add earlier context cancellation in a few cases where
redundant work was being performed despite a context cancellation.
For golang/go#66647
Change-Id: I67da1c186848286ca7b6221330a655d23820fd5d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577695
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Previously, gopls would log package contents whenever reloading (but not
reinitializing) a workspace--essentially whenever the query did not
contain a '...'. But following a big invalidation, gopls may reload
hundreds or even thousands of packages. Logging them is tremendously
verbose.
Change the behavior to simply log packages if and only if verboseOutput
is set. This is easier to understand and greatly reduces the noisiness
of loading.
Also annotate the one test that broke when I experimented with this
behavior. We should really have a more robust framework for asserting on
loads, but that is a project for another CL.
For golang/go#66746
Change-Id: I0eff7fad1b2deb2f170a1c336abf2c2a9e4f56ba
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577875
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
- Fix a bug in the logic to suppress a context cancellation error
computing the shared import graph.
- Remove ErrIsDefinition. It was introduced for testing in CL 196560,
and is no longer needed.
- Update golang.SignatureHelp to return (nil, nil) in the case where
SignatureHelp is simply not available.
- Suppress the mod tidy diagnostic error when GOPROXY=off. It is too
noisy, and golang/go#56395 tracks the fix for the root cause.
- Suppress noisy errors about semantic tokens. Researching the history
behind this error, it seems it was only added to force the
invalidation of semantic tokens client-side. The same can be achieved
by returning a non-nil, empty result.
- Enforce that marker tests encounter no error logs, with an -errors_ok
flag to allow them in select cases.
- Update the -min_go version of some marker tests that use a go.mod file
with a too-new go version. This was an error log.
For golang/go#66746
Change-Id: I21fe274e320cf5fa7d6b85d7402330fb647dbd29
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577655
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Tests are regularly timing out on this builder. Since it is not a first
class port, it's OK to skip these tests with -short.
Fixesgolang/go#66748
Change-Id: I7fdf50db47b2e79fb87813ac98dea1d25e0d4453
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577795
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Also, use go1.19 generic helpers for atomics.
Updates golang/go#66732
Change-Id: I7aa447144353ff2ac5906ca746e2f98b115aa732
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577435
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
If the primary diagnostic has invalid position information (due to a bug
in the parser or AST fix logic), gopls may panics when linking primary
and related information. Avoid this panic.
Fixesgolang/go#66731
Change-Id: Ie2f95d158a1c93d00603a7ce4d38d8097bd8cb08
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577259
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This CL causes the summary displayed by stacks to:
- display each unique stack ID for new issues;
- display a count for each issue (if known) or stack ID (if not);
- sort by descending order of those counts;
- separate the existing and new issues; and
- ignore "devel" binaries in the total count figure.
The "new issue" form logic is extracted to a function.
Change-Id: I52249915f63865fba4e9fa836b47de1f290f16a9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576683
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
The type checker produces an error if the Go version is too new. When
compiled with Go 1.21, this error is silently dropped on the floor and
the type checked package is empty, due to golang/go##66525.
Guard against this very problematic failure mode by filtering out Go
versions that are too new. We should also produce a diagnostic, but that
is more complicated and covered by golang/go#61673.
Also: fix a bug where sandbox cleanup would fail due to being run with a
non-local toolchain.
Fixesgolang/go#66677
Change-Id: Ia66f17c195382c9c55cf0ef883e898553ce950e3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576678
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The anchor of a promoted method should be based on the documented
type, not the embedded type.
Change-Id: I547ec00bffce223b045b1f1bb7b101769451b844
Reviewed-on: https://go-review.googlesource.com/c/tools/+/577296
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Poor parser error recovery may cause Node.End to be zero, or
a small positive displacement from zero due to recursive Node.End
computation (#66683).
This change further refines the bug.Reports for such problems,
and additionally repairs the values heuristically to avoid
downstream bug.Reports after toGobDiagnostics (#64547).
Updates golang/go#66683
Updates golang/go#64547
Change-Id: I7c795622ec6b63574978d2953c82036fcc4425af
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576655
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
In some code paths, it was possible to call testing.Short() before flags
had been parsed. Fix this by calling
flag.Parse() earlier.
See https://build.golang.org/log/7f2caf626b9d159437f3d406a1e992ec9ada2c90
for an example failure.
Change-Id: If12e3257503cc5491cd35f379a1f9ff776868e93
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576918
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL adds a <select> element with an index of all
sections in the page, following the design of pkg.go.dev.
Change-Id: I41b6719e85d0ace7edb864c5b20ae1f5b8e39d1b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576596
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This change causes Hover to reveal the percentage of a struct
type's size that is wasted due to suboptimal field ordering,
if >=20%.
+ test, release note
Fixesgolang/go#66582
Change-Id: I618f68d8a277eb21c27a320c7a62cca09d8eef0a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/575375
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
The random base64 strings in gopls URLs caused occasional
spurious matches for the substring t1. This change uses
a non-base64 letter to eliminate this source of
nondeterminism.
Change-Id: I1db1dca8d93cc299b13b591567fa72b2227ac436
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576140
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
It changes (improves) the behavior of the test; we will reenable
it once the default has changed.
Updates golang/go#65294
Change-Id: I716da405a9f0c03c303c4c0be8b738dd7c5ebdcd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576137
Reviewed-by: Tim King <taking@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Fix the same crash as golang/go#66195, this time in Analyze: don't set
invalid Go versions on the types.Config.
The largest part of this change was writing a realistic test, since the
lack of a test for golang/go#66195 caused us to miss this additional
location. It was possible to create a test that worked, by using a flag
to select a go1.21 binary location.
For this test, it was required to move a couple additional integration
test preconditions into integration.Main (otherwise, the test would be
skipped due to the modified environment).
Fixesgolang/go#66636
Change-Id: I24385474d4a6ebf6b7e9ae8f20948564bad3f55e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/576135
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Filter diagnostics only to the "best" view for a file. This reduces the
likelihood that we show spurious import diagnostics due to module graph
pruning, as reported by golang/go#66425.
Absent a reproducer this is hard to test, yet the change makes intuitive
sense (arguably): it is confusing if diagnostics are inconsistent with
other operations like jump-to-definition that find the "best" view.
Fixesgolang/go#66425
Change-Id: Iadb1a01518a30cc3dad2d412b1ded612ab35d6cc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/574718
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Working with go.sum files in integration tests was cumbersome, involving
temporarily modifying the test to call env.DumpGoSum(...), then copying
its output into the static go.sum file.
For some tests, we may actually want to check in a fixed go.sum file,
but the majority of the tests just want to avoid go.sum errors. To
support this use case, add a new RunOption that writes the go.sum file
before starting the test. Use this option in a test to illustrate its
usage.
Also, update DumpGoSum to use the faster "./..." pattern to create the
go.sum output. "..." matches all modules, but we only care about the
main module.
Change-Id: I32cca005e10411033422cf8fee64cbd56e83f64c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/575700
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Previously, a variety of links generated by the go/doc/comment
package were not valid. This CL configures the parser and printer
hooks to query the cache.Package representation, and now
generates valid crosslinks for all references.
Tested interactively on a wide variety of cross links.
Change-Id: Iaf1fffe52ea96e3be5df78227cf8395006caa59a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/575377
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
staticcheck will need work to accommodate gotypesalias=1.
In the meantime we disable this test in that mode; we plan
to reenable it before the next gopls release.
Updates golang/go#65294
Change-Id: I857edb758f83f180a4d92b7100e99c3401b1d957
Reviewed-on: https://go-review.googlesource.com/c/tools/+/575698
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL causes the package rendering to emit an <a id=...> anchor
for each name defined by a var or const declaration.
Also, choose the initial fragment id (e.g. "#Buffer.Len") based
on the selection.
+ Test that anchors are emitted.
Change-Id: Idb4838a6d2741a26cd9dbb5ad31a76d6f811ff26
Reviewed-on: https://go-review.googlesource.com/c/tools/+/575697
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
When "View package documentation" is invoked in a _test.go file,
this change ensures that the documentation for the package under
test is displayed, instead of a 404 Not Found.
Change-Id: I9c9f2205d6ec72cbe5acea1480acdbf2bd3b8b53
Reviewed-on: https://go-review.googlesource.com/c/tools/+/575695
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This CL causes the "View package doc" pages to display a
banner when the gopls server dies, at which point the page
is no longer operative. (A new server will have a different
port.)
Change-Id: Icde10a7b4c25a28177d043c573dfdab4ce7c7964
Reviewed-on: https://go-review.googlesource.com/c/tools/+/575536
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This analyzer shows a very high rate of true positives,
indicating that code is (at best) unnecessarily complex
and confusing but, often, not working as intended.
+ release note
Fixesgolang/go#44461
Change-Id: I1c86e7305a8d512d7e4d1bbec3b2b44ce3a9c9ab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/575061
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This change causes Hover to report size information for
types and struct fields when hovering over
the declaring identifier, plus offset information for
struct fields.
Some tests needed tweaks to make them CPU-independent,
or to disable them on 32-bit machines.
Also, add the first release notes using the new
git+gerrit workflow (and backfill a missing item for
"View package documentation").
Change-Id: Ibe8ac5937912c3802f3ad79e3d9f92ba24eb51de
Reviewed-on: https://go-review.googlesource.com/c/tools/+/573076
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
The associated bug was a crash in package doc rendering due to
invalid ASTs resulting from mutation caused by doc.New, even
in PreserveAST mode. (This bug has been latent since 2018.)
The mutation occurs when filtering out unexported symbols.
This change is a workaround: we enable AllDecls mode to
suppress filtering, and perform the filtering ourselves.
Also, add a regression test for the panic, and check
the new filtering behavior. This required some test
refactoring.
Fixesgolang/go#66449
Updates golang/go#66453
Change-Id: I434c97e08930728d4894b90c1c7e010e7a922209
Reviewed-on: https://go-review.googlesource.com/c/tools/+/573575
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Fixes Google Issue 331478795
Change-Id: I0258bdd6a50000effc66c4e1fe03f99e2720c0cc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/575056
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Adds a RunOption NoLogsOnFailure() to suppress the printing of
the voluminous LSP logs when there is a test failure. While debugging
tests these logs can be a noisy distraction.
Change-Id: I41379fdcef8ba00f8b8a49ec89d8795274797888
Reviewed-on: https://go-review.googlesource.com/c/tools/+/574035
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This CL does for the gopls module what CL 565035 did
for the x/tools module.
In particular, we don't yet address desired behavior
changes for materialized aliases, nor generic aliases.
Change-Id: Iace78c1f8466afe8a6a24e3da3d6f431bf5ebc3e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/565756
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Tim King <taking@google.com>
This change defines Deref(T), which returns the type of
the variable pointed to by T if its core type is a pointer,
or T otherwise, and removes all the various other flavors
of 'deref' helper that exist across the repo.
Also fix and test a generics bug in fillstruct.
Updates golang/go#65294
Change-Id: I14f6f35b58eefbad316391745745d662b061c013
Reviewed-on: https://go-review.googlesource.com/c/tools/+/565456
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Change-Id: Id4d3bcdcb663dccaf149ccadae1d0513c868a049
GitHub-Last-Rev: 4534649b01
GitHub-Pull-Request: golang/tools#486
Reviewed-on: https://go-review.googlesource.com/c/tools/+/573015
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Tim King <taking@google.com>
x/vuln/cmd/govulncheck -json output format has changed to include
module level vulnerabilities. We knew this change would eventually
come and had handling in gopls/internal/mod/diagnostics.go
'foundVuln'. But we didn't update TestRunVulncheckWarning &
TestRunVulncheckInfo to get prepared. This is a test-only bug.
Fixesgolang/go#65942
Change-Id: Ib7545279d07caf708c4f5392b51df273256432e6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/567155
Reviewed-by: Suzy Mueller <suzmue@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Rearrange the counters so that there is one set describing the
set of suggested completions (empty, short, long) and one set
describing what the user does (used, not used, can't tell).
A set of suggested completions is long if there are more than ten
of them, which is about how many clients show to users.
If a client sends back changes to file, gopls can decide if a
completion was used or not, but it does not try to tell if the client
sends back a new whole file.
These sets of counters correspond to two partitions.
Change-Id: I861a5743b16e488074fd2e58cb63c6a8aea52735
Reviewed-on: https://go-review.googlesource.com/c/tools/+/572315
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
slices has been supported in go1.21 and go1.22
Fixesgolang/go#66222
Change-Id: I7b319e649632a713200dcaabd1e68d1f01e1b2f2
GitHub-Last-Rev: fc1d59d86a
GitHub-Pull-Request: golang/tools#484
Reviewed-on: https://go-review.googlesource.com/c/tools/+/572116
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Update the canRemoveParameter logic to reflect that we don't try to
remove parameters when there are parse or type errors.
Change-Id: Ia8239a383b56763d100c69c9a3633c87025b81cf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/572297
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Make a minimal change to the removeparam algorithm to support removing
unused parameters of methods.
This doesn't address the problem of expanding the set of signatures or
calls based on interface satisfaction constraints: that is really a
separate concern, which also affects method renaming (golang/go#58461).
Were we to have a more general solution to that problem, it would be
relatively straightforward to also expand the change signature
algorithm.
For golang/go#38028
Change-Id: I296cb1f828f07d841c83b1fd33593ccd2fee3539
Reviewed-on: https://go-review.googlesource.com/c/tools/+/572296
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Unbreak the x/tools build dashboard due to a semantic conflict between
CL 572475 and CL 571215.
Change-Id: I8ce1308ee99e75aa7cf8f7bd1d8b1dedcb845f4d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/572515
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL moves the stdversion analyzer out of gopls,
into the exported analyzer tree, whence it will be
vendored into cmd/vet in a follow-up.
Updates golang/go#46136
Change-Id: I039ef78ecdcfd6bc64d5b089017a9b8635cf9aa5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/572035
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL adds a "View package documentation" code action to any
selection in a Go source file. Its command has two effects:
1) to start a web server associated with the current LSP
server, with coupled lifetimes, and register a handler
to serve the package documentation; and
2) to direct the client to open a browser page
showing the package documentation for the selection.
The server is a minimal imitation of pkg.go.dev,
working off the cache.Package representation. This means
we can display doc markup even for unsaved editor buffers,
and show type-derived information such as method sets
of each type.
Clicking through to the source--which on pkg.go.dev goes
to cs.opensource.google--causes the client editor to
navigate directly to the source file, using the magic
of showDocument requests.
The web server is secure in that all its endpoint URLs contain
an unguessable secret, so a local process belonging to
a different user can't access source code by port scanning.
The CL includes a test that
(a) the webserver content reflects edits (even unsaved)
in the source buffers, and
(b) that "clicking" through the source link causes
the server to navigate the client editor to the
correct source location.
A couple of tests that asserted "no code actions" needed
to be tweaked, as now there is always at least one
code action in any Go source file.
Change-Id: I2fe1f97e4e2b0b15cff6a4feb66501fce349b97d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/571215
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
The runtime.GOROOT function is about to be marked as deprecated, which
will generate a diagnostic that isn't expected in format.txt test case.
Pick another hopefully somewhat future-proof identifier, since dealing
with this doesn't seem to be in scope of this test case.
For golang/go#51473.
Change-Id: I54d7ed0b43860bcd41938328211797a0cdd60e36
Reviewed-on: https://go-review.googlesource.com/c/tools/+/564275
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Rename methods on cache.Package to eliminate unnecessary "Get" prefixes.
Also rename pkg.go to package.go.
Change-Id: I42f2fa7a18a5973a53af93c63b48c7db19cba3b2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/572475
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Starting soon, go/types will quote some user defined names in error
messages as `a'. Update tests to be tolerant of the new syntax.
Notably, logic in the unusedvariable analyzer which extracts the
variable name had to be updated.
Change-Id: I091ab2af9b5ed82aa7eacad8f4a9405f34fcded7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/571517
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
I clicked the wrong button (twice) before including
the last round of changes. This CL adds a necessary
copyright notice, and adjusts init() and comments in
a test file.
Change-Id: Ibf5d8e4d071fb571eab910902f0bc22009f2b81c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/571755
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This CL introduces code to count how often proposed completions are accepted.
The results of the latest completion request are stored and compared
with the next DidChange notification. If the change is at the same
position, more than one character long, and matches a completion
item, the code counts the completion as accepted.
If, for some reason, DidChange returns multiple changes, only the
first is checked.
If the whole file is replaced there is no check for using any
proposed completion.
The test does not exercise the multi-change counter.
Change-Id: I08e9e39f651e4975a86c2dda9f6f5ceab7787e53
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562248
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Now that go.dev/doc/telemetry exists, use it as a better link for
information about go telemetry.
For golang/go#63883
Change-Id: Ibe561c435e648b324a5ac444a8aade953354e92b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/570677
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Fix a crash found via telemetry, which may be encountered when finding
references for a package where one of the package files lacks a valid
name.
Fixesgolang/go#66250
Change-Id: Ifca9b6b7ab2ab8181b70e97d343fa2b072e866eb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/570676
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Fix a crash when Snapshot.fileOf fails, due to e.g. context
cancellation. The defer of release should only occur after the error is
checked. Unfortunately, this is very hard to test since it likely occurs
only due to races.
This is our first bug found completely via automated crash reporting.
Fixesgolang/go#66090
Change-Id: I7c22b5c2d1a0115718cd4bf2b84c31cc38a891ec
Reviewed-on: https://go-review.googlesource.com/c/tools/+/570675
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This change causes the stdversion checker not to report
any diagnostics in modules with go versons before go1.21,
because the semantics of the go declaration were not
clearly defined to correspond to toolchain requirements
at that point. Also, those Go versions are now unsupported,
so reporting diagnostics just creates unnecessary churn.
Updates golang/go#46136
Change-Id: I323f704c4d4f1f0fe5fc8b5680824bc07d3c4112
Reviewed-on: https://go-review.googlesource.com/c/tools/+/570140
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This change enables RunDespiteErrors, after auditing the code.
This should give more timely feedback while editing.
Also, it moves the vet/gopls common code (DisallowedSymbols)
to typesinternal.TooNewStdSymbols, out of the gopls module,
in anticipation of adding this analyzer to vet.
Updates golang/go#46136
Change-Id: I8d742bf543c9146376d43ae94f7adae3b453e471
Reviewed-on: https://go-review.googlesource.com/c/tools/+/570138
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This change is the counterpart to CL 569435 for completions
in imported packages.
Updates golang/go#46136
Change-Id: I57011897c395d37a89a8e3a99e8c3511de017ad3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/569796
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
When a vendor directory is present, we must explicitly use -mod=readonly
to query upgrades with `go list`. This was broken by the fixes for
workspace vendoring, which removed the `-mod` flag in most usage of the
gocommand package.
Fixesgolang/go#66055
Change-Id: I29efb617a8fe56e9752dc088dc5ea884f1cefb86
Reviewed-on: https://go-review.googlesource.com/c/tools/+/569877
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This change fixes a gopls panic caused by giving go/types@go.1.20
a Go version string with three components, e.g. go1.2.3.
Unfortunately this is hard to write a test for, since it requires
building gopls with go1.20 and running it with a go1.21 toolchain.
Fixesgolang/go#66195
Change-Id: I09257e6ded69568812b367ee80cafea30add93d3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/570135
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
In golang/go#66145, users reported spurious import errors in multi-root
workspaces. The problem was related to scenarios where module A had a
local replace of module B, and the user opened a file F in module B that
wasn't in the forward dependencies of module A. In this case, if the
View of module A tried to load F, it would get real packages (not
command-line-arguments), but due to module graph pruning the View of
module A would not have access to the full set of dependencies for
module B, resulting in the potential for import errors. Even this would
not be a problem, as long as the package that module A loaded for F was
not considered a 'workspace' package.
Unfortunately a couple of incorrect heuristics in gopls added along with
the zero-config work of gopls@v0.15.0 allowed us to diagnose these
broken packages:
1. In resolveImportGraph, we called MetadataForFile for each overlay. As
a result, the import graph optimization caused gopls to attempt
loading packages for each open file, for each View. It was wrong for
an optimization to have this side effect.
2. In golang/go#65801, we observed that it was inconsistent to diagnose
changed packages independent of whether they're workspace packages.
To fix that, I made all open packages workspace packages. It was
probably wrong for the set of workspace packages to depend on open
files. To summarize a rather long philosophical digression: open
files should determine Views, not packages.
Fixing either one of these incorrect heuristics would have prevented
golang/go#66145. In this CL, we fix (2) by building the import graph
based on existing metadata, without triggering an additional load.
For (1), we check IsWorkspacePackage in diagnoseChangedFiles to enforce
consistency in the set of diagnosed packages. It would be nice to also
remove the heuristic that "all open packages are workspace packages",
but we can't do that yet as it would mean no diagnostics for files
outside the workspace, after e.g. jumping to definition. A TODO is left
to address this another day, when we can be less conservative.
Fixesgolang/go#66145
Change-Id: Ic4cf2bbbb515b6ea0df24b8e6e46c725b82b4779
Reviewed-on: https://go-review.googlesource.com/c/tools/+/569836
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This change causes the imports package to return not just
lists of symbol names, but stdlib.Symbol structures, and
causes gopls to use them to filter candidates based on
the Go version of the requesting file.
This filtering is not applied to imported completions,
or to methods of unimported types (which apparently
we don't complete at all, surprisingly). These are
left for follow-up changes.
Also, support negative assertions in completion
@rank markers.
Change-Id: I2ae62c2b83a366a37bdd8db88e28cca4c5f92ae5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/569435
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Code comments contain many references to identifiers, highlighting these
references will make reading easier. Due to consistency and performance
considerations, only doc links are supported this time. Subsequent
changes will support hover and definition.
Updates golang/go#64648
Change-Id: I6b32075d703c03d06aaa16028cf55012d2e52c3c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/553836
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This CL adds to gopls a new analyzer that reports references to
standard library symbols that are too new for the version of
Go in force in the referring file.
The test is duplicated within the gopls marker framework to
ensure we have coverage of per-file Go versions within gopls.
Updates golang/go#46136
Change-Id: Ic7e04e0ae75a586dc0ac5dfc86c6b4aa67d5d194
Reviewed-on: https://go-review.googlesource.com/c/tools/+/568237
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Use Func.Origin instead.
Change-Id: Ie4d29f2bd319a46901ce137107689e37d8e1edfa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/569316
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Tim King <taking@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Fix two bugs discovered during the investigation of golang/go#66109,
which revealed the strange and broken intermediate test variant form
"path/to/command/package [command-line-arguments.test]", referenced from
the equally broken
"command-line-arguments [command-line-arguments.test]". This latter
package was *also* detected as an ITV, which is why we never tried to
type check it in gopls@v0.14.2.
- Snapshot.orphanedFileDiagnostics was not pruning intermediate test
variants, causing it to be the one place where we were now type
checking ITVs.
- Fix the latent bug that caused gopls to record a dangling edge between
the two ITVs.
There is a third bug in go/packages, filed as golang/go#66126.
Fixesgolang/go#66109
Change-Id: Ie5795b6d5a4831bf2f73217c8eb22c6ba18e59cd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/569035
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
There are no major changes, but new are:
- ClientSignatureInformationOptions.NoActiveParameterSupport
- CodeActionClientCapabilities.DocumentationSupport
- type CodeActionDocumentation referred to in
- CodeActionOptions.Documentation
- Command.Tooltip (which might be useful)
- type LanguageKind string
- type RegularExpressionEngineKind = string
which is referred to in RegularExpressionClientCapabilities
- new CodeActionKind RefactorMove, and Notebook
The values of InlineCompletionTriggerKind have changed but the names did not.
And, the type TraceValues was renamed TraceValue.
Change-Id: I54d2ecb4a2a55cc24abc71f1f7b292f25bcac8d5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/568475
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Change-Id: I78701b620029a94b47524f5d4a77c35a86df2e4e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/564337
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Commit-Queue: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Add a test for the fix in golang/go#65952: a nil pointer exception when
highlighting a return value in a function returning no results.
Also, merge tests related to control flow highlighting, since it is
convenient to be able to run them together, and since there is
nontrivial overhead to tiny tests.
Updates golang/go#65952
Change-Id: Ibf8c7c6f0f4feed6dc7a283736bc038600a0bf04
Reviewed-on: https://go-review.googlesource.com/c/tools/+/567256
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
The result of funcType will be nil, if a function does not return any
values. This caused an SIGSEGV before.
Fixesgolang/go#65952
Change-Id: Ibf4ac3070744f42033504220f05b35a78c97d992
GitHub-Last-Rev: 74182b258d
GitHub-Pull-Request: golang/tools#480
Reviewed-on: https://go-review.googlesource.com/c/tools/+/567275
Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Implement a code action to easily refactor function arguments, return
values, and composite literal elements into separate lines or a single
line.
This feature would be particularly helpful when working on large
business-related features that often require long variable names.
The ability to quickly split or group lines would significantly
accelerate refactoring efforts to ensure code adheres to specified
character limits.
Fixesgolang/go#65156
Change-Id: I00aaa1377735f14808424e8e99fa0b7eeab90a7a
GitHub-Last-Rev: d3c5b57d81
GitHub-Pull-Request: golang/tools#470
Reviewed-on: https://go-review.googlesource.com/c/tools/+/558475
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Fixesgolang/go#64973
Change-Id: I2af27de59dc48e48c49e425342b6a25486b1656d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/566956
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Adds support for integer constants expressed in float syntax '2.0'.
Also fixes the same issue in gopls/internal/golang/hover.go.
Fixesgolang/go#65939
Change-Id: Ic42b4cc7c6aedf7f115e3195044f06001cb6fb12
Reviewed-on: https://go-review.googlesource.com/c/tools/+/567115
TryBot-Result: Gopher Robot <gobot@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Run-TryBot: Tim King <taking@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Rewrite the now-obsolete documentation. Notably, remove the section on
experimental workspace mode, as it is long unsupported.
For golang/go#57979
Change-Id: I8ba77d626d0b24b0ab34a78103985a5a881def21
Reviewed-on: https://go-review.googlesource.com/c/tools/+/566936
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
This change (part of the types.Alias audit) defines two
helper functions for dealing with pointer types:
1. internal/typeparams.MustDeref (formerly go/ssa.mustDeref)
is the type equivalent of a LOAD instruction.
2. internal/typesinternal.Unpointer strips off a
Pointer constructor (possibly wrapped in an Alias).
The golang.Deref function, which recursively strips off
Pointer and Named constructors, is a meaningless
operation. It has been replaced in all cases by
Unpointer + Unalias.
There are far too many functions called 'deref' with
subtle variations in their behavior. I plan to
standardize x/tools on few common idioms.
(There is more to do.)
Updates golang/go#65294
Change-Id: I502bab95e8d954715784b7e35ec801f4be4bc959
Reviewed-on: https://go-review.googlesource.com/c/tools/+/565476
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Tim King <taking@google.com>
CL 565838 changed the way initialization cycle errors were reporting,
and would have broken gopls' parsing of continuation errors, yet no
x/tools tests failed.
Add a test for this behavior. Due to golang/go#65877, duplicate
diagnostics had to be suppressed for this test.
Updates golang/go#65877
Change-Id: I48244ac469ab78d2e40bf92ec061671cef72c6d9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/566075
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
...and factor numerous places to use it.
(This pattern kept recurring during my types.Alias audit,
golang/go#65294.)
Change-Id: I93228b735f7a8ff70df5c998017437d43742d9f3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/565075
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Tim King <taking@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
We should not create Views for vendored modules, just as we don't create
Views for modules in the module cache. With this change, gopls behaves
similarly to gopls@v0.14.2 when navigating around the Kubernetes repo.
Also add some test coverage that vendored packages are not workspace
packages.
Fixesgolang/go#65830
Change-Id: If9883dc9616774952bd49c395e1c0d37ad3c2a6a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/565458
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Fix two bugs related to workspace packages that contributed to
golang/go#65801.
The first is that we didn't consider packages with open files to be
workspace packages. This was pre-existing behavior, but in the past we
simply relied on diagnoseChangedFiles to provide diagnostics for open
packages, even though we didn't consider them to be part of the
workspace. That led to races and inconsistent behavior: a change in one
file would wipe out diagnostics in another, and so on. It's much simpler
to say that if the package is open, it is part of the workspace. This
leads to consistent behavior, no matter where diagnostics originate.
The second bug is related to loading std and cmd. The new workspace
package heuristics relied on go/packages.Package.Module to determine if
a package is included in the workspace. For std and cmd, this field is
nil (golang/go#65816), apparently due to limitations of `go list`. As a
result, we were finding no workspace packages for std or cmd. Fix this
by falling back to searching for the relevant go.mod file in the
filesystem. Unfortunately this required reinstating the lockedSnapshot
file source.
These bugs revealed a rather large gap in our test coverage for std. Add
a test that verifies that we compute std workspace packages.
Fixesgolang/go#65801
Change-Id: Ic454d4a43e34af10e1224755a09d6c94c728c97d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/565475
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
As described in golang/go#65762, treating locally replaced modules the
same as workspace modules doesn't really work, since there will be
missing go.sum entries. Fortunately, this functionality was guarded by
the "includeReplaceInWorkspace" setting. Revert the default value for
this setting.
Fixesgolang/go#65762
Change-Id: I521acb2863404cba7612887aa7730075dcfebd96
Reviewed-on: https://go-review.googlesource.com/c/tools/+/564558
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Also, simplify the handling of constants and address a
couple of TODOs from the preceding refactoring.
Change-Id: Ic2e2231a4eb2172b365969d2d2b251572287e511
Reviewed-on: https://go-review.googlesource.com/c/tools/+/564655
Reviewed-by: Peter Weinberger <pjw@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL applies a number of readability enhancements:
- eliminate unnecessary local variables
- reorder and document struct fields
- strength-reduce cache.Package to metadata.Package
- more conventional variable names. e.g.
items -> tokens
lng -> length
ti -> info
x -> id
y -> obj, ancestor
nd -> parent
def/use -> obj
- rename unexpected() to errorf(), with printf semantics
- remove unreachable cases
(e.g. info == nil; impossible types.Object subtypes)
- use is[T] to eliminate statements
- eta-reduce inspector function
- add doc comments
- add TODOs for further simplification and suspected bugs.
This is a pure refactoring; no behavior changes are intended.
Change-Id: I97a8f0b76008b0f784328f155f41bea8799f8389
Reviewed-on: https://go-review.googlesource.com/c/tools/+/561056
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
We no longer hard-code the gopls version.
Change-Id: I6f91f6e8f97732803aa5dfe05203ceee0611c489
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562677
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
In golang/go#65312, we observed a test flake due to what I believe is a
mostly benign race in diagnostics:
1. diagnoseSnapshot requests session.Views()
2. diagnoseSnapshot computes diagnostics
3. diagnoseSnapshot calls updateDiagnostics for the views from (1)
This means that if a view V were created and diagnosed between (1) and
(3), the call to updateDiagnostics will prune the diagnostics for V,
because it doesn't think it's an active view.
This is fundamentally a flaw in the design of multi-view diagnostics: we
don't a priori know which views apply to a given file, so we have to do
this complicated join. Nevertheless, I think this race is avoidable by
requesting session.Views() inside the critical section of
updateDiagnostics. In this way, it's not possible to overwrite
diagnostics reported by a different View, without also observing that
View.
Fixesgolang/go#65312
Change-Id: Ie4116fbfac2837f7d142c7865758ff6bac92fce4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/563957
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This change adds a disambiguating suffix such as " [windows,arm64]"
to diagnostics that do not appear in the default build configuration.
Fixesgolang/go#65496
Change-Id: Ided7a7110ff630e57b7a96a311a240fef210ca93
Reviewed-on: https://go-review.googlesource.com/c/tools/+/563876
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Fix a NoOutstandingWork assertion that was not ignoring the telemetry
prompt.
Fixesgolang/go#65561
Change-Id: I194660965b681cc82e958dbbcf5df1404a576e09
Reviewed-on: https://go-review.googlesource.com/c/tools/+/563958
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
FindDeclAndField in hover.go used panic() and recover() for flow control.
This CL replaces that with tests for early successful completion. The new
code is somewhat fiddlier.
When running all the gopls tests, the old code returned 32,339 non-nil values.
The new code returns exactly the same results in all these cases.
The new code is generally faster than the old code. As they use wall-clock
times, the timings are somewhat noisy, but the median and 99th percentiles
were 1520ns, 12070ns for the new code, and 7870ns, 26500ns for the old.
For 270 of the 32,339 calls, the old code was faster.
The stack is preallocated to capacity 20. The 99th percentile of stack
size is 80, which is only 2 reallocations. The large stacks appear to
happend looking in the go source tree while processing deep completions
(This change was written by pjw in https://go.dev/cl/445337 but it
went stale.)
Change-Id: If23c756d0d671b70ad6286d5e0487c78ed3eb277
Reviewed-on: https://go-review.googlesource.com/c/tools/+/563935
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Peter Weinberger <pjw@google.com>
After setting CL 563475 for autosubmit, I realized we should use a
slightly more ergonomic pattern for configuring folder settings.
Change-Id: I3a4e9e3dca3f4865c3bca02258cd81ff2b7024b3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/563402
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Fix two bugs related to dynamic configuration, that combine to prevent
several clients from correctly configuring gopls, as reported in
golang/go#65519 (Eglot), slack (lsp-mode), and team chat (Helix).
The first bug has existed ~forever: when we make a
workspace/configuration request in response to a didChangeConfiguration
notification, we attempt to fetch the "global" settings by passing
"scopeURI": "". The LSP spec defines "scopeURI" as a nullable URI:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#configurationItem
Apparently, Javascript-based clients such as VS Code and coc.nvim (the
two clients I regularly test with) will happily accept "" as a falsy
value, and so this global query works. However, this query fails in
Eglot (and likely other clients), causing didChangeConfiguration not to
work.
The second bug is new: When adding a new workspace folder we were
failing to overwrite opts with the correct value (:= vs =, alas). This
initial query had been masking the bug described above in Emacs, whereas
in VS Code the (incorrectly) successful workspace/configuration request
above masked the new bug. Since they both fail in Eglot, they are
revealed.
The fake editor is updated to reject the "" scope, highlighting the
first bug. A new integration test is added to exercise the second bug,
by way of a new integration test option to add per-folder configuration.
Additionally, a marker test is added to exercise static configuration,
which is when the client does not support the configuration capability
at all. This wasn't actually broken, as first suspected, but the test is
useful to include anyway, as we had no tests for this client behavior.
Fixesgolang/go#65519
Change-Id: Ie7170e3a26001546d4e334b83e6e73cd4ade10d8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/563475
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Found while thinking about typestate.
Change-Id: Ia98ace4782be34d1201f0ce5c8e536314fc494d7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/563155
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Commit-Queue: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Update to pick up the fix in https://go.dev/cl/562857.
Change-Id: I0708338068e69d863fcbe3d5b6e0505f27735426
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562683
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Peter Weinberger <pjw@google.com>
...and v0.14.2 is the last to do so without warnings.
Change-Id: Ie8cfd5e0337632fb60900938667e0a2a682ecb65
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562682
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Resolve edits for inline refactorings when the client supports it.
For golang/go#64510
Change-Id: I67101ab19576054b1d03c46e3d318a4660088a63
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562118
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Update the view selection algorithm to consider replace directives,
treating locally replaced modules in a go.mod file as workspace modules.
This causes gopls to register watch patterns for these local replaces.
Since this is a fundamental change in behavior, add a hidden off switch
to revert to the old behavior: an "includeReplaceInWorkspace" setting.
Fixesgolang/go#64762Fixesgolang/go#64888
Change-Id: I0fea97a05299acd877a220982d51ba9b4d4070e3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562680
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
To prepare for the follow up change, where we honor local replaces in
the view selection algorithm, treat go.mod files more like go.work files
in a couple places:
- Re-run the view selection algorithm whenever a go.mod file changes on
disk.
- Use the workspaceModFiles field in the bestView algorithm, rather than
checking the gomod field directly.
Apart from perhaps running the view selection algorithm more frequently,
this change should have no observable impact.
For golang/go#64888
Change-Id: I9d9a74b876791a77e284a1a1d5fcc7a691199901
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562679
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This brings in the new notation for stacks.
Change-Id: Iff5e0053798fb3984a25f3109fd6ca3a468575e2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562855
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
The "missing Diagnostic.Code" assertion checks an invariant
that isn't guaranteed. This change downgrades it to a bug.Reportf.
Also, we add a similar assertion to analysistest.RunWithSuggestedFixes
which has much better coverage of all the analyzer's different
ways of constructing a SuggestedFix, unlike gopl's own superficial
tests of each analyzer it carries. (This assertion may be enabled
only for analyzers in x/tools, since the invariant is stricter
than is required by the public API.)
Also, fix a couple of places in unusedvariable where it could
conceivably return a fix with no TextEdits; they are not intended
to be lazy fixes.
Fixesgolang/go#65578
Change-Id: I5ed6301b028d184ea896988ca8f210fb8f3dd64f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562397
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Commit-Queue: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
vscode complains in the Window channel about zero-length semantic
tokens. It may not matter but it seems cleaner to avoid the error
and not send zero-length semantic tokens.
Fixes: golang/go#65254
Change-Id: I470f8052a6288f5bf4b592794cf6cd54d55b5539
Reviewed-on: https://go-review.googlesource.com/c/tools/+/561155
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Peter Weinberger <pjw@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Gopls' refresh of the goimports resolver already introduces
non-determinism into imports operations: gopls does not observe changes
until the asynchronous refresh occurs.
This change allows operations to continue to run on the stale resolver
until a new resolver is ready.
Due to inherent raciness, it's hard to benchmark the impact of this
change: one would have to catch gopls during a refresh, which occurs at
an automatically adjusted pacing.
Also update TODOs.
Fixesgolang/go#59216
Change-Id: I303df998d804c9a1cd1c0e307872d1d271eed601
Reviewed-on: https://go-review.googlesource.com/c/tools/+/561235
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
When using the gopls daemon, or with the multi-View workspaces that will
be increasingly common following golang/go#57979, there is a lot of
redundant work performed scanning the module cache. This CL eliminates
that redundancy, by moving module cache information into the cache.Cache
shared by all Sessions and Views.
There should be effectively no change in behavior for gopls resulting
from this CL. In ModuleResolver.scan, we still require that module cache
roots are scanned. However, we no longer invalidate this scan in
ModuleResolver.ClearForNewScan: re-scanning the module cache is the
responsibility of a new ScanModuleCache function, which is independently
scheduled. To enable this separation of refresh logic, a new
refreshTimer type is extracted to encapsulate the refresh logic.
For golang/go#44863
Change-Id: I333d55fca009be7984a514ed4abdc9a9fcafc08a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559636
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Make the following improvements to the completion benchmarks:
- Run the "CompletionFollowingEdit" benchmarks with and without edits.
Accordingly, rename it to just "BenchmarkCompletion", and shorten
subtest names.
- BenchmarkCompletion is a cleaner way to specify completion tests.
Deprecate other styles.
- Support running completion tests with a massive GOPATH, by passing
-completion_gopath.
- Add a tools_unimportedselector test that seems to do a good job of
exercising slow unimported completion. Other unimportedcompletion
tests were short-circuited in one way or another (for example, because
kubernetes uses vendoring, gopls doesn't scan GOPATH).
This will invalidate our benchmark dashboard, so we should merge this
change before subsequent CLs that optimize unimported completion.
For golang/go#63459
Change-Id: I9d7a06e3c1a7239b531ed8ff534f3174f4ac4ae8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560457
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Switch the ModuleResolver from a lazy initialization model to a
constructor, so that the resolver returned by ProcessEnv.GetResolver is
ready to use.
A constructor is easier to maintain as it involves less state, avoids
redundant calls to init, and avoids bugs where the call to init is
missing (such as was the case for the scoreImportPath method). It also
lets the caller differentiate between a failure to construct the
resolver and a resolver that only returns errors.
Pragmatically, I'd like to move toward a model where the caller re-scans
imports by asynchronously cloning, priming, and replacing the resolver,
rather than blocking. This is a step in that direction.
This change is not without risk, but it looks like all calls to
ModuleResolver methods are preceded by a call to GetResolver, and
errors from GetResolver are handled similarly errors from methods.
There is some messiness resulting from this change, particularly in
tests. These are noted inline, and can eventually be fixed by similarly
avoiding lazy initialization of ProcessEnv.
For golang/go#59216
Change-Id: I3b7206417f61a5697ed83e811c177f646afce3b2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559635
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Add a test to verify that gopls now does the right thing in the scenario
described by golang/go#63917, where the users has a go.work file inside
a Go module. Gopls now behaves consistently with the go command.
Fixesgolang/go#63917
Change-Id: I0c9d251477e7403b4f2b75f0333e991a98c82ba4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/562015
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Deprecate the "allowModfileModifications" setting, per the rationale in
golang/go#56570. This setting cannot be working well, and is a relic of
earlier versions of Go modules where modfile mutation was expected.
Fixesgolang/go#56570
Change-Id: I240e09776367ff3a274212c8eb69bbc57fde7d9f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/561975
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
With zero-config gopls (golang/go#57979), this function is no longer
used.
Change-Id: Ie59a7d39c62eab340fb6e44ddd9b7a0b1cabd92e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560467
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
No matter where the go.work file is, the addition or deletion of
workspace-vendored modules mandates reinitialization.
Add the necessary watch pattern, and detect that reinitialization is
required in snapshot.clone. The latter required threading through the
file modification, so that we can detect creations and deletions
explicitly.
This is a great example of where having a realistic fake file watcher
paid off.
Fixesgolang/go#63375
Change-Id: Id3658e85bd2d915639d24fed31aba2d54ebc75e7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560717
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Add a check for nil funcBody that was missing from CL 503439.
Fixesgolang/go#65516
Change-Id: Iba8a8c3410ddb2c130d6cd2ecb229929a9f4f681
Reviewed-on: https://go-review.googlesource.com/c/tools/+/561435
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
This change--a pure refactoring--moves the common logic for
LSP semantic token encoding into protocol/semtok, and pushes
the Go- and template-specific logic into their respective
packages.
Change-Id: I308c37c9c8017c5fc38e3ce213a1d4ddb6b50e21
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560718
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Interestingly, this turned up one false positive of the form
var hook = func(err error) {}
where other build-tagged files not visible to the analysis
assigned other values to hook that actually used the
parameter.
Change-Id: I7d230160fd7e5ad67ade7b6e57db150bf68fa1c3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560716
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This change pushes the assertions more deeply into the type
checking logic so that we might have a chance of understanding
the recurring inconsistency problems reported by telemetry.
Updates golang/go#63822
Change-Id: I1371f7aa7104df0c55e055a11e9e6bb15219d79a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560795
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Support workspace vendoring by simply removing logic in gopls for
deriving the `-mod` flag. If we don't need to set `-mod=mod`, let the go
command decide what to do. We don't need to worry about explicitly
setting `-mod=readonly`, since this has been the default since Go 1.16.
For golang/go#63375
Change-Id: I1adbe20cef5b9e3edcb7ac2445c4d5ae63f3a3a9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560466
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
I am able to reproduce the bug by commenting out declarations
in builtin.go or unsafe.go, which is very unlikely to happen
in practice but reason enough not to call bug.Reportf.
Fixesgolang/go#64604
Change-Id: Idc7e0b2bda0b6070f3d09ba9cb825b7f28b8f796
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560897
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The previous assertion required exactly one, but this condition
is violated by a file=emptyfile.go query, which produces no
CompiledGoFiles and IgnoredFiles=[emptyfile.go]. So we relax
the assertion, and use the first ignored file as the suffix.
Fixesgolang/go#64557
Change-Id: I097badd1b102bdc73af2ffa8a4871da1e359b173
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560465
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This change causes the metadata graph to assert that it is
acyclic before applying the updates. This should be an
invariant, but the attached issues make us skeptical.
Updates golang/go#64227
Updates golang/vscode-go#3126
Change-Id: I40b4fd06fcf2c64594b34b8c300f20ca0676d0fa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560463
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This change causes the hover information for a type to
display the set of fields that are accessible due to
promotion through one or more embedded fields.
Change-Id: I7795daaeef6d5e910ae7917aa44d3012f4c016b7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559499
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Rewrite the control flow highlighting logic to be more understandable,
and to fix a bug where multi-name result parameters were not properly
counted.
Fixesgolang/go#60589
Change-Id: Id12ada78852be0a8376fbd482326322fc1b87fcf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/503439
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This change enables the crash monitor in gopls, so that it
increments a telemetry counter for the stack any time
the Go runtime crashes.
x/telemetry cannot be built with go1.18, which gopls still
supports (though not for long). So we must ensure that there
are no direct references to x/telemetry from gopls; all
references must go through the build-tagged non-functional
shims, which avoids the dependency.
Also, internal/telemetry can no longer depend on protocol,
to avoid a cycle via protocol -> bug -> telemetry.
Updates golang/go#42888
Change-Id: I7a57b9a93ab76a58ec6dd73f895d4b42ed29ea86
Reviewed-on: https://go-review.googlesource.com/c/tools/+/558256
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Update gofumpt, xurls, and staticcheck. Also update hooks and tests
according to their version support.
Change-Id: I62c1424b1640563cf56fe8956e099cb026ea8f92
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559220
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL changes the name of the fix from "Create variable"
to "Create function" when appropriate. Oddly the names of
fixes appear to be systematically not covered by our
tests.
Also tidy up the code.
Updates golang/go#47558
Change-Id: Ibbefeb90874d7ce481893cfa401c59a421aad5e4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/560475
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Remove the ProcessEnv.ModFile field, which was never set. Also add
additional comments and superficial cleanup.
Change-Id: I45d63e5039043d01d4280d2d916df6794e437d8a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559506
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Now that views are immutable, we can simplify the invalidation of
imports state. The imports.ProcessEnv can also be immutable, and we need
only invalidate module state (i.e. dependencies) if a go.mod file
changes.
While at it, add documentation and perform some superficial cleanup in
the internal/imports package. This is a first step toward larger state
optimizations needed for the issues below. TODOs are added for remaining
work.
For golang/go#44863
For golang/go#59216
Change-Id: I23c1ea96f241334efdbfb4c09f6265637b38f497
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559496
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This change displays the methods of a type after its doc comments
when hovering over a type.
Also:
- unexport HoverJSON.
- remove leading/trailing newlines in markdown blocks.
- clarify formatHover.
Fixesgolang/go#56331
Change-Id: Icf7ce775e1c01cc252f9cd218b6a0220d82c4087
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559495
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
Change-Id: I541c41b2d4c3618a47c05b367b9a9830121a19fe
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559219
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Fix benchmarks that were inadvertently broken by find-and-replace
refactoring associated with the file move of CL 557718.
Benchmarks run at a fixed commit, so their test data need not change
when the tools repository is refactored.
Change-Id: I24b9bcc0e36e8da072113093d21a80e634ef9fc9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559160
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Now that options are immutable on the view, we can safely memoize the
view's directory filter function. Do this, which fixes the regression in
BenchmarkReload observed in golang/go#64724.
For golang/go#64724
Change-Id: Iaf8c2d5e69e68e0077f07a72026ef46c0e19bcc0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559157
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Add testenv.NeedsExec to this test, since it must be able to run the Go
command. Also fix the test error message.
Fixesgolang/go#65271
Change-Id: I525c5fd59cab7e057ab765eb41f2a84a7dd03635
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559155
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
This is now possible as the linked issues have been fixed.
Change-Id: Icf1b6c2f00efac65db79a4cdcb2062e30d5960b3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/553575
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Michael Stapelberg <stapelberg@google.com>
Missing or null arguments can be expressed in a variety of ways,
so provide a helper function to consolidate the necessary checks.
Change-Id: I595f71a6485d84883667f3c6adf5a87c41fbd6aa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556695
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TestInconsistentVendoring was awaiting only the initial workspace load
before capturing diagnostics and running their code action. But opening
a/a1.go also triggers diagnostics. This can cause the file lock failures
observed in golang/go#64229. To fix this, wait for the change (the
didOpen) to be fully processed.
Fixesgolang/go#64229
Change-Id: I747ea22db02b8def4bf7764714db7f901891792d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/558076
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
The lsp directory is no more.
Change-Id: I7badd01645c7faab841e6219387cb84d8b893f2b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/557741
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
The relative pattern support added in version 3.17 of the LSP solves
several problems related to watched file patterns. Notably, relative
patterns do not suffer from windows drive letter casing problems (since
they are expressed relative to a URI), and generally work more
consistently across clients. Absolute glob patterns do not work well on
many clients.
Fixesgolang/go#64763
Change-Id: I040560f236463c71dc0efaac1ea0951fb8d98fab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/557499
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This change hides the two concrete io.Writer types used
by this package, and improves a number of doc comments.
Change-Id: Ib9653759a4fa882fe720e5bbcee049bc4304a25a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/557399
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Change-Id: Id1070d775d1a3bcfa5b2302b862b4e78f1411678
Reviewed-on: https://go-review.googlesource.com/c/tools/+/557735
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This change eliminates everything from the public API that
isn't actually needed today by some part of gopls.
The removed parts are moved into test files. Perhaps
they are there to enable testing, or perhaps they are
features that were anticipated but never needed;
I don't know. Reviewers: feel free to tell me which of
them we should delete outright.
The old API had 41 symbols; the new API has just 6:
- ConnectToRemote
- ExecuteCommand
- NewForwarder
- ParseAddr
- QueryServerState
- NewStreamServer
Also, tweak doc comments.
Change-Id: I31a5385bbb954bb32372f952b0892f41ba81a42a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/557497
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL adds a test that confirms the bug reported in
golang/go#60592 is fixed at master; the test fails at
commit 87ad891.
Fixesgolang/go#60592
Change-Id: Id9436d019a782a385228f77ce92988005711e700
Reviewed-on: https://go-review.googlesource.com/c/tools/+/557715
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
This change unexports various parts of the API, including
DiskFile, Overlay, BundleQuickFixes, XrefIndex, and
the methods of fileMap.
It also moves some declarations for clarity and updates
doc comments. This is is a preparatory step for moving
this package out of lsp/.
Change-Id: I1ff745b612315f9b9c0cd188c67bdfa695f9708f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/557498
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
go/packages will return test command-line-arguments test variants for
standalone test files. Handle this case, and refine our bug report.
Fixesgolang/go#64233
Change-Id: I15fd8a50476ece58eedbf09233827f55bfb79a2e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/557635
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
While working on performance I disabled staticcheck. Reenabling it
today turned up several suggestions, and a few real bugs.
Change-Id: I032d0cfb12ec8fda2573a2663faec4126b321afc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/557496
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Re-enable (with adjustment) some tests that were disabled while staging
standard library changes to parsing and printing.
Fixesgolang/go#54822
Change-Id: I7d54ce96b10f294dae257e8403983b25541a158f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/557397
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Make several mechanical clean-ups to analyzer implementations, reducing
the complexity of their configuration and normalizing behavior.
- Remove the "type error" analyzer category, since it is now an unused
distinction.
- Make "stubmethods" an ordinary analyzer, since it no longer needs
special logic in the code action handler.
- Revert infertypeargs to be an ordinary analyzer (golang/go#63821),
but make its severity "hint", so that it doesn't show up in the
problems tab.
- Remove the "convenience" analyzer category, which only existed for
the enabled/disabled configuration. The only effect of this is that
the "fillstruct" analyzer can no longer be disabled, but since code
actions should not be disruptive (golang/go#65167), it should be OK
to have them all available. Notably, we don't allow disabling other
code actions.
Fixesgolang/go#63821Fixesgolang/go#61559
Change-Id: I2a1cd01ea331c26fdbb35d6b5c97562c2a1f9240
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556555
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Plus a unit test.
Fixesgolang/go#63472
Change-Id: I69466024eb876457d0d3f262cef3467b25879d93
Reviewed-on: https://go-review.googlesource.com/c/tools/+/557215
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Fix two cases that could lead to the bug reports related to position
conversion reported in golang/go#64547:
1. The lostcancel analyzer reports diagnostics for a synthetic range
that could overflow the file.
2. The cgocall analyzer, which runs despite errors, could report
diagnostics for invalid "fixed" positions.
Issue (1) was easy enough to prove with a test, though (2) is admittedly
just a hypothesis. Nevertheless, we should close the telemetry-derived
issue and see if it recurs.
Fixesgolang/go#64547
Change-Id: Ie69bdae474abdd25235d856fc0ae0bfaf7e214ed
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556815
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This change moves all the logic in server for Go-specific
code actions to source.CodeActions. Some symbols are renamed,
documented, and unexported.
Unexporting several functions caused the brand-new unusedparams
analyzer (on whose behalf this work was done) to report unused
parameters, allowing further deletions. Yay.
No behavior changes.
Change-Id: I72b6ca45137a24bf855f3c7418bbe8b26ed0aefa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556818
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This change updates the logic to display the method set of the
selected type so that:
- it includes promoted methods;
- it includes interface methods, if that would not be
redundant with the syntax of the the type;
- receiver variables are properly named, and have a
pointer type if appropriate.
Fixesgolang/go#61634
Updates golang/go#56331
Change-Id: Ied9c06c36178424575012adb8fde0b1ddbd688c3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/555456
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
CL 555457 copied the copyright header from another file without fixing
the year.
Change-Id: I95e0a732902c6f354105732380fce5f26ee1fbe5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556819
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
This change causes server.diagnose to choose a covering set
of packages for the open files preferring the widest package
for a given package path. This has two benefits: correctness,
for analyzers (such as unusedparams) that make incorrect
deductions when additional files may be added to the package;
and efficiency, as it requires fewer packages in general.
Change-Id: I21576c6ce4c136d6c72ccce76971a782abd4df53
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556816
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
As discussed in golang/go#63803, add a version variable to allow package
managers to set the gopls version at linktime, thereby avoiding the need
to patch source.
Fixesgolang/go#63803
Change-Id: Ie6696283e1007577e82d96f01c96e16e89cfcb6f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/555457
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This change causes completion to omit type parameters
in the snippet template when the instantiations can be
inferred from the arguments to the call.
This is only implemented when types are available,
not for unimported completions. (In principle it could be
implemented using a best-effort syntactic traversal, similar
to inlining's "freeishNames", but it seems fiddly.)
Fixesgolang/go#51783
Change-Id: I4c793628e7f2cbbb28a7f5ca7b5e305ab683491e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/554855
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Previously, many diagnostics were produced with
messages that were in fact the titles of suggested fixes
(e.g. "Fill struct S"); conversely, many suggested fixes
appeared in the client UI with a message that was in fact
the text of the diagnostic it was supposed to fix
(e.g. "undeclared name: x").
This change reorganizes the way suggested fixes are handled
so that, for fixes produced by analyzers, the analyzer output
(Diagnostic and SuggestedFix) determines the LSP output.
An SuggestedFix with no edits is assumed
to have fixer support in ApplyFix; the name of the fixer
is determined by the Diagnostic's Category.
(Ideally we would use SuggestedFix.Category in case
a diagnostic has more than one lazy fix, but there's
no such field.)
Fixers whose commands are constructed directly by the
Code Action method are essentially unchanged.
There is still a need for some kind of framework for
registering (analyzers, category, fixer) triples, but
this is a first step.
Fixesgolang/go#65087
Change-Id: Id72f3fd187d42df9c3711f4d3be140f9ea7af1d7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556755
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Fix the semantic conflict of CL 550355 and CL 548276.
Change-Id: I5bf92a228032bbbf8e64193794d3c42714758fa9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556835
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This change is a substantial rewrite of the unusedparams analyzer
designed to eliminate essentially all false positives, based on
the rule that unless we can see all uses of a function, we can't
tell whether it is safe to alter its signature.
The new algorithm computes the set of address-taken functions,
that is, functions that are referenced other than in the
CallExpr.Fun position of a function call. For soundness we must
assume that such values may be required to conform to some func
type, so we cannot alter their signature.
Exported functions and methods must also be treated
conservatively, as they may be address-taken in another package.
And unexported methods are ignored if they match the name of an
interface method declared in the same package, since they may be
required to conform to it.
Anonymous functions that are immediately assigned to a var are
treated like named functions, with the var symbol standing for
the function name.
Since the false positive rate is now close to zero, this change
also enables the analyzer by default in gopls.
Number of findings:
Repo Before After Comments
x/tools 86 26 100% true positives
k8s 2457 1987 Only 14% are in non-generated
files, and of those, all that
I sampled were true positives.
Updates golang/go#64753
Change-Id: I35dfc116a97bb5e98a2b098047fce176f2c1a9d6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/550355
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Allow apply fix and change signature commands to return edits instead
of applying the edits.
Added a marker test for removing parameters using the new resolve logic.
We probably want most refactoring code actions to work both ways. This
also updates the fill struct test to make sure that the capabilities
of the editor are being correctly respected.
For golang/go#64510
Change-Id: If58f7bdff52ec8e1621c007d029c5b9b60bbdd3a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/548276
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Update TestURIFromPath not to assume the drive name for the default
volume.
Fixesgolang/go#63366
Change-Id: I0196f11e628821b941d418831f3a3da7cf125ddf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556496
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The orphaned file and critical error logic has been rewritten, and we no
longer diagnose broken workspaces in this way, because they are no
longer broken due to zero-config gopls.
Fixesgolang/go#61006
Change-Id: I7e532c1ecf11b5151ff03103bb19806caaeec565
Reviewed-on: https://go-review.googlesource.com/c/tools/+/555755
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Simplify the DiagnoseFiles command handler to just diagnose all
snapshots for requested files, rather than implement ad-hoc diagnostic
logic. This may be somewhat slower, but this is only used for command
line commands and is therefore performance is not critical.
Also adjust updateDiagnostics to not overwrite final diagnostics with
non-final diagnostics.
These two changes together avoid the race encountered in
golang/go#64765: DiagnoseFiles does not return until diagnostics are
finalized, and these final diagnostics are not overwritten.
Fixesgolang/go#64765
Change-Id: I54ef7309487a9803a8bbd45ab2a8de4dbf30c460
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556475
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Avoid flaky dependence on go list error messages in
TestResolveImportCycle. It would be better if we could guarantee that
the error is from go list, but since the error from golang/go#64899 is
reasonable, it is best to just avoid the flake for now.
Fixesgolang/go#64899
Change-Id: If5038acbdf020323d8fa9db0d8e1e8e054cfe464
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556495
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This fixes a predicted bug that was noticed while reading
the code.
Fixesgolang/go#61216
Change-Id: I9614454fbd8538cb0b9eb1f56f11934cb88a7aed
Reviewed-on: https://go-review.googlesource.com/c/tools/+/555635
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
As with the darwin builders, this builder is too slow for this test. Not
investigated.
Fixesgolang/go#64473
Change-Id: Ice9b3f25ddb70327dd8d259063bf351db167eff5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/555715
Auto-Submit: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
The syntax generated for type func(...T) was func([]T),
causing extract operations to misbehave.
Fixesgolang/go#63287
Change-Id: I37978a13b2b51f0802ea694b7711a7d6444384fb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/555458
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Add View Type, EnvOverlay, and Root to the view debug page. Also inline
the View debug page into the Session debug page, to save a click.
For golang/go#57979
Change-Id: Id6fbf86a55329078adcada049e34607ee918da11
Reviewed-on: https://go-review.googlesource.com/c/tools/+/555197
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Clean up configuration options for the gopls command that are no longer
relevant:
1. remove Application.name, since it can always be "gopls"
2. remove Application.wd, since it can always be os.Getwd()
3. remove Application.env, since it can always be os.Environ()
4. remove magic internal@ connection strings
For (1) I don't know why the capabilities test modified
Application.name, but since this is the only place that customized the
name, and it is a test which still passes, it can't be important.
Items (2), (3), and (4) only existed for subverting the existing means
of configuring gopls via either (1) the operating system or (2) the LSP.
For example, if we want to modify gopls's environment, we can do so via
a subprocess or via environment passed into the gopls session during
initialization. No need to have a third and unofficial means of doing
so.
For golang/go#63803
Change-Id: Ib5a478b689f315813bdfff263afd9d037f368d3c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/555636
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
CL 554717 was merged before rebasing to pick up the change to stubbed
method receivers added in CL 544916, resulting in test failures.
Change-Id: Ia3a82605a8725ce1f88465eb4cd6ae04e8455fa9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/555555
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Russ Cox <rsc@golang.org>
This change removes a "/v2" (etc) suffix from the module path when
choosing the local import name for a package during the "stub methods"
operation. (CL 469155 caused the regression in behavior.)
Also, don't use a path-based heuristic to guess the local PkgName
defined by an import; look up the correct answer in the metadata.
Also, a test.
Also, replace last (degenerate) call to stubmethods.RelativeToFiles
with source.Qualifier, renamed to typeutil.FileQualifier.
Fixesgolang/go#65024
Change-Id: Ie5645b06657fc7bb729fce953b6d2ea8f3efe87d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/554717
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
CL 549415 (rightly) changed the logic for View shutdown to not await all
work on the Snapshot, as this leads to potential deadlocks: we should
never await work while holding a mutex. However, we still need to await
all work when shutting down the Session, otherwise we end up with
failures like golang/go#64971 ("directory not empty"). Therefore, we
need a new synchronization mechanism.
Introduce a sync.WaitGroup on the Session to allow awaiting the
destruction of all Snapshots created on behalf of the Session. In order
to support this, View invalidation becomes a method on the Session,
rather than the View, and requires the Session.viewMu.
Also make a few unrelated cosmetic improvements.
Fixesgolang/go#64971
Change-Id: I43fc0b5ff8a7762887fbfd64df7596e524383279
Reviewed-on: https://go-review.googlesource.com/c/tools/+/554996
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
variferr)
Go require explicit error handle, you must check and return error
after some function call. These postfix completion can help to write
the check error code.
The postfix completion iferr replace
strconv.Atoi("32").iferr
to
if _, err := strconv.Atoi("32"); err != nil {
return zero1, zero2, err
}
The postfix completion variferr replace
strconv.Atoi("32").variferr
to
value, err := strconv.Atoi("32");
if err != nil {
return zero1, zero2, err
}
The "zero1", "zero2" represent the zero value of enclosed function
results.
Fixesgolang/go#64178
Change-Id: I8313168514bfdfd22ad03b0228d4ca738ba9e9e3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/550455
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Marker tests involve a lot of Go command invocations, which themselves
involve a lot of I/O. This seems to make them remarkably slower on
legacy darwin builders. Skip if -short for now, to mitigate flakiness.
Per chat with the release team, LUCI darwin builders are significantly
faster and more stable. Since we only have to support the legacy
builders for a bit longer, a skip seems warranted.
Fixesgolang/go#64473
Change-Id: I9c6d6379017943faee4c02eef32768a6f2af6551
Reviewed-on: https://go-review.googlesource.com/c/tools/+/554716
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
These uses of the unnecessarily indirect v.snapshot.view were missed.
Change-Id: I00d464c2fbbb1a73f95fe9f47773b6879a02c56b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/554058
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The critical error logic was hard to follow, and ill defined because it
was constructed in two places: once during initialization, and another
time in Snapshot.CriticalError.
CriticalError is now rebranded as an InitializationError, and
constructed only during snapshot initialization. It covers a load error,
and an unparsable go.work or go.mod file. Critical errors are applied
to orphaned files.
For golang/go#57979
Change-Id: Ib3cdf602954202be0c87594c26dbbd0ff7e6458a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/553097
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
With zero-config gopls, we no longer need to scan for modules when
defining the default view for a folder. If there is no go.mod or go.work
file in a parent directory, just use an ad-hoc view until the first file
is opened.
Delete tests that were explicitly testing the view narrowing logic, and
so no longer make sense.
For golang/go#57979
Change-Id: Ib2ff96068b2e17d652f24d5ec05e1f2335a7f222
Reviewed-on: https://go-review.googlesource.com/c/tools/+/553096
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The validBuildConfiguration helper never had a well-defined meaning, and
now just means that the view is an ad-hoc view. Delete it and check the
view type directly.
Also, revisit the log message formatting view information.
For golang/go#57979
Change-Id: Ia09f697dd96c1930f1c97c74f08a81698ad17f30
Reviewed-on: https://go-review.googlesource.com/c/tools/+/553095
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Offer a snippet completing a return statement with return values.
Each return value is wrapped in a snippet placeholder, with the
corresponding zero value as default.
The snippet is only offered inside functions that have return parameters.
It is ranked below a plain return keyword.
Closesgolang/go#64266
Change-Id: Ifd7cd83f57e8d60ed5c45c2ff049378670473b7d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/546775
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This CL leverages the new zero-config implementation to add support for
dynamic build tags (golang/go#29202). While this CL is large, the gist
of the change is relatively simple:
- in bestView, check that the view GOOS/GOARCH actually matches the
file
- in defineView, loop through supported ports to find one that matches
the file, and apply the necessary GOOS= GOARCH= env overlay
- detect that views must be re-selected whenever a build constraint
changes
Everything else in the CL is supporting / refactoring around this minor
adjustment to view selection. Notably, the logic to check whether a file
matches a port (using go/build) required some care, because the go/build
API is cumbersome and not particularly efficient. We therefore check
ports as little as possible, and trim the file content that is passed
into build.Context.MatchFile. Earlier attempts at this change were
simpler, because they simply matched all available ports all the time,
but this had significant cost (around a millisecond overhead added to
every operation, including change processing).
However, the good news is that with the logic as it is, I believe it is
safe to support all available ports, because we only loop through this
list when checking views, an infrequent operation.
For golang/go#57979
For golang/go#29202
Change-Id: Ib654e18038dda74164b57d51b2d5274f91a1306d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/551897
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Run-TryBot: Robert Findley <rfindley@google.com>
This change simplifies the interface to "suggested fix"
functions so that the general case and single-file case
are more alike.
The argument types have changed thus:
(file.Handle, protocol.Range) ->
(pkg *cache.Package, pgf *parsego.File, start, end token.Pos)
and the results thus:
[]protocol.TextDocumentEdit -> (*token.FileSet, *analysis.SuggestedFix)
The extra FileSet result is needed because Pos values in
the SuggestedFix are not necessarily mapped by pkg.FileSet().
The logic of the singleFile wrapper moved into ApplyFix,
so it is now trivial, but unfortunately we can't get rid
of it yet because it breaks an import cycle from the
analyzer packages to cache. We first need to stop using
analyzers for quick fixes.
Also:
- rename suggestedFixFunc -> Fixer
- add various missing comments.
- merge the two parts of stubMethodsFixer.
(This was preparatory work while trying to understand
the associated issue.)
Updates golang/vscode-go#3101
Change-Id: I4602f7e22730a9037707e13f08b52d5cb7c714d6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/552735
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
This change renames beginFileRequest to fileOf,
removes its 'ok' result, and reorders (snapshot, fh).
Previously, its postcondition was complex because of two
kinds of error result, plus a cleanup function to be called
only after one kind of success.
Now, all the file.kind=Go checks are done explicitly
after beginFileRequest returns.
This uncovered a number of latent bugs:
- We return (nil, nil), an empty result, in a number of
places where we should really return an error
(unsupported/unimplemented operation for this file type).
Most of these behavior changes have been left for a
follow-up due to the risk of breaking something.
Only Rename/PrepareRename seem clear-cut, as does
SelectionRange, which immediately invoked ParseGo.
- The snapshot release was called prematurely for
an async command.
- template.Highlight was unreachable!
It must have no tests.
Change-Id: I3fda3763dda951f19d8425eaf2bfee70b27061f6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/552175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Add a custom command to check the set of Views from integration tests,
and use this to update TestAddAndRemoveGoWork.
For golang/go#57979
Change-Id: Ib1215086d0f7236985a44b6ab851c955d98e9440
Reviewed-on: https://go-review.googlesource.com/c/tools/+/552355
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
It looks like zero-config gopls slowed down the marker tests
significantly on darwin, most likely due to the go command being invoked
to select views whenever a file is opened (and the marker tests open all
the files!). Darwin has significantly higher overhead for Go command
invocations, particularly those that read from the filesystem.
With the exception of GOWORK, the Go env required to define views
depends only on the workspace folder, not any nested directory.
Therefore, to fix this regression we can request this environment just
once, when folders are added, or when their configuration changes, and
capture the GOWORK value from the os environment for later
interpretation.
For golang/go#57979
Change-Id: I482b05b406a41246e8618e9faf4c3d4f87b0db87
Reviewed-on: https://go-review.googlesource.com/c/tools/+/552315
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL finishes wiring in the zero-config gopls algorithm of
golang/go#57979, by using the new logic for updating the set of views.
Most of the heavy lifting was done in previous CLs, so the change here
is relatively trivial. Tests had to be adjusted to account for the lack
of diagnostics now that the zero-config logic allows module loading to
succeed in more cases.
For golang/go#57979
Change-Id: I98aeea8777a354f22c952875b0e655c535d4b995
Reviewed-on: https://go-review.googlesource.com/c/tools/+/551896
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Preserving sequence IDs may be convenient for debugging, but is no
longer necessary for correctness, and is an invariant that is tricky to
preserve in zero-config gopls. Remove this feature.
For golang/go#57979
Change-Id: I64b529d16e05216c651e3035564a2f93f82b2db4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/551895
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL is the first (and larger) half of integrating the zero-config
logic with gopls, switching to the new bestViewForURI logic. The next CL
will integrate the selectViews logic for building the view set.
As a result of the new logic, bestViewForURI provides a static
association of workspace files with views that depends only on the
environment and nearest go.mod file. Notably, this logic no longer
depends on the set of 'known files', which is a source of
path-dependence and bugs (golang/go#57558). Particularly with a dynamic
set of views, as with zero-config gopls, depending on the historical
observation of files for View association leads to confusing bugs, which
disappear when gopls is restarted.
With knownFiles gone, the logic of DidModifyFiles can be simplified
significantly. We no longer need to avoid invalidating files in
snapshots: every View can observe every change.
But notably, bestViewForURI may return nil if no View can be
'statically' determined to own a file (i.e. without loading package
data). And we can't wait for packages.Load inside DidModifyFiles, which
holds the big View lock. So how do we determine which View to use when
operating on a file that has no best view (e.g. a file outside the
workspace)? This CL provides one solution, which is to make ViewOf wait
for package loading before determining the best View. This may
theoretically make operating on a non-workspace file slower, since we
can't make the faster file= query in go/packages (since we don't know
which View to use), but in the common case all metadata is already
loaded, so this should be fine. If no View has a package containing the
file, ViewOf returns views[0] by convention, so that we have some place
to put the command-line-arguments package resulting from loading the
file. This means that while we do not yet do better for these orphaned
files, we at least do no worse.
This CL also rewrites the logic for tracking Views that need to be
diagnosed, which also previously relied on knownFiles. Just as with
ViewOf, the new logic relies on a combination of bestViewForURI and
looking at package data. Specifically, it reports when package data or
other diagnostic inputs are invalidated in a View, and keeps track of a
set of Views needing diagnosis. See the documentation of
server.viewsToDiagnose for more details.
Finally, this CL rewrites orphaned file diagnostics. It never really
made sense for individual Snapshots to produce orphaned file
diagnostics, since a file is not orphaned relative to a Snapshot, but
rather relative to the Session. OrphanedFileDiagnostics is moved to the
Session, and tracked by a new field on fileDiagnostics that is
independent of any view.
These changes to diagnostics required a new monotonic clock for tracking
diagnostics passes, implemented on the server.
Fixesgolang/go#57558
For golang/go#57979
Change-Id: I3170babd9d82065e562c18bd33b4a20a9e2b5f52
Reviewed-on: https://go-review.googlesource.com/c/tools/+/551295
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Update the zero-config algorithm to define views for modules that are
unused by the relevant go.work file. In these cases, load the module
independently via GOWORK=off.
For golang/go#57979
Change-Id: I7b126c41694131660a91380340fa9ceeeeef948d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/550915
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Completing a use directive for a nonexistent directory crashed gopls.
Fix this, and switch the operation to use the more performant
filepath.WalkDir.
Fixesgolang/go#64225
Change-Id: I11c463d2a19bfeefce552b6efdcc9c5892a722f9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/550376
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Add an implementation of the new zero-config workspace algorithm, by way
of new logic for bestViewForURI, a forURI argument for getViewDefinition
(renamed to defineView), and a new selectViews algorithm.
For now, this logic is only exercised in a new unit test. Subsequent CLs
will refine this logic, and wire it in to gopls. Once this is wired in a
lot of tests will need to change, so it is probably simpler to first
review the logic in isolation, as is done here.
For golang/go#57979
Change-Id: I150ba87ac9e51119f35e095b4c41984b5c4f9b47
Reviewed-on: https://go-review.googlesource.com/c/tools/+/550815
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Simplify cache.viewDefinition, by eliminating unnecessary types (goEnv
and go111module), simplifying comparison, and by expressing more logic
in terms of the ViewType.
Also make some minor cleanup in related code.
For golang/go#57979
Change-Id: I4f24e36fd9dea3f84dfca6f528cad4fa560483d1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/550378
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This CL rewrites various logic related to the workspace scope in terms
of View root and ViewType, as defined by zero config gopls
(golang/go#57979).
Specifically:
- viewDefinition.goCommandDir is redefined as viewDefinition.root, and
no longer has any relationship to the 'expandWorkspaceToModule'
setting. For all view types cases the view root is a logical root,
and it is OK to run the Go command from this root. The previous
semantics existed to indirectly implement 'expandWorkspaceToModule'.
- expandWorkspaceToModule is reinterpreted in the workspace package
algorithm. This is essentially equivalent to the previous behavior
(though that is hard to see), since view.contains used the union of
workspace folder and 'goCommandDir'.
- FileWatchingGlobPatterns is rewritten, based on recent updates to VS
Code and feedback from golang/go#63536. Notably, in module mode we
need only watch active modules, which may have a significant impact
on performance. More work is required to verify the new behavior,
particularly on windows, but I believe the current approach is at
least principled.
- Fix windows file patterns to be '/'-separated, which is required by
the spec.
Updates golang/go#63536
Updates golang/go#57979Fixesgolang/go#63742
Change-Id: I07ac703d61467eed8f101cb123469591a58aa5a4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/550075
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
CL 548175 removed the sergey/diffmatchpatch diff implementation,
which had been disabled by default for some time.
This change removes the dependency from gopls to the myers
implementation, and the associated configuration plumbing
for alternative implementations. The myers implementation
was not actually used because the GoDiff=true default
in Hooks (see default.go) was always overridden.
Unfortunately we cannot yet delete the myers implementation
completely because the marker tests depend on the details
of its behavior. A follow-up change should apply the diff
and compare the result, instead of comparing diffs.
Fixesgolang/go#52967
Change-Id: I67796e260ac00f7edc31ce18fda7b1042e8374f8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/548856
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TestInconsistentVendoring is flaking on Windows with the not-so-helpful
"exit status 1". Include stderr in the error message to help debug.
For golang/go#64229
Change-Id: Iadb946e799e866af683ea9ea7fc039f34b45b1ba
Reviewed-on: https://go-review.googlesource.com/c/tools/+/550375
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
For zero-config gopls, we need diagnostics to work seamlessly across
multiple views. The existing diagnostic logic is already complicated, so
rewrite it to significantly simplify and allow for merging diagnostics
from multiple views. Rather than using a global snapshot ID as a counter
that enforces freshness, use the set of views and their snapshot
sequence IDs.
Replace the store->publish model with a simple update model, so that we
store and publish in one atomic operation. I believe this avoids most of
the races that the older more complicated schema was designed around.
For simplicity, delete the formatting of diagnostics in debug templates.
I do not think they provide any value, and so there is no point in
maintaining them.
Note that this reverts CL 546855, rolling forward CL 546415, As the
deadlock was fixed in CL 549415.
Change-Id: Ia5b63c4ee87ec29fab334badd81989d0f0f260b5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/549895
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
CL 546415 inadvertently introduced deadlock, because the Session called
View.shutdown while holding the view lock, which in turn called
Snapshot.destroy, which waited on Snapshot.refcount. Meanwhile, the new
diagnostic logic checks Session.Views while leasing a Snapshot.
This is all too complicated, but what principle is violated? I suggest
the following: holding a Snapshot should not block any critical section.
The lifetime of a goroutine leasing a Snapshot does not have any
relationship to the lifetime of that Snapshot: cancelling the snapshot
does not cause the goroutine to exit.
This CL removes the blocking in View.shutdown, by addressing a TODO to
instead destroy the snapshot's persistent data structures on its final
decref. With this change we do lose some diagnostic information about
Snapshot lifecycle bugs; if need be we can reinstate this debugging
information by way of more formal leasing semantics (for example, a
Lease[T] type that encapsulates the lifecycle of the lease, and checks
invariants via runtime.SetFinalizer).
Also address some minor staticcheck suggestions.
Change-Id: I94d873b3de7b38bb628526690fd8ae6509549fd1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/549415
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
The previous code would attempt to decode both of these
as JSON quick-fixes, which would fail. (At least, we assume
that was the cause of the attached field report.)
Fixesgolang/go#64503
Change-Id: Id705c6700d601ebf4720e1f749a1594244ffea9c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/549575
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Found by a mix of staticcheck and luck.
Also, fix a typo in a help message.
Change-Id: I82b42b20d24f6e98130a23af92de0428a94875b5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/549537
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Alan Donovan
Ho Cheung
racequite
rogeryk
Rob Findley
Peter Weinberger
Gopher Robot
vitalmotif
Hana (Hyang-Ah) Kim
Zxilly
Dmitri Shuralyov
cui fliter
Martin Asquino
Patrick Pichler
Fata Nugraha
Tim King
Suzy Mueller
qiulaidongfeng
Michael Stapelberg
Ariel Chenet
toad
Viktor Blomqvist