From 37a2cddabc552e7e3ce86f94d92f171be76f2f97 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Mon, 1 May 2017 11:45:18 -0700 Subject: [PATCH] Give the class element completion on typing keywords like public, private, readonly Also when name of the function is location, make sure we are actually looking at the same symbol before using the declaration to get signature to display --- src/harness/fourslash.ts | 17 +++++ src/services/completions.ts | 41 ++++++++++-- src/services/symbolDisplay.ts | 44 +++++++------ .../completionEntryForClassMembers.ts | 63 +++++++++---------- ...mpletionListBuilderLocations_properties.ts | 2 +- tests/cases/fourslash/fourslash.ts | 2 + ...cesForClassMembersExtendingGenericClass.ts | 2 +- 7 files changed, 111 insertions(+), 60 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index a56d21ae345..74790400b98 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -3417,6 +3417,17 @@ namespace FourSlashInterface { export class VerifyNegatable { public not: VerifyNegatable; + public allowedClassElementKeywords = [ + "public", + "private", + "protected", + "static", + "abstract", + "readonly", + "get", + "set", + "constructor" + ]; constructor(protected state: FourSlash.TestState, private negative = false) { if (!negative) { @@ -3453,6 +3464,12 @@ namespace FourSlashInterface { this.state.verifyCompletionListIsEmpty(this.negative); } + public completionListContainsClassElementKeywords() { + for (const keyword of this.allowedClassElementKeywords) { + this.completionListContains(keyword, keyword, /*documentation*/ undefined, "keyword"); + } + } + public completionListIsGlobal(expected: boolean) { this.state.verifyCompletionListIsGlobal(expected); } diff --git a/src/services/completions.ts b/src/services/completions.ts index 44915a958c2..da59dab8828 100644 --- a/src/services/completions.ts +++ b/src/services/completions.ts @@ -988,6 +988,10 @@ namespace ts.Completions { return undefined; } + function isFromClassElementDeclaration(node: Node) { + return isClassElement(node.parent) && isClassLike(node.parent.parent); + } + /** * Returns the immediate owning class declaration of a context token, * on the condition that one exists and that the context implies completion should be given. @@ -1010,6 +1014,13 @@ namespace ts.Completions { return location; } break; + + default: + if (isFromClassElementDeclaration(contextToken) && + (isClassMemberCompletionKeyword(contextToken.kind) || + isClassMemberCompletionKeywordText(contextToken.getText()))) { + return contextToken.parent.parent as ClassLikeDeclaration; + } } } @@ -1148,7 +1159,7 @@ namespace ts.Completions { isFunction(containingNodeKind); case SyntaxKind.StaticKeyword: - return containingNodeKind === SyntaxKind.PropertyDeclaration; + return containingNodeKind === SyntaxKind.PropertyDeclaration && !isClassLike(contextToken.parent.parent); case SyntaxKind.DotDotDotToken: return containingNodeKind === SyntaxKind.Parameter || @@ -1165,13 +1176,17 @@ namespace ts.Completions { containingNodeKind === SyntaxKind.ExportSpecifier || containingNodeKind === SyntaxKind.NamespaceImport; + case SyntaxKind.GetKeyword: + case SyntaxKind.SetKeyword: + if (isFromClassElementDeclaration(contextToken)) { + return false; + } + // falls through case SyntaxKind.ClassKeyword: case SyntaxKind.EnumKeyword: case SyntaxKind.InterfaceKeyword: case SyntaxKind.FunctionKeyword: case SyntaxKind.VarKeyword: - case SyntaxKind.GetKeyword: - case SyntaxKind.SetKeyword: case SyntaxKind.ImportKeyword: case SyntaxKind.LetKeyword: case SyntaxKind.ConstKeyword: @@ -1180,6 +1195,13 @@ namespace ts.Completions { return true; } + // If the previous token is keyword correspoding to class member completion keyword + // there will be completion available here + if (isClassMemberCompletionKeywordText(contextToken.getText()) && + isFromClassElementDeclaration(contextToken)) { + return false; + } + // Previous token may have been a keyword that was converted to an identifier. switch (contextToken.getText()) { case "abstract": @@ -1373,8 +1395,8 @@ namespace ts.Completions { }); } - const classMemberKeywordCompletions = filter(keywordCompletions, entry => { - switch (stringToToken(entry.name)) { + function isClassMemberCompletionKeyword(kind: SyntaxKind) { + switch (kind) { case SyntaxKind.PublicKeyword: case SyntaxKind.ProtectedKeyword: case SyntaxKind.PrivateKeyword: @@ -1386,7 +1408,14 @@ namespace ts.Completions { case SyntaxKind.SetKeyword: return true; } - }); + } + + function isClassMemberCompletionKeywordText(text: string) { + return isClassMemberCompletionKeyword(stringToToken(text)); + } + + const classMemberKeywordCompletions = filter(keywordCompletions, entry => + isClassMemberCompletionKeywordText(entry.name)); function isEqualityExpression(node: Node): node is BinaryExpression { return isBinaryExpression(node) && isEqualityOperatorKind(node.operatorToken.kind); diff --git a/src/services/symbolDisplay.ts b/src/services/symbolDisplay.ts index 5bf3e37f28a..4268a52c419 100644 --- a/src/services/symbolDisplay.ts +++ b/src/services/symbolDisplay.ts @@ -200,27 +200,33 @@ namespace ts.SymbolDisplay { (location.kind === SyntaxKind.ConstructorKeyword && location.parent.kind === SyntaxKind.Constructor)) { // At constructor keyword of constructor declaration // get the signature from the declaration and write it const functionDeclaration = location.parent; - const allSignatures = functionDeclaration.kind === SyntaxKind.Constructor ? type.getNonNullableType().getConstructSignatures() : type.getNonNullableType().getCallSignatures(); - if (!typeChecker.isImplementationOfOverload(functionDeclaration)) { - signature = typeChecker.getSignatureFromDeclaration(functionDeclaration); - } - else { - signature = allSignatures[0]; - } + // Use function declaration to write the signatures only if the symbol corresponding to this declaration + const locationIsSymbolDeclaration = findDeclaration(symbol, declaration => + declaration === (location.kind === SyntaxKind.ConstructorKeyword ? functionDeclaration.parent : functionDeclaration)); - if (functionDeclaration.kind === SyntaxKind.Constructor) { - // show (constructor) Type(...) signature - symbolKind = ScriptElementKind.constructorImplementationElement; - addPrefixForAnyFunctionOrVar(type.symbol, symbolKind); - } - else { - // (function/method) symbol(..signature) - addPrefixForAnyFunctionOrVar(functionDeclaration.kind === SyntaxKind.CallSignature && - !(type.symbol.flags & SymbolFlags.TypeLiteral || type.symbol.flags & SymbolFlags.ObjectLiteral) ? type.symbol : symbol, symbolKind); - } + if (locationIsSymbolDeclaration) { + const allSignatures = functionDeclaration.kind === SyntaxKind.Constructor ? type.getNonNullableType().getConstructSignatures() : type.getNonNullableType().getCallSignatures(); + if (!typeChecker.isImplementationOfOverload(functionDeclaration)) { + signature = typeChecker.getSignatureFromDeclaration(functionDeclaration); + } + else { + signature = allSignatures[0]; + } - addSignatureDisplayParts(signature, allSignatures); - hasAddedSymbolInfo = true; + if (functionDeclaration.kind === SyntaxKind.Constructor) { + // show (constructor) Type(...) signature + symbolKind = ScriptElementKind.constructorImplementationElement; + addPrefixForAnyFunctionOrVar(type.symbol, symbolKind); + } + else { + // (function/method) symbol(..signature) + addPrefixForAnyFunctionOrVar(functionDeclaration.kind === SyntaxKind.CallSignature && + !(type.symbol.flags & SymbolFlags.TypeLiteral || type.symbol.flags & SymbolFlags.ObjectLiteral) ? type.symbol : symbol, symbolKind); + } + + addSignatureDisplayParts(signature, allSignatures); + hasAddedSymbolInfo = true; + } } } } diff --git a/tests/cases/fourslash/completionEntryForClassMembers.ts b/tests/cases/fourslash/completionEntryForClassMembers.ts index da1051e784a..c52977a0002 100644 --- a/tests/cases/fourslash/completionEntryForClassMembers.ts +++ b/tests/cases/fourslash/completionEntryForClassMembers.ts @@ -1,6 +1,9 @@ /// ////abstract class B { +//// private privateMethod() { } +//// protected protectedMethod() { }; +//// static staticMethod() { } //// abstract getValue(): number; //// /*abstractClass*/ ////} @@ -69,25 +72,7 @@ //// static identi/*classThatStartedWritingIdentifierAfterStaticModifier*/ ////} -const allowedKeywords = [ - "public", - "private", - "protected", - "static", - "abstract", - "readonly", - "get", - "set", - "constructor" -]; - -const allowedKeywordCount = allowedKeywords.length; -function verifyAllowedKeyWords() { - for (const keyword of allowedKeywords) { - verify.completionListContains(keyword, keyword, /*documentation*/ undefined, "keyword"); - } -} - +const allowedKeywordCount = verify.allowedClassElementKeywords.length; const nonClassElementMarkers = [ "InsideMethod" ]; @@ -104,7 +89,7 @@ const onlyClassElementKeywordLocations = [ ]; for (const marker of onlyClassElementKeywordLocations) { goTo.marker(marker); - verifyAllowedKeyWords(); + verify.completionListContainsClassElementKeywords(); verify.completionListCount(allowedKeywordCount); } @@ -113,9 +98,8 @@ const classElementCompletionLocations = [ "classThatIsEmptyAndExtendingAnotherClass", "classThatHasAlreadyImplementedAnotherClassMethod", "classThatHasAlreadyImplementedAnotherClassMethodAfterMethod", - // TODO should we give completion for these keywords - //"classThatHasWrittenPublicKeyword", - //"classElementContainingStatic", + "classThatHasWrittenPublicKeyword", + "classElementContainingStatic", "classThatStartedWritingIdentifier", "propDeclarationWithoutSemicolon", "propDeclarationWithSemicolon", @@ -126,17 +110,30 @@ const classElementCompletionLocations = [ "methodImplementation", "accessorSignatureWithoutSemicolon", "accessorSignatureImplementation", - // TODO should we give completion for these keywords - //"classThatHasWrittenGetKeyword", - //"classThatHasWrittenSetKeyword", - //"classThatStartedWritingIdentifierOfGetAccessor", - //"classThatStartedWritingIdentifierOfSetAccessor", - //"classThatStartedWritingIdentifierAfterModifier", - //"classThatStartedWritingIdentifierAfterStaticModifier" + "classThatHasWrittenGetKeyword", + "classThatHasWrittenSetKeyword", + "classThatStartedWritingIdentifierOfGetAccessor", + "classThatStartedWritingIdentifierOfSetAccessor", + "classThatStartedWritingIdentifierAfterModifier", + "classThatStartedWritingIdentifierAfterStaticModifier" +]; + +const validMembersOfBase = [ + ["getValue", "(method) B.getValue(): number"], + ["protectedMethod", "(method) B.protectedMethod(): void"] +]; +const invalidMembersOfBase = [ + ["privateMethod", "(method) B.privateMethod(): void"], + ["staticMethod", "(method) B.staticMethod(): void"] ]; for (const marker of classElementCompletionLocations) { goTo.marker(marker); - verify.completionListContains("getValue", "(method) B.getValue(): number", /*documentation*/ undefined, "method"); - verifyAllowedKeyWords(); - verify.completionListCount(allowedKeywordCount + 1); + for (const [validMemberOfBaseSymbol, validMemberOfBaseText] of validMembersOfBase) { + verify.completionListContains(validMemberOfBaseSymbol, validMemberOfBaseText, /*documentation*/ undefined, "method"); + } + for (const [invalidMemberOfBaseSymbol, invalidMemberOfBaseText] of invalidMembersOfBase) { + verify.not.completionListContains(invalidMemberOfBaseSymbol, invalidMemberOfBaseText, /*documentation*/ undefined, "method"); + } + verify.completionListContainsClassElementKeywords(); + verify.completionListCount(allowedKeywordCount + validMembersOfBase.length); } \ No newline at end of file diff --git a/tests/cases/fourslash/completionListBuilderLocations_properties.ts b/tests/cases/fourslash/completionListBuilderLocations_properties.ts index 806d8c1de4f..2cdc3b7e7ba 100644 --- a/tests/cases/fourslash/completionListBuilderLocations_properties.ts +++ b/tests/cases/fourslash/completionListBuilderLocations_properties.ts @@ -10,4 +10,4 @@ //// public static a/*property2*/ ////} -goTo.eachMarker(() => verify.completionListIsEmpty()); +goTo.eachMarker(() => verify.completionListContainsClassElementKeywords()); diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index 374dfd33c7d..31d8b8b4ef2 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -133,11 +133,13 @@ declare namespace FourSlashInterface { class verifyNegatable { private negative; not: verifyNegatable; + allowedClassElementKeywords: string[]; constructor(negative?: boolean); completionListCount(expectedCount: number): void; completionListContains(symbol: string, text?: string, documentation?: string, kind?: string, spanIndex?: number): void; completionListItemsCountIsGreaterThan(count: number): void; completionListIsEmpty(): void; + completionListContainsClassElementKeywords(): void; completionListAllowsNewIdentifier(): void; signatureHelpPresent(): void; errorExistsBetweenMarkers(startMarker: string, endMarker: string): void; diff --git a/tests/cases/fourslash/referencesForClassMembersExtendingGenericClass.ts b/tests/cases/fourslash/referencesForClassMembersExtendingGenericClass.ts index 3e453663f63..448c2627531 100644 --- a/tests/cases/fourslash/referencesForClassMembersExtendingGenericClass.ts +++ b/tests/cases/fourslash/referencesForClassMembersExtendingGenericClass.ts @@ -26,7 +26,7 @@ const methods = ranges.get("method"); const [m0, m1, m2] = methods; verify.referenceGroups(m0, [{ definition: "(method) Base.method(a?: T, b?: U): this", ranges: methods }]); verify.referenceGroups(m1, [ - { definition: "(method) Base.method(): void", ranges: [m0] }, + { definition: "(method) Base.method(a?: T, b?: U): this", ranges: [m0] }, { definition: "(method) MyClass.method(): void", ranges: [m1, m2] } ]); verify.referenceGroups(m2, [