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

reference: Update type matching to align w/ Terraform's #243

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Apr 4, 2023

Terraform itself basically doesn't use cty's TestConformance, but instead uses the convert.Convert function. This makes the type comparison a lot more lenient, where it accepts basically any primitive type for any other. It does however reject mismatch between complex types.

Other products / language servers may benefit from the stricter comparison (as implemented by TestConformance) but we do not have enough data to understand this problem space and so we optimise for Terraform for now. This should not hurt other products unless they have a lot of references to filter. At worst they'll get more completion items and hover/semtokens for references which are not relevant, but should still get the ones which are relevant. They may also happen to implement type comparison same way as Terraform. 🤷🏻

Why now?

Because #232 is introducing support for functions and needs to decide how to compare constraints with return types, which is a similar problem to comparing references and so I believe the two places should agree on how to do this.

The alternative would be reusing the strict TestConformance there, which is what I first attempted, but it had some other odd edge cases, where cty.DynamicPseudoType [function return type] is considered mismatch for cty.String constraint.

Example

variable "test" {
  type    = string
  default = "42"
}

output "test" {
  value = abs(var.test)
}

^ this is valid configuration which produces 42 (number)


There is a wider conversation to have about promoting best practice by suppressing (but accepting) patterns which do not follow best practice. This could mean not providing completion for examples such as the one above, but providing hover, semantic tokens, go-to-* etc. This has two pre-requisites though:

  • We'd have to use different matching logic for completion than for all other methods
  • We should have other (more obvious and visible) ways to communicate this, rather than just suppressing completion items.
    • This could be addressed as part of decoder: Introduce ValidateFilePerSchema() #57 but we'll need to make sure we fully understand the impact of precise comparisons there and the edge cases (e.g. by running a lot of existing configuration through the validation logic).

Terraform itself basically doesn't use cty's TestConformance, but instead uses the convert.Convert function.
This makes the type comparison a lot more lenient, where it accepts basically any primitive type for any other.
It does however reject mismatch between complex types.

Other products / language servers may benefit from the stricter comparison (as implemented by TestConformance)
but we do not have enough data to understand this problem space and so we optimise for Terraform for now.
This should not hurt other products unless they have a lot of references to filter. At worst they'll get
*more* completion items and hover/semtokens for references which are not relevant, but should still get
the ones which are relevant.
@radeksimko radeksimko added the enhancement New feature or request label Apr 4, 2023
@radeksimko radeksimko requested a review from a team as a code owner April 4, 2023 07:12
@radeksimko radeksimko self-assigned this Apr 4, 2023
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

👍 LGTM, thanks for the in-depth explanation

@radeksimko radeksimko merged commit 31f5cd3 into main Apr 4, 2023
@radeksimko radeksimko deleted the f-ref-matching-convert branch April 4, 2023 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants