Add API option to getCompletionsAtPosition to expose completion symbol information

[zardoy]

Fixes #51936

#51936 (comment)

Only 2 tests fail: The baseline file api/typescript.d.ts has changed.

@andrewbranch Let me know if I'm going here into right direction.

[andrewbranch]

I think I’m ok with this if the implementation can be restructured to conditionally assign rather than always assign and conditionally delete. Beyond that, the API might need a little bikeshedding. Some things to consider:

• The name of the option
• Once we settle on the API shape, we will want to add JSDoc explaining the functionality and caveats (e.g. the symbol may retain a whole TypeChecker in it, so don’t store it longer than you need it!)
• Is it worth it to make a getCompletionsAtPosition overload for when the option is true that changes the return type?
• Should the symbol go right on CompletionEntry, or should it be wrapped in an apiData property or something?

@jakebailey @gabritto do you have any input or overall concerns?

[gabritto]

I don't think I have any concerns, this looks fine besides the comments you already left. @andrewbranch I do have a question though: if we make the symbol part of the API, what would that imply for us regarding future, possibly breaking changes in completions? For example, until recently I think (but I might be wrong) we only had one completion entry per symbol, but that changed with object method completions (although that feature can be disabled). Would we have to be more mindful of that kind of change going forward?

[zardoy]

In first version this case wasn't handled, now fixed to be consistent

[zardoy]

Should the symbol go right on CompletionEntry, or should it be wrapped in an apiData property or something?

From my perpective it is easiest solution for us and most convenient for consumer

Also feel free to edit jsdoc

[typescript-bot]

The TypeScript team hasn't accepted the linked issue #51936. If you can get it accepted, this PR will have a better chance of being reviewed.

[andrewbranch]

For example, until recently I think (but I might be wrong) we only had one completion entry per symbol, but that changed with object method completions (although that feature can be disabled). Would we have to be more mindful of that kind of change going forward?

That’s not an issue for this particular change, since still every completion has at most one symbol. The only way this property could break a future API change is if individual completions started being derived from multiple symbols 🤔.

Thinking about it this morning, though, I’m wondering if the change needs to be so generic. What additional data might someone need on a completion item in the future? Maybe we should just call the option includeSymbol, and put symbol as an optional property on CompletionEntry. The extra types and overload really don’t help that much, because symbol is always optional at best.

[zardoy]

Maybe we should just call the option includeSymbol, and put symbol as an optional property on CompletionEntry.

tbh I more like general name, as idea can be extended without adding new properties. Like what if also type to string completions or container of completions will also be added in future (though I cant imagine a usecase for them for now)? Anyway, its just a naming for current implementation so let me know what you think guys.

The extra types and overload really don’t help that much, because symbol is always optional at best.

It will change already existing calls so plugin devs might start wondering what is new symbol property is added and why its never defined. On the other hand jsdoc can be added to clarify it, so this is totally fine to me. Don't mind of any change here.

[zardoy]

To summarize requested changes here:

• Replace all includeApiData to includeSymbol
• Remove overload, CompletionEntryWithApiData and add symbol to just CompletionEntry

@andrewbranch Is that right and anything else to consider?

I don't really care of typings and so on, I'm only interested in adding a way to use symbol :)

P.S. feel free to make quick edits directly!

[andrewbranch]

• Replace all includeApiData to includeSymbol
• Remove overload, CompletionEntryWithApiData and add symbol to just CompletionEntry

Discussed this in #52790 and yes, along with my suggested JSDoc change, if you do these two things it should be good to go 👍

[zardoy]

@andrewbranch All done, but decided to also add jsdoc on symbol property itself (since its always visible to user even if option is off). Anyway, don't see anything bad in extra documentation 😄

[andrewbranch]

Looks good, just run the 'Public API' test and accept the baselines to make the tests green