Incorrect auto import added for suggestions where same symbol name is exported from multiple files

[mjbvz]

Bug Report

From microsoft/vscode#116375

🔎 Search Terms

• completionEntryDetails
• completion
• suggest / suggestions
• source

🕗 Version & Regression Information

Seen on 4.2.0-20210210 and 4.1.5

💻 Repo

1. In the VS Code codebase, open this file
2. Delete the existing import for Disposable
3. Now for export class ExtensionsLifecycle extends Disposable, trigger suggestions at the end of Disposable
4. Accept the suggestion for Disposable from web.api:

🙁 Actual behavior

I expected this to add an import from web.api

🙂 Expected behavior

It actually ends up adding the import from import { Disposable } from 'vs/base/common/lifecycle';

I believe this is because the Disposable from web.api is a re-export of the one from lifecycle

You can also get into the reverse state, where the suggestion for lifecycle ends up adding an import from web.api. Here are the completionEntryDetails from that case:

[Trace  - 01:51:33.997] <semantic> Sending request: completionEntryDetails (110). Response expected: yes. Current queue length: 0
Arguments: {
    "file": "/Users/matb/projects/vscode/src/vs/platform/extensionManagement/node/extensionLifecycle.ts",
    "line": 16,
    "offset": 52,
    "entryNames": [
        {
            "name": "Disposable",
            "source": "/Users/matb/projects/vscode/src/vs/base/common/lifecycle"
        }
    ]
}
[Trace  - 01:51:34.82] <semantic> Response received: completionEntryDetails (110). Request took 85 ms. Success: true 
Result: [
    {
![Screen Shot 2021-02-10 at 6 03 31 PM](https://user-images.githubusercontent.com/12821956/107595705-534c3080-6bca-11eb-9510-44151ad711e0.png)
        "name": "Disposable",
        "kindModifiers": "abstract,export",
        "kind": "class",
        "displayParts": [
            {
                "text": "class",
                "kind": "keyword"
            },
            {
                "text": " ",
                "kind": "space"
            },
            {
                "text": "Disposable",
                "kind": "className"
            }
        ],
        "documentation": [],
        "codeActions": [
            {
                "description": "Import 'Disposable' from module \"vs/workbench/workbench.web.api\"",
                "changes": [
                    {
                        "fileName": "/Users/matb/projects/vscode/src/vs/platform/extensionManagement/node/extensionLifecycle.ts",
                        "textChanges": [
                            {
                                "start": {
                                    "line": 15,
                                    "offset": 1
                                },
                                "end": {
                                    "line": 15,
                                    "offset": 1
                                },
                                "newText": "import { Disposable } from 'vs/workbench/workbench.web.api';\n"
                            }
                        ]
                    }
                ]
            }
        ],
        "source": [
            {
                "text": "vs/workbench/workbench.web.api",
                "kind": "text"
            }
        ]
    }
]

[andrewbranch]

@mjbvz ok, so, there is a TypeScript bug here, but it’s not the fact that the path shown is not the module specifier you end up with. The path text you’re looking at was never meant to be shown to humans. As I mentioned in #42005, that path is purely an identifier that TypeScript uses to correlate completion entries between the CompletionsRequest and the CompletionDetailsRequest, so we can come up with a good module specifier in the latter. It’s specifically commented in the protocol as “not necessarily human-readable,” but VS Code decided to start displaying it in this PR. Once you focus a completion, we calculate the actual module specifier that we think you want, and that’s what you’re guaranteed to get. The left side of this screenshot shows the calculated module specifier; the right side is simply the path to the file where it was declared:

Occasionally, the path will be the same as the module specifier, but I would say this is actually rare for most projects. Even if we could show the user something better immediately for every item in the list, we would need to send you that in a separate property of the response, because we have to have this simple path to be able to fulfill the completions details request correctly, resolving ambiguities between different entries of the same name. I would really love to be able to calculate final module specifiers for every auto-import completion, but we currently don’t because it would increase the amount of time it takes for completions to show up at all.

So, there are two issues:

1. There is a TypeScript bug here: we’re only supposed to show one instance of the same symbol re-exported across different files. The fact that both src/vs/workbench/workbench.web.api and src/vs/base/common/lifecycle show up is a mistake, and is definitely contributing to the confusion here. (That mistake is even leading to a LS crash when I focus the latter item.)
2. That said, the path shown in the completions list is known to be a bad indicator of the final module specifier, and TypeScript has no plans on changing what path is sent in that part of the response, and cannot currently offer a better alternative. Whether or not you want to keep displaying it or revert Improve TS auto import label vscode#98228 is up to you all, but when the actual TS bug here is fixed, it may still end up that the source says /src/vs/workbench/workbench.web.api but the actual module specifier is vs/base/common/lifecycle—I’m not sure off the top of my head which completion will be dropped.

[mjbvz]

Thanks for the investigation @andrewbranch

We think there's a lot of value in showing auto-import information inline for suggestions and went ahead with this hacking approach knowing that we'd hit some limitations. We had #36265 but it look like that has stalled

@jrieken Can you please follow up on #36265 if needed

[andrewbranch]

The part of this that was a real bug has been fixed—the remaining concerns are well-covered by #36265 and #42005, so I’m going to close this.