From c41a8f58b5eaa5216af79a94eee5b444e436208b Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Fri, 8 Nov 2019 13:25:29 -0500 Subject: [PATCH] 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 Reviewed-by: Rebecca Stambler --- internal/lsp/cache/session.go | 62 ++++++++++++++++++++++++++------- internal/lsp/cache/view.go | 23 ++++++++++-- internal/lsp/completion_test.go | 8 +++-- internal/lsp/lsp_test.go | 14 ++++++-- internal/lsp/source/view.go | 7 ++-- 5 files changed, 92 insertions(+), 22 deletions(-) diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 573ebc8e54..2174df4e5a 100644 --- a/internal/lsp/cache/session.go +++ b/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() + 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 1488a368bb..5bd8d10e16 100644 --- a/internal/lsp/cache/view.go +++ b/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) { - 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 7c20ce103a..0ee2e06afd 100644 --- a/internal/lsp/completion_test.go +++ b/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) - 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 225f9434ab..8e2615b570 100644 --- a/internal/lsp/lsp_test.go +++ b/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) { diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index dcbd1b8137..1f3f18d721 100644 --- a/internal/lsp/source/view.go +++ b/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. - // 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.