Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding refactor code actions #1525

Closed
1 task done
ziyeqf opened this issue Dec 19, 2023 · 5 comments
Closed
1 task done

Adding refactor code actions #1525

ziyeqf opened this issue Dec 19, 2023 · 5 comments
Labels

Comments

@ziyeqf
Copy link

ziyeqf commented Dec 19, 2023

Language Server Version

v0.32.4

Problem Statement

want to add some code actions to the language server.

Attempted Solutions

No response

Proposal

terraform-ls only supports source.formatAll.terraform action for now.

I'm planing to adding some code actions ( mainly about refactor.extract.xxx ) to it.

To help improve the editing experience. And also for our project which help editing(refactoring) terraform configuration files by those not familar with terraform. We have a basic (rough) list of actions we want to implement and want to discuss about them, some might not fit to be in the language server, I have marked them with ❓.

action list

  1. ❓delete a block
  2. ❓create a block (provider/output/variable), acts like a snippet
  3. ❓show all unused parameters of a resource, ( to help users find which parameter they want to use in an editing)
  4. remove unused parameters of a resource
  5. extract a property to a variable block
  6. extract a property to an output block
  7. ❓extract a property to reference another property in another resource

Any response would be appericiated.

Related LSP methods

  • textDocument/codeActions

References

No response

Help Wanted

  • I'm interested in contributing a fix myself

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment
@ziyeqf
Copy link
Author

ziyeqf commented Dec 19, 2023

I tried to open a PR(#1526) for extract a property to an output block, and it would be appericiated to take a look at it.

@ziyeqf ziyeqf changed the title adding some code actions Adding refactor code actions Dec 19, 2023
@radeksimko
Copy link
Member

Hi @ziyeqf
Thank you for filing the issue. Code Actions is something we plan to work on and are actively researching, I just cannot commit to a particular date when it comes to the implementation. You reminded me that we can and should track individual code actions as public issues though - I will look into creating these!

I really appreciate your suggestions but I think they each need some further discussion before we jump straight to implementing them.


❓delete a block

Could you elaborate on the use case here and how this could be any different from simply selecting the block and hitting Backspace or Delete? One way of potentially making this valuable would be taking care of references to the block being deleted. However, I don't really know what that would look like as removing a reference could mean we end up with wildly incomplete configuration.

❓create a block (provider/output/variable), acts like a snippet

How is this different or better compared to completion, which provides snippets today? Is there a more specific use case you can describe?

❓show all unused parameters of a resource, ( to help users find which parameter they want to use in an editing)
remove unused parameters of a resource

I am not sure what would qualify as "unused parameter" in the context of a resource. Whether or not an attribute is used is a matter for the provider logic, not something one can guess from the configuration alone.

If an attribute is "unused" then it would typically be marked as deprecated by the provider - which is already being surfaced as a diagnostic today. Having some quickfixes to address deprecations would indeed be amazing but it's not simple as providers currently do not expose any more (structured) details about the meaning of a deprecation. i.e. all we know is that attribute is deprecated, we don't know if it's being replaced by another one or removed etc.

I plan to file an issue to track deprecation quick fixes however, in spite of these known challenges.

extract a property to a variable block

This looks like a good idea but IMO we should be thinking about extraction of an expression, not an attribute (that it the right side after =). As in, the attribute remains in place after extraction, and of course we could infer the name of the variable from the attribute, but it's only the expression which can be extracted.

The only potential difficulty here is that this may be a lossy refactoring. We could retain the expression as part of variable default but that field does not allow function calls or references. This may be fine for some users but I think the challenge will be how to communicate this lossy nature in the UX.

Relatedly I think there's another action we could ticket out, which is extraction to a local value (entry under locals {}) which does not have any of these limitations.

extract a property to an output block

I think I roughly understand the use case here but I'd like to confirm. Is the following what you mean?

Original config

resource "azurerm_app_service_environment" "name" {
  name                = "foo"
  resource_group_name = "toot"
  subnet_id           = "noot" # Refactoring action triggered on subnet_id attribute name
}

Refactored config

resource "azurerm_app_service_environment" "example" {
  name                = "foo"
  resource_group_name = "toot"
  subnet_id           = "noot"
}

output "subnet_id" {
  value = azurerm_app_service_environment.example.subnet_id
}

Side note: I am not entirely sure this would fall under the code action kind of refactor.extract as there isn't any code actually being extracted but that's probably a nitpick. I can see how such an action could be useful.

❓extract a property to reference another property in another resource

I'm afraid I don't understand the use case here. Could you elaborate a bit more on this one? Maybe try illustrating it via an example "before and after" as I did above.

@ziyeqf
Copy link
Author

ziyeqf commented Dec 20, 2023

Thanks for your response @radeksimko.

Here is my explaination:

  1. delete a block

As you said, it could help take care of references, and it might be useful in some other tools not only vscode or text editor.
Like the refactoring tool mentioned in the description, it's a wizard which provides some operations rather than manual editing on text.
And yes, tbh, I'm also worried it might not be a suitable candidate for language server espically in text editor context.

2.create a block (provider/output/variable), acts like a snippet

I thought it could pre-define some snippet, and accept customized snippet like:

provider `{provider_name}`{
}

and same as #1, it might be useful in other clients other than text editor.
So here might be a question is if the language server only faces text editor or it could support some editing actions so it could be used by other clients.

3.show all unused parameters of a resource, ( to help users find which parameter they want to use in an editing)
remove unused parameters of a resource

Here the unused might be confusing, I want to say is the not specified optional parameters. Acts like https://github.com/lonegunmanb/newres , it could list the not specified optional parameters for the user to pick and fill it, then remove the still not specified parameters when the edit finished.

I think it could acts like
original:

resource "azurerm_resource_group" "test"{
     name = "test"
}

expand the not specified parameters:

resource "azurerm_resource_group" "test"{
     name = "test"
     tags = (null)
}

then if it's still not specified at the save time:

resource "azurerm_resource_group" "test"{
     name = "test"
}

Still thanks for you insight about deprecation parameters, and yes it's really amazing but hard to implement.

4.extract a property to a variable block

Thanks for your insight, my rough thought is we can detect if the expression could be extract lossless and only allow this action for them. And yes that's a good idea to extract them into locals.

5.extract a property to an output block

That's what exactly what I thought! While I'm not sure about the action name either, just went through the VSCode API list and found it. There might be a better place for this action.

6.extract a property to reference another property in another resource

It could help a user to select a parameter in another resource to reference, like
before:

resource "azurerm_resource_group" "rg" {
   name = "foo"
   location = "bar
}

resource "azurerm_app_service_environment" "env" {
  name                = "foo"
  resource_group_name = "toot"
  subnet_id           = "noot" # Refactoring action triggered on subnet_id attribute name
}

then, the user could select the azurerm_app_service_environment.env.resource_group_name, trigger the action, then there could be a pop-up list

1. azurerm_resource_group.rg.name
2. azurerm_resource_group.rg.location

If he choose the first one, then the config is edited to

resource "azurerm_resource_group" "rg" {
   name = "foo"
   location = "bar
}

resource "azurerm_app_service_environment" "env" {
  name                = "foo"
  resource_group_name = azurerm_resource_group.rg.name
  subnet_id           = "noot" # Refactoring action triggered on subnet_id attribute name
}

Like the 1#, I thought it might help the user to edit the configuration but it might not be a good language server action candidate, but still want to have a discussion about it.

Thanks for your detailed and patiently reply!

@radeksimko
Copy link
Member

1. delete a block

I can imagine some scenarios with interactive UI letting the user resolve tricky situations but I still think it would be very hard to get right. I'd say that the existing validation which flags up orphaned references may be a good enough tool for this, especially once we expand the support as per #1364

If you still believe this is possible to solve via code action, feel free to raise a separate issue because I'm pretty sure this would be big enough problem to deserve its own.

2. create a block (provider/output/variable), acts like a snippet

and same as #1, it might be useful in other clients other than text editor.
So here might be a question is if the language server only faces text editor or it could support some editing actions so it could be used by other clients.

I'm curious in what other contexts do you imagine such a code action in a language server to be useful? I can see the need for some semi-automated CLI-based refactoring tooling outside of LS. I would think of such tooling to be more standalone tooling not reliant on LSP at all. In fact I expect dependency in the other direction. i.e. one could create a CLI tool which e.g. generates provider blocks and the same repository could provide some Go interfaces to be plugged into LS as a code action at some point.

TL;DR I think this should start out as a separate project/repo.

3. show all unused parameters of a resource

I filed a separate issue to track this:

4. extract a property to a variable block

I have created the following issues to track it:

5. extract a property to an output block

I filed a separate issue to track this:

6. extract a property to reference another property in another resource

I have much better understanding of what you mean now - thank you.

I am not too sure about the value of such an action though. It feels like it would require quite a lot of interaction which seems relatively high cost (UX/time) given that the same could be achieved more easily and quickly with just typing and autocompletion.


With all that in mind - thank you again for all these ideas, they will be very helpful in validating the interfaces and overall design of code actions in the context of hcl-lang and this language server.

I'm going to close this issue however in favour of the individual ones I linked above which should be more easily addressable once we have the interfaces in place. Similarly if you come up with other ideas for other code actions which aren't yet covered in our tracker you welcomed to open a new issue, preferably one per code action.

@radeksimko radeksimko closed this as not planned Won't fix, can't repro, duplicate, stale Dec 20, 2023
Copy link

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants