gopls/internal/server: workspace: skip adding already added file

This CL adds a check to prevent re-adding a folder to the workspace
if it has already been added. It resolves an issue where the 'loading
packages...' status bar would get stuck in the editor.

The problem occurs when the editor sends an initialization request with
a rootURI but no workspaceFolders, followed by a DidChangeWorkspaceFolders
request for the same folder. Re-adding the same folder is unnecessary and
causes the status bar to hang.

Additionally, this change cleans the provided URI to handle cases where
the client appends a trailing slash to the same folder.

An alternative approach is to reload the added folder again and end the
process of loading package as expected so the status bar could end soon,
which is unneccessary because there is no change in given file.

Fixes golang/go#71967

Change-Id: Ib28c327a5a85f1fdd54e4facedf97d500b909fb2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/663295
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>

Author: xieyuschen

gopls/internal/cache/session.go

@@ -139,11 +139,18 @@ func (s *Session) NewView(ctx context.Context, folder *Folder) (*View, *Snapshot
 	}
 	view, snapshot, release := s.createView(ctx, def)
 	s.views = append(s.views, view)
-	// we always need to drop the view map
-	s.viewMap = make(map[protocol.DocumentURI]*View)
+	s.viewMap[protocol.Clean(folder.Dir)] = view
 	return view, snapshot, release, nil
 }
 
+// HasView checks whether the uri's view exists.
+func (s *Session) HasView(uri protocol.DocumentURI) bool {
+	s.viewMu.Lock()
+	defer s.viewMu.Unlock()
+	_, ok := s.viewMap[protocol.Clean(uri)]
+	return ok
+}
+
 // createView creates a new view, with an initial snapshot that retains the
 // supplied context, detached from events and cancelation.
 //
@@ -389,7 +396,7 @@ func (s *Session) SnapshotOf(ctx context.Context, uri protocol.DocumentURI) (*Sn
 		// View is shut down. Forget this association.
 		s.viewMu.Lock()
 		if s.viewMap[uri] == v {
-			delete(s.viewMap, uri)
+			delete(s.viewMap, protocol.Clean(uri))
 		}
 		s.viewMu.Unlock()
 	}
@@ -478,7 +485,7 @@ func (s *Session) viewOfLocked(ctx context.Context, uri protocol.DocumentURI) (*
 			// (as in golang/go#60776).
 			v = relevantViews[0]
 		}
-		s.viewMap[uri] = v // may be nil
+		s.viewMap[protocol.Clean(uri)] = v // may be nil
 	}
 	return v, nil
 }

gopls/internal/protocol/uri.go

@@ -67,6 +67,11 @@ func (uri *DocumentURI) UnmarshalText(data []byte) (err error) {
 	return
 }
 
+// Clean returns the cleaned uri by triggering filepath.Clean underlying.
+func Clean(uri DocumentURI) DocumentURI {
+	return URIFromPath(filepath.Clean(uri.Path()))
+}
+
 // Path returns the file path for the given URI.
 //
 // DocumentURI("").Path() returns the empty string.

gopls/internal/server/general.go

@@ -312,10 +312,15 @@ func (s *server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol
 	// but the list can grow over time.
 	var filtered []protocol.WorkspaceFolder
 	for _, f := range folders {
-		if _, err := protocol.ParseDocumentURI(f.URI); err != nil {
+		uri, err := protocol.ParseDocumentURI(f.URI)
+		if err != nil {
 			debuglog.Warning.Logf(ctx, "skip adding virtual folder %q - invalid folder URI: %v", f.Name, err)
 			continue
 		}
+		if s.session.HasView(uri) {
+			debuglog.Warning.Logf(ctx, "skip adding the already added folder %q - its view has been created before", f.Name)
+			continue
+		}
 		filtered = append(filtered, f)
 	}
 	folders = filtered

gopls/internal/test/integration/fake/editor.go

@@ -108,6 +108,14 @@ type EditorConfig struct {
 	// To explicitly send no workspace folders, use an empty (non-nil) slice.
 	WorkspaceFolders []string
 
+	// NoDefaultWorkspaceFiles is used to specify whether the fake editor
+	// should give a default workspace folder when WorkspaceFolders is nil.
+	// When it's true, the editor will pass original WorkspaceFolders as is to the LSP server.
+	NoDefaultWorkspaceFiles bool
+
+	// RelRootPath is the root path which will be converted to rootUri to configure on the LSP server.
+	RelRootPath string
+
 	// Whether to edit files with windows line endings.
 	WindowsLineEndings bool
 
@@ -322,8 +330,9 @@ func (e *Editor) initialize(ctx context.Context) error {
 		Version: "v1.0.0",
 	}
 	params.InitializationOptions = makeSettings(e.sandbox, config, nil)
-	params.WorkspaceFolders = makeWorkspaceFolders(e.sandbox, config.WorkspaceFolders)
 
+	params.WorkspaceFolders = makeWorkspaceFolders(e.sandbox, config.WorkspaceFolders, config.NoDefaultWorkspaceFiles)
+	params.RootURI = protocol.DocumentURI(makeRootURI(e.sandbox, config.RelRootPath))
 	capabilities, err := clientCapabilities(config)
 	if err != nil {
 		return fmt.Errorf("unmarshalling EditorConfig.CapabilitiesJSON: %v", err)
@@ -447,7 +456,10 @@ var uriRE = regexp.MustCompile(`^[a-z][a-z0-9+\-.]*://\S+`)
 
 // makeWorkspaceFolders creates a slice of workspace folders to use for
 // this editing session, based on the editor configuration.
-func makeWorkspaceFolders(sandbox *Sandbox, paths []string) (folders []protocol.WorkspaceFolder) {
+func makeWorkspaceFolders(sandbox *Sandbox, paths []string, useEmpty bool) (folders []protocol.WorkspaceFolder) {
+	if len(paths) == 0 && useEmpty {
+		return nil
+	}
 	if len(paths) == 0 {
 		paths = []string{string(sandbox.Workdir.RelativeTo)}
 	}
@@ -466,6 +478,14 @@ func makeWorkspaceFolders(sandbox *Sandbox, paths []string) (folders []protocol.
 	return folders
 }
 
+func makeRootURI(sandbox *Sandbox, path string) string {
+	uri := path
+	if !uriRE.MatchString(path) { // relative file path
+		uri = string(sandbox.Workdir.URI(path))
+	}
+	return uri
+}
+
 // onFileChanges is registered to be called by the Workdir on any writes that
 // go through the Workdir API. It is called synchronously by the Workdir.
 func (e *Editor) onFileChanges(ctx context.Context, evts []protocol.FileEvent) {
@@ -1645,8 +1665,8 @@ func (e *Editor) ChangeWorkspaceFolders(ctx context.Context, folders []string) e
 	config := e.Config()
 
 	// capture existing folders so that we can compute the change.
-	oldFolders := makeWorkspaceFolders(e.sandbox, config.WorkspaceFolders)
-	newFolders := makeWorkspaceFolders(e.sandbox, folders)
+	oldFolders := makeWorkspaceFolders(e.sandbox, config.WorkspaceFolders, config.NoDefaultWorkspaceFiles)
+	newFolders := makeWorkspaceFolders(e.sandbox, folders, config.NoDefaultWorkspaceFiles)
 	config.WorkspaceFolders = folders
 	e.SetConfig(config)
 

gopls/internal/test/integration/options.go

@@ -135,6 +135,22 @@ func WorkspaceFolders(relFolders ...string) RunOption {
 	})
 }
 
+// NoDefaultWorkspaceFiles is used to specify whether the fake editor
+// should give a default workspace folder to the LSP server.
+// When it's true, the editor will pass original WorkspaceFolders to the LSP server.
+func NoDefaultWorkspaceFiles() RunOption {
+	return optionSetter(func(opts *runConfig) {
+		opts.editor.NoDefaultWorkspaceFiles = true
+	})
+}
+
+// RootPath configures the roo path which will be converted to rootUri and sent to the LSP server.
+func RootPath(relpath string) RunOption {
+	return optionSetter(func(opts *runConfig) {
+		opts.editor.RelRootPath = relpath
+	})
+}
+
 // FolderSettings defines per-folder workspace settings, keyed by relative path
 // to the folder.
 //

gopls/internal/test/integration/workspace/workspace_test.go

@@ -1412,6 +1412,66 @@ func TestInitializeWithNonFileWorkspaceFolders(t *testing.T) {
 	}
 }
 
+// TestChangeAddedWorkspaceFolders tests issue71967 which an editor sends the following requests.
+//
+//  1. send an initialization request with rootURI but no workspaceFolders,
+//     which gopls helps to find a workspaceFolders for it.
+//  2. send a DidChangeWorkspaceFolders request with the exact the same folder gopls helps to find.
+//
+// It uses the same approach to simulate the scenario, and ensure we can skip the already added file.
+func TestChangeAddedWorkspaceFolders(t *testing.T) {
+	for _, tt := range []struct {
+		name          string
+		after         []string
+		wantViewRoots []string
+	}{
+		{
+			name:          "add an already added file",
+			after:         []string{"modb"},
+			wantViewRoots: []string{"./modb"},
+		},
+		{
+			name:          "add an already added file but with an ending slash",
+			after:         []string{"modb/"},
+			wantViewRoots: []string{"./modb"},
+		},
+		{
+			name:          "add an already added file and a new file",
+			after:         []string{"modb", "moda/a"},
+			wantViewRoots: []string{"./modb", "moda/a"},
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			opts := []RunOption{ProxyFiles(workspaceProxy), RootPath("modb"), NoDefaultWorkspaceFiles()}
+			WithOptions(opts...).Run(t, multiModule, func(t *testing.T, env *Env) {
+				summary := func(typ cache.ViewType, root, folder string) command.View {
+					return command.View{
+						Type:   typ.String(),
+						Root:   env.Sandbox.Workdir.URI(root),
+						Folder: env.Sandbox.Workdir.URI(folder),
+					}
+				}
+				checkViews := func(want ...command.View) {
+					got := env.Views()
+					if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(command.View{}, "ID")); diff != "" {
+						t.Errorf("SummarizeViews() mismatch (-want +got):\n%s", diff)
+					}
+				}
+				var wantViews []command.View
+				for _, root := range tt.wantViewRoots {
+					wantViews = append(wantViews, summary(cache.GoModView, root, root))
+				}
+				env.ChangeWorkspaceFolders(tt.after...)
+				env.Await(
+					LogMatching(protocol.Warning, "skip adding the already added folder", 1, false),
+					NoOutstandingWork(IgnoreTelemetryPromptWork),
+				)
+				checkViews(wantViews...)
+			})
+		})
+	}
+}
+
 // Test that non-file scheme Document URIs in ChangeWorkspaceFolders
 // notification does not produce errors.
 func TestChangeNonFileWorkspaceFolders(t *testing.T) {