If a package is being operated on, getPackage returns that package information; otherwise nil.
Change-Id: I881056510b8d6862c274a7532fdfbc840c938468
Reviewed-on: https://go-review.googlesource.com/c/tools/+/418791
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Dylan Le <dungtuanle@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This enables some fill struct code actions for instances of structs with
type parameters.
This additionally adds a filtering mechanism to the suggested fixes in
order to account for multiple suggested fixes in the same location.
Change-Id: I98866b462b026f4c5a4897bc278f704381623f25
Reviewed-on: https://go-review.googlesource.com/c/tools/+/418415
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
token.File.LineStart panics if line < 1, but we were checking line < 0.
Surprising that this was not hit more often: it looks like we
pre-validate input except for parsed positions coming from the Go
command. It is possible that an older version of the Go command returned
invalid positions.
Also report a bug for one error condition in ToOffset: the position
returned by LineStart should always be valid.
Fixesgolang/go#54006
Change-Id: I5965af9c62976b3e00b023512df334a8de943a3d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419109
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
The new implementation is based on Myers' paper, and is in the
package diff/lcs.
There is a new option newDiff, that can be set to 'old', 'new',
or 'both'. The default is 'both', although that may not be
the right choice for a release. See gopls/hooks/diff.go.
'both' runs both the old and new diff algorithm and saves some
statistics in a file in os.Tempdir(),
When (or if) the new code becomes the default, this logging (and
some internal checking) will be removed.
The new implementation has internal checking, which currently
panics. The code in gopls/hooks/diff.go tries to save an encrypted
(for privacy) version of the failing input.
The package diff/myers has not been replaced, but it could be.
Fixesgolang/go#52966
Change-Id: Id38d76ed383c4330d9373580561765b5a2412587
Reviewed-on: https://go-review.googlesource.com/c/tools/+/396855
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Peter Weinberger <pjw@google.com>
The fundamental bug causing TestChangePackageName to fail has been
fixed, yet unskipping it revealed a new bug: tracking whether or not a
package should be loaded requires that we actually store that package in
s.meta. In cases where we drop metadata, we also lose the information
that a package path needs to be reloaded.
Fix this by significantly reworking the tracking of pending loads, to
simplify the code and separate the reloading logic from the logic of
tracking metadata. As a nice side-effect, this eliminates the needless
work necessary to mark/unmark packages as needing loading, since this is
no longer tracked by the immutable metadata graph.
Additionally, eliminate the "shouldLoad" guard inside of snapshot.load.
We should never ask for loads that we do not want, and the shouldLoad
guard either masks bugs or leads to bugs. For example, we would
repeatedly call load from reloadOrphanedFiles for files that are part of
a package that needs loading, because we skip loading the file scope.
Lift the responsibility for determining if we should load to the callers
of load.
Along the way, make a few additional minor improvements:
- simplify the code where possible
- leave TODOs for likely bugs or things that should be simplified in
the future
- reduce the overly granular locking in getOrLoadIDsForURI, which could
lead to strange races
- remove a stale comment for a test that is no longer flaky.
Updates golang/go#53878
Change-Id: I6d9084806f1fdebc43002c7cc75dc1b94f8514b9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417576
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Avoid panicking when allocating an array for a nil results list
by returning early.
Change-Id: I26953b5cef7832bad3006bd316d59978a5d94cbd
Reviewed-on: https://go-review.googlesource.com/c/tools/+/418416
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
In FilterDisallow, change filter to regex form to match with file paths. Add a unit regtest for FilterDisallow.
For golang/go#46438
Change-Id: I7de1986c1cb1b65844828fa618b72b1e6b76b5b9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414317
Run-TryBot: Dylan Le <dungtuanle@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Now that we await ongoing work during shutdown, we are seeing regtest
flakes simply due to outstanding go command invocations.
Allow more time for cleanup. If this is insufficient, we can be more
aggressive about terminating go command processes when context is
cancelled.
For golang/go#53820
Change-Id: I3df3c5510dae34cb14a6efeb02c2963a71e64f3a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417583
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dylan Le <dungtuanle@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
This should have been in CL 417116.
Also:
- (related to CL 417415), rename packageHandle.check to await
to indicate its blocking nature.
- rename typeCheck to typeCheckImpl, following the pattern.
- move "prefetch" parallel loop into typeCheckImpl.
- add some comments.
Change-Id: Iea2c8e1f1f74fb65afd0759b493509147d87a4bb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417581
Run-TryBot: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This analyzer suggests fixes for unused variable errors.
In declarations it will remove the whole statement if the offending
variable is the only one declared in that statement, otherwise it will
just delete the offending variable.
In assignments it will remove the whole statement if the offending
variable is the only one assigned in that statement, otherwise it will
rename the offending variable to `_`. If the assignment RHS contains a
statement that can cause a side effect (a function call or reading from
a channel), the assignment will be removed but RHS will be preserved.
Fixesgolang/go#48975
Change-Id: I3850f1b0340cd5ae63249931df3a5403d8617080
Reviewed-on: https://go-review.googlesource.com/c/tools/+/394934
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Also:
- add test of NewHandle
- update package doc and other doc comments
- factor Store.Handle with NewHandle
- declare Handle before Store
Change-Id: I4bcea2c9debf1e77f973ef7ea9dbe2fd7a373996
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417417
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This is used by internal/lsp/analysis/fillreturns and
internal/lsp/analysis/fillstruct. This doesn't need to be in this
analysisinternal package.
This removes the dependency path
go/analysis/internal/checker ->
internal/analysisinternal ->
internal/lsp/fuzzy
Change-Id: I5db674ca30eb06ae6ce7021397cf5530a695af4e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417418
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
The trimming optimization deletes parts of the syntax tree
that don't affect the type checking of package-level declarations.
It used to remove unexported struct fields, but this had
observable consequences: it would affect the offset of later
fields, and the size and aligment of structs, causing the
'fieldalignment' analyzer to report incorrect findings.
Also, it required a complex workaround in the UI element
for hovering over a type to account for the missing parts.
This change restores unexported fields.
The logic of recordFieldsUses has been inlined and specialized
for each case (params+results, struct fields, interface
methods) as they are more different than alike.
BenchmarkMemStats on k8s shows +4% HeapAlloc:
a lot, but a small part of the 32% saving of the trimming
optimization as a whole.
Also:
- trimAST: delete func bodies without visiting them.
- minor clarifications.
Updates golang/go#51016
Change-Id: Ifae15564a8fb86af3ea186af351a2a92eb9deb22
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415503
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
All Get/Set operations on the maps now happen within a single
function (buildPackageKey, actionHandle).
No behavior change.
Change-Id: I347dfda578c28657a28538e228ecfb6f0871b94b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417415
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
As with mod tidy in CL 417116, we can rely on invalidation
rather than cache keys to avoid reusing stale results, and
there's no need to save these handles in the global store.
Change-Id: I3763c01fa21c6114248c1d541e3c168fc6a128c9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417416
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Previously, the modtidy operation used a persistent map
of handles in the central store that cached the result
of a parsing the go.mod file after running 'go mod tidy'.
The key was complex, including the session, view, imports
of all dependencies, and the names of all unsaved overlays.
The fine-grained key prevented spurious cache hits for
invalid inputs by (we suspect) preventing nearly all cache hits.
The existing snapshot invalidation mechanism should be
sufficient to solve this problem, as the map entry is evicted
whenever the metadata or overlays change. So, this change
avoids keeping handles in the central store, so they are
never shared across views.
Also, modtidy exploited the fact that a packageHandle
used to include a copy of all the Go source files
of each package, to avoid having to read the files
itself. As a result it would entail lots of unnecessary
work building package handles and reading dependencies
when it has no business even thinking about type checking.
This change:
- extracts the logic to read Metadata.{GoFiles,CompiledGo}Files
so that it can be shared by modtidy and buildPackageHandle.
- packageHandle.imports has moved into mod_tidy.
One call (to compute the hash key) has gone away,
as have various other hashing operations.
- removes the packagesMap typed persistent.Map wrapper.
- analysis: check cache before calling buildPackageHandle.
- decouple Handle from Store so that unstored handles may
be used.
- adds various TODO comments for further simplification.
Change-Id: Ibdc086ca76d6483b094ef48aac5b1dd0cdd04973
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417116
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Before, these methods of the Source interface used to use
a cache of buildASTCache, which built a Pos-keyed map for
a whole file, but the necessary algorithm is essentially
a binary search which is plenty fast enough to avoid the
need for cache.
This change implements that algorithm and moves
both methods out of the interface into a single function,
source.FindDeclAndField.
--
I measured the duration of all calls to astCacheData (before)
and FindDeclAndField (after) occurring within this command:
$ go test -bench=TestBenchmarkConfiguredCompletion -v ./gopls/internal/regtest/bench -completion_workdir=$HOME/w/kubernetes -completion_file=../kubernetes/pkg/generated/openapi/zz_generated.openapi.go -completion_regexp=Get
(The numbers reported by this benchmark are problematic,
which is why I measured call times directly; see
https://github.com/golang/go/issues/53798.)
Results:
before (n=4727) max = 21ms, 90% = 4.4ms, median = 19us
after (n=6282) max = 2.3ms, 90% = 25us, median = 14us
The increased number of calls to the function after the change
is due to a longstanding bug in the benchmark: each iteration of
the b.N loop doesn't do a fixed amount of work, it does as much
as it can in 10s. Thus making the code faster simply causes
the benchmark to spend the same amount of time on other parts of
the program--such as the loop that calls FindDeclAndField.
See https://go-review.googlesource.com/c/tools/+/221021 for
background on the previous implementation.
Change-Id: I745ecc4e65378fbe97f456228cafba84105b7e49
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416880
Auto-Submit: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Cleaning the regtest sandbox sometimes fails on windows with errors that
may be spurious. Copy logic from the testing package to retry these
errors.
For golang/go#53819
Change-Id: I059fbb5e023af1cd52a5d231cd11a7c2ae72bc92
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417117
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
CL 409936 eliminated cases where we close over a Context during progress
reporting, except in one instance where it wasn't possible: the
WorkDoneWriter that must implement the io.Writer interface.
Unfortunately it contained a glaring bug that the ctx field was never
set, and the regression test for progress reporting during `go generate`
was disabled due to flakiness (golang/go#49901).
Incidentally, the fundamental problem that CL 409936 addressed may also
fix the flakiness of TestGenerateProgress.
Fix the bug, and re-enable the test.
Fixesgolang/go#53781
Change-Id: Ideb99a5525667e45d2e41fcc5078699ba1e0f1a3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/417115
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Configuration of LSP settings within the regression test runner had
become a bit of a grab-bag: some were configured via explicit fields on
EditorConfig, some via the catch-all EditorConfig.Settings field, and
others via custom RunOption implementations.
Consolidate these fields as follows:
- Add an EnvVars and Settings field, for configuring environment and
LSP settings.
- Eliminate the EditorConfig RunOption wrapper. RunOptions help build
the config.
- Remove RunOptions that just wrap a key-value settings pair. By
definition settings are user-facing and cannot change without
breaking compatibility. Therefore, our tests can and should set the
exact string keys they are using.
- Eliminate the unused SendPID option.
Also clean up some logic to change configuration.
For golang/go#39384
Change-Id: Id5d1614f139550cbc62db2bab1d1e1f545ad9393
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416876
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Add a new WaitGroup to the View that allows waiting for all snapshot
destroy operations to complete. This helps ensure that the server
properly cleans up resources when shutting down, and lets us remove
work-arounds in the gopls regtests intended to avoid races during
shutdown.
Also:
- re-enable postfix completion tests that had to be disabled due to
being too racy
- rename the inlayHints regtest package to follow lower-cased naming
conventions
- add several TODOs
Fixesgolang/go#50707Fixesgolang/go#53735
Change-Id: If216763fb7a32f487f6116459e3dc45f4c903b8a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416594
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
CL 416874 changed the logic of populateProcessEnv and incorrectly
removed a write of ProcessEnv.ModFlag. We should explicitly set
-mod=readonly.
Change-Id: Ibacf3d4b4c0c978d65fde345741945d6136db159
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416877
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
This reverts CL 416879, which itself was a revert of CL 416874.
Reason for revert: failure is understood now, as described in
https://github.com/golang/go/issues/53796#issuecomment-1181157704 and fixed in CL 416881.
Change-Id: I1d6a4ae46fbb1bf78e2f23656de7885b439f43fb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416882
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
We were already using a token.File-agnostic position for object
positions in our references algorithm, but still relied on token.Pos for
identifying duplicate references. This breaks down when a file may have
multiple parsed representations in different packages.
While we should endeavor to eliminate duplicate parsing, our algorithms
should not rely on this for correctness. Update the reference
de-duplication to use the same position key as object search.
For golang/go#53796
Change-Id: Ic2e6c23380ea4e6b2747e4e5b45d7bfa6e656f0f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416881
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Previously, snapshots were born with a reference count of zero.
Although leases were created and returned by functions that
return a snapshot, and correctly disposed of by the caller, the
view did not hold a lease for its snapshot. The view's lease
on the snapshot was implicit, and for this reason the view
made explicit calls to Destroy when these implicit leases ended,
which would then wait for all explicit leases to end.
Now that the view holds an explicit lease to its snapshot, it is
safe (as a follow-up change) to move the destroy logic into the
call of release() that brings the refcount to zero.
Also:
- clarify that release functions should be called when
and only when err is nil.
- in View: remove unused View.cancel field.
- in tempModFile: clarify tricky return + defer + named-result-var statement.
- in DidModifyFiles: simplify type of releases from []func() to func().
- in Server.addFolders: fix reference leak (looks minor).
Change-Id: Ibf61f4ce109e91060e2ccd854c32214b119f2f0a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416875
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Whenever a view.importsState value is created, we use
snapshot.goCommandInvocation to extract information used to run the go
command in the imports.ProcessEnv. Before this CL, that process acquired
the current snapshot (whatever it was), and held onto it for the purpose
of sharing a workspace directory. As a consequence all the memory in
that snapshot was pinned for the lifecycle of the importsState, which
can be the entire editing session. This results in a memory leak as
information in the session is invalidated.
Fix this by creating a copy of the workspace directory to be owned by
the importsState.
Also:
- Add some TODOs
- Clean up some stale comments
Fixesgolang/go#53780
Change-Id: I2c55cc26b2d46c9320c6c7cd86b3e24971cd5073
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416874
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
...now that I have understood why they are not
in fact inessential.
Change-Id: I1ab881a7d24cd71ee183bc8c6a1a058dbda641e2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416077
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Now that the lifetime of all handles in the store is
determined by reference counting, we no longer need
the generation feature.
The Arg interface, renamed RefCounted, is now optional,
and causes the lifetime of the argument to be extended
for the duration of the Function call. This is important
when the Get(ctx) context is cancelled, causing the
function call to outlive Get: if Get's reference to
the argument was borrowed, it needs to increase the
refcount to prevent premature destruction.
Also:
- add missing snapshot.release() call in
importsState.populateProcessEnv.
- remove the --memoize_panic_on_destroyed flag.
Change-Id: I0b3d37c16f8b3f550bb10120c066b628c3db244b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416076
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This change simplifes the ParseGo interface to
make it consistent with the other handle+map operations:
- ParseGoImpl is the basic parser.
- The 'fixed' bool result is a field of ParsedGoFile.
- ParseGo is the caching wrapper.
The map accessors have been inlined into it.
- goFiles (renamed parsedGoFiles) is now just a bare
persistent.Map.
- parseGoHandle is replaced by *memoize.Handle
- the operations of "make a handle" and "wait for it"
are no longer separate (since clients never want
one without the other).
- cachedPGF and peekOrParse have been combined into
peekParseGoLocked.
Change-Id: If01a6aaa7e6a8d78cb89c305e5279738e8e7bb55
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416223
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
If metadata is refreshed during the execution of buildPackageHandle, we
should not store the resulting package handle in the snapshot, as it
breaks the invariant that computed packages match the currently loaded
metadata.
This strictness revealed another bug: because of our fine-grained
locking in snapshot.load, it is possible that we set valid metadata
multiple times, leading to unnecessary invalidation and potential
further races to package handles. Fix this by building all metadata when
processing the go/packages result, and only filtering updates after
grabbing the lock.
For golang/go#53733
Change-Id: Ib63ae4fbd97d0d25d45fe04f9bcd835996db41da
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416224
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This change replaces the 5 remaining calls to Bind (generational
lifetime) with GetHandle (reference counting). The handles
are now stored in persistent.Maps, which simplifies the
invalidation logic.
All 5 have span.URIs as keys:
symbolizeHandles
parse{Mod,Work}Handles
mod{Tidy,Why}Handles
Also, factor the functions that use these maps to have a common form:
- a fooImpl function that returns an R result and an error;
- a foo wrapper that decorates it with caching.
- a local fooResult type, defined struct{R; error} that is the cache entry.
The functions for getting/setting map entries are all inlined.
The fooHandle types are all replaced by *memoize.Handle, now that
their use is local.
No behavior change is intended.
The other uses of Bind are deleted in these CLs:
https://go-review.googlesource.com/c/tools/+/415975 (astCacheData)
https://go-review.googlesource.com/c/tools/+/415504 (actions)
Change-Id: I77cc4e828936fe171152ca13a12f7a639299e9e5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415976
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Now that we have a token.File (albeit throwaway)
for a parsed go.mod file, we can combine the .go and go.mod
logic for turning it into a protocol.DocumentLink.
Change-Id: Id1783644cbd450f0e8dc807beb8ba625675d8540
Reviewed-on: https://go-review.googlesource.com/c/tools/+/410136
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
This change replaces Bind (generational lifetime) with GetHandle
(reference counting) for the cache of buildASTCache calls, as
Bind is deprecated.
Also:
- add missing commentary, particularly on the question of why
this cache is needed at all.
- remove unused field astCacheData.err
- simplify SignatureHelp to avoid unnecessary use of Declaration.
- minor simplifications to logic surrounding FindPackageFromPos
and PosTo{Decl,Field}.
Change-Id: I2b7a798b84f23856037797fa6e9ccc5595422e7c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415975
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This change uses a persistent.Map for actions, just like packages.
Actions are now reference counted rather than generational.
Also:
- note optimization opportunities.
- minor cleanups.
Change-Id: Ibbac8848a3beb3fe19056a7b160d2185155e7021
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415504
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
span.NewRange now accepts a *token.File and two token.Pos.
It is the caller's responsibility to look up the File
in the FileSet, if necessary (it usually isn't), and to
ensure the Pos values are valid. Ditto NewMappedRange.
This reduces the creep of Snapshot into functions that
have no need to know about it. Also the bug.Report call
in NewRange has been pushed up into the caller and
logically eliminated in all but one case.
I think we should aim for the invariant that functions that
operate on a single file should accept a *token.File, not
a FileSet; only functions that operate on sets of files
(e.g. type checking, analysis) should use a FileSet.
This is not always possible: some public functions
accept a FileSet only to re-lookup a single file already
known to the caller; if necessary we could provide token.File
variants of these.
This may ultimately allow us to create a new FileSet per
call to the parser, so that Files and FileSets are in
1:1 correspondance and there is no global FileSet.
(It currently grows without bound, on the order of kilobytes
per keystroke.) FileSets containing multiple files,
needed when we interact with the type checker and analysis,
may be temporarily synthesized on demand from a set of Files,
analogous to mmap'ing a few files into a blank address space.
Also:
- replace File.Position(pos).Line by File.Line(pos)
- replace pos == token.NoPos by pos.IsValid()
- avoid fishy token.Pos conversions in link.go
- other minor simplifications
Change-Id: Ia3119e0ac7e193801fbafa81c8f48acfa14e9ae4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/409935
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This speeds up workspace initialization time in DegradeClosed memory
mode from 3m to 1m by avoiding unnecessary recomputation of results.
Change-Id: Ie5df82952d50ab42125defd148136329f0d50a48
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413658
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: David Chase <drchase@google.com>
This on average reduces latency from 12ms to 4ms on internal codebase.
Updates golang/go#45686
Change-Id: Id376fcd97ce375210f2ad8b88e42f6ca283d29d3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413657
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
...so that each client doesn't have to.
Change-Id: I039c493031c5c90c4479741cf6f7572dad480808
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415502
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Before, buildPackageHandle and buildKey were mutually recursive.
Together they performed a sequential recursion over Metadata.Deps,
calling GetFile and parseGoHandle for every file, and then
finally (in postorder) binding a Handle for the type checking step.
This change inlines buildKey to make the recursion more obvious,
performs the recursion over dependencies first, followed by
the reading of Go source files for this package, in parallel.
(The IWL benchmark reports improvement but its variance is so
high I'm not sure I trust it.) Other opportunities for parallelism
are pointed out in new comments.
The Bind operation for typechecking calls dep.check for each
dependency in a separate goroutine. It no longer waits for
each one since it is only prefetching the information
that will be required during import processing, which will
block until the information becomes available.
Before, both reading and parsing appear to occur twice:
once in buildPackageHandle and once in doTypeCheck.
(Perhaps the second was a cache hits, but there's no need
to rely on a cache.)
Now, only file reading (GetFile) occurs in buildPackageHandle,
since that's all that's needed for the packageKey.
And parsing only occurs in doTypeCheck. The source.FileHandles
are plumbed through as parameters.
Also:
- move parseGoHandles to a local function, since it exists only
for buildPackageKey. It no longer parses, it only reads.
- lots of TODO comments for possible optimizations,
and typical measured times of various operations.
- remove obsolete comment re: Bind and finalizers.
Change-Id: Iad049884607b73eaa6701bdf7771f96b042142d5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411913
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Before, NewMapper accounts for 2.1% of bytes allocated in the
WorkspaceSymbols benchmark. This change causes the newline index
table to be allocated once instead of by appending.
The function now accounts for 0.55%.
Change-Id: I9172dd34ee2be9e7175e311d4a6518f1e6660a5f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415501
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
dynamicSymbolMatch is an allocation hotspot (9% of all bytes),
because it allocates a 3-element []string that quickly becomes
garbage. This change passes in an empty slice with spare capacity
allowing the same array to be reused throughout the matchFile loop.
BenchmarkSymbols on k8s shows -72% bytes, -88% allocs, -9% wall time.
Change-Id: Id20c7cd649874a212e4d4c5f1aa095277b044a5b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415500
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
We can use a property that split does a dfs search for the key before
doing an actual work. This allows us to do a low-cost early return if
there is no key to delete.
Change-Id: I6ed8068945f9f2dacc356d72b18afce04ec89a3c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413659
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Now that the workspace directory uses Snapshot.Destroy
to clean up (see https://go-review.googlesource.com/c/tools/+/414499)
there is no need for this feature.
Change-Id: Id5782273ce5030b4fb8f3b66a8d16a45a831ed91
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414500
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
This change causes (*snapshot).getWorkspaceDir to create a temporary
directory directly, rather than via the Store/Generation/Handle
mechanism. The work is done at most once per snapshot, and the
directory is deleted in Snapshot.Destroy.
This removes the last remaining use of Handle's cleanup mechanism,
which will be deleted in a follow-up.
Change-Id: I32f09a67846d9b5577cb8849b226427f86443303
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414499
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
In Go 1.18 types.AssignableTo() started reporting that an invalid type
is assignable to any interface. *types.PkgName (i.e. an import at the
top of the file) has an invalid type for its Type(), so we started
thinking all in scope imports were great candidates when the expected
type was an interface.
Fix by wrapping the AssignableTo (and AssertableTo) to explicitly
return false if either operand is invalid.
Updates golang/go#53595
Change-Id: Ie5a84b7f410ff5c73c6b7870e052bafaf3e21e99
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415595
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
This adds a snippet that applies to variables of type chan.
When used, it replaces `channel.range!` with the following snippet:
```
for e := range channel {
|
}
```
Where `|` indicates the location of the cursor.
Change-Id: I8b2f889b22b9f2c292041e5ca5f63c5d0ca98f11
GitHub-Last-Rev: 9cb894be80d0c5243a5e42779c3e96ba79aa66b5
GitHub-Pull-Request: golang/tools#386
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414194
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
This allows reducing critical section of `g.store.mu` as the vast
majority of entries do not rely on generation-based GC.
Change-Id: I985af0b38504ddedb22649290deac91797577b75
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413656
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
This on average reduces latency from 25ms to 12ms on internal codebase.
Updates golang/go#45686
Change-Id: I49c8f09f8e54b7b486d7ff7eb8f4ba9f0d90b278
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413655
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This function was actually left behind, after making suggested changes
to CL 413683.
Change-Id: I6933e870ded9da5af06724c28839c37d58fb4cdc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414856
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Add text edits that a user can accept to make the unkeyed composite
literals keyed from the inlay hints. The text edits modify all of
the unkeyed fields in a composite literal, since a mixture of keyed
and unkeyed fields are not allowed.
Change-Id: I0683fbaa5e22bc004b91c98fc09e495e797826ee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414855
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Our symbol query only searches Go files, so there is no point to
building (broken) symbol information for non-Go files. Doing so
introduces a lot more symbol handles that need to be tracked and walked.
Change-Id: I96dd62766d079805fcb1d16eb361adfc0c31eea1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415199
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This change should have been included in
https://go-review.googlesource.com/c/tools/+/415057
but I hastily submitted it without a CI run
thinking "how can a doc only change break something?".
Well now I know. Sorry. :(
Change-Id: Ib0fd25fddd7f9580961b44dcad032d4851684f63
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415058
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
This is an optimization to avoid calculating inlayhints that are
not in the requested range.
Change-Id: I311f297d2998ae7d0db822eac540b1c12cae6e23
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412455
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Add some additional logging to help debug golang/go#53586
For golang/go#53586
Change-Id: I0574fb01be47b265cd6e412855794bc2cb836dff
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414854
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Add regtests for inlay hints to verify we can turn on
and off different inlay hints.
Change-Id: Id88450c40c048b6c2544d22a0d3eadb57b70a723
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411911
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Unlike generational handles, when reference counted handles are evicted
from the Store we don't know that they are also no longer in use by
active goroutines. Destroying them causes goroutine leaks.
Also fix a data race because Handle.mu was not acquired in the release
func returned by GetHandle.
Change-Id: Ida7bb6961a035dd24ef8566c7e4faa6890296b5b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/414455
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
When setting new metadata for a package ID, we invalidate the
corresponding type-checked packages. Whenever we invalidate a package,
we must also invalidate its reverse transitive closure. Otherwise we may
end up in a scenario where the go/types import graph does not match
gopls' view of the import graph.
Change-Id: I8db6ff3e4a722656e6dde7907e7c0470375c4847
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413683
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
If gopls believes it has valid metadata for a package, don't set new
metadata. This is consistent with previous behavior that was changed in
CL 340851.
In principle this shouldn't matter, but in practice there are places
where gopls doesn't yet want to invalidate packages, *even though* their
metadata may have changed (such as while editing a go.mod file before
saving). In the future we should eliminate these places, but for now we
should let snapshot.clone control this invalidation.
This also reduces the number of type-checked packages we invalidate on
load.
Change-Id: I0cc9bd4186245bec401332198de0047ff37e7ec7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413681
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Previously, gopls would fall back on a gopath resolver when running
goimports from a directory containing go.work (but not go.mod). Fix this
by update the code to recognize that GOWORK also puts goimports into
module mode.
All the work to _support_ go.work had already been done, but the tests
were only passing because they were setting GO111MODULE=on explicitly
(and therefore GOMOD=/dev/null was satisfying the pre-existing check).
Also add a test for the regression in gopls.
Fixesgolang/go#52784
Change-Id: I31df6f71a949a5668e8dc001b3ee25ad26f2f927
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413689
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
To properly format these field names in the vscode config ui
these fields should be camel case.
Change-Id: I3b8b8fb6371172ecb464710f7d91b9fc67e0ed42
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413684
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
The final LSP spec for 3.17 changed the name of ViewPort to Range
for both InlayHints and InlineValues. This manually updates just
these fields in our protocol.
Change-Id: I0303a36536016ca59c87dc45f55fadcd80e72bfc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413677
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
This on average reduces latency from 34ms to 25ms on internal codebase.
Updates golang/go#45686
Change-Id: I57b05e5679620d8481b1f1a051645cf1cc00aca5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413654
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Use treap (https://en.wikipedia.org/wiki/Treap) as a persistent map to avoid copying s.goFiles across generations.
Maintain an additional s.parseKeysByURIMap to avoid scanning s.goFiles on individual file's content invalidation.
This on average reduces didChange latency on internal codebase from 160ms to 150ms.
In a followup the same approach can be used to avoid copying s.files, s.packages, and s.knownSubdirs.
Updates golang/go#45686
Change-Id: Ic4a9b3c8fb2b66256f224adf9896ddcaaa6865b1
GitHub-Last-Rev: 0abd2570ae9b20ea7126ff31bee69aa0dc3f40aa
GitHub-Pull-Request: golang/tools#382
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411554
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change adds user settings for enabling inlay hints, modeled
roughly after analyzers. This will allow users to turn on specific
inlay hints that they like and leave others off.
With all of the inlay hints turned off by default, we can now enable
inlay hints.
Change-Id: Ie5dfcbbab1e0b7312eafcc4aa08cb4fe8a83fc31
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411906
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Package metadata is small; there is no reason not to keep it around, and
pruning it on every clone is needless work.
Change-Id: I9ea73315cc6b673625f0f7defe1fd61c2e1eb123
Reviewed-on: https://go-review.googlesource.com/c/tools/+/373695
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
When using Go workspaces, the go.work file should be used to determine
which packages are workspace packages.
For golang/go#48929
Change-Id: I1a8753ab7887daf193e093fca5070b4cc250a245
Reviewed-on: https://go-review.googlesource.com/c/tools/+/400822
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
This change adds a fast-path check for the common case:
"file:", lowercase, followed by a simple POSIX absolute
file name without special characters.
This function used to account for 1% of CPU on the DidChange
benchmark (and I'm sure I've seen higher fractions on other
tests--but perhaps that was before the clone optimizations?).
It was tested by adding an assertion that it agrees with the
slow path and running all our tests.
Change-Id: I15492b8a317715468870b00041bf8f6b0bb53bb2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411900
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Minor cleanups based on studying the code in preparation
for saving a persistent index:
- Remove unused error result from Symbols method.
- Remove unnecessary fields from symbolHandle.
- Add various explanatory comments.
- In workspace_symbols.go:
- separate extract and match phases of collectSymbols clearly
- replace symbolCollector and matchWorker types by simple parameters
- combine loops (roots, buildMatcher)
- move buildMatcher creation down to where it is needed.
Change-Id: Ifcad61a9a8c7d70f573024bcfa76d476552ee428
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412822
Reviewed-by: Robert Findley <rfindley@google.com>
Update References to detect if the package is referenced and a regtest to test within and external package references.
Updates golang/go#41567
Change-Id: I607a47bf15f1c9f8236336f795fcef081db49d6a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/408714
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Dylan Le <dungtuanle@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
The tooltip for truncated inlay hint labels is redundant
with the hover state of the target identifier. This matches
the behavior of inlay hint implementations in other languages.
Change-Id: I209054f8c65df504cae67121e3cbc3eacaf02710
Reviewed-on: https://go-review.googlesource.com/c/tools/+/413417
Run-TryBot: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This will show inferred type information for generic function
call expressions.
Example:
SumNumbers<[string, int64]>(ints)
For golang/go#52343
Change-Id: I05595f236626e8fb3666af5160611e074e8265a4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412994
Reviewed-by: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Jamal Carvalho <jamal@golang.org>
This change reduces the sizes of the critical section
in traces.ProcessEvent, in particular moving allocations
ahead of Lock. This reduces the contention according
to the trace profiler.
See https://go-review.googlesource.com/c/go/+/411909 for
another reduction in contention. The largest remaining
contention is Handle.Get, which thousands of goroutines
wait for because we initiate typechecking top down.
(Second attempt at https://go-review.googlesource.com/c/tools/+/411910,
reverted in https://go-review.googlesource.com/c/tools/+/412694.
The changes to Generation.Bind have been dropped.)
Change-Id: Ia9050c97bd12d2d75055f8d1dfcda3ef1f5ad334
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412820
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Add inlay hints for composite literal types. This will show type
information for composite literals with no explicit types.
Example:
<struct {in, want string}>{"hello", "goodbye"}
For golang/go#52343
Change-Id: Ia1f03b82669387c864353b8033940759fa1128e7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411905
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change parallelizes the buildSymbolHandle().Get computation for
each file, with 2xGOMAXPROCS goroutines, since it is a mix of I/O (read)
and CPU (parse). (The symbolize AST walk happens in other goroutines.)
This reduces the time for the source.WorkspaceSymbols trace task
applied to kubernetes from 3981ms to 630ms (6x faster).
Change-Id: I5f1ee4afc2f6b2dd752791a30d33a21f50180a9c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412818
Reviewed-by: Robert Findley <rfindley@google.com>
The region name includes the type of the key, such as packageHandleKey
or actionHandleKey, so we can separate these deferred computations
into their own buckets.
Change-Id: I0359127ccf47b158f353fae2bf74ba000668a40b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412817
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Now that the entire metadata graph and workspace packages are derived
from metadata, there should be no need to validate coherency.
This results in a small improvement to didChange benchmarking (within
statistical noise).
For golang/go#45686
Change-Id: I32683e025f42d768d62864683e55d4c00146a31c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340855
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Since ids is derived from metadata, we should not have to walk ids to
see which metadata is still active. Just compute metadata updates
directly.
Benchmark (didChange in k8s): ~45ms->41ms
For golang/go#45686
Change-Id: Id557ed3f2e05c903e4bb3f3f6a4af864751c4546
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340854
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
We with immutable metadata, we don't need to clone if nothing was
invalidated.
Benchmark (didChange in k8s): 65ms->45ms
For golang/go#45686
Change-Id: I6b5e764c53a35784fd8c7b43bc26361f4ee8d928
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340853
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The Metadata type was mutated in exactly one place: when setting the
ShouldLoad bit after package loading completes. Address this by cloning
the metadata graph when clearing ShouldLoad. After this change, metadata
graphs and the data within them are immutable.
This also fixes a range-variable capture bug in load.go: previously we
were deferring a call to clearShouldLoad for the range variable scope.
After this change, we properly clear the ShouldLoad bit for all scopes.
Change-Id: I8f9140a490f81fbabacfc9e0102d9c638c7fbb37
Reviewed-on: https://go-review.googlesource.com/c/tools/+/400821
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Rather than updating metadata directly in snapshot.clone, build a set of
updates to apply and call metadata.Clone.
After this change, metadata is only updated by cloning, so we can
eliminate some code that works with mutable metadata.
In the next CL we'll only update the metadata if something changed, but
this is intentionally left out of this CL to isolate the change.
Benchmark (didChange in kubernetes): ~55ms->65ms, because it is now more
work to compute uris.
For golang/go#45686
Change-Id: I048bed65760b266a209f67111c57fae29bd3e6f0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340852
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Introduce a metadataGraph.Clone method that can be used to clone a
metadata graph, applying a set of updates. During clone, ids and imports
are recomputed from scratch based on the known metadata.
Also refine the check for "real" packages when determining whether a
command-line-arguments package should be kept as a workspace package: if
all other packages are invalid, but the command-line-arguments package
is valid, we should keep the command-line-arguments package.
Updates golang/go#45686
Change-Id: Iea8d4f19c1d1c5a2b0582b9dda5f9143482a34af
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340851
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Now that we preserve stale metadata, we can derive workspace packages
entirely from known metadata and files. This consolidates the logic to
compute workspace packages into a single location, which can be invoked
whenever metadata changes (via load or invalidation in clone).
Additionally:
- Precompute 'HasWorkspaceFiles' when loading metadata. This value
should never change for a given Metadata, and our view.contains func
is actually quite slow due to evaluating symlinks.
- Track 'PkgFilesChanged' on KnownMetadata, since we don't include
packages whose package name has changed in our workspace.
Also introduce a few debug helpers, so that we can leave some
instrumentation in critical functions.
For golang/go#45686
Change-Id: I2c994a1e8ca05c3c42f67bd2f4519bea5095c54c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340735
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This reverts commit 654a14b527.
Reason for revert: my flawed understanding of the concurrency
Change-Id: I31a35267323bb1ff4dff1d9244d3ce69c36cdda4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412694
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This info is still useful to tell users that some required modules
have known vulnerabilities, but the analyzed packages/workspaces
are not affected.
Those vulnerabilities are missing Symbol/PkgPath/CallStacks.
Change-Id: I94ea0d8f9ebcb1270e05f055caff2a18ebacd034
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412457
Reviewed-by: Jonathan Amsterdam <jba@google.com>
This change reduces the sizes of the critical sections
in traces.ProcessEvent and Generation.Bind, in particular
moving allocations ahead of Lock. This reduces the contention
according to the trace profiler.
See https://go-review.googlesource.com/c/go/+/411909 for
another reduction in contention. The largest remaining
contention is Handle.Get, which thousands of goroutines
wait for because we initiate typechecking top down.
Also, add a couple of possible optimization TODO comments,
and delete a stale comment re: Bind.
Change-Id: I995a0bb46e8c9bf0c23492fb62b56f4539bc32f8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411910
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Known subdirs change rarely and it's quite expensive to compute
a glob pattern derived from them, so cache computation result
and inherit it across generations.
Computation cost is divided ≈evenly between `sort.Sort` and
`span.URI.Filename` calls, and there is no trivial way to optimize
them away besides caching.
Benchmark (didChange in kubernetes): ~37ms->30ms
Change-Id: Idb1691c76b8ff163dc61f637f07229498888606c
GitHub-Last-Rev: cd99a9ce5c797afb5aaa9b478fcf433edd0dc03c
GitHub-Pull-Request: golang/tools#383
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411636
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
The change to implement inlay hints has been merged, so we need to
enable the tests.
Change-Id: I47e7ab343d0ab10283caac0a3d6677dd69c7504a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411898
Run-TryBot: Suzy Mueller <suzmue@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
I had hoped to see a reduction in total allocation, but it does not
appear to be significant according to the included crude benchmark.
Nonetheless this is a slight code clarity improvement.
Change-Id: I94a503b377dd1146eb371ff11222a351cb5a43b7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411655
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
The marker tests are updated to include padding values
when mapping inlay hints to text edits.
Change-Id: Ieb421088238c65b07abdad12763816d3d1e757c8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411654
Run-TryBot: Jamal Carvalho <jamal@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
This change implements support for textDocument/inlayHint and
adds inlay hints for parameter names.
For golang/go#52343.
For golang/vscode-go#1631.
Change-Id: I3f989838b86cef4fd2b4076cb6340010fff7c24c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411094
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
Run-TryBot: Jamal Carvalho <jamal@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change replaces the single large map used for snapshot.goFiles
by a map of 256 stripes, each of which becomes immutable once shared.
This optimizes the common case in which the copy is nearly identical
to the original.
We still need to visit each map entry to see whether it needs to be
deleted (which is rare) and to inherit the handle in the usual case.
This is now done concurrently.
Also, share the (logically immutable) []PackageIDs slices across
old and new snapshots. This was worth 5% of CPU and 1/3 of allocations
(all small).
Benchmark on darwin/arm64 shows a 29% reduction for DidChange.
$ go test -v ./gopls/internal/regtest/bench -run=TestBenchmarkDidChange -didchange_dir=$HOME/w/kubernetes -didchange_file=pkg/util/hash/hash.go
Before:
BenchmarkStatistics 100 22955469 ns/op 11308095 B/op 47412 allocs/op
BenchmarkStatistics 100 23454630 ns/op 11226742 B/op 46882 allocs/op
BenchmarkStatistics 100 23618532 ns/op 11258619 B/op 47068 allocs/op
After goFilesMap:
BenchmarkStatistics 100 16643972 ns/op 8770787 B/op 46238 allocs/op
BenchmarkStatistics 100 17805864 ns/op 8862926 B/op 46762 allocs/op
BenchmarkStatistics 100 18618255 ns/op 9308864 B/op 49776 allocs/op
After goFilesMap and ids sharing:
BenchmarkStatistics 100 16703623 ns/op 8772626 B/op 33812 allocs/op
BenchmarkStatistics 100 16927378 ns/op 8529491 B/op 32328 allocs/op
BenchmarkStatistics 100 16632762 ns/op 8557533 B/op 32497 allocs/op
Also:
- Add comments documenting findings of profiling.
- preallocate slice for knownSubdirs.
- remove unwanted loop over slice in Generation.Inherit
Updates golang/go#45686
Change-Id: Id953699191b8404cf36ba3a7ab9cd78b1d19c0a2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/410176
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
When test awaiting fails, we often fail to shut down the server because
the pipe is closed. Fix this by using a detached context for running the
connection.
Also clean up some unnecessary context arguments.
Change-Id: I535c1cc1606e44df5f8e2177c92293d57836f992
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340850
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Set up the tests for inlay hints. We test inlay hints by converting them to text edits
and verifying the output is as we expected it.
This change does not yet deal with making sure the server
settings are correct.
Change-Id: I136f971a87bf9936fd44047d45fe0a3f03c9164e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411095
Run-TryBot: Suzy Mueller <suzmue@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
It is inconsistent to only compute IsIntermediateTestVariant for
workspace packages, and could be a source of bugs. Always compute it.
Change-Id: I16f40fe44a1145a74ef77fee4b7fd813abe603cb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340732
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
In preparation for making metadata immutable, move metadata-related
fields to a new MetadataGraph type. Other than instantiating this type
when cloning, this CL contains no functional changes.
For golang/go#45686
Change-Id: I7ad29d1f331ba7e53dad3f012ad7ecdae4f7d4b7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/340730
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
When using go.work, the go command packs module-specific errors into
synthetic results corresponding to the module query ("modulepath/...").
Extract these for use in diagnostics, by packing them into a new
moduleErrorMap error type.
Fixesgolang/go#50862
Change-Id: I68bf9cf4e9ebf4595acdc4da0347ed97171d637f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/405259
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
This reverts commit 5ca4cc8b9a.
Reason for revert: appears insufficient to land CL 410955.
Change-Id: If7029c9a43568e210e89d473b738400f5155715e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/410995
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
1. Avoid unnecessary intermediate map updates.
2. Avoid accumulating defers in a loop when the control is simple.
Yield: -10% CPU, -37% allocs.
Typical results:
$ go test -v ./gopls/internal/regtest/bench -run=TestBenchmarkDidChange -didchange_dir=$HOME/w/kubernetes -didchange_file=pkg/util/hash/hash.go
Before:
BenchmarkStatistics 100 25932206 ns/op 11684109 B/op 75458 allocs/op
After:
BenchmarkStatistics 100 23294195 ns/op 11293472 B/op 47299 allocs/op
Also, move profiling logic outside the loop so that later runs
don't overwrite earlier runs. (This doesn't appear to be a problem
in practice, presumably because the last run is the big one.)
Updates golang/go#45686
Change-Id: I538ca6bb88cc18f1eefe35d2db29a62e5190280e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/410697
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change replaces various uses of FileSet with either
nothing (when the parameter wasn't really needed) or token.File.
Notably, astutil.Imports was being used to extract the imports
of a file (available at ast.File.Imports), forcing a number
of wrappers to have a FileSet parameter.
Also, simplify various expressions file.Position().Line to file.Line().
Change-Id: I19fe86a18aba50352275f77ed737513744d3930c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/410366
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Use a detached Context for all progress notifications. In particular,
using a detached Context for the window/workDoneProgress/create
notification avoids races where the $/cancelRequest notification and
create response cross paths, such that the client has created a progress
dialog but the server thinks that starting progress failed.
Also, as a matter of best practice don't store a context on the WorkDone
type, despite the fact that this Context is detached. Instead, only
close over a Context in the WorkDoneWriter, which requires a Context in
order to function but which implements the io.Writer interface.
The TestProgressBarErrors test should now pass reliably.
Fixesgolang/go#46930
Change-Id: I0d115ed3a62de97fe545c8dc0403e7bb55f6e481
Reviewed-on: https://go-review.googlesource.com/c/tools/+/409936
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Logic in mod.Hover relied upon ColumnMapper.TokFile being the only file
in its FileSet. Fix this to compare offsets with offsets.
Also use safetoken.InRange in semantic.go, to fix an incorrect bounds
check.
Change-Id: Ia18a0799fea1120674e3772efdf0bdb724e554e7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/409934
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
It turns out PointSpan was only ever used as part of an oft repeated
pattern to get a token.Pos from a protocol position. Cut out the
middle.
Change-Id: I0b2c0fc3d335e6bbd3c1ac72c6f75e2c40c60ca5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/408717
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This eliminates some duplication, and lays the groundwork for removing
the use of token.File within ColumnMapper.
Change-Id: I54fe570bfc4f7bca0da643b8727e890dc6343208
Reviewed-on: https://go-review.googlesource.com/c/tools/+/406135
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
The TokenConverter has been trimmed down to a thin wrapper around
token.File, and can now be removed.
Change-Id: I9985492274c88e6a13e6d62dadab5595c75c7840
Reviewed-on: https://go-review.googlesource.com/c/tools/+/406134
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Eliminate the need to work around newline terminated files in
completion, by storing selection ranges as token.Pos and using an
lsppos.TokenMapper derived from the file content, which does not have
problems with newline termination.
This simplifies the completion logic, and removes the last use of
MappedRange.SpanRange, which is an inconisitent API in that it returns
positions in the compiled source, rather than edited source.
Change-Id: I65232787956325172b48fd42d85cbb598039ee5a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/407889
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Due to not having a pointer receiver, memoization of the computed
MappedRange.protocolRange had no effect.
Rather than fix the memoization, just remove it since it has apparently
not affected performance significantly.
Change-Id: Ib34becef44dca4074dcc7c8af93883d9a1060f8d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/408716
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
CL 405546 introduced a latent bug in MappedRange, because it naively
used the wrong TokenConverter to convert mapped positions to offsets.
This was detected via related clean-up work in another CL. Fix this by
passing the correct converter from MappedRange.Range. Add a test that
would have demonstrated the breakage.
More cleanup is needed here. It is subtle that MappedRange.Converter
maps the adjusted position for its start and end, and there may be some
places where this invariant has been broken over the years.
Add additional documentation and bug reports.
Change-Id: If7f177894bac1242ddcc1786e79c7559455e9291
Reviewed-on: https://go-review.googlesource.com/c/tools/+/407887
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Add additional bug reporting related to position invariants, and update
marker test runners to panic on encountered bugs.
This revealed a panic on older Go versions, where we try to create a
range for a missing package name span. Lift the check into the caller
for this case, and leave a big comment explaining that
checkForOrphanedFile probably shouldn't be so tolerant of invalid calls.
Change-Id: Ie16a07afba0f8a5682cca50ad8f04450bfa2da65
Reviewed-on: https://go-review.googlesource.com/c/tools/+/407885
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Package loading (at least via go list) fails when the import header
doesn't parse, so we need to invalidate metadata upon a state change
from non parsing->parsing. Refactor metadata invalidation to implement
this feature, add additional documentation, and avoid unnecessary work.
This change revealed a latent bug (via TestDeleteDirectory): when
statting a deleted directory failed, we could fail to invalidate any
package IDs, even those we already knew about. This bug was masked by
the somewhat complicated semantics of pkgNameChanged. The semantics of
pkgNameChanged are simplified to encapsulate any change to a
package->file association.
Also refactor the parsing API somewhat, and add a bug report if we don't
get a ParseGoFile while inspecting for metadata changes.
Update most regtests to panic upon receiving bug reports.
Fixesgolang/go#52981
Change-Id: I1838963ecc9c01e316f887aa9d8f1260662281ab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/407501
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Our mechanism for reporting internal bugs doesn't work unless we
actually notice them. Add an undocumented option to receive showMessage
dialogs on the first bug occurring server-side.
Change-Id: I259a4c13161271c350fae06dc6ab0e1621725c92
Reviewed-on: https://go-review.googlesource.com/c/tools/+/399624
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Adds a function for computing the core type of a type for use within x/tools.
Updates golang/go#52940
Change-Id: I91645c0b27031506be51a5f9d53b3e125e6fd1c2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/406838
Run-TryBot: Tim King <taking@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sometimes one wants to run the regtests with verbose output without
seeing the wall of RPC logs. Add a new flag to control the printing of
logs, rather than overloading testing.Verbose().
Change-Id: Iea5727e3079005d229319cbf86f67067f7c1f85d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/405894
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change makes it so that the stub methods analysis can recognize
errors happening to method and function call expressions that are being
passed a concrete type to an interface parameter. This way, a method stub CodeAction will appear at the call site.
Updates golang/go#37537
Change-Id: I886d53f06a85b9e5160d882aa742bb2b7fcea139
Reviewed-on: https://go-review.googlesource.com/c/tools/+/404655
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
The only real implementation of position conversion was via a
*token.File, so refactor the converter logic to eliminate the Converter
interface, and just use a single converter implementation that uses a
*token.File to convert between offsets and positions.
This change is meant to be a zero-impact refactoring for non-test code.
As such, I abstained from panicking in several places where it would
make sense. In later CLs, once the bug reporting API lands, we can
insert bug reports in these places.
Change-Id: Id2e503acd80d089bc5d73e983215784015471f04
Reviewed-on: https://go-review.googlesource.com/c/tools/+/405546
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The existing debug.Bug mechanism for reporting internal bugs is
insufficient for several reasons:
- It requires a context, which is not always available.
- By being defined in the debug package, it is subject to import
cycles.
- It is too complicated. Listening for bugs requires understanding the
event package.
Replace this with a simpler 'bug' package with no dependencies, that
allows reporting, listing, and listening on internal bugs. Hopefully
this will fulfill the goal of debug.Bug, to help us track down rare
bugs.
Change-Id: I30cab58429b29bd2d944d62e94f5657e40a760fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/399623
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
In JSON output, the Go command was only setting the Error field for one
package involved in an import cycle. As a result, TestResolveImportCycle
was dependent on the chosen package, causing flakes when the chosen
package is not deterministic.
Arguably this behavior should be fixed, both in the go command and in
gopls, but for now make the test resilient to choice by asserting on any
of the possible errors.
Add a new AnyOf expectation to support this type of assertion and tweak the
test output formatting. Also update the test to eagerly fail once the
didOpen notification has been fully processed, so that we don't have to
wait for the assertion timeout.
Updates golang/go#52904
Change-Id: Ic209d8fdcb7308c041b287a8f122c47e96d29a96
Reviewed-on: https://go-review.googlesource.com/c/tools/+/406274
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
If -config=true, gopls vulncheck reads the package load
configuration JSON (build flags, env, tests, and later
maybe overlay info) from stdin.
And, add some logging to gopls/internal/vulncheck/command.go
that helps measuring the package loading overhead.
Update golang/go#50577
Change-Id: I5c1ce145b07f2bed03911613f42c09a3d6be6c28
Reviewed-on: https://go-review.googlesource.com/c/tools/+/404575
Reviewed-by: Robert Findley <rfindley@google.com>
In passing I noticed that this three-way comparison
is not (anti)symmetric. Such comparisons should consist
of a list of pairs of tests of this form:
if x.key1 < y.key1 { return -1 }
if x.key1 > y.key1 { return +1 }
...key2, etc...
return 0
Also in passing:
- simplify panic-proof debug string function.
- augment doc comment of (*Server).beginFileRequest
Change-Id: Idcd0844ea4e96fc2dee5bbc270f5a200b5c27aa0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/405480
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
This CL adds a new analyzer for //go:embed directive, which
checks for the "embed" import.
Along with it, it improves doc for analysistest.Run for
comments of the the form "//...// want..." or
"//...// want..."
Updates #50262
Change-Id: I60ef0ab740feadd4fff3a4d758123b27ceda0bc6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/400854
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Nooras Saba <saba@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
And fix a minor issue in copytermlist.go --
copytermlist.go replaces type names from go/types with
qualified type names. Use of token.NoPos (filled with
ast.NewIdent) however confuses the go/format printer.
As a result, while transforming
func (x *term) includes(t Type) bool
to
func (x *term) includes(t types.Type) bool
go/format printer failed to compute the end position of
the parameter list, concluded RPAREN should be in a
different line from the parameter list, added a comma,
and printed
func (x *term) includes(t types.Type,) bool
Reuse the replaced node's position instead. (not 100%
correct, but better than NoPos)
Change-Id: Ia34e11562cc80c68dcf4b921ffffd926971c2215
Reviewed-on: https://go-review.googlesource.com/c/tools/+/405536
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
The internal/typeparams directory contains an initial draft of the
documentation later published under the golang.org/x/exp/typeparams
module. Remove this draft to avoid confusion.
Change-Id: I1f40468f99c0d2885e8fc380c75669f654ba971e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/405260
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Optimize building the symbol index for a file, in two ways:
- use the cached full parse tree, if it already exists
- if it doesn't exist, optimize parsing by skipping both comments and
object resolution, which aren't necessary for symbols
This results in around 3x faster initial indexing of symbols. In my
manual testing, indexing of Kubernetes went from 16s->5s, and indexing
of x/tools went from 2.4s->700ms.
Also fix a typo in gopls/internal/regtest/bench/bench_test.go.
Fixes#52602
Change-Id: I0893e95410be96e94e5e9dee7a3aab30b59c19c5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/403679
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This avoids an import cycle that prevented these wrappers from being
used in the lsppos package.
Change-Id: I9eedd256db983dfcf962edba39e3d4f3a1aabdeb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/403680
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
For use-cases that work only with token.Pos and protocol.Position, the
span package is unnecessarily indirect, and inefficient. It also loses
information about newline termination, and handles positions within CRLF
line endings incorrectly.
The lsppos package was written to bypass this complexity, but had
limited use and lacked tests.
Add tests, and an wrapper API that operates on token.Pos. Also fix
source.TestTokenOffset to not panic, and add a temporary exemption of
the new token.Offset usage.
This change also fixes position calculation in the case of empty file
content. The mapper now finds position (0, 0) at offset 0 of an empty
file.
Change-Id: I639bd3fac78a127b1c8eddad60b890449901c68c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/403678
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Instead of invoking the command through the LSP custom command,
call the vulncheck command hook directly. That reduces the extra
overhead of bringing up the full gopls server & package loading.
The vulncheck hook loads packages again any way, so the benefit
of running the check inside gopls's custom command framework
is not huge any more.
Still `gopls vulncheck` is useful - editors don't need to install
another binary for vulncheck feature, and it will output the
result in the format easier to handle than what `govulncheck`
currently offers.
Updates golang/go#50577
Change-Id: Ia21e6d7e0c37c4a1b02dc8bbca860143524c3d1b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/404574
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
While working on golang/go#52715, I discovered an infinite recursion in
gopls' completion logic: eachField assumes a finiteness of type pointers.
It is almost certainly a go/types bug that type-checked types expand
infinitely, but nevertheless we should use the more accurate
typeutil.Map for short-circuiting our search.
Change-Id: Ib1c7125e624f42882869acd4e0476e317d4da056
Reviewed-on: https://go-review.googlesource.com/c/tools/+/404335
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Provide a default value for unsafe.Pointer in fillstruct.
Fixesgolang/go#52640
Change-Id: I10a1878fbf53b082f83f44e0ba2788ead14439d6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/403535
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Suzy Mueller <suzmue@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: David Chase <drchase@google.com>
'gopls help cmd...' is redirected to 'gopls cmd... -h'.
The tool.Run operation for each subcommand already has
logic to show the usage message when it parses a -h flag.
Examples:
$ gopls help
$ gopls help remote
$ gopls help remote sessions
Fixesgolang/go#52598
Change-Id: I5135c6e7b130d839f880d0613534ac08de199bcf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/403677
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
When a file is new or its package name has changed, we should invalidate
all packages for files in the current directory, not just packages
previously containing the file.
Fixesgolang/go#52500
Change-Id: I9fc9857a7abcd4e730068871c899d274e1736967
Reviewed-on: https://go-review.googlesource.com/c/tools/+/401795
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
Give placeholders for type params in func literal completions. For
example:
func foo[T any](func(T) T) {}
foo(<>)
Will now give "func(<T>) <T> {}" where <> denotes a placeholder.
Change-Id: Iadde73ed6b88e1410c28dfa33a20ab6a51235c93
Reviewed-on: https://go-review.googlesource.com/c/tools/+/400616
Run-TryBot: Muir Manders <muir@mnd.rs>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Dylan Le
Suzy Mueller
Robert Findley
Peter Weinberger
Alan Donovan
Francesco Renzi
Hana (Hyang-Ah) Kim
Ruslan Nigmatullin
Muir Manders
Davide Masserut
Jamal Carvalho
Robert Griesemer
Robert Griesemer
Tim King
Marwan Sulaiman
Nooras Saba