From e7acef125d8a4453542e9728be9ceaa724db0f21 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 17 Jun 2016 13:21:47 -0700 Subject: [PATCH] Allow to find all references of the 'this 'keyword --- src/harness/fourslash.ts | 10 +-- src/services/services.ts | 61 +++++++++++++++---- .../cases/fourslash/findAllRefsThisKeyword.ts | 33 ++++++++++ .../findAllRefsThisKeywordMultipleFiles.ts | 17 +++--- tests/cases/fourslash/fourslash.ts | 2 +- tests/cases/fourslash/renameThis.ts | 22 +++++++ 6 files changed, 116 insertions(+), 29 deletions(-) create mode 100644 tests/cases/fourslash/findAllRefsThisKeyword.ts create mode 100644 tests/cases/fourslash/renameThis.ts diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 695e19b4667..60d3bb83619 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -760,7 +760,7 @@ namespace FourSlash { // Find the unaccounted-for reference. for (const actual of actualReferences) { if (!ts.forEach(expectedReferences, r => r.start === actual.textSpan.start)) { - this.raiseError(`A reference ${actual} is unaccounted for.`); + this.raiseError(`A reference ${stringify(actual)} is unaccounted for.`); } } // Probably will never reach here. @@ -907,13 +907,13 @@ namespace FourSlash { assert.equal(getDisplayPartsJson(actualQuickInfo.documentation), getDisplayPartsJson(documentation), this.messageAtLastKnownMarker("QuickInfo documentation")); } - public verifyRenameLocations(findInStrings: boolean, findInComments: boolean) { + public verifyRenameLocations(findInStrings: boolean, findInComments: boolean, ranges?: Range[]) { const renameInfo = this.languageService.getRenameInfo(this.activeFile.fileName, this.currentCaretPosition); if (renameInfo.canRename) { let references = this.languageService.findRenameLocations( this.activeFile.fileName, this.currentCaretPosition, findInStrings, findInComments); - let ranges = this.getRanges(); + ranges = ranges || this.getRanges(); if (!references) { if (ranges.length !== 0) { @@ -3128,8 +3128,8 @@ namespace FourSlashInterface { this.state.verifyRenameInfoFailed(message); } - public renameLocations(findInStrings: boolean, findInComments: boolean) { - this.state.verifyRenameLocations(findInStrings, findInComments); + public renameLocations(findInStrings: boolean, findInComments: boolean, ranges?: FourSlash.Range[]) { + this.state.verifyRenameLocations(findInStrings, findInComments, ranges); } public verifyQuickInfoDisplayParts(kind: string, kindModifiers: string, textSpan: { start: number; length: number; }, diff --git a/src/services/services.ts b/src/services/services.ts index 983ddfbf250..853817b05e8 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -5811,17 +5811,32 @@ namespace ts { return undefined; } - if (node.kind !== SyntaxKind.Identifier && - // TODO (drosen): This should be enabled in a later release - currently breaks rename. - // node.kind !== SyntaxKind.ThisKeyword && - // node.kind !== SyntaxKind.SuperKeyword && - node.kind !== SyntaxKind.StringLiteral && - !isLiteralNameOfPropertyDeclarationOrIndexAccess(node)) { - return undefined; + switch (node.kind) { + case SyntaxKind.NumericLiteral: + if (!isLiteralNameOfPropertyDeclarationOrIndexAccess(node)) { + break; + } + // Fallthrough + case SyntaxKind.Identifier: + case SyntaxKind.ThisKeyword: + // case SyntaxKind.SuperKeyword: TODO:GH#9268 + case SyntaxKind.StringLiteral: + return getReferencedSymbolsForNode(node, program.getSourceFiles(), findInStrings, findInComments); } + return undefined; + } - Debug.assert(node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.NumericLiteral || node.kind === SyntaxKind.StringLiteral); - return getReferencedSymbolsForNode(node, program.getSourceFiles(), findInStrings, findInComments); + function isThis(node: Node): boolean { + switch (node.kind) { + case SyntaxKind.ThisKeyword: + // case SyntaxKind.ThisType: TODO: GH#9267 + return true; + case SyntaxKind.Identifier: + // 'this' as a parameter + return (node as Identifier).originalKeywordKind === SyntaxKind.ThisKeyword && node.parent.kind === SyntaxKind.Parameter; + default: + return false; + } } function getReferencedSymbolsForNode(node: Node, sourceFiles: SourceFile[], findInStrings: boolean, findInComments: boolean): ReferencedSymbol[] { @@ -5841,7 +5856,7 @@ namespace ts { } } - if (node.kind === SyntaxKind.ThisKeyword || node.kind === SyntaxKind.ThisType) { + if (isThis(node)) { return getReferencesForThisKeyword(node, sourceFiles); } @@ -6376,7 +6391,7 @@ namespace ts { cancellationToken.throwIfCancellationRequested(); const node = getTouchingWord(sourceFile, position); - if (!node || (node.kind !== SyntaxKind.ThisKeyword && node.kind !== SyntaxKind.ThisType)) { + if (!node || !isThis(node)) { return; } @@ -8003,11 +8018,11 @@ namespace ts { const node = getTouchingWord(sourceFile, position, /*includeJsDocComment*/ true); - // Can only rename an identifier. if (node) { if (node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.StringLiteral || - isLiteralNameOfPropertyDeclarationOrIndexAccess(node)) { + isLiteralNameOfPropertyDeclarationOrIndexAccess(node) || + isThis(node)) { const symbol = typeChecker.getSymbolAtLocation(node); // Only allow a symbol to be renamed if it actually has at least one declaration. @@ -8054,6 +8069,26 @@ namespace ts { } } } + else if (node.kind === SyntaxKind.ThisKeyword) { + const container = ts.getThisContainer(node, /*includeArrowFunctions*/ false); + // Only allow rename to change a function with a 'this' type to one with a regular parameter, + // e.g. `function(this: number) { return this; }` to `function(x: number) { return x; }` + if (isFunctionLike(container)) { + const sig = typeChecker.getSignatureFromDeclaration(container); + if (sig.thisType) { + return { + canRename: true, + kind: ScriptElementKind.parameterElement, + displayName: "this", + localizedErrorMessage: undefined, + fullDisplayName: "this", + kindModifiers: "", + triggerSpan: createTriggerSpanForNode(node, sourceFile) + }; + } + } + // fallthrough to error + } } } diff --git a/tests/cases/fourslash/findAllRefsThisKeyword.ts b/tests/cases/fourslash/findAllRefsThisKeyword.ts new file mode 100644 index 00000000000..f08a70ccf65 --- /dev/null +++ b/tests/cases/fourslash/findAllRefsThisKeyword.ts @@ -0,0 +1,33 @@ +/// +// @noLib: true + +////[|this|]; +////function f([|this|]) { +//// return [|this|]; +//// function g([|this|]) { return [|this|]; } +////} +////class C { +//// static x() { +//// [|this|]; +//// } +//// static y() { +//// () => [|this|]; +//// } +//// constructor() { +//// [|this|]; +//// } +//// method() { +//// () => [|this|]; +//// } +////} +////// These are *not* real uses of the 'this' keyword, they are identifiers. +////const x = { [|this|]: 0 } +////x.[|this|]; + +const [global, f0, f1, g0, g1, x, y, constructor, method, propDef, propUse] = test.ranges(); +verify.referencesOf(global, [global]); +verify.rangesReferenceEachOther([f0, f1]); +verify.rangesReferenceEachOther([g0, g1]); +verify.rangesReferenceEachOther([x, y]); +verify.rangesReferenceEachOther([constructor, method]); +verify.rangesReferenceEachOther([propDef, propUse]); diff --git a/tests/cases/fourslash/findAllRefsThisKeywordMultipleFiles.ts b/tests/cases/fourslash/findAllRefsThisKeywordMultipleFiles.ts index 4b9f7a450ec..a4bab7f876c 100644 --- a/tests/cases/fourslash/findAllRefsThisKeywordMultipleFiles.ts +++ b/tests/cases/fourslash/findAllRefsThisKeywordMultipleFiles.ts @@ -1,18 +1,15 @@ /// // @Filename: file1.ts -////this; this; +////[|this|]; [|this|]; // @Filename: file2.ts -////this; -////this; +////[|this|]; +////[|this|]; // @Filename: file3.ts -//// ((x = this, y) => t/**/his)(this, this); +//// ((x = [|this|], y) => [|this|])([|this|], [|this|]); +//// // different 'this' +//// function f(this) { return this; } -goTo.file("file1.ts"); -goTo.marker(); - -// TODO (drosen): The CURRENT behavior is that findAllRefs doesn't work on 'this' or 'super' keywords. -// This should change down the line. -verify.referencesCountIs(0); \ No newline at end of file +verify.rangesReferenceEachOther(); diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index dd8a61ac458..9a6e19150c3 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -216,7 +216,7 @@ declare namespace FourSlashInterface { }[]): void; renameInfoSucceeded(displayName?: string, fullDisplayName?: string, kind?: string, kindModifiers?: string): void; renameInfoFailed(message?: string): void; - renameLocations(findInStrings: boolean, findInComments: boolean): void; + renameLocations(findInStrings: boolean, findInComments: boolean, ranges?: Range[]): void; verifyQuickInfoDisplayParts(kind: string, kindModifiers: string, textSpan: { start: number; length: number; diff --git a/tests/cases/fourslash/renameThis.ts b/tests/cases/fourslash/renameThis.ts new file mode 100644 index 00000000000..011110ef72e --- /dev/null +++ b/tests/cases/fourslash/renameThis.ts @@ -0,0 +1,22 @@ +/// + +////function f([|this|]) { +//// return [|this|]; +////} +////this/**/; +////const _ = { [|this|]: 0 }.[|this|]; + +let [r0, r1, r2, r3] = test.ranges() +for (let range of [r0, r1]) { + goTo.position(range.start); + verify.renameLocations(/*findInStrings*/ false, /*findInComments*/ false, [r0, r1]); +} + +// Trying to rename a legitimate 'this' should fail +goTo.marker(); +verify.renameInfoFailed("You cannot rename this element."); + +for (let range of [r2, r3]) { + goTo.position(range.start); + verify.renameLocations(/*findInStrings*/ false, /*findInComments*/ false, [r2, r3]); +}