From 69a79c76c724d155f688554366acffe9536449c4 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 21 Nov 2019 23:42:27 -0500 Subject: [PATCH] internal/lsp: add some minimal validation for client capabilities We've had a couple of breakages with changes to the protocol that are bugs on our end. It's hard to review changes to the protocol, and it's not reasonable to expect that we would remember the correct types for everything, so we should have a test that validates some basic expectations about the expected responses. We can add more here as issues come up. Also, change RenameProvider back to an interface. Updates golang/go#32703 Change-Id: Ic5f0b0ece40b05e4425cd98ab7bf18db3ad74601 Reviewed-on: https://go-review.googlesource.com/c/tools/+/208272 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Ian Cottrell --- internal/lsp/cmd/capabilities_test.go | 101 ++++++++++++++++++++++++++ internal/lsp/general.go | 14 ++-- internal/lsp/protocol/tsprotocol.go | 2 +- 3 files changed, 108 insertions(+), 9 deletions(-) create mode 100644 internal/lsp/cmd/capabilities_test.go diff --git a/internal/lsp/cmd/capabilities_test.go b/internal/lsp/cmd/capabilities_test.go new file mode 100644 index 0000000000..68683d281c --- /dev/null +++ b/internal/lsp/cmd/capabilities_test.go @@ -0,0 +1,101 @@ +package cmd + +import ( + "context" + "io/ioutil" + "os" + "path/filepath" + "testing" + + "golang.org/x/tools/internal/lsp" + "golang.org/x/tools/internal/lsp/cache" + "golang.org/x/tools/internal/lsp/protocol" + "golang.org/x/tools/internal/span" + errors "golang.org/x/xerrors" +) + +// TestCapabilities does some minimal validation of the server's adherence to the LSP. +// The checks in the test are added as changes are made and errors noticed. +func TestCapabilities(t *testing.T) { + tmpDir, err := ioutil.TempDir("", "fake") + if err != nil { + t.Fatal(err) + } + tmpFile := filepath.Join(tmpDir, "fake.go") + if err := ioutil.WriteFile(tmpFile, []byte(""), 0775); err != nil { + t.Fatal(err) + } + if err := ioutil.WriteFile(filepath.Join(tmpDir, "go.mod"), []byte(`module fake`), 0775); err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + app := New("gopls-test", tmpDir, os.Environ(), nil) + c := newConnection(app) + ctx := context.Background() + defer c.terminate(ctx) + + params := &protocol.ParamInitialize{} + params.RootURI = string(span.FileURI(c.Client.app.wd)) + params.Capabilities.Workspace.Configuration = true + + // Send an initialize request to the server. + ctx, c.Server = lsp.NewClientServer(ctx, cache.New(app.options), c.Client) + result, err := c.Server.Initialize(ctx, params) + if err != nil { + t.Fatal(err) + } + // Validate initialization result. + if err := validateCapabilities(result); err != nil { + t.Error(err) + } + // Complete initialization of server. + if err := c.Server.Initialized(ctx, &protocol.InitializedParams{}); err != nil { + t.Fatal(err) + } + + // Open the file on the server side. + uri := protocol.NewURI(span.FileURI(tmpFile)) + if err := c.Server.DidOpen(ctx, &protocol.DidOpenTextDocumentParams{ + TextDocument: protocol.TextDocumentItem{ + URI: uri, + LanguageID: "go", + Version: 1, + Text: `package main; func main() {};`, + }, + }); err != nil { + t.Fatal(err) + } + + // If we are sending a full text change, the change.Range must be nil. + // It is not enough for the Change to be empty, as that is ambiguous. + if err := c.Server.DidChange(ctx, &protocol.DidChangeTextDocumentParams{ + TextDocument: protocol.VersionedTextDocumentIdentifier{ + TextDocumentIdentifier: protocol.TextDocumentIdentifier{ + URI: uri, + }, + Version: 2, + }, + ContentChanges: []protocol.TextDocumentContentChangeEvent{ + { + Range: nil, + Text: `package main; func main() {}; func main2() {};`, + }, + }, + }); err != nil { + t.Fatal(err) + } +} + +func validateCapabilities(result *protocol.InitializeResult) error { + // If the client sends "false" for RenameProvider.PrepareSupport, + // the server must respond with a boolean. + if v, ok := result.Capabilities.RenameProvider.(bool); !ok { + return errors.Errorf("RenameProvider must be a boolean if PrepareSupport is false (got %T)", v) + } + // The same goes for CodeActionKind.ValueSet. + if v, ok := result.Capabilities.CodeActionProvider.(bool); !ok { + return errors.Errorf("CodeActionSupport must be a boolean if CodeActionKind.ValueSet has length 0 (got %T)", v) + } + return nil +} diff --git a/internal/lsp/general.go b/internal/lsp/general.go index 3b371bcdd1..c8c11aeddf 100644 --- a/internal/lsp/general.go +++ b/internal/lsp/general.go @@ -52,7 +52,7 @@ func (s *Server) initialize(ctx context.Context, params *protocol.ParamInitializ } } - var codeActionProvider interface{} + var codeActionProvider interface{} = true if ca := params.Capabilities.TextDocument.CodeAction; len(ca.CodeActionLiteralSupport.CodeActionKind.ValueSet) > 0 { // If the client has specified CodeActionLiteralSupport, // send the code actions we support. @@ -61,14 +61,12 @@ func (s *Server) initialize(ctx context.Context, params *protocol.ParamInitializ codeActionProvider = &protocol.CodeActionOptions{ CodeActionKinds: s.getSupportedCodeActions(), } - } else { - codeActionProvider = true } - // This used to be interface{}, when r could be nil - var renameOpts protocol.RenameOptions - r := params.Capabilities.TextDocument.Rename - renameOpts = protocol.RenameOptions{ - PrepareProvider: r.PrepareSupport, + var renameOpts interface{} = true + if r := params.Capabilities.TextDocument.Rename; r.PrepareSupport { + renameOpts = protocol.RenameOptions{ + PrepareProvider: r.PrepareSupport, + } } return &protocol.InitializeResult{ Capabilities: protocol.ServerCapabilities{ diff --git a/internal/lsp/protocol/tsprotocol.go b/internal/lsp/protocol/tsprotocol.go index 5657378cb8..209b3ead49 100644 --- a/internal/lsp/protocol/tsprotocol.go +++ b/internal/lsp/protocol/tsprotocol.go @@ -2477,7 +2477,7 @@ type ServerCapabilities = struct { * specified if the client states that it supports * `prepareSupport` in its initial `initialize` request. */ - RenameProvider RenameOptions/*boolean | RenameOptions*/ `json:"renameProvider,omitempty"` + RenameProvider interface{}/*boolean | RenameOptions*/ `json:"renameProvider,omitempty"` /** * The server provides folding provider support. */