internal/lsp: make View.SetOptions save and useful

It attempts to detect changes that would invalidate the view and replace itself
with a new view when that happens

Change-Id: I0f1a8cd3bd6ddcef115fedc6c57ae0398b16d3b6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206147
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/cache/session.go

@@ -80,9 +80,20 @@ func (s *session) Cache() source.Cache {
 }
 
 func (s *session) NewView(ctx context.Context, name string, folder span.URI, options source.Options) (source.View, error) {
-	index := atomic.AddInt64(&viewIndex, 1)
 	s.viewMu.Lock()
 	defer s.viewMu.Unlock()
+	v, err := s.createView(ctx, name, folder, options)
+	if err != nil {
+		return nil, err
+	}
+	s.views = append(s.views, v)
+	// we always need to drop the view map
+	s.viewMap = make(map[span.URI]source.View)
+	return v, nil
+}
+
+func (s *session) createView(ctx context.Context, name string, folder span.URI, options source.Options) (*view, error) {
+	index := atomic.AddInt64(&viewIndex, 1)
 	// We want a true background context and not a detached context here
 	// the spans need to be unrelated and no tag values should pollute it.
 	baseCtx := trace.Detach(xcontext.Detach(ctx))
@@ -141,10 +152,6 @@ func (s *session) NewView(ctx context.Context, name string, folder span.URI, opt
 			return nil, err
 		}
 	}
-
-	s.views = append(s.views, v)
-	// we always need to drop the view map
-	s.viewMap = make(map[span.URI]source.View)
 	debug.AddView(debugView{v})
 	return v, loadErr
 }
@@ -223,20 +230,51 @@ func (s *session) bestView(uri span.URI) source.View {
 func (s *session) removeView(ctx context.Context, view *view) error {
 	s.viewMu.Lock()
 	defer s.viewMu.Unlock()
-	// we always need to drop the view map
+	i, err := s.dropView(ctx, view)
-	s.viewMap = make(map[span.URI]source.View)
+	if err != nil {
-	for i, v := range s.views {
+		return err
-		if view == v {
+	}
 	// delete this view... we don't care about order but we do want to make
 	// sure we can garbage collect the view
 	s.views[i] = s.views[len(s.views)-1]
 	s.views[len(s.views)-1] = nil
 	s.views = s.views[:len(s.views)-1]
-			v.shutdown(ctx)
 	return nil
+}
+
+func (s *session) updateView(ctx context.Context, view *view, options source.Options) (*view, error) {
+	s.viewMu.Lock()
+	defer s.viewMu.Unlock()
+	i, err := s.dropView(ctx, view)
+	if err != nil {
+		return nil, err
+	}
+	v, err := s.createView(ctx, view.name, view.folder, options)
+	if err != nil {
+		// we have dropped the old view, but could not create the new one
+		// this should not happen and is very bad, but we still need to clean
+		// up the view array if it happens
+		s.views[i] = s.views[len(s.views)-1]
+		s.views[len(s.views)-1] = nil
+		s.views = s.views[:len(s.views)-1]
+	}
+	// substitute the new view into the array where the old view was
+	s.views[i] = v
+	return v, nil
+}
+
+func (s *session) dropView(ctx context.Context, view *view) (int, error) {
+	// we always need to drop the view map
+	s.viewMap = make(map[span.URI]source.View)
+	for i, v := range s.views {
+		if view == v {
+			// we found the view, drop it and return the index it was found at
+			s.views[i] = nil
+			v.shutdown(ctx)
+			return i, nil
 		}
 	}
-	return errors.Errorf("view %s for %v not found", view.Name(), view.Folder())
+	return -1, errors.Errorf("view %s for %v not found", view.Name(), view.Folder())
 }
 
 // TODO: Propagate the language ID through to the view.

internal/lsp/cache/view.go

@@ -12,6 +12,7 @@ import (
 	"go/token"
 	"os"
 	"os/exec"
+	"reflect"
 	"strings"
 	"sync"
 	"time"
@@ -103,8 +104,26 @@ func (v *view) Options() source.Options {
 	return v.options
 }
 
-func (v *view) SetOptions(options source.Options) {
+func minorOptionsChange(a, b source.Options) bool {
+	// Check if any of the settings that modify our understanding of files have been changed
+	if !reflect.DeepEqual(a.Env, b.Env) {
+		return false
+	}
+	if !reflect.DeepEqual(a.BuildFlags, b.BuildFlags) {
+		return false
+	}
+	// the rest of the options are benign
+	return true
+}
+
+func (v *view) SetOptions(ctx context.Context, options source.Options) (source.View, error) {
+	// no need to rebuild the view if the options were not materially changed
+	if minorOptionsChange(v.options, options) {
 		v.options = options
+		return v, nil
+	}
+	newView, err := v.session.updateView(ctx, v, options)
+	return newView, err
 }
 
 // Config returns the configuration used for the view's interaction with the

internal/lsp/completion_test.go

@@ -125,8 +125,12 @@ func (r *runner) callCompletion(t *testing.T, src span.Span, options source.Comp
 	modified := original
 	modified.InsertTextFormat = protocol.SnippetTextFormat
 	modified.Completion = options
-	view.SetOptions(modified)
+	view, err := view.SetOptions(r.ctx, modified)
-	defer view.SetOptions(original)
+	if err != nil {
+		t.Error(err)
+		return nil
+	}
+	defer view.SetOptions(r.ctx, original)
 
 	list, err := r.server.Completion(r.ctx, &protocol.CompletionParams{
 		TextDocumentPositionParams: protocol.TextDocumentPositionParams{

internal/lsp/lsp_test.go

@@ -104,7 +104,11 @@ func (r *runner) FoldingRanges(t *testing.T, spn span.Span) {
 
 	// Test all folding ranges.
 	modified.LineFoldingOnly = false
-	view.SetOptions(modified)
+	view, err := view.SetOptions(r.ctx, modified)
+	if err != nil {
+		t.Error(err)
+		return
+	}
 	ranges, err := r.server.FoldingRange(r.ctx, &protocol.FoldingRangeParams{
 		TextDocument: protocol.TextDocumentIdentifier{
 			URI: protocol.NewURI(uri),
@@ -118,7 +122,11 @@ func (r *runner) FoldingRanges(t *testing.T, spn span.Span) {
 
 	// Test folding ranges with lineFoldingOnly = true.
 	modified.LineFoldingOnly = true
-	view.SetOptions(modified)
+	view, err = view.SetOptions(r.ctx, modified)
+	if err != nil {
+		t.Error(err)
+		return
+	}
 	ranges, err = r.server.FoldingRange(r.ctx, &protocol.FoldingRangeParams{
 		TextDocument: protocol.TextDocumentIdentifier{
 			URI: protocol.NewURI(uri),
@@ -129,7 +137,7 @@ func (r *runner) FoldingRanges(t *testing.T, spn span.Span) {
 		return
 	}
 	r.foldingRanges(t, "foldingRange-lineFolding", uri, ranges)
-	view.SetOptions(original)
+	view.SetOptions(r.ctx, original)
 }
 
 func (r *runner) foldingRanges(t *testing.T, prefix string, uri span.URI, ranges []protocol.FoldingRange) {

internal/lsp/source/view.go

@@ -237,9 +237,10 @@ type View interface {
 	Options() Options
 
 	// SetOptions sets the options of this view to new values.
-	// Warning: Do not use this, unless in a test.
+	// Calling this may cause the view to be invalidated and a replacement view
-	// This function does not correctly invalidate the view when needed.
+	// added to the session. If so the new view will be returned, otherwise the
-	SetOptions(Options)
+	// original one will be.
+	SetOptions(context.Context, Options) (View, error)
 
 	// CheckPackageHandles returns the CheckPackageHandles for the packages
 	// that this file belongs to.