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

Add support for SignatureAtPos #135

Merged
merged 5 commits into from
Mar 15, 2023
Merged

Add support for SignatureAtPos #135

merged 5 commits into from
Mar 15, 2023

Conversation

dbanck
Copy link
Member

@dbanck dbanck commented Sep 23, 2022

This PR adds the SignatureAtPos method to enable support for signature help. Function signatures must be present in the path context for it to work. We added the function signatures to terraform-schema in hashicorp/terraform-schema#145.

If you want to try it, there is a draft PR in terraform-ls, tying it all together: hashicorp/terraform-ls#1077

UX

Nested function calls with variadic parameters
2023-03-14 14 06 18

Functions without any arguments
2023-03-14 14 07 34

No signature help, if passing too many arguments
2023-03-14 14 08 07

Multiline function calls
2023-03-14 14 17 06


@dbanck dbanck added the enhancement New feature or request label Sep 23, 2022
@dbanck dbanck self-assigned this Sep 23, 2022
@dbanck dbanck force-pushed the f-function-support branch 2 times, most recently from 4f04df4 to 7b57575 Compare January 16, 2023 10:43
@dbanck dbanck force-pushed the f-function-support branch 2 times, most recently from 3d0849f to 2461920 Compare March 1, 2023 12:07
@dbanck dbanck changed the title Add support for built-in functions Add support for SignatureAtPos Mar 1, 2023
@dbanck dbanck force-pushed the f-function-support branch 6 times, most recently from 6aff898 to d76eb5a Compare March 14, 2023 12:46
@dbanck dbanck marked this pull request as ready for review March 14, 2023 13:26
@dbanck dbanck requested a review from a team as a code owner March 14, 2023 13:26
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.

Works pretty well for the happy path! 👍🏻 I just left some notes in-line about the implementation and the "negative cases".

Comment on lines +26 to +29
hclsyntax.VisitAll(body, func(node hclsyntax.Node) hcl.Diagnostics {
if !node.Range().ContainsPos(pos) {
return nil // Node not under cursor
}
Copy link
Member

Choose a reason for hiding this comment

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

This is smart, but it doesn't take the schema into account, which means that we provide signature help in all contexts/expressions, regardless of whether function calls are expected there or not.

In most contexts, users will less likely type the function name - where it would be nitpicking, but TypeDeclaration also uses FunctionCallExpr to express various types like list, set etc. and we do want to avoid the signature help there, or do we not? Otherwise we'd end up with signature help e.g. representing the list() function, rather than the type, wouldn't we?

I wouldn't expect you to account for nested expressions (although that would probably be a long-term ideal we can 1st solve that in AnyExpression and then maybe reuse here later) nor prevent signatureHelp for mismatching types.

I think that we could still check whether the outermost constraint for the expression is AnyExpression though?

Copy link
Member Author

Choose a reason for hiding this comment

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

With the current implementation, the feature is decoupled from any expression work and can work stand-alone. Also, nested expressions/function calls should work out of the box, as we always try to find the specific FunctionCallExpr node.

There is no overlap between the functions in the schema and the type declarations. So, even though list( or tuple( is technically a FunctionCallExpr, no signature help should come up. Only for Terraform < 0.15 do we have the deprecated list and map functions which will trigger signature help. Is it worth extending the code to account for that? Adding schema and constraint checks while walking the AST sounds expensive.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't think it's expensive in terms of performance (not significantly more than the implementation of hclsyntax.VisitAll). I can totally see it being expensive in terms of effort/complexity though and I can see why this is worth shipping as-is and how it can deliver immense value even if we don't account for these edge cases. 👍🏻

So with that in mind I'd just leave a note either in a comment, or in a new issue to help us remember to revisit this and/or explain why we took a slightly different approach here which doesn't take schema into account and call the PR done.

decoder/signature_test.go Show resolved Hide resolved
schema/signature.go Outdated Show resolved Hide resolved
schema/signature.go Outdated Show resolved Hide resolved
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