Completion details can’t distinguish between a default export and a named export matching the filename

[andrewbranch]

Bug Report

🔎 Search Terms

auto import default

🕗 Version & Regression Information

This has probably been a bug since auto-imports were invented

💻 Code

// @Filename: someModule.ts
export const someModule = 0;
export default 1;

// @Filename: index.ts
someMo/**/

🙁 Actual behavior

Two auto-import completions, but both details resolve to the named export.

🖼 GIF demonstrating problem

🙂 Expected behavior

Two auto-import completions that correctly differentiate between the named export and the default export.

Discussion

The reason this happens is that the info we have to go on in getCompletionDetails is a really poor starting point for trying to figure out what completion entry we’re talking about. All we have is a name and source, where name is the identifier text you want to insert with the completion, and source is the symbol name of the module. Notable information we don’t have, because we threw it away at the end of getCompletionsAtPosition, includes

• the file name of the external module (if the module was an external module—it can also be an ambient module)
• what kind of export the completion came from / what kind of import we expected to use (named, default, CJS, namespace)
• whether the export was found in the main program or the AutoImportProvider

Throwing all of this away has two main consequences:

1. It’s way more work than should be necessary to find the right export and module again, because there’s no lookup table for module symbol names. So instead, we randomly loop through every source file and ambient module we know about and check whether its symbol’s name matches source. Then we loop through every member/property of its exports seeing if the names we come up with there match name.
2. It’s ambiguous! name is not always the actual name from the exports symbol table; default and export= get converted into a valid identifier for the import, which can be the file name (camel-cased). In the example above, the file name substitution for the default gets “shadowed” by the named export.

Fixing this is a near-prerequisite for #31658—it gets extra ugly trying to work around this.

Proposal

Add a new opaquely-typed data property (named to match the LSP CompletionItem field that has the same purpose) to protocol.CompletionEntry to replace source in what gets sent back to TS Server for getCompletionDetails. (To transition, editors can continue to send source as well, and TS Server can continue to use it as a fallback for a while.) Then, TypeScript can add whatever properties we need to that new property.

/cc @mjbvz @uniqueiniquity any concerns about compatibility issues? @DanielRosenwasser will we need to update any other editors?

[sheetalkamat]

While you are at it, we probably can send symbol id or something like that which is used to create those extra import etc info. You probably also want to consider caching that information but then you would need to think about what if project changes between the completion and completion entry request and the data results in ambiguity or other error cases related to cache invalidation.

[mjbvz]

This proposal sounds good to me and I don't have any concerns about tracking this on the VS Code side and sending it back to the TS Server

@andrewbranch Would you continue sending along source as well? I know VS Code is misusing that field but I'm not sure we want to stop until we have a good replacement

[uniqueiniquity]

Sounds good to me as well - no concerns tracking this in either of our implementations.

[andrewbranch]

How long will we need to assume that clients might not return data to us? VS Code updates quickly—if we ship an update to JS/TS support in VS, how long does it take for a critical mass of people to get that update? At what point could we say “if you want auto imports to work, you’re either stuck on TS 4.2, or you need to update your VS installation”?

[minestarks]

@andrewbranch copying this comment from https://github.com/microsoft/TypeScript/pull/42890/files#r580647177 to here

Short answer: the de facto policy is "forever" - we don't have a formal versioning or compatibility story for the protocol. Many users decide to stay on older versions of VS for some reason or other, and many users always upgrade to the latest TS, and there's some overlap between the two, so we try to be as compatible as possible. If it becomes a big maintenance burden, it may be reasonable to introduce some breaking changes.

Note that I think your question not only applies to the protocol (which has implications for editors), but also to the API as well (which has implications for plugin developers). Which Daniel would have more insight on.