-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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 method signature completions #46370
Conversation
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
Fair to say I significantly underestimated how much work this feature would take 😅 |
This comment has been minimized.
This comment has been minimized.
Yes, this is a bug in |
I fixed the bug of the extra overload. I'm open to suggestions, but I think we should leave this as is for now, and revisit later if/when we are improving the generation of implementation signatures. |
Moving the escaping to a custom writer looks great! Throw this behind a user preference and I think this is good to go. We'll put the rest of @amcasey’s suggestions in a follow-up bug to improve the pre-existing “add missing member” codefix. |
Edit: |
For future reference: I looked into this and that's because the needed imports get added to a |
Ah, that is something we should be able to fix later—that’s how auto-import completions work; the changes are calculated during CompletionDetails and attached to the that response on |
@gabritto are you planning to change the order of the tabstops before merging? Anything else you want to get in before a final review? |
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.
Nice work 🌟
replacementSpan: { | ||
fileName: "", | ||
pos: 0, | ||
end: 0, | ||
}, |
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 guess VS Code is ignoring this, but this doesn’t look right 🤔
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 guess I thought this was the default replacement span. Should this have been undefined
?
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.
Yeah, I think so.
* prototype creation for method override completion snippet * WIP: start using codefix `addNewNodeForMemberSymbol` to create method decl node * update type of addNewNodeForMemberSymbol * add more tests and support more cases * add more tests and fix some details * wip: more fixes and tests * expose check override modifier in checker * fix test * WIP: add snippet support * WIP: snippet support on emitter, adding snippets in completions * make add snippets work with overloads (not synced) * fix snippet adding * rebase * WIP: try to add snippet escaping in emitter * support escaping in snippets * small fixes; fixed tests * more tests and fixes * fix new tests * fix modifier inheritance for overloads * merge conflict fixes; remove comments * throw error if setOptions is called but not implemented * fix newline handling * fix weird stuff * fix tests * fix more tests * Fix unbound host.getNewLine * fix isParameterDeclaration changes * rename diagnostic to status and remove snippets from public api * rename emitter functions + fix indentation * check completion kind before calling isclasslikemembercompletion * fix missing type parameters * Revert "fix missing type parameters" This reverts commit 7bdeaa8. * add isAmbient flag to addNewNodeForMemberSymbol * add test for abstract overloads * refactor snippet escaping support * add user preference flag for enabling class member snippets * update API baseline * update tabstop order Co-authored-by: Andrew Branch <[email protected]>
Fixes #45670.
Overview
Currently, when we offer completions for methods inside a class-like declaration, we only offer method names:
but in many cases, we have enough information to offer the whole method signature/declaration, like so:
This PR makes it so that we offer method and property declarations inside class declarations whenever possible.
The rough outline of how it works is:
completions.ts
, when answering a completions request, we gather symbols to be offered as completions, and then we callcreateCompletionEntry
for each of those symbols.createCompletionEntry
then converts a symbol into a properCompletionEntry
, which is what we will send back in response to a completion request.CompletionEntry
is created for a symbol, in the cases mentioned above.The main thing is the
insertText property of a
CompletionEntry:
insertTextis the text that is inserted when a user accepts a completion, so for this new feature we want the
insertText` to be a whole signature/declaration, instead of just a name/identifier.createCompletionEntry
:isClassLikeMemberCompletion(symbol)
: this decides whether or not a completion symbol is a class member and we should offer whole signature completions for it;getEntryForMemberCompletion(symbol)
for that completion symbol. This function will define what theinsertText
should be for that completion.Core functionality
The main functionality of converting a symbol into a member node is provided by
codefix.addNewNodeForMemberSymbol
, called insidegetEntryForMemberCompletion
. This function already existed and is used by multiple codefixes, likeclassIncorrectlyImplementsInterface
, to generate the member nodes that are missing in a class-like declaration.Since
codefix.addNewNodeForMemberSymbol
was implemented for this missing member node scenario, there are some things we need to add on top of it to make everything work for a completions scenario, e.g. checking whether we should add anoverride
modifier. Those additional constraints are mostly dealt with in the callback function passed tocodefix.addNewNodeForMemberSymbol
.Snippets
Completions can use snippets to control where the cursor should go to when the completion is inserted (i.e. tab stops), to indicate the user should fill in some part of the completion, and to make it easier for the user to change parts of the completion-inserted text (i.e. placeholders). Snippet syntax reference can be found here: https://code.visualstudio.com/docs/editor/userdefinedsnippets#_snippet-syntax.
This new completion, so far, inserts method signatures or declarations. With that in mind, there are many places in such a signature/declaration where we could add snippet placeholders, to make it easy for the user to modify parts of the completion.
First, a final tabstop (i.e.
$0
) is always placed in the completion declaration's implementation body, if there is one. This means that the cursor will be at the empty body as its final position, once the completion is inserted.Other than that, several parts of the declaration can be modified in ways that make sense (i.e. the code will type check). For example, the parameter and return types can be modified, as long as they conform to the type of the declaration being overridden/implemented.
The question of where to add placeholders/tabstops was discussed in a meeting. One of the initial points was that there were some cases when it was very likely that the user would want to change parts of the declaration, e.g. when changing the parameter types of an overload implementation, and in the rest of the cases it was less likely that the user would want to do so.
However, it was pointed out that snippets rely on "muscle memory" and predictability about where there will be tabstops/placeholders. So even though for the same declaration position (e.g. parameter type), we sometimes want to have a placeholder and sometimes not, it is better to favor uniformity and have the same tabstops/placeholders in the same declaration parts.
Emitter changes
The main concern implementation-wise with snippet support is emitting an AST into a string properly.
Some completions already use snippets (e.g. import completions), but these completion snippets are manipulated as strings. The completion feature in this PR requires a more complex completion text (e.g. a method signature), which becomes very tricky to manipulate as string, and is better to handle as an AST. So I decided to add support for snippets in our AST and emitter directly.
There is a new property,
SnippetElement
, added toEmitNode
. AnEmitNode
is an optional property of nodes that gathers information pertinent to the emitter. The newSnippetElement
property, when present in the emit node of a node, indicates that that node should be interpreted as some type of snippet element, e.g. a placeholder or a tabstop. The emitter will then handle printing that node into an appropriate string.Example:
In the code
the
string | number
type node will have aSnippetElement
of kindSnippetKind.Placeholder
attached to its emit node.Then, when the emitter is printing this type node, it will first print
${n:
, then it will recursively print the (escaped) actual node, i.e.string | number
, and then it will print}
. In the end, the node will be printed as${n:string | number}
, which is the syntax for a placeholder whose text isstring | number
. (Note:n
refers to a natural number here, corresponding to the order in which this placeholder/tabstop will be visited when the user presses tab sequentially).Another thing added to the emitter is support for escaping text that would make the editor incorrectly understand a node as a snippet element.
For instance, if we have a method declaration like
"$usd"(a: number): number
, if we don't escape the$
, the editor will think that$usd
is a snippet variable instead of an identifier name.Future work