Fix Code Action Resolution (hashicorp#680)

* Fix Code Action Resolution

This fixes several parts of the code action handler workflow.

- Remove `source` as it is a base type and not a specific action we can use.
- Remove `source.fixAll` because the action implies fixing errors, but terraform fmt only formats code style.
- Remove `source.formatAll` to allow fine grained selection of actions.
- Rename `source.formatAll.terraform-ls` to `source.formatAll.terraform` to follow existing Code Action conventions.
- Correctly set the code action value in the returned `lsp.CodeAction` response to the one chosen by the user.
- Exit early so as to not format a document without an explicit request. When additional code actions are added, we can modify this to return a merged list of actions to apply.
- Documents Code Actions, their kinds, and general workflow

Co-authored-by: Radek Simko <radek.simko@gmail.com>

docs/code-actions.md

@@ -0,0 +1,54 @@
+# Code Actions
+
+The Terraform Language Server implements a set of Code Actions which perform different actions on the current document. These commands are typically code fixes to either refactor code, fix problems or to beautify/refactor code.
+
+## Available Actions
+
+### `source.formatAll.terraform`
+
+The server will format a given document according to Terraform formatting conventions.
+
+
+## Usage
+
+### VS Code
+
+To enable the format code action globally, set `source.formatAll.terraform` to *true* for the `editor.codeActionsOnSave` setting and set `editor.formatOnSave` to *false*.
+
+```json
+"editor.formatOnSave": false,
+"editor.codeActionsOnSave": {
+  "source.formatAll.terraform": true
+},
+"[terraform]": {
+  "editor.defaultFormatter": "hashicorp.terraform",
+}
+```
+
+> *Important:* Disable `editor.formatOnSave` if you are using `source.formatAll.terraform` in `editor.codeActionsOnSave`. The `source.formatAll.terraform` code action is is meant to be used instead of `editor.formatOnSave`, as it provides a [guarantee of order of execution](https://github.com/microsoft/vscode-docs/blob/71643d75d942e2c32cfd781c2b5322521775fb4a/release-notes/v1_44.md#explicit-ordering-for-editorcodeactionsonsave) based on the list provided. If you have both settings enabled, then your document will be formatted twice.
+
+If you would like `editor.formatOnSave` to be *true* for other extensions but *false* for the Terraform extension, you can configure your settings as follows:
+
+```json
+"editor.formatOnSave": true,
+"editor.codeActionsOnSave": {
+  "source.formatAll.terraform": true
+},
+"[terraform]": {
+  "editor.defaultFormatter": "hashicorp.terraform",
+  "editor.formatOnSave": false,
+},
+```
+
+Alternatively, you can include all terraform related Code Actions inside the language specific setting if you prefer:
+
+```json
+"editor.formatOnSave": true,
+"[terraform]": {
+  "editor.defaultFormatter": "hashicorp.terraform",
+  "editor.formatOnSave": false,
+  "editor.codeActionsOnSave": {
+    "source.formatAll.terraform": true
+  },
+},
+```

docs/language-clients.md

@@ -59,11 +59,47 @@ in the `initialize` request per LSP.
 
 The server implements a set of opt-in code actions which perform different actions for the user. The code action request is sent from the client to the server to compute commands for a given text document and range. These commands are typically code fixes to either fix problems or to beautify/refactor code.
 
-### Format Document
+See [code-actions](./code-actions.md) for a list of supported code actions.
 
-The server will format a given document according to Terraform formatting conventions.
+A Code Action is an action that changes content in the active editor. Each Code Action is grouped into kinds that have a `command` and/or a series of `edits`. They are triggered either by the user or through events.
 
-This action is available as `source.formatAll.terraform-ls` for clients which configure actions globally (such as Sublime Text LSP) and as `source.formatAll` for clients which allow languageID or server specific configuration (such as VS Code).
+> Documentation for code actions outside of VS Code is unfortunately very limited beyond description of the LSP methods. VS Code internally makes certain assumptions. We follow these assumptions (as documented below) and we recommend other clients to follow these assumptions for best experience, unless/until LSP documentation recommends otherwise.
+
+### Triggers
+
+In VS Code, code action can be _invoked manually_ or _automatically_ based on the respective [CodeActionTriggerKind](https://code.visualstudio.com/api/references/vscode-api#CodeActionTriggerKind).
+
+**Manually invoked** actions come from the contextual in-line💡 icon inside the editor, and are chosen by the user. The user can choose which action is invoked and *then* invoke it. However, in order for the client to display the contextual actions, the client requests LS to "pre-calculate" any actions relevant to the cursor position. Then, when the user selects the action in the UI, the client applies the `edits` or executes the `command` as provided by the server.
+
+**Automatically triggered** actions come from events such as "on save", as configured via the `editor.codeActionsOnSave` setting. These usually do not give much choice to the user, they are either on or off, as they cannot accept user input. For example, formatting a document or removing simple style errors don't prompt for user action before or during execution.
+
+### Kinds
+
+Each `Code Action` has a [`CodeActionKind`](https://code.visualstudio.com/api/references/vscode-api#CodeActionKind). `Code Action Kinds` are a hierarchical list of identifiers separated by `.`. For example in `refactor.extract.function`: `refactor` is the trunk, `extract` is the branch, and `function` is the leaf. The branches and leaves are the point of intended customization, you add new branches and/or leaves for each type of function you perform. In this example, a new code action that operated on variables would be called `refactor.extract.variable`.
+
+#### Custom Kinds
+
+Adding new roots or branches of the hierarchy is not emphasized or really encouraged. In most cases they are meant to be concepts that can be applied generically, not specifically to a certain language. For example, extracting a value to a variable is something common to most languages. You do not need a `refactor.extract.golang.variable` to extract a variable in Go, it still makes sense to use `refactor.extract.variable` whether in Go or in PowerShell.
+
+Keeping to existing `kinds` also helps in registration of supported code actions. If you register support for `source.fixAll` instead of `source.fixAll.languageId`, then a user that has `source.fixAll` in their `codeActionsOnSave` does not need to add specific lines for your extension, all supported extensions are called on save. Another reason is VS Code won't list your custom supported code actions inside `editor.codeActionsOnSave`, and there isn't a way for the extension to get them there. The user will have to consult your documentation to find out what actions are supported and add them without intellisense.
+
+A reason to add custom _kinds_ is if the action is sufficiently different from an existing base action. For example, formatting of the current file on save. The interpretation of `source.fixAll` is to _apply any/all actions that address an existing diagnostic and have a clear fix that does not require user input_. Formatting therefore doesn't fit the interpretation of `source.fixAll`.
+
+A custom kind `source.formatAll.terraform` may format code. A user can request both `source.fixAll` and `source.formatAll.terraform` via their editor/client settings and the server would run `source.formatAll.terraform` only. Other servers may run `source.fixAll` but not `source.formatAll.terraform`, assuming they do not support that custom code action kind.
+
+Unlike generic kinds, custom ones are only discoverable in server-specific documentation and only relevant to the server.
+
+### Execution of Code Actions
+
+A request can have zero or more code actions to perform and the [LS is responsible for processing](https://github.com/microsoft/language-server-protocol/issues/970) all requested actions. The client will send a list of any code actions to execute (which may also be empty).
+
+An empty list means execute anything supported and return the list of edits to the client. This often comes from _manually invoked_ actions by the user. This is the easiest situation for the LS to choose what to do. The LS has a list of supported actions it can execute, so it executes and returns the edits. However, such actions will not include any that either require user input or make a change that could introduce an error (creating files, changing code structure, etc).
+
+A list of actions means to execute anything in that list, which is actually interpreted as _execute the hierarchy of a code action from these actions in the list_. For example, if a client requests a `refactor` action the LS should return all of the possible `refactor` actions it can execute (`refactor.extract`, `refactor.inline`, `refactor.rewrite`, etc.), and the user can choose which to execute. A client that sends `refactor.extract.method` would receive just that single code action from the LS. So a request with one action could return ten results, or just one.
+
+Clients are expected to filter actions to only send unique ones. For example, the user may have configured both `source.fixAll` and `source.fixAll.eslint` which are equivalent from the perspective of a single server (eslint). The client should favour the most specific (in this case `source.fixAll.eslint`) if possible, such that the server doesn't have to perform any de-duplication and the same action doesn't run multiple times.
+
+Clients may also impose a timeout for returning response for any of these requests. If the LS takes too long to process and return an action, the client may either give up and not do anything, or (preferably) display a progress indicator. This timeout may be configurable by the user, but ideally the default one is sufficient.
 
 ## Code Lens
 

internal/langserver/handlers/code_action.go

@@ -23,12 +23,26 @@ func (h *logHandler) TextDocumentCodeAction(ctx context.Context, params lsp.Code
 func (h *logHandler) textDocumentCodeAction(ctx context.Context, params lsp.CodeActionParams) ([]lsp.CodeAction, error) {
 	var ca []lsp.CodeAction
 
+	// For action definitions, refer to https://code.visualstudio.com/api/references/vscode-api#CodeActionKind
+	// We only support format type code actions at the moment, and do not want to format without the client asking for
+	// them, so exit early here if nothing is requested.
+	if len(params.Context.Only) == 0 {
+		h.logger.Printf("No code action requested, exiting")
+		return ca, nil
+	}
+
+	for _, o := range params.Context.Only {
+		h.logger.Printf("Code actions requested: %q", o)
+	}
+
 	wantedCodeActions := ilsp.SupportedCodeActions.Only(params.Context.Only)
 	if len(wantedCodeActions) == 0 {
 		return nil, fmt.Errorf("could not find a supported code action to execute for %s, wanted %v",
 			params.TextDocument.URI, params.Context.Only)
 	}
 
+	h.logger.Printf("Code actions supported: %v", wantedCodeActions)
+
 	fh := ilsp.FileHandlerFromDocumentURI(params.TextDocument.URI)
 
 	fs, err := lsctx.DocumentStorage(ctx)
@@ -46,13 +60,13 @@ func (h *logHandler) textDocumentCodeAction(ctx context.Context, params lsp.Code
 
 	for action := range wantedCodeActions {
 		switch action {
-		case lsp.Source, lsp.SourceFixAll, ilsp.SourceFormatAll, ilsp.SourceFormatAllTerraformLs:
+		case ilsp.SourceFormatAllTerraform:
 			tfExec, err := module.TerraformExecutorForModule(ctx, fh.Dir())
 			if err != nil {
 				return ca, errors.EnrichTfExecError(err)
 			}
 
-			h.logger.Printf("formatting document via %q", tfExec.GetExecPath())
+			h.logger.Printf("Formatting document via %q", tfExec.GetExecPath())
 
 			edits, err := formatDocument(ctx, tfExec, original, file)
 			if err != nil {
@@ -61,7 +75,7 @@ func (h *logHandler) textDocumentCodeAction(ctx context.Context, params lsp.Code
 
 			ca = append(ca, lsp.CodeAction{
 				Title: "Format Document",
-				Kind:  lsp.SourceFixAll,
+				Kind:  action,
 				Edit: lsp.WorkspaceEdit{
 					Changes: map[string][]lsp.TextEdit{
 						string(fh.URI()): edits,

internal/langserver/handlers/code_action_test.go

@@ -101,14 +101,14 @@ func TestLangServer_codeAction_basic(t *testing.T) {
 				"start": { "line": 0, "character": 0 },
 				"end": { "line": 1, "character": 0 }
 			},
-			"context": { "diagnostics": [], "only": ["source.fixAll"] }
+			"context": { "diagnostics": [], "only": ["source.formatAll.terraform"] }
 		}`, tmpDir.URI())}, fmt.Sprintf(`{
 			"jsonrpc": "2.0",
 			"id": 3,
 			"result": [
 				{
 					"title": "Format Document",
-					"kind": "source.fixAll",
+					"kind": "source.formatAll.terraform",
 					"edit":{
 						"changes":{
 							"%s/main.tf": [
@@ -145,3 +145,194 @@ func TestLangServer_codeAction_basic(t *testing.T) {
 			]
 		}`, tmpDir.URI()))
 }
+
+func TestLangServer_codeAction_no_code_action_requested(t *testing.T) {
+	tmpDir := TempDir(t)
+
+	tests := []struct {
+		name    string
+		request *langserver.CallRequest
+		want    string
+	}{
+		{
+			name: "no code action requested",
+			request: &langserver.CallRequest{
+				Method: "textDocument/codeAction",
+				ReqParams: fmt.Sprintf(`{
+						"textDocument": { "uri": "%s/main.tf" },
+						"range": {
+							"start": { "line": 0, "character": 0 },
+							"end": { "line": 1, "character": 0 }
+						},
+						"context": { "diagnostics": [], "only": [""] }
+					}`, tmpDir.URI())},
+			want: `{
+				"jsonrpc": "2.0",
+				"id": 3,
+				"result": null
+			}`,
+		},
+		{
+			name: "source.formatAll.terraform code action requested",
+			request: &langserver.CallRequest{
+				Method: "textDocument/codeAction",
+				ReqParams: fmt.Sprintf(`{
+						"textDocument": { "uri": "%s/main.tf" },
+						"range": {
+							"start": { "line": 0, "character": 0 },
+							"end": { "line": 1, "character": 0 }
+						},
+						"context": { "diagnostics": [], "only": ["source.formatAll.terraform"] }
+					}`, tmpDir.URI())},
+			want: fmt.Sprintf(`{
+				"jsonrpc": "2.0",
+				"id": 3,
+				"result": [
+					{
+						"title":"Format Document",
+						"kind":"source.formatAll.terraform",
+						"edit":{
+							"changes": {
+								"%s/main.tf": [
+									{
+										"range": {
+											"start": {
+												"line": 0,
+												"character": 0
+											},
+											"end": {
+												"line": 1,
+												"character": 0
+											}
+										},
+										"newText": "provider \"test\" {\n"
+									},
+									{
+										"range": {
+											"start": { "line": 2, "character": 0 },
+											"end": { "line": 3, "character": 0 }
+										},
+										"newText": "}\n"
+									}
+								]
+							}
+						}
+					}
+				]
+				}`, tmpDir.URI()),
+		},
+		{
+			name: "source.fixAll and source.formatAll.terraform code action requested",
+			request: &langserver.CallRequest{
+				Method: "textDocument/codeAction",
+				ReqParams: fmt.Sprintf(`{
+						"textDocument": { "uri": "%s/main.tf" },
+						"range": {
+							"start": { "line": 0, "character": 0 },
+							"end": { "line": 1, "character": 0 }
+						},
+						"context": { "diagnostics": [], "only": ["source.fixAll", "source.formatAll.terraform"] }
+					}`, tmpDir.URI()),
+			},
+			want: fmt.Sprintf(`{
+				"jsonrpc": "2.0",
+				"id": 3,
+				"result": [
+					{
+						"title": "Format Document",
+						"kind": "source.formatAll.terraform",
+						"edit": {
+							"changes": {
+								"%s/main.tf": [
+									{
+										"range": {
+											"start": { "line": 0, "character": 0 },
+											"end": { "line": 1, "character": 0 }
+										},
+										"newText": "provider \"test\" {\n"
+									},
+									{
+										"range": {
+											"start": { "line": 2, "character": 0 },
+											"end": { "line": 3, "character": 0 }
+										},
+										"newText": "}\n"
+									}
+								]
+							}
+						}
+					}
+				]
+			}`, tmpDir.URI()),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ls := langserver.NewLangServerMock(t, NewMockSession(&MockSessionInput{
+				TerraformCalls: &exec.TerraformMockCalls{
+					PerWorkDir: map[string][]*mock.Call{
+						tmpDir.Dir(): {
+							{
+								Method:        "Version",
+								Repeatability: 1,
+								Arguments: []interface{}{
+									mock.AnythingOfType(""),
+								},
+								ReturnArguments: []interface{}{
+									version.Must(version.NewVersion("0.12.0")),
+									nil,
+									nil,
+								},
+							},
+							{
+								Method:        "GetExecPath",
+								Repeatability: 1,
+								ReturnArguments: []interface{}{
+									"",
+								},
+							},
+							{
+								Method:        "Format",
+								Repeatability: 1,
+								Arguments: []interface{}{
+									mock.AnythingOfType(""),
+									[]byte("provider  \"test\"   {\n\n      }\n"),
+								},
+								ReturnArguments: []interface{}{
+									[]byte("provider \"test\" {\n\n}\n"),
+									nil,
+								},
+							}},
+					},
+				},
+			}))
+			stop := ls.Start(t)
+			defer stop()
+
+			ls.Call(t, &langserver.CallRequest{
+				Method: "initialize",
+				ReqParams: fmt.Sprintf(`{
+					"capabilities": {},
+					"rootUri": %q,
+					"processId": 123456
+			}`, tmpDir.URI())})
+			ls.Notify(t, &langserver.CallRequest{
+				Method:    "initialized",
+				ReqParams: "{}",
+			})
+			ls.Call(t, &langserver.CallRequest{
+				Method: "textDocument/didOpen",
+				ReqParams: fmt.Sprintf(`{
+				"textDocument": {
+					"version": 0,
+					"languageId": "terraform",
+					"text": "provider  \"test\"   {\n\n      }\n",
+					"uri": "%s/main.tf"
+				}
+			}`, tmpDir.URI())})
+
+			ls.CallAndExpectResponse(t, tt.request, tt.want)
+		})
+	}
+}

internal/langserver/handlers/handlers_test.go

@@ -43,7 +43,7 @@ func initializeResponse(t *testing.T, commandPrefix string) string {
 				"referencesProvider": true,
 				"documentSymbolProvider": true,
 				"codeActionProvider": {
-					"codeActionKinds": ["source", "source.fixAll", "source.formatAll", "source.formatAll.terraform-ls"]
+					"codeActionKinds": ["source.formatAll.terraform"]
 				},
 				"codeLensProvider": {},
 				"documentLinkProvider": {},