Add support for textDocument/rename function for terraform resource names and local variables

[teddylear]

Use-cases

Rename terraform resources of different types and it's references much easier. Also rename local variables much safer an easier

Proposal

Allow renaming of local variables and terraform resource names more easily.
Example 1:

resource "aws_s3_bucket" "bucket" {
  bucket = "your-bucket-name"
}

resource "aws_s3_bucket_notification" "bucket_notification" {
  bucket = aws_s3_bucket.bucket.id

  topic {
    topic_arn     = aws_sns_topic.topic.arn
    events        = ["s3:ObjectCreated:*"]
    filter_suffix = ".log"
  }
}

Running on name one bucket at the end of first line and renaming foobar:

resource "aws_s3_bucket" "foobar" {
  bucket = "your-bucket-name"
}

resource "aws_s3_bucket_notification" "bucket_notification" {
  bucket = aws_s3_bucket.foobar.id

  topic {
    topic_arn     = aws_sns_topic.topic.arn
    events        = ["s3:ObjectCreated:*"]
    filter_suffix = ".log"
  }
}

Example 2:

locals {
  bucket_name = "your-bucket-name"
}
resource "aws_s3_bucket" "foobar" {
  local.bucket_name = "your-bucket-name"
}

Run on bucket_name on second line and rename to b_name

locals {
  b_name = "your-bucket-name"
}
resource "aws_s3_bucket" "foobar" {
  local.b_name = "your-bucket-name"
}

Related LSP methods

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_rename

Also more than willing to implement this. Just wanted to open issue to make sure this was alright.

[radeksimko]

Hi @teddylear
Thanks for filing the issue.

The feature makes sense. It is something we are planning to support, although there isn't a particular date yet.
Also I appreciate your willingness to implement this. The only issue with this one is that it's entirely new "LSP feature" and we'd likely need to have some conversations about how we can fit it into https://github.com/hashicorp/hcl-lang Go API, i.e. how we can drive this through the schema and make renaming work not just for Terraform, but for any future LS built on top of that library.

If you feel adventurous 😅 you could open an issue in that repo where we can discuss the design details - i.e. what methods would be changed and roughly how, before jumping to the implementation. Otherwise one of us on the internal team will do it at some point.

[teddylear]

@radeksimko thanks for the quick reply! I'll take a stab at implementing, and open a PR once I get a little traction. We can discuss design there if that's alright. Even if this doesn't pan out, fun exercise for me to dig into the codebase.

[teddylear]

@radeksimko Thinking more about this, I'm noticing some issues and weird behavior that I wanted to confirm with the lsp as is. Ideally to do the rename operation, I would use the same logic as textDocument/definition and textDocument/references to get all the regions required to rename, but they seem to not go to where I would expect looking at the unit tests for these.

textDocument/definition seems go to start of the line a resource is located, not the name of the resource itself. For example below:

variable "test" {
}

output "foo" {
	value = var.test
}`

Running textDocument/definition on the first t in line 4 puts the cursor on v in line 0, not the first t as I would expect.

textDocument/references seems to do the same thing in reverse as well, where you run in on variable in line 0 to get the references. My question is are these intentional? Because it seems like they are jumping off of a keyword and not the name of the variable itself. Also, if there is somewhere else I should open this issue/have this discussion please let me know.

[radeksimko]

The thought process behind those ranges was that var.test refers to the whole variable "test" block, not just to test. I am guessing that you are implying that some other servers do it differently? The challenge is that every language is a bit different, especially when comparing a DSL such as HCL and Turing-complete language like Go.

I'm not entirely against changing the ranges, but would be hesitant to do it merely to enable renaming - in part because we'd make the addressing logic potentially more complex and in part because I'm not sure whether people genuinely expect to land just on the label when the reference refers to the whole block.

Also the data blocks have two labels, e.g. data.aws_instance.web refers to data "aws_instance" "web" {. So would you expect to land on just web? I guess for renaming yes, but for go-to-definition? I'm not sure. If each segment of the address was clickable and you were to click on the last web segment then yes - that would make sense, but where would that leave us wrt UX? Where would data segment lead? Wouldn't that lead to confusion if the user cannot click to the whole reference?

The only context where we point to the name is local values, mostly because we cannot simultaneously point to the locals block name and the attribute within.

locals {
  test = "foo"
}
output "foo" {
  value = local.test
}

TL;DR I can see how go-to-definition related logic would be appealing to reuse here, but I think we may not be able to leverage it as-is. Also I'm not sure that we want to imply that a block is rename-able just because go-to-definition/go-to-references is enabled on it. These are the reasons we may need to discuss the design.

[teddylear]

@radeksimko Ah makes sense. Thanks again for quick reply. I'll open a ticket on the upstream repo and talk about design.