From 80313e1ba7181ff88576941b25b4beb004e348db Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Fri, 15 Nov 2019 12:43:45 -0500 Subject: [PATCH] internal/lsp: fix panic in bestView Rather than panicking when we have not created any views for the packages, we should show a reasonable error to the user. This change propagates the errors to the user. Updates golang/go#35599 Change-Id: I49789d8ce18e154f111bc3584488f468a129e30c Reviewed-on: https://go-review.googlesource.com/c/tools/+/207344 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Michael Matloob --- internal/lsp/cache/session.go | 25 +++++++++++++++++-------- internal/lsp/code_action.go | 5 ++++- internal/lsp/command.go | 5 ++++- internal/lsp/completion.go | 5 ++++- internal/lsp/completion_test.go | 7 +++++-- internal/lsp/definition.go | 10 ++++++++-- internal/lsp/folding_range.go | 5 ++++- internal/lsp/format.go | 5 ++++- internal/lsp/general.go | 14 +++++++++++++- internal/lsp/highlight.go | 5 ++++- internal/lsp/hover.go | 5 ++++- internal/lsp/implementation.go | 5 ++++- internal/lsp/link.go | 5 ++++- internal/lsp/lsp_test.go | 12 +++++++++--- internal/lsp/references.go | 5 ++++- internal/lsp/rename.go | 10 ++++++++-- internal/lsp/signature_help.go | 5 ++++- internal/lsp/source/view.go | 2 +- internal/lsp/symbols.go | 5 ++++- internal/lsp/text_synchronization.go | 15 ++++++++++++--- 20 files changed, 121 insertions(+), 34 deletions(-) diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index cb80f19116..8f1158f923 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -169,18 +169,21 @@ func (s *session) View(name string) source.View { // ViewOf returns a view corresponding to the given URI. // If the file is not already associated with a view, pick one using some heuristics. -func (s *session) ViewOf(uri span.URI) source.View { +func (s *session) ViewOf(uri span.URI) (source.View, error) { s.viewMu.Lock() defer s.viewMu.Unlock() // Check if we already know this file. if v, found := s.viewMap[uri]; found { - return v + return v, nil } // Pick the best view for this file and memoize the result. - v := s.bestView(uri) + v, err := s.bestView(uri) + if err != nil { + return nil, err + } s.viewMap[uri] = v - return v + return v, nil } func (s *session) viewsOf(uri span.URI) []*view { @@ -208,7 +211,10 @@ func (s *session) Views() []source.View { // bestView finds the best view to associate a given URI with. // viewMu must be held when calling this method. -func (s *session) bestView(uri span.URI) source.View { +func (s *session) bestView(uri span.URI) (source.View, error) { + if len(s.views) == 0 { + return nil, errors.Errorf("no views in the session") + } // we need to find the best view for this file var longest source.View for _, view := range s.views { @@ -220,10 +226,10 @@ func (s *session) bestView(uri span.URI) source.View { } } if longest != nil { - return longest + return longest, nil } // TODO: are there any more heuristics we can use? - return s.views[0] + return s.views[0], nil } func (s *session) removeView(ctx context.Context, view *view) error { @@ -291,7 +297,10 @@ func (s *session) DidOpen(ctx context.Context, uri span.URI, kind source.FileKin } // Make sure that the file gets added to the session's file watch map. - view := s.bestView(uri) + view, err := s.bestView(uri) + if err != nil { + return err + } if _, err := view.GetFile(ctx, uri); err != nil { return err } diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index cf66505228..8db4b93827 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -21,7 +21,10 @@ import ( func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionParams) ([]protocol.CodeAction, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/command.go b/internal/lsp/command.go index 72e8647532..5075e9091c 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go @@ -17,7 +17,10 @@ func (s *Server) executeCommand(ctx context.Context, params *protocol.ExecuteCom } // Confirm that this action is being taken on a go.mod file. uri := span.NewURI(params.Arguments[0].(string)) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/completion.go b/internal/lsp/completion.go index 949a433e9c..4b1e390e83 100644 --- a/internal/lsp/completion.go +++ b/internal/lsp/completion.go @@ -18,7 +18,10 @@ import ( func (s *Server) completion(ctx context.Context, params *protocol.CompletionParams) (*protocol.CompletionList, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } options := view.Options() f, err := view.GetFile(ctx, uri) if err != nil { diff --git a/internal/lsp/completion_test.go b/internal/lsp/completion_test.go index 0ee2e06afd..6fdffa1fb1 100644 --- a/internal/lsp/completion_test.go +++ b/internal/lsp/completion_test.go @@ -120,12 +120,15 @@ func expected(t *testing.T, test tests.Completion, items tests.CompletionItems) func (r *runner) callCompletion(t *testing.T, src span.Span, options source.CompletionOptions) []protocol.CompletionItem { t.Helper() - view := r.server.session.ViewOf(src.URI()) + view, err := r.server.session.ViewOf(src.URI()) + if err != nil { + t.Fatal(err) + } original := view.Options() modified := original modified.InsertTextFormat = protocol.SnippetTextFormat modified.Completion = options - view, err := view.SetOptions(r.ctx, modified) + view, err = view.SetOptions(r.ctx, modified) if err != nil { t.Error(err) return nil diff --git a/internal/lsp/definition.go b/internal/lsp/definition.go index f8f18df9ef..6e3cae2f0f 100644 --- a/internal/lsp/definition.go +++ b/internal/lsp/definition.go @@ -14,7 +14,10 @@ import ( func (s *Server) definition(ctx context.Context, params *protocol.DefinitionParams) ([]protocol.Location, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err @@ -37,7 +40,10 @@ func (s *Server) definition(ctx context.Context, params *protocol.DefinitionPara func (s *Server) typeDefinition(ctx context.Context, params *protocol.TypeDefinitionParams) ([]protocol.Location, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/folding_range.go b/internal/lsp/folding_range.go index d0eecc49a2..5d88e746cd 100644 --- a/internal/lsp/folding_range.go +++ b/internal/lsp/folding_range.go @@ -10,7 +10,10 @@ import ( func (s *Server) foldingRange(ctx context.Context, params *protocol.FoldingRangeParams) ([]protocol.FoldingRange, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/format.go b/internal/lsp/format.go index 33e9b8d633..b16bfb6a99 100644 --- a/internal/lsp/format.go +++ b/internal/lsp/format.go @@ -14,7 +14,10 @@ import ( func (s *Server) formatting(ctx context.Context, params *protocol.DocumentFormattingParams) ([]protocol.TextEdit, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/general.go b/internal/lsp/general.go index a18fdd283f..caab70bac2 100644 --- a/internal/lsp/general.go +++ b/internal/lsp/general.go @@ -166,11 +166,23 @@ func (s *Server) initialized(ctx context.Context, params *protocol.InitializedPa debug.PrintVersionInfo(buf, true, debug.PlainText) log.Print(ctx, buf.String()) + viewErrors := make(map[span.URI]error) for _, folder := range s.pendingFolders { + uri := span.NewURI(folder.URI) if _, err := s.addView(ctx, folder.Name, span.NewURI(folder.URI)); err != nil { - return err + viewErrors[uri] = err } } + if len(viewErrors) > 0 { + errMsg := fmt.Sprintf("Error loading workspace folders (expected %v, got %v)\n", len(s.pendingFolders), len(s.session.Views())) + for uri, err := range viewErrors { + errMsg += fmt.Sprintf("failed to load view for %s: %v\n", uri, err) + } + s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ + Type: protocol.Error, + Message: errMsg, + }) + } s.pendingFolders = nil return nil diff --git a/internal/lsp/highlight.go b/internal/lsp/highlight.go index 8a579a64b0..c873127b58 100644 --- a/internal/lsp/highlight.go +++ b/internal/lsp/highlight.go @@ -16,7 +16,10 @@ import ( func (s *Server) documentHighlight(ctx context.Context, params *protocol.DocumentHighlightParams) ([]protocol.DocumentHighlight, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } rngs, err := source.Highlight(ctx, view, uri, params.Position) if err != nil { log.Error(ctx, "no highlight", err, telemetry.URI.Of(uri)) diff --git a/internal/lsp/hover.go b/internal/lsp/hover.go index ddf1a32bbb..4377e67dc8 100644 --- a/internal/lsp/hover.go +++ b/internal/lsp/hover.go @@ -17,7 +17,10 @@ import ( func (s *Server) hover(ctx context.Context, params *protocol.HoverParams) (*protocol.Hover, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/implementation.go b/internal/lsp/implementation.go index daca99ff4e..09f18e485e 100644 --- a/internal/lsp/implementation.go +++ b/internal/lsp/implementation.go @@ -14,7 +14,10 @@ import ( func (s *Server) implementation(ctx context.Context, params *protocol.ImplementationParams) ([]protocol.Location, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/link.go b/internal/lsp/link.go index 92777fbc95..c9f89f2c4d 100644 --- a/internal/lsp/link.go +++ b/internal/lsp/link.go @@ -22,7 +22,10 @@ import ( func (s *Server) documentLink(ctx context.Context, params *protocol.DocumentLinkParams) ([]protocol.DocumentLink, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index c4daa4bf21..e2ee9afffc 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -98,13 +98,16 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti func (r *runner) FoldingRanges(t *testing.T, spn span.Span) { uri := spn.URI() - view := r.server.session.ViewOf(uri) + view, err := r.server.session.ViewOf(uri) + if err != nil { + t.Fatal(err) + } original := view.Options() modified := original // Test all folding ranges. modified.LineFoldingOnly = false - view, err := view.SetOptions(r.ctx, modified) + view, err = view.SetOptions(r.ctx, modified) if err != nil { t.Error(err) return @@ -326,7 +329,10 @@ func (r *runner) Import(t *testing.T, spn span.Span) { func (r *runner) SuggestedFix(t *testing.T, spn span.Span) { uri := spn.URI() filename := uri.Filename() - view := r.server.session.ViewOf(uri) + view, err := r.server.session.ViewOf(uri) + if err != nil { + t.Fatal(err) + } f, err := view.GetFile(r.ctx, uri) if err != nil { t.Fatal(err) diff --git a/internal/lsp/references.go b/internal/lsp/references.go index 6930a6c174..a292035518 100644 --- a/internal/lsp/references.go +++ b/internal/lsp/references.go @@ -16,7 +16,10 @@ import ( func (s *Server) references(ctx context.Context, params *protocol.ReferenceParams) ([]protocol.Location, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/rename.go b/internal/lsp/rename.go index 9664b5c449..cc9f1a195e 100644 --- a/internal/lsp/rename.go +++ b/internal/lsp/rename.go @@ -14,7 +14,10 @@ import ( func (s *Server) rename(ctx context.Context, params *protocol.RenameParams) (*protocol.WorkspaceEdit, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err @@ -43,7 +46,10 @@ func (s *Server) rename(ctx context.Context, params *protocol.RenameParams) (*pr func (s *Server) prepareRename(ctx context.Context, params *protocol.PrepareRenameParams) (*protocol.Range, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/signature_help.go b/internal/lsp/signature_help.go index 5771ef16c0..f65a7a3378 100644 --- a/internal/lsp/signature_help.go +++ b/internal/lsp/signature_help.go @@ -16,7 +16,10 @@ import ( func (s *Server) signatureHelp(ctx context.Context, params *protocol.SignatureHelpParams) (*protocol.SignatureHelp, error) { uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 16907b512c..2e433fda8b 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -165,7 +165,7 @@ type Session interface { View(name string) View // ViewOf returns a view corresponding to the given URI. - ViewOf(uri span.URI) View + ViewOf(uri span.URI) (View, error) // Views returns the set of active views built by this session. Views() []View diff --git a/internal/lsp/symbols.go b/internal/lsp/symbols.go index 66f1654e68..b4d3a3f1f6 100644 --- a/internal/lsp/symbols.go +++ b/internal/lsp/symbols.go @@ -18,7 +18,10 @@ func (s *Server) documentSymbol(ctx context.Context, params *protocol.DocumentSy defer done() uri := span.NewURI(params.TextDocument.URI) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return nil, err + } f, err := view.GetFile(ctx, uri) if err != nil { return nil, err diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go index fbb67bfb25..680d46bc11 100644 --- a/internal/lsp/text_synchronization.go +++ b/internal/lsp/text_synchronization.go @@ -29,7 +29,10 @@ func (s *Server) didOpen(ctx context.Context, params *protocol.DidOpenTextDocume // Open the file. s.session.DidOpen(ctx, uri, fileKind, version, text) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return err + } // Run diagnostics on the newly-changed file. go s.diagnostics(view, uri) @@ -64,7 +67,10 @@ func (s *Server) didChange(ctx context.Context, params *protocol.DidChangeTextDo } } // Cache the new file content and send fresh diagnostics. - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return err + } wasFirstChange, err := view.SetContent(ctx, uri, params.TextDocument.Version, []byte(text)) if err != nil { return err @@ -139,7 +145,10 @@ func (s *Server) didClose(ctx context.Context, params *protocol.DidCloseTextDocu uri := span.NewURI(params.TextDocument.URI) ctx = telemetry.URI.With(ctx, uri) s.session.DidClose(uri) - view := s.session.ViewOf(uri) + view, err := s.session.ViewOf(uri) + if err != nil { + return err + } if _, err := view.SetContent(ctx, uri, -1, nil); err != nil { return err }