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.

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

Completion list show correct entry for function expression and class expression #3643

Merged
merged 25 commits into from
Jul 10, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
aab2100
Add name of function expression into completion list
Jun 24, 2015
9c9e298
Add test for completion in function expression
Jun 26, 2015
34489fa
Add test for completion in class expression
Jun 26, 2015
5467e1d
Support completion in named class expression and named function expre…
Jun 26, 2015
45182b8
Add todo comment to use getDeclaredName
Jun 26, 2015
553085f
Merge branch 'master' into completionListWithLocalName
Jun 29, 2015
389e446
Merge branch 'master' into completionListWithLocalName
Jun 29, 2015
744f640
Add comment to clarify why we don't use copySymbol
Jul 1, 2015
514d054
Address CR: Use getDeclaredName and getDeclarationOfKind
Jul 1, 2015
605ab0b
Fix rename for class expression
Jul 1, 2015
7747ad4
Clean up stripQuote and add comments
Jul 1, 2015
3079607
Update verification function to be able to test that the only symbol …
Jul 1, 2015
2e0c390
Address code review
Jul 1, 2015
06c9876
Address code review
Jul 2, 2015
6da98ce
Fix comments
Jul 6, 2015
7c52aaa
Merge branch 'master' into completionListWithLocalName
Jul 6, 2015
b01e4d8
Address code review
Jul 6, 2015
f4cd1ac
Address code review, handle type parameter in completion list inside …
Jul 7, 2015
0148915
Fix comment
Jul 7, 2015
872fdcf
Merge branch 'master' into completionListWithLocalName
Jul 7, 2015
b5b1b7b
Use symbol.getName for classExpression and functionExpression since i…
Jul 7, 2015
d0b8002
Address code review
Jul 9, 2015
41bedd2
Use getDecalredName in service layer instead of symbol.getName
Jul 10, 2015
d0d1ee9
Add test for using unicode escape as function name
Jul 10, 2015
f16f9d1
Merge branch 'master' of https://github.com/Microsoft/TypeScript into…
Jul 10, 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
31 changes: 23 additions & 8 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13414,18 +13414,25 @@ namespace ts {
copySymbols(getSymbolOfNode(location).exports, meaning & SymbolFlags.EnumMember);
break;
case SyntaxKind.ClassExpression:
if ((<ClassExpression>location).name) {
let className = (<ClassExpression>location).name;
if (className) {
copySymbol(location.symbol, meaning);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you still need the fall through? For type parameters in a class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You don't need it. Break from the switch and fall through in this case behaves the same

Copy link
Contributor

Choose a reason for hiding this comment

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

var c = class C<T> {
    prop: /*completion should show T*/
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🌵

// Fall through
// fall through; this fall-through is necessary because we would like to handle
// type parameter inside class expression similar to how we handle it in classDeclaration and interface Declaration
case SyntaxKind.ClassDeclaration:
case SyntaxKind.InterfaceDeclaration:
// If we didn't come from static member of class or interface,
// add the type parameters into the symbol table
// (type parameters of classDeclaration/classExpression and interface are in member property of the symbol.
// Note: that the memberFlags come from previous iteration.
if (!(memberFlags & NodeFlags.Static)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what does line do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which line ?

Copy link
Member

Choose a reason for hiding this comment

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

the logic below, GitHub doesn't show it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @JsonFreeman for explaining this. This is the logic so that if we didn't come from static member of class or interface, add the type parameters into the symbol table (type-parameters of class Declaration and interface are in member property of the symbol.
Note: that the memberFlags come from previous iteration

Copy link
Contributor

Choose a reason for hiding this comment

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

Also note that for type aliases and signatures, the type parameters are in the locals, but for classes and interfaces, the type parameters are in the members.

copySymbols(getSymbolOfNode(location).members, meaning & SymbolFlags.Type);
}
break;
case SyntaxKind.FunctionExpression:
if ((<FunctionExpression>location).name) {
let funcName = (<FunctionExpression>location).name;
if (funcName) {
copySymbol(location.symbol, meaning);
}
break;
Expand All @@ -13438,11 +13445,20 @@ namespace ts {
copySymbols(globals, meaning);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove the reserved check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we will be doing it already when convert symbols to array (see symbolsToArray) 🌵

}

// Returns 'true' if we should stop processing symbols.
/**
* Copy the given symbol into symbol tables if the symbol has the given meaning
* and it doesn't already existed in the symbol table
* @param key a key for storing in symbol table; if undefined, use symbol.name
* @param symbol the symbol to be added into symbol table
* @param meaning meaning of symbol to filter by before adding to symbol table
*/
function copySymbol(symbol: Symbol, meaning: SymbolFlags): void {
if (symbol.flags & meaning) {
let id = symbol.name;
if (!isReservedMemberName(id) && !hasProperty(symbols, id)) {
// We will copy all symbol regardless of its reserved name because
// symbolsToArray will check whether the key is a reserved name and
// it will not copy symbol with reserved name to the array
if (!hasProperty(symbols, id)) {
symbols[id] = symbol;
}
}
Expand All @@ -13451,9 +13467,8 @@ namespace ts {
function copySymbols(source: SymbolTable, meaning: SymbolFlags): void {
if (meaning) {
for (let id in source) {
if (hasProperty(source, id)) {
copySymbol(source[id], meaning);
}
let symbol = source[id];
copySymbol(symbol, meaning);
}
}
}
Expand Down
8 changes: 5 additions & 3 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@ namespace ts {

export function getDeclarationOfKind(symbol: Symbol, kind: SyntaxKind): Declaration {
let declarations = symbol.declarations;
for (let declaration of declarations) {
if (declaration.kind === kind) {
return declaration;
if (declarations) {
for (let declaration of declarations) {
if (declaration.kind === kind) {
return declaration;
}
}
}

Expand Down
56 changes: 52 additions & 4 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -704,13 +704,61 @@ module FourSlash {
}
}

public verifyCompletionListDoesNotContain(symbol: string) {
/**
* Verify that the completion list does NOT contain the given symbol.
* The symbol is considered matched with the symbol in the list if and only if all given parameters must matched.
* When any parameter is omitted, the parameter is ignored during comparison and assumed that the parameter with
* that property of the symbol in the list.
* @param symbol the name of symbol
* @param expectedText the text associated with the symbol
* @param expectedDocumentation the documentation text associated with the symbol
* @param expectedKind the kind of symbol (see ScriptElementKind)
*/
public verifyCompletionListDoesNotContain(symbol: string, expectedText?: string, expectedDocumentation?: string, expectedKind?: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do any of these parameters help? You're interested n the lack of symbol in the list, so if symbol isn't in the list, then why do you care about any of the other parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so for the completion list, I also want to make sure the following case

class Foo {} -> additional parameter will be able to make sure that the list doesn't contain symbol with name Foo and of kind local class
var x = class Foo { /**/ } -> similar here but for kind class

Note: you can just pass the name if you don't care the rest

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Got it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix the comment at line 709

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🌵

let that = this;
function filterByTextOrDocumentation(entry: ts.CompletionEntry) {
let details = that.getCompletionEntryDetails(entry.name);
let documentation = ts.displayPartsToString(details.documentation);
let text = ts.displayPartsToString(details.displayParts);
if (expectedText && expectedDocumentation) {
return (documentation === expectedDocumentation && text === expectedText) ? true : false;
}
else if (expectedText && !expectedDocumentation) {
return text === expectedText ? true : false;
}
else if (expectedDocumentation && !expectedText) {
return documentation === expectedDocumentation ? true : false;
}
// Because expectedText and expectedDocumentation are undefined, we assume that
// users don't care to compare them so we will treat that entry as if the entry has matching text and documentation
// and keep it in the list of filtered entry.
return true;
}
this.scenarioActions.push('<ShowCompletionList />');
this.scenarioActions.push('<VerifyCompletionDoesNotContainItem ItemName="' + escapeXmlAttributeValue(symbol) + '" />');

var completions = this.getCompletionListAtCaret();
if (completions && completions.entries.filter(e => e.name === symbol).length !== 0) {
this.raiseError('Completion list did contain ' + symbol);
if (completions) {
let filterCompletions = completions.entries.filter(e => e.name === symbol);
filterCompletions = expectedKind ? filterCompletions.filter(e => e.kind === expectedKind) : filterCompletions;
filterCompletions = filterCompletions.filter(filterByTextOrDocumentation);
if (filterCompletions.length !== 0) {
// After filtered using all present criterion, if there are still symbol left in the list
// then these symbols must meet the criterion for Not supposed to be in the list. So we
// raise an error
let error = "Completion list did contain \'" + symbol + "\'.";
let details = this.getCompletionEntryDetails(filterCompletions[0].name);
if (expectedText) {
error += "Expected text: " + expectedText + " to equal: " + ts.displayPartsToString(details.displayParts) + ".";
}
if (expectedDocumentation) {
error += "Expected documentation: " + expectedDocumentation + " to equal: " + ts.displayPartsToString(details.documentation) + ".";
}
if (expectedKind) {
error += "Expected kind: " + expectedKind + " to equal: " + filterCompletions[0].kind + "."
}
this.raiseError(error);
}
}
}

Expand Down Expand Up @@ -2167,7 +2215,7 @@ module FourSlash {

var itemsString = items.map((item) => JSON.stringify({ name: item.name, kind: item.kind })).join(",\n");

this.raiseError('Expected "' + JSON.stringify({ name: name, text: text, documentation: documentation, kind: kind }) + '" to be in list [' + itemsString + ']');
this.raiseError('Expected "' + JSON.stringify({ name, text, documentation, kind }) + '" to be in list [' + itemsString + ']');
}

private findFile(indexOrName: any) {
Expand Down
81 changes: 46 additions & 35 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1428,6 +1428,9 @@ namespace ts {
// class X {}
export const classElement = "class";

// var x = class X {}
export const localClassElement = "local class";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have a separate tag for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be consistent with function expression which has kind of "local function":cactus:

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why we do it for either one then.

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree. let's take them both out.


Copy link
Contributor

Choose a reason for hiding this comment

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

note. you need to handle this on the managed side as well.

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 you mean be handling in the managed side?

Copy link
Member

Choose a reason for hiding this comment

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

Probably best to discuss this in person with someone but I believe the idea is that VS needs to handle styles differently depending on what comes in to the managed side.

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you see in navbar when you do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🌵 file a bug

// interface Y {}
export const interfaceElement = "interface";

Expand Down Expand Up @@ -2799,18 +2802,15 @@ namespace ts {
program.getGlobalDiagnostics(cancellationToken));
}

/// Completion
function getCompletionEntryDisplayNameForSymbol(symbol: Symbol, target: ScriptTarget, performCharacterChecks: boolean): string {
let displayName = symbol.getName();
if (displayName) {
// If this is the default export, get the name of the declaration if it exists
if (displayName === "default") {
let localSymbol = getLocalSymbolForExportDefault(symbol);
if (localSymbol && localSymbol.name) {
displayName = symbol.valueDeclaration.localSymbol.name;
}
}
/**
* Get the name to be display in completion from a given symbol.
*
* @return undefined if the name is of external module otherwise a name with striped of any quote
*/
function getCompletionEntryDisplayNameForSymbol(symbol: Symbol, target: ScriptTarget, performCharacterChecks: boolean, location: Node): string {
Copy link
Member

Choose a reason for hiding this comment

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

Can you not just use getDeclaredName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't use getDeclaredName. For example: "/u0042" will be store on its symbol as "B" while if you use getDeclaredName it will get the unicode escape sequences

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps getDeclaredName should be amended then.

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 feel the same and I had some debate with myself about that. I also consider moving logic to check the external module name into getDeclaredName all together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per our discussion, there is another way to fix the case of classExpression and functionExpression which is to change bindAnonymouseDeclaration to bind classExpression or functionExpression to its declared name if it has one. I will experiment with that in a separate PR

let displayName: string = getDeclaredName(program.getTypeChecker(), symbol, location);

if (displayName) {
let firstCharCode = displayName.charCodeAt(0);
// First check of the displayName is not external module; if it is an external module, it is not valid entry
if ((symbol.flags & SymbolFlags.Namespace) && (firstCharCode === CharacterCodes.singleQuote || firstCharCode === CharacterCodes.doubleQuote)) {
Expand All @@ -2823,37 +2823,38 @@ namespace ts {
return getCompletionEntryDisplayName(displayName, target, performCharacterChecks);
}

function getCompletionEntryDisplayName(displayName: string, target: ScriptTarget, performCharacterChecks: boolean): string {
if (!displayName) {
/**
* Get a displayName from a given for completion list, performing any necessary quotes stripping
* and checking whether the name is valid identifier name.
*/
function getCompletionEntryDisplayName(name: string, target: ScriptTarget, performCharacterChecks: boolean): string {
if (!name) {
return undefined;
}

let firstCharCode = displayName.charCodeAt(0);
if (displayName.length >= 2 &&
firstCharCode === displayName.charCodeAt(displayName.length - 1) &&
(firstCharCode === CharacterCodes.singleQuote || firstCharCode === CharacterCodes.doubleQuote)) {
// If the user entered name for the symbol was quoted, removing the quotes is not enough, as the name could be an
// invalid identifier name. We need to check if whatever was inside the quotes is actually a valid identifier name.
displayName = displayName.substring(1, displayName.length - 1);
}
name = stripQuotes(name);

if (!displayName) {
if (!name) {
return undefined;
}

// If the user entered name for the symbol was quoted, removing the quotes is not enough, as the name could be an
// invalid identifier name. We need to check if whatever was inside the quotes is actually a valid identifier name.
// e.g "b a" is valid quoted name but when we strip off the quotes, it is invalid.
// We, thus, need to check if whatever was inside the quotes is actually a valid identifier name.
if (performCharacterChecks) {
if (!isIdentifierStart(displayName.charCodeAt(0), target)) {
if (!isIdentifierStart(name.charCodeAt(0), target)) {
return undefined;
}

for (let i = 1, n = displayName.length; i < n; i++) {
if (!isIdentifierPart(displayName.charCodeAt(i), target)) {
for (let i = 1, n = name.length; i < n; i++) {
if (!isIdentifierPart(name.charCodeAt(i), target)) {
return undefined;
}
}
}

return unescapeIdentifier(displayName);
return name;
}

function getCompletionData(fileName: string, position: number) {
Expand Down Expand Up @@ -3607,7 +3608,7 @@ namespace ts {
// Try to get a valid display name for this symbol, if we could not find one, then ignore it.
// We would like to only show things that can be added after a dot, so for instance numeric properties can
// not be accessed with a dot (a.1 <- invalid)
let displayName = getCompletionEntryDisplayNameForSymbol(symbol, program.getCompilerOptions().target, /*performCharacterChecks:*/ true);
let displayName = getCompletionEntryDisplayNameForSymbol(symbol, program.getCompilerOptions().target, /*performCharacterChecks:*/ true, location);
if (!displayName) {
return undefined;
}
Expand Down Expand Up @@ -3664,7 +3665,7 @@ namespace ts {
// We don't need to perform character checks here because we're only comparing the
// name against 'entryName' (which is known to be good), not building a new
// completion entry.
let symbol = forEach(symbols, s => getCompletionEntryDisplayNameForSymbol(s, target, /*performCharacterChecks:*/ false) === entryName ? s : undefined);
let symbol = forEach(symbols, s => getCompletionEntryDisplayNameForSymbol(s, target, /*performCharacterChecks:*/ false, location) === entryName ? s : undefined);

if (symbol) {
let { displayParts, documentation, symbolKind } = getSymbolDisplayPartsDocumentationAndSymbolKind(symbol, getValidSourceFile(fileName), location, location, SemanticMeaning.All);
Expand Down Expand Up @@ -3697,7 +3698,8 @@ namespace ts {
function getSymbolKind(symbol: Symbol, location: Node): string {
let flags = symbol.getFlags();

if (flags & SymbolFlags.Class) return ScriptElementKind.classElement;
if (flags & SymbolFlags.Class) return getDeclarationOfKind(symbol, SyntaxKind.ClassExpression) ?
ScriptElementKind.localClassElement : ScriptElementKind.classElement;
if (flags & SymbolFlags.Enum) return ScriptElementKind.enumElement;
if (flags & SymbolFlags.TypeAlias) return ScriptElementKind.typeElement;
if (flags & SymbolFlags.Interface) return ScriptElementKind.interfaceElement;
Expand Down Expand Up @@ -3906,7 +3908,16 @@ namespace ts {
}
}
if (symbolFlags & SymbolFlags.Class && !hasAddedSymbolInfo) {
displayParts.push(keywordPart(SyntaxKind.ClassKeyword));
if (getDeclarationOfKind(symbol, SyntaxKind.ClassExpression)) {
// Special case for class expressions because we would like to indicate that
// the class name is local to the class body (similar to function expression)
// (local class) class <className>
pushTypePart(ScriptElementKind.localClassElement);
}
else {
// Class declaration has name which is not local.
displayParts.push(keywordPart(SyntaxKind.ClassKeyword));
}
displayParts.push(spacePart());
addFullSymbolName(symbol);
writeTypeParametersOfSymbol(symbol, sourceFile);
Expand Down Expand Up @@ -5112,7 +5123,7 @@ namespace ts {

// Get the text to search for.
// Note: if this is an external module symbol, the name doesn't include quotes.
let declaredName = getDeclaredName(typeChecker, symbol, node);
let declaredName = stripQuotes(getDeclaredName(typeChecker, symbol, node));

// Try to get the smallest valid scope that we can limit our search to;
// otherwise we'll need to search globally (i.e. include each file).
Expand Down Expand Up @@ -5189,10 +5200,10 @@ namespace ts {
* a reference to a symbol can occur anywhere.
*/
function getSymbolScope(symbol: Symbol): Node {
// If this is the symbol of a function expression, then named references
// are limited to its own scope.
// If this is the symbol of a named function expression or named class expression,
// then named references are limited to its own scope.
let valueDeclaration = symbol.valueDeclaration;
if (valueDeclaration && valueDeclaration.kind === SyntaxKind.FunctionExpression) {
if (valueDeclaration && (valueDeclaration.kind === SyntaxKind.FunctionExpression || valueDeclaration.kind === SyntaxKind.ClassExpression)) {
return valueDeclaration;
}

Expand Down Expand Up @@ -6863,7 +6874,7 @@ namespace ts {
}
}

let displayName = getDeclaredName(typeChecker, symbol, node);
let displayName = stripQuotes(getDeclaredName(typeChecker, symbol, node));
let kind = getSymbolKind(symbol, node);
if (kind) {
return {
Expand Down
11 changes: 9 additions & 2 deletions src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -667,7 +667,7 @@ namespace ts {

let name = typeChecker.symbolToString(localExportDefaultSymbol || symbol);

return stripQuotes(name);
return name;
}

export function isImportOrExportSpecifierName(location: Node): boolean {
Expand All @@ -676,9 +676,16 @@ namespace ts {
(<ImportOrExportSpecifier>location.parent).propertyName === location;
}

/**
* Strip off existed single quotes or double quotes from a given string
*
* @return non-quoted string
*/
export function stripQuotes(name: string) {
let length = name.length;
if (length >= 2 && name.charCodeAt(0) === CharacterCodes.doubleQuote && name.charCodeAt(length - 1) === CharacterCodes.doubleQuote) {
if (length >= 2 &&
name.charCodeAt(0) === name.charCodeAt(length - 1) &&
(name.charCodeAt(0) === CharacterCodes.doubleQuote || name.charCodeAt(0) === CharacterCodes.singleQuote)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, that is cute :)

return name.substring(1, length - 1);
};
return name;
Expand Down
Loading