gopls/internal/cmd: cleanup: unify connection, cmdClient

This cleanup change merges connection and cmdClient, since
both represent the client-side state of a single connection.

Details:
- client holds the server (and its initialize result)
- client.initialize is the operation that ties
  the client and server together. We can use it to
  simplify capabilities_test.go.
- protocol.Server is no longer embedded
- filesMu locking pushed down into client.getFile
- more doc comments
- the server helpers diagnosefiles, executeCommand
  are no longer methods.

(The improvement is positive but rather marginal; this
change grew out of a question about the role and cardinality
of conn, cmdClient and server.)

Change-Id: I9a16a4b8281e61d2cdf116f2b9570fa3a8f5b1f5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/681155
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

gopls/internal/cmd/call_hierarchy.go

@@ -60,7 +60,7 @@ func (c *callHierarchy) Run(ctx context.Context, args ...string) error {
 		TextDocumentPositionParams: protocol.LocationTextDocumentPositionParams(loc),
 	}
 
-	callItems, err := conn.PrepareCallHierarchy(ctx, &p)
+	callItems, err := conn.server.PrepareCallHierarchy(ctx, &p)
 	if err != nil {
 		return err
 	}
@@ -69,7 +69,7 @@ func (c *callHierarchy) Run(ctx context.Context, args ...string) error {
 	}
 
 	for _, item := range callItems {
-		incomingCalls, err := conn.IncomingCalls(ctx, &protocol.CallHierarchyIncomingCallsParams{Item: item})
+		incomingCalls, err := conn.server.IncomingCalls(ctx, &protocol.CallHierarchyIncomingCallsParams{Item: item})
 		if err != nil {
 			return err
 		}
@@ -89,7 +89,7 @@ func (c *callHierarchy) Run(ctx context.Context, args ...string) error {
 		}
 		fmt.Printf("identifier: %s\n", printString)
 
-		outgoingCalls, err := conn.OutgoingCalls(ctx, &protocol.CallHierarchyOutgoingCallsParams{Item: item})
+		outgoingCalls, err := conn.server.OutgoingCalls(ctx, &protocol.CallHierarchyOutgoingCallsParams{Item: item})
 		if err != nil {
 			return err
 		}
@@ -109,7 +109,7 @@ func (c *callHierarchy) Run(ctx context.Context, args ...string) error {
 
 // callItemPrintString returns a protocol.CallHierarchyItem object represented as a string.
 // item and call ranges (protocol.Range) are converted to user friendly spans (1-indexed).
-func callItemPrintString(ctx context.Context, conn *connection, item protocol.CallHierarchyItem, callsURI protocol.DocumentURI, calls []protocol.Range) (string, error) {
+func callItemPrintString(ctx context.Context, conn *client, item protocol.CallHierarchyItem, callsURI protocol.DocumentURI, calls []protocol.Range) (string, error) {
 	itemFile, err := conn.openFile(ctx, item.URI)
 	if err != nil {
 		return "", err

gopls/internal/cmd/capabilities_test.go

@@ -21,10 +21,7 @@ import (
 // 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) {
-	// TODO(bcmills): This test fails on js/wasm, which is not unexpected, but the
-	// failure mode is that the DidOpen call below reports "no views in session",
-	// which seems a little too cryptic.
-	// Is there some missing error reporting somewhere?
+	// server.DidOpen fails to obtain metadata without go command (e.g. on wasm).
 	testenv.NeedsTool(t, "go")
 
 	tmpDir, err := os.MkdirTemp("", "fake")
@@ -41,35 +38,28 @@ func TestCapabilities(t *testing.T) {
 	defer os.RemoveAll(tmpDir)
 
 	app := New()
-
-	params := &protocol.ParamInitialize{}
-	params.RootURI = protocol.URIFromPath(tmpDir)
-	params.Capabilities.Workspace.Configuration = true
-
-	// Send an initialize request to the server.
 	ctx := context.Background()
+
+	// Initialize the client.
+	// (Unlike app.connect, we use minimal Initialize params.)
 	client := newClient(app)
 	options := settings.DefaultOptions(app.options)
 	server := server.New(cache.NewSession(ctx, cache.New(nil)), client, options)
-	result, err := server.Initialize(ctx, params)
-	if err != nil {
+	params := &protocol.ParamInitialize{}
+	params.RootURI = protocol.URIFromPath(tmpDir)
+	params.Capabilities.Workspace.Configuration = true
+	if err := client.initialize(ctx, server, params); err != nil {
 		t.Fatal(err)
 	}
-	// Validate initialization result.
-	if err := validateCapabilities(result); err != nil {
+	defer client.terminate(ctx)
+
+	if err := validateCapabilities(client.initializeResult); err != nil {
 		t.Error(err)
 	}
-	// Complete initialization of server.
-	if err := server.Initialized(ctx, &protocol.InitializedParams{}); err != nil {
-		t.Fatal(err)
-	}
-
-	c := newConnection(server, client)
-	defer c.terminate(ctx)
 
 	// Open the file on the server side.
 	uri := protocol.URIFromPath(tmpFile)
-	if err := c.Server.DidOpen(ctx, &protocol.DidOpenTextDocumentParams{
+	if err := server.DidOpen(ctx, &protocol.DidOpenTextDocumentParams{
 		TextDocument: protocol.TextDocumentItem{
 			URI:        uri,
 			LanguageID: "go",
@@ -82,7 +72,7 @@ func TestCapabilities(t *testing.T) {
 
 	// 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{
+	if err := server.DidChange(ctx, &protocol.DidChangeTextDocumentParams{
 		TextDocument: protocol.VersionedTextDocumentIdentifier{
 			TextDocumentIdentifier: protocol.TextDocumentIdentifier{
 				URI: uri,
@@ -100,7 +90,7 @@ func TestCapabilities(t *testing.T) {
 	}
 
 	// Send a code action request to validate expected types.
-	actions, err := c.Server.CodeAction(ctx, &protocol.CodeActionParams{
+	actions, err := server.CodeAction(ctx, &protocol.CodeActionParams{
 		TextDocument: protocol.TextDocumentIdentifier{
 			URI: uri,
 		},
@@ -118,7 +108,7 @@ func TestCapabilities(t *testing.T) {
 		}
 	}
 
-	if err := c.Server.DidSave(ctx, &protocol.DidSaveTextDocumentParams{
+	if err := server.DidSave(ctx, &protocol.DidSaveTextDocumentParams{
 		TextDocument: protocol.TextDocumentIdentifier{
 			URI: uri,
 		},
@@ -129,7 +119,7 @@ func TestCapabilities(t *testing.T) {
 	}
 
 	// Send a completion request to validate expected types.
-	list, err := c.Server.Completion(ctx, &protocol.CompletionParams{
+	list, err := server.Completion(ctx, &protocol.CompletionParams{
 		TextDocumentPositionParams: protocol.TextDocumentPositionParams{
 			TextDocument: protocol.TextDocumentIdentifier{
 				URI: uri,
@@ -159,10 +149,10 @@ func TestCapabilities(t *testing.T) {
 			t.Errorf("textEdit is not TextEdit nor InsertReplaceEdit, instead it is %T", textEdit)
 		}
 	}
-	if err := c.Server.Shutdown(ctx); err != nil {
+	if err := server.Shutdown(ctx); err != nil {
 		t.Fatal(err)
 	}
-	if err := c.Server.Exit(ctx); err != nil {
+	if err := server.Exit(ctx); err != nil {
 		t.Fatal(err)
 	}
 }

gopls/internal/cmd/check.go

@@ -86,7 +86,7 @@ func (c *check) Run(ctx context.Context, args ...string) error {
 		}
 		checking[uri] = file
 	}
-	if err := conn.diagnoseFiles(ctx, uris); err != nil {
+	if err := diagnoseFiles(ctx, conn.server, uris); err != nil {
 		return err
 	}