Enable method signature completion for object literals

[gabritto]

Fixes #46590.

This PR enables signature completions for methods in object literals. In the following code:

interface IFoo {
    bar(x: number): void;
}

const obj: IFoo = {
    |
}

we now offer as completion: bar(x: number): void { }:

interface IFoo {
    bar(x: number): void;
}

const obj: IFoo = {
    bar(x: number): void {
        |  
    },
}

Design decisions

• This signature completion is only offered for object literal expressions, and for methods (properties that are functions).
• For an object literal method, we now offer two completion entries:
  • A regular one, whose insertText is simply the name of the method (e.g. bar in the example above), which is the old behavior
  • A signature one, whose insertText is the method declaration (e.g. bar(x: number): void {} in the example above)
    The regular completion entry should appear before its corresponding signature one
• No support for methods that have overloads, as there's no (non-hacky, generalizable) way to have overloads declared inside object literals.
• No support for accessors. This could be added later, if we think it's important, but for now it wasn't completely clear to me when/how we should offer accessor signatures, so I left it out.
• No support for JS, because that would require us being able to handle JSDoc annotations when creating the completion signature. If we decide to add this later, we could probably share a good chunk of the implementation and make it so that both this and class member completions can work in JS.
• For now, we always generate method declarations (e.g. bar(x: number): void {}). If there's a need for it, we can make this configurable later, and generate other forms of declaration (e.g. bar: (x: number): void => {})
• We don't add async (and similarly don't support the asterisk modifier for generators), see discussions in await completions should auto-add an async modifier to containing method #48093.

[gabritto]

Things that I have to fix in this PR: for some reason, the entries are shown in the wrong order in vscode. I'm investigating that to see what's the best way to address it.

Edit: updated the sort text on the method signature completion entries so that they show up lower than the normal entries. They now have a sortText that is one lower than the normal entries would have.

[andrewbranch]

Looking good at a first glance—I’ll try to give it a try soon.

[RyanCavanaugh]

@typescript-bot pack this

[typescript-bot]

Heya @RyanCavanaugh, I've started to run the tarball bundle task on this PR at 814561f. You can monitor the build here.

[typescript-bot]

Hey @RyanCavanaugh, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/121521/artifacts?artifactName=tgz&fileId=DA6CBC84DA5E5CF30D3454243FDA8980270092508288925B1CD311BF1EE7888902&fileName=/typescript-4.7.0-insiders.20220309.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

[gabritto]

I realized I put the feature behind a new flag, so it is not set by default. Going to remove that part of the code in a bit so that people can use this more easily.

[gabritto]

@typescript-bot pack this

[typescript-bot]

Heya @gabritto, I've started to run the tarball bundle task on this PR at b032aa5. You can monitor the build here.

[andrewbranch]

@typescript-bot pack this

[typescript-bot]

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 411bc18. You can monitor the build here.

[typescript-bot]

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/122534/artifacts?artifactName=tgz&fileId=60AB0E85050006EA44CE384DB33288D1AEF05E4219B1A590F13CE020D289F31B02&fileName=/typescript-4.7.0-insiders.20220323.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

[andrewbranch]

If you close the detail pane of the completions widget, the displayParts of the name entry gets inlined into the right side of the label and you can’t really tell these apart anymore:

I think we discussed in an editor sync putting the signature, or something like the first line of insertText, in the left side of the label. Quick hack:

No strong opinion on whether it should end in { or { … } or no braces, but I think having something very similar to what’s about to get inserted left-aligned in the label makes it more clear. (I think this change should apply to class method snippet completions too.)

Another side note is that VS Code and the LSP spec support another newer label scheme with three parts: one for the main part of the name (bar in this case), one rendered right next to it but perhaps with different font treatment (the rest of the method signature in this case) and one right-aligned bit. If we already supported that format, it would be the clear way to go as it was originally designed specifically with names, signatures, and module names/specifiers in mind for the three label parts respectively. We don’t need to adopt it right now, but its design does reinforce that the name and the rest of the signature should be rendered together on the left side of the label. That will make for a less intrusive change when we someday align more with the LSP spec.

[gabritto]

Another side note is that VS Code and the LSP spec support another newer label scheme with three parts: one for the main part of the name (bar in this case), one rendered right next to it but perhaps with different font treatment (the rest of the method signature in this case) and one right-aligned bit.

I think I want to try and add support for it after this PR. It's doable to do it, right? We need a new field in CompletionEntry, and to convert between our completion entry field and the vscode entry label.details field in the vscode extension.

[andrewbranch]

Yes, I think it should be possible.

[andrewbranch]

Ok, so the main thing I’m wondering is if we can consolidate a few of the separate steps here. My understanding of the process is something like this:

1. When processing symbols in tryGetObjectLikeCompletionSymbols, you do a fast check via isObjectLiteralMethodSymbol and as long as that passes, you push the symbol and an origin (with no info other than kind) onto the symbols array.

2. After pushing all the symbols on, you update their SortText—I can’t quite tell why this can’t be done in (1).

3. At the very end of the completions process, in createCompletionEntry, you call into this function here for any symbols that got their origin set accordingly in (1). This function does all the hard work and introduces some extra conditions which might result in us dropping the entry altogether.

I could definitely be missing something, but I don’t see much of a reason for these three steps to be separate. It seems like you could go as far as creating the MethodDeclaration where you currently are only doing step (1), and then store that on the origin. That way, if you can’t create the MethodDeclaration, you don’t need to bother with setting SortText. In other words, by the time you push a symbol and an origin onto those arrays, you know for sure it will be able to turn into a CompletionEntry because most of the work to do it is already done. It seems like this would consolidate the implementation a little. Does that make sense, or what did I miss?

[gabritto]

That made sense. I updated the code so that (3) is now done in (1).

About (2), we can't do that when pushing the method snippet symbols, because we need all of the object completions to have the transformed format. Still, I moved the functions that set/transform sort texts to avoid doing some checks.

[andrewbranch]

Makes sense about (2). Looks much better now without having to create the Set of seen symbols 👍