gopls/internal/lsp: cleanups to view logic
CL 541124 exposed a number of subleties in the view
logic that this CL cleans up. Specifically:
- addFolders was called in one place with potentially
invalid folder URIs, violating its recently added
precondition. The precondition has been removed
and addFolder now handles the error.
- DidChangeWorkspaceFolders used the Folder.Name as the
key for removing the view, which is incorrect as the
name is merely decorative; the URI is the key.
- The ViewByName and RemoveView loops have been fused;
each is used only once.
Change-Id: I1df39d5739464530b4e0ec826a7c08bb73b5b314
Reviewed-on: https://go-review.googlesource.com/c/tools/+/545555
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go
index 4383e87..9b632ca 100644
--- a/gopls/internal/lsp/cache/session.go
+++ b/gopls/internal/lsp/cache/session.go
@@ -213,16 +213,24 @@
return v, snapshot, snapshot.Acquire(), nil
}
-// ViewByName returns a view with a matching name, if the session has one.
-func (s *Session) ViewByName(name string) *View {
+// RemoveView removes from the session the view rooted at the specified directory.
+// It reports whether a view of that directory was removed.
+func (s *Session) RemoveView(dir protocol.DocumentURI) bool {
s.viewMu.Lock()
defer s.viewMu.Unlock()
for _, view := range s.views {
- if view.Name() == name {
- return view
+ if view.folder.Dir == dir {
+ i := s.dropView(view)
+ if i == -1 {
+ return false // can't happen
+ }
+ // delete this view... we don't care about order but we do want to make
+ // sure we can garbage collect the view
+ s.views = removeElement(s.views, i)
+ return true
}
}
- return nil
+ return false
}
// View returns the view with a matching id, if present.
@@ -295,20 +303,6 @@
return views[0]
}
-// RemoveView removes the view v from the session
-func (s *Session) RemoveView(view *View) {
- s.viewMu.Lock()
- defer s.viewMu.Unlock()
-
- i := s.dropView(view)
- if i == -1 { // error reported elsewhere
- return
- }
- // delete this view... we don't care about order but we do want to make
- // sure we can garbage collect the view
- s.views = removeElement(s.views, i)
-}
-
// updateViewLocked recreates the view with the given options.
//
// If the resulting error is non-nil, the view may or may not have already been
diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go
index 4029af0..8d52fc8 100644
--- a/gopls/internal/lsp/cache/view.go
+++ b/gopls/internal/lsp/cache/view.go
@@ -44,7 +44,7 @@
// Folders must not be mutated, as they may be shared across multiple views.
type Folder struct {
Dir protocol.DocumentURI
- Name string
+ Name string // decorative name for UI; not necessarily unique
Options *settings.Options
}
diff --git a/gopls/internal/lsp/general.go b/gopls/internal/lsp/general.go
index 663edbd..cf154d6 100644
--- a/gopls/internal/lsp/general.go
+++ b/gopls/internal/lsp/general.go
@@ -289,11 +289,9 @@
// directories) to the session. It does not return an error, though it
// may report an error to the client over LSP if one or more folders
// had problems.
-//
-// addFolders must be called only with valid file URIs.
func (s *server) addFolders(ctx context.Context, folders []protocol.WorkspaceFolder) {
originalViews := len(s.session.Views())
- viewErrors := make(map[protocol.DocumentURI]error)
+ viewErrors := make(map[protocol.URI]error)
var ndiagnose sync.WaitGroup // number of unfinished diagnose calls
if s.Options().VerboseWorkDoneProgress {
@@ -310,7 +308,7 @@
for _, folder := range folders {
uri, err := protocol.ParseDocumentURI(folder.URI)
if err != nil {
- bug.Reportf("addFolders: invalid folder URI: %v", err)
+ viewErrors[folder.URI] = fmt.Errorf("invalid folder URI: %v", err)
continue
}
work := s.progress.Start(ctx, "Setting up workspace", "Loading packages...", nil, nil)
@@ -319,7 +317,7 @@
if err == cache.ErrViewExists {
continue
}
- viewErrors[uri] = err
+ viewErrors[folder.URI] = err
work.End(ctx, fmt.Sprintf("Error loading packages: %s", err))
continue
}
diff --git a/gopls/internal/lsp/workspace.go b/gopls/internal/lsp/workspace.go
index e2a2da8..88d1e50 100644
--- a/gopls/internal/lsp/workspace.go
+++ b/gopls/internal/lsp/workspace.go
@@ -15,34 +15,34 @@
)
func (s *server) DidChangeWorkspaceFolders(ctx context.Context, params *protocol.DidChangeWorkspaceFoldersParams) error {
- event := params.Event
- for _, folder := range event.Removed {
- view := s.session.ViewByName(folder.Name)
- if view != nil {
- s.session.RemoveView(view)
- } else {
- return fmt.Errorf("view %s for %v not found", folder.Name, folder.URI)
+ for _, folder := range params.Event.Removed {
+ dir, err := protocol.ParseDocumentURI(folder.URI)
+ if err != nil {
+ return fmt.Errorf("invalid folder %q: %v", folder.URI, err)
+ }
+ if !s.session.RemoveView(dir) {
+ return fmt.Errorf("view %q for %v not found", folder.Name, folder.URI)
}
}
- s.addFolders(ctx, event.Added)
+ s.addFolders(ctx, params.Event.Added)
return nil
}
// addView returns a Snapshot and a release function that must be
// called when it is no longer needed.
-func (s *server) addView(ctx context.Context, name string, uri protocol.DocumentURI) (*cache.Snapshot, func(), error) {
+func (s *server) addView(ctx context.Context, name string, dir protocol.DocumentURI) (*cache.Snapshot, func(), error) {
s.stateMu.Lock()
state := s.state
s.stateMu.Unlock()
if state < serverInitialized {
return nil, nil, fmt.Errorf("addView called before server initialized")
}
- options, err := s.fetchFolderOptions(ctx, uri)
+ options, err := s.fetchFolderOptions(ctx, dir)
if err != nil {
return nil, nil, err
}
folder := &cache.Folder{
- Dir: uri,
+ Dir: dir,
Name: name,
Options: options,
}
