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

Fix LocalAddr creation in target collection for JSON files #202

Merged
merged 2 commits into from
Feb 13, 2023

Conversation

dbanck
Copy link
Member

@dbanck dbanck commented Feb 10, 2023

Sometimes, we create local addresses with nil steps, and the LS crashes when iterating over those. This PR aims to fix that.

We don't get a body range for JSON-based configuration files, so we can never set the TargetableFromRangePtr for collected targets.

if bAddrSchema.DependentBodySelfRef && content.RangePtr != nil && selfRefBodyRangePtr == nil {
selfRefBodyRangePtr = content.RangePtr
}

Without this pointer, we can't build a local address. So instead of trying to be clever and set a specific length, we now create all local addresses with a zero length.

Fixes hashicorp/vscode-terraform#1338
Fixes hashicorp/terraform-ls#1157

@dbanck dbanck added the bug Something isn't working label Feb 10, 2023
@dbanck dbanck self-assigned this Feb 10, 2023
@dbanck dbanck requested a review from a team as a code owner February 10, 2023 13:28
radeksimko

This comment was marked as off-topic.

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crash was related to a different PR which I had accidentally checked out and forgot about https://github.com/hashicorp/terraform-schema/pull/174/files

it causes the crash on main as well, so it doesn't seem affected by this PR.


One more thing I forgot to mention in my previous review is that if it makes things easier, we could gate LocalAddr setting on hclsyntax / JSON, since we won't ever need local addresses inside JSON anyway, since we don't do completion inside JSON files anyway.

@dbanck
Copy link
Member Author

dbanck commented Feb 13, 2023

I think gating LocalAddr wouldn't make the code considerably easier to read, so I would keep it as is. Happy to revisit this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform language server crashes panic when targeting resource in JSON config
2 participants