Wrong dedupe on auto-import completions

[cyrilletuzi]

TypeScript Version: 2.6.0-dev.20171019 and 2.7.0-dev.20171021
Tested in VS Code Insiders.

Following #7849 and microsoft/vscode#2635

Code

Install an Angular project :

npm install @angular/cli -g
ng new autoimport

Then in src/app/app.component.ts, add a new property and instantiate it with EventEmitter, using the new auto-import on completion :

export class AppComponent {
  title = 'app';
  test = new EventEmi // ...then choose from the completion list
}

Expected behavior:

Should be able to select the good class between the multiple ones available to have the good import :

import { Component, EventEmitter } from '@angular/core';

Actual behavior:

Problem is : several librairies export a class EventEmitter with the same name.

Currently, completion suggestions seem to be deduped, so just one is proposed, resulting in the wrong auto import :

import { EventEmitter } from 'events';

By the way, in case of multiple tokens with the same name, the completion list should display an information about the packages concerned, to be able to choose the good one.

[mjbvz]

Here's the completion entry we get back when trying to import one of two classes named Foo:

  {
        "name": "Foo",
        "kind": "class",
        "kindModifiers": "export",
        "sortText": "0",
        "hasAction": true
    },

And here's the entry itself:

[Trace  - 10:41:30 AM] Response received: completionEntryDetails (77). Request took 4 ms. Success: true 
Result: [
    {
        "name": "Foo",
        "kindModifiers": "export",
        "kind": "class",
        "displayParts": [
            {
                "text": "class",
                "kind": "keyword"
            },
            {
                "text": " ",
                "kind": "space"
            },
            {
                "text": "Foo",
                "kind": "text"
            }
        ],
        "documentation": [],
        "tags": [],
        "codeActions": [
            {
                "description": "Import 'Foo' from \"./a\".",
                "changes": [
                    {
                        "fileName": "/Users/matb/projects/san/src/test.ts",
                        "textChanges": [
                            {
                                "start": {
                                    "line": 1,
                                    "offset": 1
                                },
                                "end": {
                                    "line": 1,
                                    "offset": 1
                                },
                                "newText": "import Foo from \"./a\";\n\n"
                            }
                        ]
                    }
                ]
            }
        ]
    }
]

The quickFix normally returned when the import is missing is:

[Trace  - 10:43:22 AM] Response received: getCodeFixes (94). Request took 5 ms. Success: true 
Result: [
    {
        "description": "Import 'Foo' from \"./a\".",
        "changes": [
            {
                "fileName": "/Users/matb/projects/san/src/test.ts",
                "textChanges": [
                    {
                        "start": {
                            "line": 1,
                            "offset": 1
                        },
                        "end": {
                            "line": 1,
                            "offset": 1
                        },
                        "newText": "import Foo from \"./a\";\n\n"
                    }
                ]
            }
        ]
    },
    {
        "description": "Import 'Foo' from \"./b\".",
        "changes": [
            {
                "fileName": "/Users/matb/projects/san/src/test.ts",
                "textChanges": [
                    {
                        "start": {
                            "line": 1,
                            "offset": 1
                        },
                        "end": {
                            "line": 1,
                            "offset": 1
                        },
                        "newText": "import Foo from \"./b\";\n\n"
                    }
                ]
            }
        ]
    }
]

[cyrilletuzi]

Thanks, hope it will be fixed for TS 2.6 final and October VS Code release, because it makes the feature unusable for now as it generates wrong code (when you are far in a file, you won't even see the import is bad).

[mjbvz]

@andy-ms / @mhegazy What are your thoughts on how this should be fixed? Return multiple completion items, one for each unique symbol? Or have one completion that returns multiple codeActions in its completion details?

Having a completion item for each of these symbols could be better UX in my option. The multiple code action approach would require that VSCode show some UI after you accept an completion so that you can then pick the correct import

[ghost]

@mjbvz I think we can provide multiple CompletionEntrys with the same name, then give you a new source property in the CompletionEntryDetails telling where the entry came from.

[mjbvz]

Ok. To support that with the TSServer API, I'm thinking we'd need to add a source field on the CompletionEntry:

interface CompletionEntry {
    name: string;
    source?: string;
  ...
}

And then extend CompletionDetailsRequestArgs to either take a name or a source and name:

  interface CompletionDetailsRequestArgs extends FileLocationRequestArgs {
        entryNames: (string | { name: string, source?: string})[];
    }

Any better idea for this?

[ghost]

You're right, we should provide the source on the CompletionEntry like you said and not on the details because we couldn't disambiguate the entry without a source.
The API you suggested looks good. But I'm thinking the source in CompletionEntry should not be human-readable, and the human-readable one should go on CompletionEntryDetails instead as a SymbolDisplayPart[].

[mjbvz]

That sounds good to me. We'll just treat the source as an opaque handle that we pass back to TS when getting the completion details

[cyrilletuzi]

@andy-ms @mhegazy @mjbvz

I've tested in the last VS Code Insiders, and there is now a new problem : multiple imports are suggested when relevant, but the paths are now the bad ones : only deep imports are available. For example, instead of :

import { EventEmitter } from '@angular/core';

It's now :

import { EventEmitter } from '@angular/core/src/event_emitter';

It was handled fine when just one auto-import was suggested. Also, there was the same issue with the Import quick fix (#15223), but which was resolved a long time ago.

[mhegazy]

@cyrilletuzi mind filing a new issue to track that.

[cyrilletuzi]

Ok, in TypeScript or VS Code repo ?

[mhegazy]

TypeScript.