gopls/internal/server: don't reset views if configuration did not change

In CL 538796, options were made immutable on the View, meaning any
change to options required a new View. As a result, the
minorOptionsChange heuristic, which avoided recreating views on certain
options changes, was removed.

Unfortunately, in golang/go#66647, it appears that certain clients may
send frequent didChangeConfiguration notifications. Presumably the
configuration is unchanged, and yet gopls still reinitializes the view.
This may be a cause of significant performance regression for these
clients.

Fix this by making didChangeConfiguration a no op if nothing changed,
using reflect.DeepEqual to compare Options. Since Hooks are not
comparable due to the GofumptFormat func value, they are excluded from
comparison. A subsequent CL will remove hooks altogether.

For golang/go#66647

Change-Id: I280059953d6b128461bef1001da3034f89ba3226
Reviewed-on: https://go-review.googlesource.com/c/tools/+/578037
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Robert Findley <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: findleyr

gopls/doc/commands.md

@@ -703,6 +703,7 @@ Result:
 
 ```
 []{
+	"ID": string,
 	"Type": string,
 	"Root": string,
 	"Folder": string,

gopls/internal/cache/session_test.go

@@ -351,7 +351,7 @@ replace (
 					Dir:     toURI(f.dir),
 					Name:    path.Base(f.dir),
 					Options: opts,
-					Env:     env,
+					Env:     *env,
 				})
 			}
 

gopls/internal/cache/view.go

@@ -51,7 +51,7 @@ type Folder struct {
 	Dir     protocol.DocumentURI
 	Name    string // decorative name for UI; not necessarily unique
 	Options *settings.Options
-	Env     *GoEnv
+	Env     GoEnv
 }
 
 // GoEnv holds the environment variables and data from the Go command that is
@@ -426,7 +426,7 @@ func viewEnv(v *View) string {
 		v.root.Path(),
 		strings.TrimRight(v.folder.Env.GoVersionOutput, "\n"),
 		v.folder.Options.BuildFlags,
-		*v.folder.Env,
+		v.folder.Env,
 		v.envOverlay,
 	)
 

gopls/internal/protocol/command/interface.go

@@ -527,6 +527,7 @@ type DiagnoseFilesArgs struct {
 
 // A View holds summary information about a cache.View.
 type View struct {
+	ID         string               // view ID (the index of this view among all views created)
 	Type       string               // view type (via cache.ViewType.String)
 	Root       protocol.DocumentURI // root dir of the view (e.g. containing go.mod or go.work)
 	Folder     protocol.DocumentURI // workspace folder associated with the view

gopls/internal/server/command.go

@@ -1430,6 +1430,7 @@ func (c *commandHandler) Views(ctx context.Context) ([]command.View, error) {
 	var summaries []command.View
 	for _, view := range c.s.session.Views() {
 		summaries = append(summaries, command.View{
+			ID:         view.ID(),
 			Type:       view.Type().String(),
 			Root:       view.Root(),
 			Folder:     view.Folder().Dir,

gopls/internal/server/general.go

@@ -460,29 +460,7 @@ func (s *server) SetOptions(opts *settings.Options) {
 	s.options = opts
 }
 
-func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, name string) (*cache.Folder, error) {
-	opts := s.Options()
-	if opts.ConfigurationSupported {
-		scope := string(folder)
-		configs, err := s.client.Configuration(ctx, &protocol.ParamConfiguration{
-			Items: []protocol.ConfigurationItem{{
-				ScopeURI: &scope,
-				Section:  "gopls",
-			}},
-		},
-		)
-		if err != nil {
-			return nil, fmt.Errorf("failed to get workspace configuration from client (%s): %v", folder, err)
-		}
-
-		opts = opts.Clone()
-		for _, config := range configs {
-			if err := s.handleOptionResults(ctx, settings.SetOptions(opts, config)); err != nil {
-				return nil, err
-			}
-		}
-	}
-
+func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, name string, opts *settings.Options) (*cache.Folder, error) {
 	env, err := cache.FetchGoEnv(ctx, folder, opts)
 	if err != nil {
 		return nil, err
@@ -491,7 +469,7 @@ func (s *server) newFolder(ctx context.Context, folder protocol.DocumentURI, nam
 		Dir:     folder,
 		Name:    name,
 		Options: opts,
-		Env:     env,
+		Env:     *env,
 	}, nil
 }
 

gopls/internal/server/workspace.go

@@ -7,10 +7,12 @@ package server
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"sync"
 
 	"golang.org/x/tools/gopls/internal/cache"
 	"golang.org/x/tools/gopls/internal/protocol"
+	"golang.org/x/tools/gopls/internal/settings"
 	"golang.org/x/tools/internal/event"
 )
 
@@ -37,7 +39,11 @@ func (s *server) addView(ctx context.Context, name string, dir protocol.Document
 	if state < serverInitialized {
 		return nil, nil, fmt.Errorf("addView called before server initialized")
 	}
-	folder, err := s.newFolder(ctx, dir, name)
+	opts, err := s.fetchFolderOptions(ctx, dir)
+	if err != nil {
+		return nil, nil, err
+	}
+	folder, err := s.newFolder(ctx, dir, name, opts)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -68,15 +74,45 @@ func (s *server) DidChangeConfiguration(ctx context.Context, _ *protocol.DidChan
 	s.SetOptions(options)
 
 	// Collect options for all workspace folders.
-	seen := make(map[protocol.DocumentURI]bool)
-	var newFolders []*cache.Folder
-	for _, view := range s.session.Views() {
+	// If none have changed, this is a no op.
+	folderOpts := make(map[protocol.DocumentURI]*settings.Options)
+	changed := false
+	// The set of views is implicitly guarded by the fact that gopls processes
+	// didChange notifications synchronously.
+	//
+	// TODO(rfindley): investigate this assumption: perhaps we should hold viewMu
+	// here.
+	views := s.session.Views()
+	for _, view := range views {
 		folder := view.Folder()
-		if seen[folder.Dir] {
+		if folderOpts[folder.Dir] != nil {
 			continue
 		}
-		seen[folder.Dir] = true
-		newFolder, err := s.newFolder(ctx, folder.Dir, folder.Name)
+		opts, err := s.fetchFolderOptions(ctx, folder.Dir)
+		if err != nil {
+			return err
+		}
+
+		// Ignore hooks for the purposes of equality.
+		sameOptions := reflect.DeepEqual(folder.Options.ClientOptions, opts.ClientOptions) &&
+			reflect.DeepEqual(folder.Options.ServerOptions, opts.ServerOptions) &&
+			reflect.DeepEqual(folder.Options.UserOptions, opts.UserOptions) &&
+			reflect.DeepEqual(folder.Options.InternalOptions, opts.InternalOptions)
+
+		if !sameOptions {
+			changed = true
+		}
+		folderOpts[folder.Dir] = opts
+	}
+	if !changed {
+		return nil
+	}
+
+	var newFolders []*cache.Folder
+	for _, view := range views {
+		folder := view.Folder()
+		opts := folderOpts[folder.Dir]
+		newFolder, err := s.newFolder(ctx, folder.Dir, folder.Name, opts)
 		if err != nil {
 			return err
 		}

gopls/internal/settings/api_json.go

@@ -5,10 +5,19 @@
 package settings
 
 import (
+	"reflect"
 	"testing"
 	"time"
 )
 
+func TestDefaultsEquivalence(t *testing.T) {
+	opts1 := DefaultOptions()
+	opts2 := DefaultOptions()
+	if !reflect.DeepEqual(opts1, opts2) {
+		t.Fatal("default options are not equivalent using reflect.DeepEqual")
+	}
+}
+
 func TestSetOption(t *testing.T) {
 	tests := []struct {
 		name      string

gopls/internal/settings/settings_test.go

@@ -39,7 +39,7 @@ var FooErr = errors.New("foo")
 			NoDiagnostics(ForFile("a/a.go")),
 		)
 		cfg := env.Editor.Config()
-		cfg.Settings = map[string]interface{}{
+		cfg.Settings = map[string]any{
 			"staticcheck": true,
 		}
 		env.ChangeConfiguration(cfg)
@@ -49,6 +49,73 @@ var FooErr = errors.New("foo")
 	})
 }
 
+func TestIdenticalConfiguration(t *testing.T) {
+	// This test checks that changing configuration does not cause views to be
+	// recreated if there is no configuration change.
+	const files = `
+-- a.go --
+package p
+
+func _() {
+	var x *int
+	y := *x
+	_ = y
+}
+`
+	Run(t, files, func(t *testing.T, env *Env) {
+		// Sanity check: before disabling the nilness analyzer, we should have a
+		// diagnostic for the nil dereference.
+		env.OpenFile("a.go")
+		env.AfterChange(
+			Diagnostics(
+				ForFile("a.go"),
+				WithMessage("nil dereference"),
+			),
+		)
+
+		// Collect the view ID before changing configuration.
+		viewID := func() string {
+			t.Helper()
+			views := env.Views()
+			if len(views) != 1 {
+				t.Fatalf("got %d views, want 1", len(views))
+			}
+			return views[0].ID
+		}
+		before := viewID()
+
+		// Now disable the nilness analyzer.
+		cfg := env.Editor.Config()
+		cfg.Settings = map[string]any{
+			"analyses": map[string]any{
+				"nilness": false,
+			},
+		}
+
+		// This should cause the diagnostic to disappear...
+		env.ChangeConfiguration(cfg)
+		env.AfterChange(
+			NoDiagnostics(),
+		)
+		// ...and we should be on the second view.
+		after := viewID()
+		if after == before {
+			t.Errorf("after configuration change, got view %q (same as before), want new view", after)
+		}
+
+		// Now change configuration again, this time with the same configuration as
+		// before. We should still have no diagnostics...
+		env.ChangeConfiguration(cfg)
+		env.AfterChange(
+			NoDiagnostics(),
+		)
+		// ...and we should still be on the second view.
+		if got := viewID(); got != after {
+			t.Errorf("after second configuration change, got view %q, want %q", got, after)
+		}
+	})
+}
+
 // Test that clients can configure per-workspace configuration, which is
 // queried via the scopeURI of a workspace/configuration request.
 // (this was broken in golang/go#65519).

gopls/internal/test/integration/misc/configuration_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"golang.org/x/tools/gopls/internal/cache"
 	"golang.org/x/tools/gopls/internal/protocol/command"
 
@@ -53,7 +54,7 @@ func main() {}
 		}
 		checkViews := func(want ...command.View) {
 			got := env.Views()
-			if diff := cmp.Diff(want, got); diff != "" {
+			if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(command.View{}, "ID")); diff != "" {
 				t.Errorf("SummarizeViews() mismatch (-want +got):\n%s", diff)
 			}
 		}
@@ -130,7 +131,7 @@ package a
 		}
 		checkViews := func(want ...command.View) {
 			got := env.Views()
-			if diff := cmp.Diff(want, got); diff != "" {
+			if diff := cmp.Diff(want, got, cmpopts.IgnoreFields(command.View{}, "ID")); diff != "" {
 				t.Errorf("SummarizeViews() mismatch (-want +got):\n%s", diff)
 			}
 		}