gopls/internal/server: don't pop up errors when resolving code actions

When codeAction/resolve fails to resolve edits, don't fail the resolve
operation: instead, simply return the codeaction (with its Command set),
and let the action fail when the client invokes the command.

This may result in redundant work, but avoids pop-ups on clients that
eagerly resolve the code actions (golang/go#75442).

Fixes golang/go#75442

Change-Id: I33df382dbbe02cd33aefdebe6bcb9102dd3deb3c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/704736
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

Author: findleyr

gopls/internal/server/code_action.go

@@ -243,19 +243,32 @@ func (s *server) ResolveCodeAction(ctx context.Context, ca *protocol.CodeAction)
 			return nil, err
 		}
 	}
+	ca.Data = nil
 	if cmd.Command != "" {
+		// Try to resolve codeaction edits by running the command.
+		//
+		// Unfortunately, we can't simply report failure to execute the command, as
+		// some clients (namely, VS Code with the Live Share extension) eagerly
+		// resolve *all* code actions, and returning an error leads to spammy
+		// popups. Instead, we simply set the command on the code action, and let
+		// it fail if/when invoked (golang/go#75442).
+		//
+		// This is arguably a client-side bug, but we work around it as it causes
+		// significant problems for our users.
+		// https://github.com/microsoft/live-share/issues/5293
 		params := &protocol.ExecuteCommandParams{
 			Command:   cmd.Command,
 			Arguments: cmd.Arguments,
 		}
-
 		handler := &commandHandler{
 			s:      s,
 			params: params,
 		}
 		edit, err := command.Dispatch(ctx, params, handler)
 		if err != nil {
-			return nil, err
+			// Resolving edits failed, so just resolve the command.
+			ca.Command = &cmd
+			return ca, nil
 		}
 		var ok bool
 		if ca.Edit, ok = edit.(*protocol.WorkspaceEdit); !ok {

gopls/internal/test/integration/fake/editor.go

@@ -1001,7 +1001,7 @@ func (e *Editor) ApplyCodeAction(ctx context.Context, action protocol.CodeAction
 			if err != nil {
 				return err
 			}
-			action.Edit = ca.Edit
+			action = *ca
 		}
 	}
 

gopls/internal/test/marker/doc.go

@@ -144,16 +144,23 @@ Here is the list of supported action markers:
     completion candidate produced at the given location with provided label
     results in the given golden state.
 
-  - codeaction(start location, kind string, end=location, edit=golden, result=golden, err=stringMatcher)
+  - codeaction(start location, kind string, diag=regexp, end=location, action=golden, edit=golden, result=golden, err=stringMatcher)
 
     Specifies a code action to request at the location, with given kind.
-
-    If end is set, the location is defined to be between start.Start and end.End.
-
-    Exactly one of edit, result, or err must be set. If edit is set, it is a
-    golden reference to the edits resulting from the code action. If result is
-    set, it is a golden reference to the full set of changed files resulting
-    from the code action. If err is set, it is the code action error.
+    If diag is set, the code action must be associated with the given
+    diagnostic.
+    If end is set, the location is defined to be between start.Start and
+    end.End.
+
+    Exactly one of action, edit, result, or err must be set:
+    If action is set, it is a golden reference to a JSON blob representing the
+    resolved code action, which is not applied.
+    If edit is set, it is a golden reference to the edits resulting from the
+    code action.
+    If result is set, it is a golden reference to the full set of changed files
+    resulting from the code action.
+    If err is set, it is expected to match the error resulting from applying
+    the code action.
 
   - codelens(location, title): specifies that a codelens is expected at the
     given location, with given title. Must be used in conjunction with

gopls/internal/test/marker/marker_test.go

@@ -588,7 +588,7 @@ var valueMarkerFuncs = map[string]func(marker){
 // See doc.go for marker documentation.
 var actionMarkerFuncs = map[string]func(marker){
 	"acceptcompletion": actionMarkerFunc(acceptCompletionMarker),
-	"codeaction":       actionMarkerFunc(codeActionMarker, "end", "result", "edit", "err"),
+	"codeaction":       actionMarkerFunc(codeActionMarker, "end", "diag", "action", "result", "edit", "err"),
 	"codelenses":       actionMarkerFunc(codeLensesMarker),
 	"complete":         actionMarkerFunc(completeMarker),
 	"def":              actionMarkerFunc(defMarker),
@@ -2168,7 +2168,7 @@ func changedFiles(env *integration.Env, changes []protocol.DocumentChange) (map[
 }
 
 func codeActionMarker(mark marker, loc protocol.Location, kind string) {
-	if !exactlyOneNamedArg(mark, "edit", "result", "err") {
+	if !exactlyOneNamedArg(mark, "action", "edit", "result", "err") {
 		return
 	}
 
@@ -2181,14 +2181,61 @@ func codeActionMarker(mark marker, loc protocol.Location, kind string) {
 	}
 
 	var (
-		edit    = namedArg(mark, "edit", expect.Identifier(""))
-		result  = namedArg(mark, "result", expect.Identifier(""))
-		wantErr = namedArgFunc(mark, "err", convertStringMatcher, stringMatcher{})
+		edit       = namedArg(mark, "edit", expect.Identifier(""))
+		result     = namedArg(mark, "result", expect.Identifier(""))
+		wantAction = namedArg(mark, "action", expect.Identifier(""))
+		wantErr    = namedArgFunc(mark, "err", convertStringMatcher, stringMatcher{})
 	)
 
-	changed, err := codeAction(mark.run.env, loc.URI, loc.Range, protocol.CodeActionKind(kind), nil)
-	if err != nil && wantErr.empty() {
-		mark.errorf("codeAction failed: %v", err)
+	var diag *protocol.Diagnostic
+	if re := namedArg(mark, "diag", (*regexp.Regexp)(nil)); re != nil {
+		d, ok := removeDiagnostic(mark, loc, false, re)
+		if !ok {
+			mark.errorf("no diagnostic at %v matches %q", loc, re)
+			return
+		}
+		diag = &d
+	}
+
+	action, err := resolveCodeAction(mark.run.env, loc.URI, loc.Range, protocol.CodeActionKind(kind), diag)
+	if err != nil {
+		if !wantErr.empty() {
+			wantErr.checkErr(mark, err)
+		} else {
+			mark.errorf("resolveCodeAction failed: %v", err)
+		}
+		return
+	}
+
+	// If when 'action' is set, we simply compare the action, and don't apply it.
+	if wantAction != "" {
+		g := mark.getGolden(wantAction)
+		if action == nil {
+			mark.errorf("no matching codeAction")
+			return
+		}
+		data, err := json.MarshalIndent(action, "", "\t")
+		if err != nil {
+			mark.errorf("failed to marshal codeaction: %v", err)
+			return
+		}
+		data = bytes.ReplaceAll(data, []byte(mark.run.env.Sandbox.Workdir.RootURI()), []byte("$WORKDIR"))
+		compareGolden(mark, data, g)
+		return
+	}
+
+	changes, err := applyCodeAction(mark.run.env, action)
+	if err != nil {
+		if !wantErr.empty() {
+			wantErr.checkErr(mark, err)
+		} else {
+			mark.errorf("codeAction failed: %v", err)
+		}
+		return
+	}
+	changed, err := changedFiles(mark.run.env, changes)
+	if err != nil {
+		mark.errorf("changedFiles failed: %v", err)
 		return
 	}
 
@@ -2319,29 +2366,20 @@ func quickfixErrMarker(mark marker, loc protocol.Location, re *regexp.Regexp, wa
 // applied. Currently, this function does not support code actions that return
 // edits directly; it only supports code action commands.
 func codeAction(env *integration.Env, uri protocol.DocumentURI, rng protocol.Range, kind protocol.CodeActionKind, diag *protocol.Diagnostic) (map[string][]byte, error) {
-	changes, err := codeActionChanges(env, uri, rng, kind, diag)
+	action, err := resolveCodeAction(env, uri, rng, kind, diag)
+	if err != nil {
+		return nil, err
+	}
+	changes, err := applyCodeAction(env, action)
 	if err != nil {
 		return nil, err
 	}
 	return changedFiles(env, changes)
 }
 
-// codeActionChanges executes a textDocument/codeAction request for the
-// specified location and kind, and captures the resulting document changes.
-// If diag is non-nil, it is used as the code action context.
-func codeActionChanges(env *integration.Env, uri protocol.DocumentURI, rng protocol.Range, kind protocol.CodeActionKind, diag *protocol.Diagnostic) ([]protocol.DocumentChange, error) {
-	// Collect any server-initiated changes created by workspace/applyEdit.
-	//
-	// We set up this handler immediately, not right before executing the code
-	// action command, so we can assert that neither the codeAction request nor
-	// codeAction resolve request cause edits as a side effect (golang/go#71405).
-	var changes []protocol.DocumentChange
-	restore := env.Editor.Client().SetApplyEditHandler(func(ctx context.Context, wsedit *protocol.WorkspaceEdit) error {
-		changes = append(changes, wsedit.DocumentChanges...)
-		return nil
-	})
-	defer restore()
-
+// resolveCodeAction resolves the code action specified by the given location,
+// kind, and diagnostic.
+func resolveCodeAction(env *integration.Env, uri protocol.DocumentURI, rng protocol.Range, kind protocol.CodeActionKind, diag *protocol.Diagnostic) (*protocol.CodeAction, error) {
 	// Request all code actions that apply to the diagnostic.
 	// A production client would set Only=[kind],
 	// but we can give a better error if we don't filter.
@@ -2379,16 +2417,6 @@ func codeActionChanges(env *integration.Env, uri protocol.DocumentURI, rng proto
 	}
 	action := candidates[0]
 
-	// Apply the codeAction.
-	//
-	// Spec:
-	//  "If a code action provides an edit and a command, first the edit is
-	//  executed and then the command."
-	// An action may specify an edit and/or a command, to be
-	// applied in that order. But since applyDocumentChanges(env,
-	// action.Edit.DocumentChanges) doesn't compose, for now we
-	// assert that actions return one or the other.
-
 	// Resolve code action edits first if the client has resolve support
 	// and the code action has no edits.
 	if action.Edit == nil {
@@ -2401,10 +2429,41 @@ func codeActionChanges(env *integration.Env, uri protocol.DocumentURI, rng proto
 			if err != nil {
 				return nil, err
 			}
-			action.Edit = resolved.Edit
+			action = *resolved
 		}
 	}
 
+	return &action, nil
+}
+
+// applyCodeAction applies the given code action, and captures the resulting
+// document changes.
+func applyCodeAction(env *integration.Env, action *protocol.CodeAction) ([]protocol.DocumentChange, error) {
+	// Collect any server-initiated changes created by workspace/applyEdit.
+	//
+	// We set up this handler immediately, not right before executing the code
+	// action command, so we can assert that neither the codeAction request nor
+	// codeAction resolve request cause edits as a side effect (golang/go#71405).
+	var changes []protocol.DocumentChange
+	restore := env.Editor.Client().SetApplyEditHandler(func(ctx context.Context, wsedit *protocol.WorkspaceEdit) error {
+		changes = append(changes, wsedit.DocumentChanges...)
+		return nil
+	})
+	defer restore()
+
+	if action.Edit == nil && action.Command == nil {
+		panic("bad action")
+	}
+
+	// Apply the codeAction.
+	//
+	// Spec:
+	//  "If a code action provides an edit and a command, first the edit is
+	//  executed and then the command."
+	// An action may specify an edit and/or a command, to be
+	// applied in that order. But since applyDocumentChanges(env,
+	// action.Edit.DocumentChanges) doesn't compose, for now we
+	// assert that actions return one or the other.
 	if action.Edit != nil {
 		if len(action.Edit.Changes) > 0 {
 			env.TB.Errorf("internal error: discarding unexpected CodeAction{Kind=%s, Title=%q}.Edit.Changes", action.Kind, action.Title)

gopls/internal/test/marker/testdata/codeaction/failedresolve.txt

@@ -0,0 +1,63 @@
+This test checks that failing to produce edits doesn't fail the
+codeAction/resolve operation. Instead, we simply return the action
+with its command, and let the action fail when its command is applied
+(golang/go#75442).
+
+-- flags --
+-ignore_extra_diags
+
+-- go.mod --
+module golang.org/lsptests/failedresolve
+
+go 1.18
+
+-- resolve.go --
+package extract
+
+func _() {
+	if x := 1; true {
+
+	} else if y := x + 1; true { //@codeaction("x + 1", "refactor.extract.variable", action=act)
+
+	}
+}
+
+-- apply.go --
+package extract
+
+func _() {
+	if x := 1; true {
+
+	} else if y := x + 1; true { //@codeaction("x + 1", "refactor.extract.variable", err=re"cannot find location to insert")
+
+	}
+}
+
+-- @act --
+{
+	"title": "Extract variable",
+	"kind": "refactor.extract.variable",
+	"command": {
+		"title": "Extract variable",
+		"command": "gopls.apply_fix",
+		"arguments": [
+			{
+				"Fix": "extract_variable",
+				"Location": {
+					"uri": "$WORKDIR/resolve.go",
+					"range": {
+						"start": {
+							"line": 5,
+							"character": 16
+						},
+						"end": {
+							"line": 5,
+							"character": 21
+						}
+					}
+				},
+				"ResolveEdits": true
+			}
+		]
+	}
+}