Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Share code between getCompletionsAtPosition and getCompletionEntryDetails. #2475

Merged
merged 13 commits into from
Mar 25, 2015
Merged

Share code between getCompletionsAtPosition and getCompletionEntryDetails. #2475

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bce9b5a
Break out completion symbol collection into its own function.
CyrusNajmabadi Mar 24, 2015
a1cbfdf
Use the same logic for completion entry details that we do for gettin…
CyrusNajmabadi Mar 24, 2015
66b3fae
Remove the active completion session.
CyrusNajmabadi Mar 24, 2015
2af1b92
CR feedback.
CyrusNajmabadi Mar 24, 2015
eed6f0d
Use an output format for quickinfo that more closely matches the orig…
CyrusNajmabadi Mar 24, 2015
c459bb7
CR feedback.
CyrusNajmabadi Mar 24, 2015
4802b4b
CR feedback.
CyrusNajmabadi Mar 24, 2015
a5bd919
PR feedback.
CyrusNajmabadi Mar 24, 2015
8d55fe0
PR feedback.
CyrusNajmabadi Mar 24, 2015
614b106
Merge branch 'master' into completionEntryDetails
CyrusNajmabadi Mar 24, 2015
63278ca
Don't bother with a predicate. It doesn't provide enough of a perf sa…
CyrusNajmabadi Mar 24, 2015
e26c260
PR feedback.
CyrusNajmabadi Mar 24, 2015
69ff9b3
PR feedback.
CyrusNajmabadi Mar 24, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 80 additions & 40 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
 */

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then does the predicate buy us any measurable perf improvements?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let symbols: SymbolTable = {};
let memberFlags: NodeFlags = 0;
function copySymbol(symbol: Symbol, meaning: SymbolFlags) {

if (isInsideWithStatementBody(location)) {
// We cannot answer semantic questions within a with block, do not proceed any further
return [];
}

populateSymbols();

return mapToArray(symbols);

function populateSymbols() {
while (location) {
if (location.locals && !isGlobalSourceFile(location)) {
if (copySymbols(location.locals, meaning)) {
return;
}
}
switch (location.kind) {
case SyntaxKind.SourceFile:
if (!isExternalModule(<SourceFile>location)) {
break;
}
case SyntaxKind.ModuleDeclaration:
if (copySymbols(getSymbolOfNode(location).exports, meaning & SymbolFlags.ModuleMember)) {
return;
}
break;
case SyntaxKind.EnumDeclaration:
if (copySymbols(getSymbolOfNode(location).exports, meaning & SymbolFlags.EnumMember)) {
return;
}
break;
case SyntaxKind.ClassDeclaration:
case SyntaxKind.InterfaceDeclaration:
if (!(memberFlags & NodeFlags.Static)) {
if (copySymbols(getSymbolOfNode(location).members, meaning & SymbolFlags.Type)) {
return;
}
}
break;
case SyntaxKind.FunctionExpression:
if ((<FunctionExpression>location).name) {
if (copySymbol(location.symbol, meaning)) {
return;
}
}
break;
}
memberFlags = location.flags;
location = location.parent;
}
if (copySymbols(globals, meaning)) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this if necessary beyond the desire for parallelism?

Note that I don't mean parallelism in the computing context, I mean it as a literary device in which components of a text are constructed in a similar manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was to keep things consistent.

}

// Returns 'true' if we should stop processing symbols.
function copySymbol(symbol: Symbol, meaning: SymbolFlags): boolean {
if (symbol.flags & meaning) {
let id = symbol.name;
if (!isReservedMemberName(id) && !hasProperty(symbols, id)) {
symbols[id] = symbol;
if (predicate) {
// If we were supplied a predicate function, then check if this symbol
// matches with it. If so, we're done and can immediately return.
// Otherwise, just ignore this symbol and keep going.
if (predicate(symbol)) {
symbols[id] = symbol;
return true;
}
}
else {
// If no predicate was supplied, then just add the symbol as is.
symbols[id] = symbol;
}
}
}

return false;
}
function copySymbols(source: SymbolTable, meaning: SymbolFlags) {

function copySymbols(source: SymbolTable, meaning: SymbolFlags): boolean {
if (meaning) {
for (let id in source) {
if (hasProperty(source, id)) {
copySymbol(source[id], meaning);
if (copySymbol(source[id], meaning)) {
return true;
}
}
}
}
}

if (isInsideWithStatementBody(location)) {
// We cannot answer semantic questions within a with block, do not proceed any further
return [];
}

while (location) {
if (location.locals && !isGlobalSourceFile(location)) {
copySymbols(location.locals, meaning);
}
switch (location.kind) {
case SyntaxKind.SourceFile:
if (!isExternalModule(<SourceFile>location)) break;
case SyntaxKind.ModuleDeclaration:
copySymbols(getSymbolOfNode(location).exports, meaning & SymbolFlags.ModuleMember);
break;
case SyntaxKind.EnumDeclaration:
copySymbols(getSymbolOfNode(location).exports, meaning & SymbolFlags.EnumMember);
break;
case SyntaxKind.ClassDeclaration:
case SyntaxKind.InterfaceDeclaration:
if (!(memberFlags & NodeFlags.Static)) {
copySymbols(getSymbolOfNode(location).members, meaning & SymbolFlags.Type);
}
break;
case SyntaxKind.FunctionExpression:
if ((<FunctionExpression>location).name) {
copySymbol(location.symbol, meaning);
}
break;
}
memberFlags = location.flags;
location = location.parent;
return false;
}
copySymbols(globals, meaning);
return mapToArray(symbols);
}

function isTypeDeclarationName(name: Node): boolean {
Expand Down
5 changes: 4 additions & 1 deletion src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1092,7 +1092,10 @@ module ts {
getSignaturesOfType(type: Type, kind: SignatureKind): Signature[];
getIndexTypeOfType(type: Type, kind: IndexKind): Type;
getReturnTypeOfSignature(signature: Signature): Type;
getSymbolsInScope(location: Node, meaning: SymbolFlags): Symbol[];

// If 'predicate' is supplied, then only the first symbol in scope matching the predicate
// will be returned. Otherwise, all symbols in scope will be returned.
getSymbolsInScope(location: Node, meaning: SymbolFlags, predicate?: (symbol: Symbol) => boolean): Symbol[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I see it, you can just use this comment.

getSymbolAtLocation(node: Node): Symbol;
getShorthandAssignmentValueSymbol(location: Node): Symbol;
getTypeAtLocation(node: Node): Type;
Expand Down
Loading