Support multiple completions with the same name but different source module #19455

ghost · 2017-10-24T22:05:27Z

Fixes #19417
A CompletionEntry may now come with a source. This identifies where it came from. We will now return multiple CompletionEntrys if they have different source properties. It's only meant to be handed back to TypeScript when getting the CompletionEntryDetails, not for being shown to a user. The CompletionEntryDetails will then have its own source property which is human readable.

…module

mjbvz

The protocol changes of this look good to me. Thank you for the quick turn around here

mjbvz · 2017-10-24T22:22:46Z

src/server/protocol.ts

+        /**
+         * Human-readable description of the `source` from the CompletionEntry.
+         */
+        source?: SymbolDisplayPart[] | undefined;


[nit] Is undefined needed since this is already optional?

mhegazy · 2017-10-24T22:37:51Z

src/services/types.ts

@@ -238,8 +238,8 @@ namespace ts {

        getCompletionsAtPosition(fileName: string, position: number): CompletionInfo;
        // "options" is optional only for backwards-compatibility
-        getCompletionEntryDetails(fileName: string, position: number, entryName: string, options?: FormatCodeOptions | FormatCodeSettings): CompletionEntryDetails;
-        getCompletionEntrySymbol(fileName: string, position: number, entryName: string): Symbol;
+        getCompletionEntryDetails(fileName: string, position: number, id: string | CompletionEntryIdentifier, options?: FormatCodeOptions | FormatCodeSettings): CompletionEntryDetails;


this is going to be a breaking change for plugin authors. pinging @chuckjaz and @angelozerr

an alternative is to make the source as a third optional parameter.

To me this change seems unnecessary as it assumes that the key to the entry is now the name + source where name should be sufficient. Currently, if I am adding something to the completion list I need to ensure that my names are unique already so the call to getCompletionEntryDetails just needs the name.

name should be sufficient

It isn't -- different completions (with different associated code actions) could share the same name. See the test completionsImport_multipleWithSameName.ts.

It is unfortunate already merge this.

Adding an optional parameter was arguably a worse breaking change than changing the type of one of the parameters. Both break clients of the API but adding an API will cause all existing plugins to appear to break TypeScript as they will silently miss passing the additional parameter as well as the compiler will not point out that this happened. Changing the type would still be a breaking change but, at least, existing implementation that wrap the language service would probably just pass the parameter unmolested to the wrapped version of the language service and it would work and recompiling the plugin would point out that something changed. In other words, if this change had not been brought to my attention I probably would have missed it entirely, publishing a broken language plugin.

The fundamental problem here is that name is treated as insertText and they should be different. This would allow plugins to be able to add duplicate insertText without repeating the name. name should have just been an arbitrary string that was guaranteed to be unique for each entry. Adding source as a documentation seems reasonable.

The plugin can check the ts.version and you can decide to throw if it is higher than 2.6 for instance.

we also have ts.servicesVersion that we can change, and then you can do the check. I would say this is cleaner than ts.version, since the two change independently.

if this works for you, we can rev tat to 0.6 (currently 0.5).

The issue is in plugins that have already shipped not in new plugins. Checking the version doesn't help older versions. The version is fine for creating a shim for older versions of the API so a plugin can support older versions of TypeScript but a breaking change a @anglular/language-service@4.4.5 does not contain the code, couldn't have known that 2.6 would break it to even know to refuse to load. Only 2.6 knows it broke the API. And only it can decide if the an older version of a plugin that doesn't know about its change will work.

I understand. i am trying to solve it for future versions. what i am proposing is use the API version. we will increment the API version every time we make a breaking change (that is ts.servicesVersion) independently from the version of the language/toolset. the plugin can check at load time if the API version is the version it expects, and if not it can chose to fail, or just issue a warning.
would that work?

The older, already shipped, version of the language service do not check this. This would guard everything from 5.0.1+ and a LTS version both of which would then check for 0.6 or 0.5 which they both could support.

mhegazy · 2017-10-24T22:42:07Z

tests/cases/fourslash/completionsImport_multipleWithSameName.ts

+
+goTo.marker("");
+verify.completionListContains({ name: "foo", source: "/a" }, "const foo: 0", "", "const", /*spanIndex*/ undefined, /*hasAction*/ true);
+verify.completionListContains({ name: "foo", source: "/b" }, "const foo: 1", "", "const", /*spanIndex*/ undefined, /*hasAction*/ true);


so global should be in this list too?

mhegazy · 2017-10-24T22:44:30Z

src/services/completions.ts

+                    ? getSymbolId(origin.moduleSymbol)
+                    // -2 for globals
+                    : symbol.parent === undefined && !some(symbol.declarations, d => d.getSourceFile() === location.getSourceFile())
+                        ? -2


consider making these a local enum for expressiveness

mhegazy · 2017-10-24T22:47:35Z

src/services/completions.ts

+                const { name } = entry;
+                let uniquesForName = uniques.get(name);
+                if (!uniquesForName) {
+                    uniques.set(name, uniquesForName = []);


this is gonna be sparse by construction. might be better to make it a map as well.

we are guaranteed to see globals/locals before ones from modules, so:

add a comment that explains that, since it is not obvious

why not make it a map of number | [], or even split them into two maps global/local and modules, the reason is the global/local-only is by far the common case, and in that case we can avoid creating an extra array per entry.

mhegazy

Let's try to avoid the breaking change to the LS interface and optimize the array allocation in getCompletionEntriesFromSymbols

mhegazy · 2017-10-25T22:52:09Z

We will need to port this to release-2.6

…module (#19455) * Support multiple completions with the same name but different source module * Use optional parameters for source * Simplify use of `uniques` * Update test * Fix `undefined` error

…module (#19455) (#19496) * Support multiple completions with the same name but different source module * Use optional parameters for source * Simplify use of `uniques` * Update test * Fix `undefined` error

Support multiple completions with the same name but different source …

f0d44ce

…module

ghost requested a review from mjbvz October 24, 2017 22:07

mjbvz approved these changes Oct 24, 2017

View reviewed changes

mhegazy reviewed Oct 24, 2017

View reviewed changes

mhegazy suggested changes Oct 24, 2017

View reviewed changes

Andy Hanson added 3 commits October 25, 2017 07:38

Use optional parameters for source

a2bed45

Simplify use of uniques

7d2568a

Update test

c09d464

mhegazy approved these changes Oct 25, 2017

View reviewed changes

Fix undefined error

264d2b7

ghost merged commit 9615e54 into master Oct 26, 2017

ghost deleted the multiple_import_completions branch October 26, 2017 15:22

mhegazy mentioned this pull request Oct 31, 2017

Add version check when loading tsserver plugins #19224

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple completions with the same name but different source module #19455

Support multiple completions with the same name but different source module #19455

ghost commented Oct 24, 2017 •

edited by ghost

mjbvz left a comment

mjbvz Oct 24, 2017

mhegazy Oct 24, 2017

mhegazy Oct 24, 2017

chuckjaz Oct 24, 2017

ghost Oct 25, 2017 •

edited by ghost

chuckjaz Oct 26, 2017

mhegazy Oct 26, 2017

mhegazy Oct 26, 2017

chuckjaz Oct 27, 2017

mhegazy Oct 27, 2017

chuckjaz Oct 27, 2017

mhegazy Oct 24, 2017

mhegazy Oct 24, 2017

mhegazy Oct 24, 2017 •

edited

mhegazy Oct 24, 2017

mhegazy left a comment

mhegazy commented Oct 25, 2017

Support multiple completions with the same name but different source module #19455

Support multiple completions with the same name but different source module #19455

Conversation

ghost commented Oct 24, 2017 • edited by ghost

mjbvz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Oct 25, 2017 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy Oct 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy left a comment

Choose a reason for hiding this comment

mhegazy commented Oct 25, 2017

ghost commented Oct 24, 2017 •

edited by ghost

ghost Oct 25, 2017 •

edited by ghost

mhegazy Oct 24, 2017 •

edited