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>
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index 573ebc8..2174df4 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -80,9 +80,20 @@
}
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 @@
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) removeView(ctx context.Context, view *view) error {
s.viewMu.Lock()
defer s.viewMu.Unlock()
+ i, err := s.dropView(ctx, view)
+ if err != nil {
+ return err
+ }
+ // 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]
+ 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 {
- // 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]
+ // we found the view, drop it and return the index it was found at
+ s.views[i] = nil
v.shutdown(ctx)
- return nil
+ 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.
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index 1488a36..5bd8d10 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -12,6 +12,7 @@
"go/token"
"os"
"os/exec"
+ "reflect"
"strings"
"sync"
"time"
@@ -103,8 +104,26 @@
return v.options
}
-func (v *view) SetOptions(options source.Options) {
- v.options = 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
diff --git a/internal/lsp/completion_test.go b/internal/lsp/completion_test.go
index 7c20ce1..0ee2e06 100644
--- a/internal/lsp/completion_test.go
+++ b/internal/lsp/completion_test.go
@@ -125,8 +125,12 @@
modified := original
modified.InsertTextFormat = protocol.SnippetTextFormat
modified.Completion = options
- view.SetOptions(modified)
- defer view.SetOptions(original)
+ view, err := view.SetOptions(r.ctx, modified)
+ 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{
diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go
index 225f943..8e2615b 100644
--- a/internal/lsp/lsp_test.go
+++ b/internal/lsp/lsp_test.go
@@ -104,7 +104,11 @@
// 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 @@
// 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 @@
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) {
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index dcbd1b8..1f3f18d 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -237,9 +237,10 @@
Options() Options
// SetOptions sets the options of this view to new values.
- // Warning: Do not use this, unless in a test.
- // This function does not correctly invalidate the view when needed.
- SetOptions(Options)
+ // Calling this may cause the view to be invalidated and a replacement view
+ // added to the session. If so the new view will be returned, otherwise the
+ // original one will be.
+ SetOptions(context.Context, Options) (View, error)
// CheckPackageHandles returns the CheckPackageHandles for the packages
// that this file belongs to.
