Fix Code Action Resolution

[jpogran]

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.

To confirm this works as intended, the following should be tested:

Configure your client with the following settings or follow the usage section of code-actions.md:

"editor.formatOnSave": true,
"[terraform]": {
  "editor.defaultFormatter": "hashicorp.terraform",
  "editor.formatOnSave": false,
  "editor.codeActionsOnSave": {
    "source.formatAll.terraform": true
  },
},

closes #679

[radeksimko]

The fix itself LGTM, do you mind adding a test to cover this?

Thanks!

[rchl]

I've looked at the code below this change and it looks to me that if the user provides multiple only items like source.fixAll and source.formatAll.terraform-ls then the server will perform the formatting multiple times, once for each. If so then that seems non-ideal. The formatting is already very slow and that wouldn't help in running the code action within the allotted time budget.

[radeksimko]

if the user provides multiple only items like source.fixAll and source.formatAll.terraform-ls then the server will perform the formatting multiple times

@rchl James mentioned that to me yesterday himself and I initially said that I would expect such a behaviour, but I see your point about the performance too and forgot that ST doesn't allow code actions to be filtered by language.

We have two options here I think:

• we can try to deduplicate the actions between passed Only and what's actually executed. I presume that would mean restructuring

  terraform-ls/internal/lsp/code_actions.go

  Lines 17 to 22 in 132e82d

  	SupportedCodeActions = CodeActions{
  	lsp.Source: true,
  	lsp.SourceFixAll: true,
  	SourceFormatAll: true,
  	SourceFormatAllTerraformLs: true,
  	}

  and modifying the filtering logic in

  terraform-ls/internal/lsp/code_actions.go

  Lines 37 to 52 in 132e82d

  	func (ca CodeActions) Only(only []lsp.CodeActionKind) CodeActions {
  	// if only is empty, assume that the client wants all code actions
  	// else build mapping of requested and determine if supported
  	if len(only) == 0 {
  	return ca
  	}
  	wanted := make(CodeActions, 0)
  	for _, kind := range only {
  	if v, ok := ca[kind]; ok {
  	wanted[kind] = v
  	}
  	}
  	return wanted
  	}

• or we can just exclude the formatting action from source.fixAll

[rchl]

I do think that the formatting doesn't quite fit within the source.fixAll code action because formatting and fixing are IMO two different concepts.

Also, producing multiple code actions with formatting changes doesn't quite make sense because those have to be run sequentially so if the first one formats the document then the second one will likely fail in mysterious way since it will be applying the changes to an already formatted document.

[rchl]

Side note: Using the versioned workspace edits (new in 3.16 LSP spec) would be preferred since that would guard against applying changes to already changed document.

[radeksimko]

Using the versioned workspace edits (new in 3.16 LSP spec) would be preferred since that would guard against applying changes to already changed document.

I don't disagree but I would hope we can address the problem without rearchitecting it completely.

I do think that the formatting doesn't quite fit within the source.fixAll code action because formatting and fixing are IMO two different concepts.

I don't disagree here either, but I struggle to find any documentation or established convention to support this opinion, as is unfortunately common with many features in LSP.

[rchl]

Using the versioned workspace edits (new in 3.16 LSP spec) would be preferred since that would guard against applying changes to already changed document.

I don't disagree but I would hope we can address the problem without rearchitecting it completely.

Sure. It's not a must have but good to have in general wherever text edits are created.

I do think that the formatting doesn't quite fit within the source.fixAll code action because formatting and fixing are IMO two different concepts.

I don't disagree here either, but I struggle to find any documentation or established convention to support this opinion, as is unfortunately common with many features in LSP.

I tend to think of "fix" addressing an existing diagnostics while "format" formatting the code with no relation to any existing diagnostics.

[jpogran]

Thanks @rchl for the help in working out this implementation! I've read through your conversation here and the content of https://code.visualstudio.com/api/references/vscode-api#CodeActionKind and come to similar conclusions.

I've removed source.fixAll from the list of supported actions as currently terraform fmt only adjusts style, it does not fix errors. I left source.formatAll, and source.formatAll.terraform-ls as they all deal with formatting documents. I've also modified the method to only run an action if one is requested.

I still have to deal with what should happen if multiple format-type actions are requested.

[jpogran]

I tried several approaches to test what would happen if multiple code actions are received, but could not reproduce this case. In each attempt, only one code action was received by the LS from VS Code.

I tried using the all languages editor.CodeActionsOnSave entry.:

"editor.codeActionsOnSave": {
  "source.formatAll": true,
  "source.formatAll.terraform-ls": true
},
"[terraform]": {
  "editor.defaultFormatter": "hashicorp.terraform"
},

I also tried variations of the language specific editor.CodeActionsOnSave entry:

"[terraform]": {
  "editor.codeActionsOnSave": {
    "source.formatAll": true,
    "source.formatAll.terraform-ls": true
  },
  "editor.defaultFormatter": "hashicorp.terraform"
},

Each time, no matter the order, the LS would only receive one code action to process.

[rchl]

I tried several approaches to test what would happen if multiple code actions are received, but could not reproduce this case. In each attempt, only one code action was received by the LS from VS Code.

Possibly VSCode has some special filtering logic.
I've tested this change in ST and got:

Request

{
  "context": {
    "diagnostics": [],
    "only": [
      "source.formatAll",
      "source.formatAll.terraform-ls"
    ]
  },
  "range": {
    "end": {
      "character": 0,
      "line": 150
    },
    "start": {
      "character": 0,
      "line": 0
    }
  },
  "textDocument": {
    "uri": "file:///Users/xxx/app.tf"
  }
}

Response

[
  {
    "edit": {
      "changes": {
        "file:///Users/xxx/app.tf": [
          {
            "newText": "    min     = 1,\n",
            "range": {
              "end": {
                "character": 0,
                "line": 21
              },
              "start": {
                "character": 0,
                "line": 20
              }
            }
          }
        ]
      }
    },
    "kind": "source.formatAll",
    "title": "Format Document"
  },
  {
    "edit": {
      "changes": {
        "file:///Users/xxx/app.tf": [
          {
            "newText": "    min     = 1,\n",
            "range": {
              "end": {
                "character": 0,
                "line": 21
              },
              "start": {
                "character": 0,
                "line": 20
              }
            }
          }
        ]
      }
    },
    "kind": "source.formatAll.terraform-ls",
    "title": "Format Document"
  }
]

Note that the ST implementation was initially based on VSCode's since it's not really specified anywhere how "code-action-on-save" should work.

So I would be fine with changing the ST implementation rather than doing anything about it here.

But regardless, returning two formatting code actions is pretty bad since, if applied, will likely result in the document's code being broken.

[rchl]

Also, on unrelated note, formatting typically takes so much time that it most of the time exceeds the time budget allocated for formatting (2s) and ends up being ignored.

I will likely create a separate issue for that although I guess that it might be hard to address if formatting relies on running terraform itself.

[rchl]

I've looked closer at VSCode's implementation and it does indeed filters/deduplicates code actions in this case. Given that, I'll update the ST LSP implementation to do the same.

But I've also realized that terraform-ls should only announce support for source.formatAll.terraform-ls and not source.formatAll. That is because announcing support for both makes it impossible to do something like:

  "editor.codeActionsOnSave": {
    "source.formatAll": true,
    "source.formatAll.terraform-ls": false, 
  },

Where the general formatting code action is enabled but it's disabled specifically for terraform.

Everything should still work as expected when only announcing support for source.formatAll.terraform-ls.

[radeksimko]

Also, on unrelated note, formatting typically takes so much time that it most of the time exceeds the time budget allocated for formatting (2s) and ends up being ignored.

@rchl I would be curious if you are able to reproduce this by running cat file.tf | time terraform fmt -. I would expect reasonably sized files to be formatted in sub-second. If you are not using any wrapper (such as tfenv, which is unfortunately known to slow down executions) then I'd be interested in some details.

[rchl]

From the terminal it's reasonably fast:

cat app.tf | time terraform fmt
terraform fmt  0.28s user 0.30s system 90% cpu 0.641 total

From the code action it's considerably slower:

:: --> terraform textDocument/codeAction(55): {'range': {'start': {'line': 0, 'character': 0}, 'end': {'line': 150, 'character': 0}}, 'textDocument': {'uri': 'file:///Users/app/infra/app.tf'}, 'context': {'only': ['source.formatAll.terraform-ls'], 'diagnostics': []}}
terraform: 2021/10/22 10:19:28 server.go:628: Received request batch of size 1 (qlen=0)
terraform: 2021/10/22 10:19:28 server.go:180: Dequeued request batch of length 1 (qlen=0)
terraform: 2021/10/22 10:19:28 rpc_logger.go:29: Incoming request for "textDocument/codeAction" (ID 55): {"range":{"start":{"line":0,"character":0},"end":{"line":150,"character":0}},"textDocument":{"uri":"file:///Users/app/infra/app.tf"},"context":{"only":["source.formatAll.terraform-ls"],"diagnostics":[]}}
terraform: 2021/10/22 10:19:28 code_action.go:25: Code action handler initiated
terraform: 2021/10/22 10:19:28 code_action.go:36: Code action "source.formatAll.terraform-ls" received
terraform: 2021/10/22 10:19:28 code_action.go:46: Code action "source.formatAll.terraform-ls" supported
terraform: 2021/10/22 10:19:28 code_action.go:73: Formatting document via "/usr/local/bin/terraform"
terraform: 2021/10/22 10:19:30 rpc_logger.go:50: Response to "textDocument/codeAction" (ID 55): [{"title":"Format Document","kind":"source.formatAll.terraform-ls","edit":{"changes":{"file:///Users/app/infra/app.tf":[{"range":{"start":{"line":20,"character":0},"end":{"line":21,"character":0}},"newText":"    min     = 1,\n"}]}}}]
terraform: 2021/10/22 10:19:30 server.go:252: Completed 1 requests [1.207214489s elapsed]
:: <<< terraform 55: [{'kind': 'source.formatAll.terraform-ls', 'title': 'Format Document', 'edit': {'changes': {'file:///Users/app/infra/app.tf': [{'newText': '    min     = 1,\n', 'range': {'start': {'line': 20, 'character': 0}, 'end': {'line': 21, 'character': 0}}}]}}}]

There are no precise timestamps here but I've manually checked it and this code action request took 1209 ms.

EDIT: Actually there is a precise time from the server also which confirms my measurement.

[rchl]

Actually yes, I'm using tfenv:

/usr/local/bin/terraform@ -> ../Cellar/tfenv/2.2.2/bin/terraform

But formatting from the terminal also uses it:

which terraform
/usr/local/bin/terraform

[jpogran]

Ok, after much research, I've landed on an implementation I think everyone can use. I wrote up alot of my notes inside code-actions.md in the developer section, I still have some more links to add. We may move the documentation around at a later date, but this is a good place for it right now.

I've removed all code action registrations except source.formatAll.terraform to simplify both client and server implementations and give the user the best experience. I explained in the usage section of code-actions.md how a user can use this without stepping on other settings for VS Code, but similar concept should apply for other editors.

I also renamed source.formatAll.terraform-ls to source.formatAll.terraform as the convention seems to be to use the language and not the server name for these kinds of settings.

I also have set it to return early if no actions are provided. We don't have any user invokable actions yet, so we shouldn't perform work if there are no requested actions. Invokable actions are explained more in developer docs section. When we start to add actions like refactor or quick fixes, this can be modified.

Clients will have to mimic VS Code's client behavior of editor.codeActionOnSave actions because VS Code both maintains the order and de-duplicates them. This means the VS Code client ensures the order of actions sent, and expects the LS to execute in that order. For example, VS Code users expect the first entry to be executed before the second, and so on (as of VS Code v 1.44). This means we can't implement any sorting on our language server as it would change behavior depending on which client you use. Link to the Github issue is in the code-actions.md.

[radeksimko]

I'm happy with the fix itself 👍🏻 , but I proposed some changes to the docs which seemed a bit too VS Code specific without explicitly noting that fact.

Also thank you for doing all that research in this area!

[radeksimko]

LGTM 🚀

(Just make sure to use the squash option when pushing the merge button)

[jpogran]

@rchl We've merged this now just to get the fix out there. If there are points you'd like to work on, let us know in an issue and we'll work through it so Sublime works just as well as VS Code does

[rchl]

It looks good to me, thanks.
I will update ST implementation when I find sometime.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.