Share code between getCompletionsAtPosition and getCompletionEntryDetails. #2475

CyrusNajmabadi · 2015-03-24T04:58:28Z

Previously, the completion item code in teh LS did something fairly unsafe. it would cache information about symbols from a previous version of the 'program' (and typechecker), and then attempt to ask questions about those symbols after files had gone through edits

The completion details code uses this cached information to try to get the type of the symbol so it can build the display information.

This is not something safe to do. Because of the edits, we'll have totally different symbols for this file. This inconsistancy can lead to situations where the typeChecker we use cannot understand what's going on. This can happen when it goes from a symbol back to the source, and back to some symbol. This symbol will then not be one it knows about and we'll usually end up with an 'unknown' symbol result.

The new approach is to extract out a common core routine for getting completion symbols. This routine can be called by getCompletionsAtPosition to get the set of symbols to display. The routine can also be called with a completion entry name. In this case, instead of collecting all symbols, it instead searches, but ignores all symbols that don't match that name, and immediately returns the moment it finds the first match.

Now there is no cached information stored between calls. We always synchronize, and the LS, program and typeChecker are always in a consistent state.

Testing this out with our own project revealed no performance issue with this approach. Completion entry details are returned just as quickly as before, and we no longer get errant 'any's in the completion entry details.

…g completion entries.

DanielRosenwasser · 2015-03-24T05:06:41Z

src/services/services.ts

+            }
+
+            // Didn't find a symbol with this name.  See if we can find a keyword instead.
+            let keywordCompletion = filter(keywordCompletions, c => c.name === entryName);


I feel like you meant to use forEach here.

Do we have a test for completion entry details on a keyword?

DanielRosenwasser · 2015-03-24T07:03:31Z

src/compiler/checker.ts

@@ -10547,62 +10547,102 @@ module ts {
            return false;
        }

-        function getSymbolsInScope(location: Node, meaning: SymbolFlags): Symbol[] {
+        function getSymbolsInScope(location: Node, meaning: SymbolFlags, predicate?: (symbol: Symbol) => boolean): Symbol[] {


Consider adding some variation of the following comment:

/** * Aggregates all symbols visible at a given location node. If two symbols with the same name exist * in different scopes, then the symbol whose declaration is lexically closer is returned. * * @param location The node at which the client would like to get available symbols * @param meaning A flag to determine which kinds of symbols to include. * If a symbol satisfies a flag, it will be considered. * @param predicate If provided and any symbol satisfies this predicate, then a singleton list * consisting of that element is returned. */

I'm somewhat ambivalent about the name predicate. But I don't care that much.

What would you prefer?

i do not think you need the predicate. I think this is an optimization that does not buy us much on the general case. do you have perf numbers before and after the fix?

I only measured after the fix. The time to get the details was in the low number of MS. Effectively, all the time was spent in the Roslyn delay timer, as well as just presenting the item. Computing of the item was effectively nothing. (This wasn't true for "Diagnostics". but that's a known issue where we end up building up a massive display string for that special type).

then does the predicate buy us any measurable perf improvements?

Looking at the numbers, it appears to save a few ms (4-5) for global completion lists. I think that does make it worth it. My concern is arond a project with several large d.ts files (like WinRT, etc.) With all the tens of thousands of symbols, it would be nice to not have to go get all of them again since we only want one.

But, i'll defer to you here. I think it would be fine to remove the predicate.

…inal source. i.e. use "var v: string" instead of "(var) v: string". The parens case should only be used when we're using an english description instead of an actual language construct.

DanielRosenwasser · 2015-03-24T07:18:12Z

src/services/services.ts

+                    // Filter down to the symbol that matches the symbolName if we were given one.
+                    let predicate: (s: Symbol) => boolean = symbolName 
+                        ? s => getValidCompletionEntryDisplayName(s, target) === symbolName
+                        : undefined;


Do you need the full type annotation for predicate? If you annotate s, I don't think you get any, do you?

DanielRosenwasser · 2015-03-24T07:33:31Z

src/services/services.ts

-                    getCompletionEntriesFromSymbols(symbols, activeCompletionSession);
+
+                    // Filter down to the symbol that matches the symbolName if we were given one.
+                    let predicate: (s: Symbol) => boolean = symbolName 


Is it possible to have a symbol with no name?

Nope. We only create the item if it has a name we can display:

if (!displayName) { return undefined; } -- Cyrus

From: Daniel Rosenwasser [mailto:[email protected]]
Sent: Tuesday, March 24, 2015 12:34 AM
To: Microsoft/TypeScript
Cc: Cyrus Najmabadi
Subject: Re: [TypeScript] Share code between getCompletionsAtPosition getCompletionEntryDetails. (#2475)

In src/services/services.tshttps://github.com//pull/2475#discussion_r27007308:

@@ -2599,24 +2585,18 @@ module ts {

/// TODO filter meaning based on the current context let scopeNode = getScopeNode(previousToken, position, sourceFile); let symbolMeanings = SymbolFlags.Type | SymbolFlags.Value | SymbolFlags.Namespace | SymbolFlags.Alias;

let symbols = typeInfoResolver.getSymbolsInScope(scopeNode, symbolMeanings);

getCompletionEntriesFromSymbols(symbols, activeCompletionSession);

// Filter down to the symbol that matches the symbolName if we were given one.

let predicate: (s: Symbol) => boolean = symbolName

Is it possible to have a symbol with no name?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2475/files#r27007308.

DanielRosenwasser · 2015-03-24T08:05:41Z

src/services/services.ts

@@ -2450,9 +2443,21 @@ module ts {
            };
        }

-        function getCompletionsAtPosition(fileName: string, position: number) {
-            synchronizeHostData();
+        function getCompletionData(fileName: string, position: number, symbolName?: string) {


Need to document the symbolName semantics

DanielRosenwasser · 2015-03-24T08:20:30Z

src/services/services.ts

+                    displayParts.push(punctuationPart(SyntaxKind.OpenParenToken));
+                    displayParts.push(textOrKeywordPart(symbolKind));
+                    displayParts.push(punctuationPart(SyntaxKind.CloseParenToken));
+                }


Consider a switch

DanielRosenwasser · 2015-03-24T20:33:55Z

👍 but discuss with @mhegazy about the predicate.

…vings.

DanielRosenwasser · 2015-03-24T23:42:49Z

src/services/services.ts

+
+                // Find the symbol with the matching entry name.
+                let target = program.getCompilerOptions().target;
+                let symbol = forEach(symbols, s => getCompletionEntryDisplayName(s, target, /*performCharacterChecks:*/ false) === entryName ? s : undefined);


Explain that you don't need to perform the validation check because entryName is anticipated to be a valid name.

Share code between getCompletionsAtPosition and getCompletionEntryDetails.

CyrusNajmabadi added 3 commits March 23, 2015 20:56

Break out completion symbol collection into its own function.

bce9b5a

Use the same logic for completion entry details that we do for gettin…

a1cbfdf

…g completion entries.

Remove the active completion session.

66b3fae

msftclas added the cla-not-required label Mar 24, 2015

DanielRosenwasser reviewed Mar 24, 2015
View reviewed changes

DanielRosenwasser mentioned this pull request Mar 24, 2015

Completion item info often reports types as 'any' when quickinfo on the completed item reports otherwise #2278

Closed

CR feedback.

2af1b92

DanielRosenwasser reviewed Mar 24, 2015
View reviewed changes

Use an output format for quickinfo that more closely matches the orig…

eed6f0d

…inal source. i.e. use "var v: string" instead of "(var) v: string". The parens case should only be used when we're using an english description instead of an actual language construct.

DanielRosenwasser reviewed Mar 24, 2015
View reviewed changes

CR feedback.

c459bb7

DanielRosenwasser reviewed Mar 24, 2015
View reviewed changes

CR feedback.

4802b4b

DanielRosenwasser reviewed Mar 24, 2015
View reviewed changes

PR feedback.

a5bd919

DanielRosenwasser reviewed Mar 24, 2015
View reviewed changes

CyrusNajmabadi changed the title ~~Share code between getCompletionsAtPosition getCompletionEntryDetails.~~ Share code between getCompletionsAtPosition and getCompletionEntryDetails. Mar 24, 2015

PR feedback.

8d55fe0

CyrusNajmabadi added 3 commits March 24, 2015 14:55

Merge branch 'master' into completionEntryDetails

614b106

Don't bother with a predicate. It doesn't provide enough of a perf sa…

63278ca

…vings.

PR feedback.

e26c260

DanielRosenwasser reviewed Mar 24, 2015
View reviewed changes

PR feedback.

69ff9b3

CyrusNajmabadi added a commit that referenced this pull request Mar 25, 2015

Merge pull request #2475 from Microsoft/completionEntryDetails

4f0dc28

Share code between getCompletionsAtPosition and getCompletionEntryDetails.

CyrusNajmabadi merged commit 4f0dc28 into master Mar 25, 2015

CyrusNajmabadi deleted the completionEntryDetails branch March 25, 2015 00:05

microsoft locked and limited conversation to collaborators Jun 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share code between getCompletionsAtPosition and getCompletionEntryDetails. #2475

Share code between getCompletionsAtPosition and getCompletionEntryDetails. #2475

CyrusNajmabadi commented Mar 24, 2015

DanielRosenwasser Mar 24, 2015

DanielRosenwasser Mar 24, 2015

CyrusNajmabadi Mar 24, 2015

DanielRosenwasser Mar 24, 2015

DanielRosenwasser Mar 24, 2015

CyrusNajmabadi Mar 24, 2015

mhegazy Mar 24, 2015

CyrusNajmabadi Mar 24, 2015

mhegazy Mar 24, 2015

CyrusNajmabadi Mar 24, 2015

DanielRosenwasser Mar 24, 2015

DanielRosenwasser Mar 24, 2015

CyrusNajmabadi Mar 24, 2015

DanielRosenwasser Mar 24, 2015

DanielRosenwasser Mar 24, 2015

DanielRosenwasser commented Mar 24, 2015

DanielRosenwasser Mar 24, 2015

Share code between getCompletionsAtPosition and getCompletionEntryDetails. #2475

Share code between getCompletionsAtPosition and getCompletionEntryDetails. #2475

Conversation

CyrusNajmabadi commented Mar 24, 2015

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

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

DanielRosenwasser commented Mar 24, 2015

Choose a reason for hiding this comment