-
Notifications
You must be signed in to change notification settings - Fork 24
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
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
a758be4
Add new function signature types
dbanck 7708d63
decoder: Implement SignatureAtPos method
dbanck 58eda6b
decoder: Test SignatureAtPos method
dbanck f2661c1
decoder: Test SignatureAtPos for JSON
dbanck f8e3ad2
Rework ParameterSignature into decoder method
dbanck File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,123 @@ | ||
package decoder | ||
|
||
import ( | ||
"bytes" | ||
"fmt" | ||
|
||
"github.com/hashicorp/hcl-lang/lang" | ||
"github.com/hashicorp/hcl/v2" | ||
"github.com/hashicorp/hcl/v2/hclsyntax" | ||
) | ||
|
||
// SignatureAtPos returns a function signature for the given pos if pos | ||
// is inside a FunctionCallExpr | ||
func (d *PathDecoder) SignatureAtPos(filename string, pos hcl.Pos) (*lang.FunctionSignature, error) { | ||
file, err := d.fileByName(filename) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
body, err := d.bodyForFileAndPos(filename, file, pos) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
var signature *lang.FunctionSignature | ||
hclsyntax.VisitAll(body, func(node hclsyntax.Node) hcl.Diagnostics { | ||
if !node.Range().ContainsPos(pos) { | ||
return nil // Node not under cursor | ||
} | ||
|
||
fNode, isFunc := node.(*hclsyntax.FunctionCallExpr) | ||
if !isFunc { | ||
return nil // No function call expression | ||
} | ||
|
||
f, ok := d.pathCtx.Functions[fNode.Name] | ||
if !ok { | ||
return nil // Unknown function | ||
} | ||
|
||
if len(f.Params) == 0 && f.VarParam == nil { | ||
signature = &lang.FunctionSignature{ | ||
Name: fmt.Sprintf("%s(%s) %s", fNode.Name, f.ParameterSignature(), f.ReturnType.FriendlyName()), | ||
Description: lang.Markdown(f.Description), | ||
} | ||
|
||
return nil // Function accepts no parameters, return early | ||
} | ||
|
||
pRange := hcl.RangeBetween(fNode.OpenParenRange, fNode.CloseParenRange) | ||
if !pRange.ContainsPos(pos) { | ||
return nil // Not inside parenthesis | ||
} | ||
|
||
activePar := 0 // default to first parameter | ||
foundActivePar := false | ||
lastArgEndPos := fNode.OpenParenRange.Start | ||
lastArgIdx := 0 | ||
for i, v := range fNode.Args { | ||
// We overshot the argument and stop | ||
if v.Range().Start.Byte > pos.Byte { | ||
break | ||
} | ||
if v.Range().ContainsPos(pos) || v.Range().End.Byte == pos.Byte { | ||
activePar = i | ||
foundActivePar = true | ||
break | ||
} | ||
lastArgEndPos = v.Range().End | ||
lastArgIdx = i | ||
} | ||
|
||
if !foundActivePar { | ||
recoveredBytes := recoverLeftBytes(file.Bytes, pos, func(byteOffset int, r rune) bool { | ||
return r == ',' && byteOffset > lastArgEndPos.Byte | ||
}) | ||
trimmedBytes := bytes.TrimRight(recoveredBytes, " \t\n") | ||
if string(trimmedBytes) == "," { | ||
activePar = lastArgIdx + 1 | ||
} | ||
} | ||
|
||
paramsLen := len(f.Params) | ||
if f.VarParam != nil { | ||
paramsLen += 1 | ||
} | ||
|
||
if activePar >= paramsLen && f.VarParam == nil { | ||
return nil // too many arguments passed to the function | ||
} | ||
|
||
if activePar >= paramsLen { | ||
// there are multiple variadic arguments passed, so | ||
// we want to highlight the variadic parameter in the | ||
// function signature | ||
activePar = paramsLen - 1 | ||
} | ||
|
||
parameters := make([]lang.FunctionParameter, 0, paramsLen) | ||
for _, p := range f.Params { | ||
parameters = append(parameters, lang.FunctionParameter{ | ||
Name: p.Name, | ||
Description: lang.Markdown(p.Description), | ||
}) | ||
} | ||
if f.VarParam != nil { | ||
parameters = append(parameters, lang.FunctionParameter{ | ||
Name: f.VarParam.Name, | ||
Description: lang.Markdown(f.VarParam.Description), | ||
}) | ||
} | ||
signature = &lang.FunctionSignature{ | ||
Name: fmt.Sprintf("%s(%s) %s", fNode.Name, f.ParameterSignature(), f.ReturnType.FriendlyName()), | ||
Description: lang.Markdown(f.Description), | ||
Parameters: parameters, | ||
ActiveParameter: uint32(activePar), | ||
} | ||
|
||
return nil // We don't want to add any diagnostics | ||
}) | ||
|
||
return signature, nil | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 usesFunctionCallExpr
to express various types likelist
,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 thelist()
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?There was a problem hiding this comment.
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(
ortuple(
is technically aFunctionCallExpr
, no signature help should come up. Only for Terraform< 0.15
do we have the deprecatedlist
andmap
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.There was a problem hiding this comment.
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.