-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Completion list show correct entry for function expression and class expression #3643
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
Changes from 17 commits
aab2100
9c9e298
34489fa
5467e1d
45182b8
553085f
389e446
744f640
514d054
605ab0b
7747ad4
3079607
2e0c390
06c9876
6da98ce
7c52aaa
b01e4d8
f4cd1ac
0148915
872fdcf
b5b1b7b
d0b8002
41bedd2
d0d1ee9
f16f9d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13371,20 +13371,17 @@ namespace ts { | |
case SyntaxKind.EnumDeclaration: | ||
copySymbols(getSymbolOfNode(location).exports, meaning & SymbolFlags.EnumMember); | ||
break; | ||
case SyntaxKind.ClassExpression: | ||
if ((<ClassExpression>location).name) { | ||
copySymbol(location.symbol, meaning); | ||
} | ||
// Fall through | ||
case SyntaxKind.ClassDeclaration: | ||
case SyntaxKind.InterfaceDeclaration: | ||
if (!(memberFlags & NodeFlags.Static)) { | ||
copySymbols(getSymbolOfNode(location).members, meaning & SymbolFlags.Type); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. var c = class C<T> {
prop: /*completion should show T*/
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🌵 |
||
break; | ||
case SyntaxKind.FunctionExpression: | ||
if ((<FunctionExpression>location).name) { | ||
copySymbol(location.symbol, meaning); | ||
case SyntaxKind.ClassExpression: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. class expression listed twice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uh oh. It'd be good to have a feature that catches this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JsonFreeman it's being tracked in #2854 |
||
let name = (<FunctionExpression|ClassExpression>location).name; | ||
if (name) { | ||
copySymbol(name.text, location.symbol, meaning); | ||
} | ||
break; | ||
} | ||
|
@@ -13396,11 +13393,17 @@ namespace ts { | |
copySymbols(globals, meaning); | ||
} | ||
|
||
// Returns 'true' if we should stop processing symbols. | ||
function copySymbol(symbol: Symbol, meaning: SymbolFlags): void { | ||
/** | ||
* 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 null, use symbol.name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if undefined There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🌵 |
||
* @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(key: string, symbol: Symbol, meaning: SymbolFlags): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you remove the reserved check? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need the key here if you make that binder change we discussed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep. I am doing it in separate PR 🌵 |
||
if (symbol.flags & meaning) { | ||
let id = symbol.name; | ||
if (!isReservedMemberName(id) && !hasProperty(symbols, id)) { | ||
let id = key || symbol.name; | ||
if (!hasProperty(symbols, id)) { | ||
symbols[id] = symbol; | ||
} | ||
} | ||
|
@@ -13409,9 +13412,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.name, symbol, meaning); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see. Got it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix the comment at line 709 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1424,6 +1424,9 @@ namespace ts { | |
// class X {} | ||
export const classElement = "class"; | ||
|
||
// var x = class X {} | ||
export const localClassElement = "local class"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have a separate tag for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why we do it for either one then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i agree. let's take them both out. |
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note. you need to handle this on the managed side as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What you mean be handling in the managed side? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you see in navbar when you do this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🌵 file a bug |
||
// interface Y {} | ||
export const interfaceElement = "interface"; | ||
|
||
|
@@ -2799,18 +2802,30 @@ namespace ts { | |
return program.getOptionsDiagnostics().concat(program.getGlobalDiagnostics()); | ||
} | ||
|
||
/// 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you not just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can't use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
// In the case of default export, function expression and class expression, | ||
// the binder bind them with "default", "__function", "__class" respectively. | ||
// However, for completion entry, we want to display its declared name rather than binder name. | ||
if (getLocalSymbolForExportDefault(symbol) || | ||
getDeclarationOfKind(symbol, SyntaxKind.FunctionExpression) || | ||
getDeclarationOfKind(symbol, SyntaxKind.ClassExpression)) { | ||
let typeChecker = program.getTypeChecker(); | ||
displayName = getDeclaredName(typeChecker, symbol, location); | ||
|
||
// At this point, we expect that all completion list entries have declared name including function expression and class expression | ||
// because when we gather all relevant symbols, we check that the function expression and class expression must have declared name | ||
// before adding the symbol into our symbols table. (see: getSymbolsInScope) | ||
Debug.assert(displayName !== undefined, "Expected displayed name from declaration to existed in this symbol: " + symbol.getName()); | ||
} | ||
else { | ||
displayName = symbol.getName(); | ||
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)) { | ||
|
@@ -2823,37 +2838,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 unescapeIdentifier(name); | ||
} | ||
|
||
function getCompletionData(fileName: string, position: number) { | ||
|
@@ -3554,7 +3570,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; | ||
} | ||
|
@@ -3611,7 +3627,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); | ||
|
@@ -3644,7 +3660,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; | ||
|
@@ -3853,7 +3870,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); | ||
|
@@ -5136,10 +5162,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; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, that is cute :) |
||
return name.substring(1, length - 1); | ||
}; | ||
return name; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
///<reference path="fourslash.ts" /> | ||
|
||
//// var x = class myClass { | ||
//// getClassName (){ | ||
//// m/*0*/ | ||
//// } | ||
//// /*1*/ | ||
//// } | ||
|
||
goTo.marker("0"); | ||
verify.completionListContains("myClass", "(local class) myClass", /*documentation*/ undefined, "local class"); | ||
|
||
goTo.marker("1"); | ||
verify.completionListContains("myClass", "(local class) myClass", /*documentation*/ undefined, "local class"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does line do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which line ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.