Skip to content

Commit

Permalink
gopls/internal/lsp/cache: switch to new bestViewForURI logic
Browse files Browse the repository at this point in the history
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.

Fixes golang/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>
  • Loading branch information
findleyr committed Dec 21, 2023
1 parent 160631f commit 59384bc
Show file tree
Hide file tree
Showing 16 changed files with 685 additions and 526 deletions.
2 changes: 2 additions & 0 deletions gopls/internal/lsp/cache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"sync/atomic"
"time"

"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/internal/memoize"
Expand Down Expand Up @@ -62,6 +63,7 @@ func NewSession(ctx context.Context, c *Cache) *Session {
gocmdRunner: &gocommand.Runner{},
overlayFS: newOverlayFS(c),
parseCache: newParseCache(1 * time.Minute), // keep recently parsed files for a minute, to optimize typing CPU
viewMap: make(map[protocol.DocumentURI]*View),
}
event.Log(ctx, "New session", KeyCreateSession.Of(s))
return s
Expand Down
41 changes: 27 additions & 14 deletions gopls/internal/lsp/cache/load.go
Original file line number Diff line number Diff line change
Expand Up @@ -592,11 +592,38 @@ func computeLoadDiagnostics(ctx context.Context, snapshot *Snapshot, mp *metadat
// isWorkspacePackageLocked reports whether p is a workspace package for the
// snapshot s.
//
// Workspace packages are packages that we consider the user to be actively
// working on. As such, they are re-diagnosed on every keystroke, and searched
// for various workspace-wide queries such as references or workspace symbols.
//
// See the commentary inline for a description of the workspace package
// heuristics.
//
// s.mu must be held while calling this function.
func isWorkspacePackageLocked(s *Snapshot, meta *metadata.Graph, pkg *metadata.Package) bool {
if metadata.IsCommandLineArguments(pkg.ID) {
// Ad-hoc command-line-arguments packages aren't workspace packages.
// With zero-config gopls (golang/go#57979) they should be very rare, as
// they should only arise when the user opens a file outside the workspace
// which isn't present in the import graph of a workspace package.
//
// Considering them as workspace packages tends to be racy, as they don't
// deterministically belong to any view.
if !pkg.Standalone {
return false
}

// If all the files contained in pkg have a real package, we don't need to
// keep pkg as a workspace package.
if allFilesHaveRealPackages(meta, pkg) {
return false
}

// For now, allow open standalone packages (i.e. go:build ignore) to be
// workspace packages, but this means they could belong to multiple views.
return containsOpenFileLocked(s, pkg)
}

// Apply filtering logic.
//
// Workspace packages must contain at least one non-filtered file.
Expand Down Expand Up @@ -631,20 +658,6 @@ func isWorkspacePackageLocked(s *Snapshot, meta *metadata.Graph, pkg *metadata.P
}
}

// Special case: consider command-line packages to be workspace packages if
// they are open and the only package containing a given file.
if metadata.IsCommandLineArguments(pkg.ID) {
// If all the files contained in m have a real package, we don't need to
// keep m as a workspace package.
if allFilesHaveRealPackages(meta, pkg) {
return false
}

// We only care about command-line-arguments packages if they are still
// open.
return containsOpenFileLocked(s, pkg)
}

// In module mode, a workspace package must be contained in a workspace
// module.
if s.view.moduleMode() {
Expand Down
Loading

0 comments on commit 59384bc

Please sign in to comment.