From e82d0af554d96738b2e8c7f9027e53ac8bc31007 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Fri, 2 Aug 2019 18:24:46 -0700 Subject: [PATCH] Fix `readonly` occurrences highlighting (#32583) * Fix readonly occurrences highlighting * Rename function * Rename again * Apply suggestions from code review Remove unused function --- src/services/documentHighlights.ts | 6 ++-- src/services/findAllReferences.ts | 29 ++++++++++++++++--- ...umentHighlightsInvalidModifierLocations.ts | 7 ++++- .../fourslash/getOccurrencesReadonly1.ts | 7 +++++ .../fourslash/getOccurrencesReadonly2.ts | 7 +++++ .../fourslash/getOccurrencesReadonly3.ts | 14 +++++++++ 6 files changed, 63 insertions(+), 7 deletions(-) create mode 100644 tests/cases/fourslash/getOccurrencesReadonly1.ts create mode 100644 tests/cases/fourslash/getOccurrencesReadonly2.ts create mode 100644 tests/cases/fourslash/getOccurrencesReadonly3.ts diff --git a/src/services/documentHighlights.ts b/src/services/documentHighlights.ts index f0f3c165d7e..86f418b990a 100644 --- a/src/services/documentHighlights.ts +++ b/src/services/documentHighlights.ts @@ -193,7 +193,7 @@ namespace ts.DocumentHighlights { function getNodesToSearchForModifier(declaration: Node, modifierFlag: ModifierFlags): ReadonlyArray | undefined { // Types of node whose children might have modifiers. - const container = declaration.parent as ModuleBlock | SourceFile | Block | CaseClause | DefaultClause | ConstructorDeclaration | MethodDeclaration | FunctionDeclaration | ClassLikeDeclaration; + const container = declaration.parent as ModuleBlock | SourceFile | Block | CaseClause | DefaultClause | ConstructorDeclaration | MethodDeclaration | FunctionDeclaration | ObjectTypeDeclaration; switch (container.kind) { case SyntaxKind.ModuleBlock: case SyntaxKind.SourceFile: @@ -213,11 +213,13 @@ namespace ts.DocumentHighlights { return [...container.parameters, ...(isClassLike(container.parent) ? container.parent.members : [])]; case SyntaxKind.ClassDeclaration: case SyntaxKind.ClassExpression: + case SyntaxKind.InterfaceDeclaration: + case SyntaxKind.TypeLiteral: const nodes = container.members; // If we're an accessibility modifier, we're in an instance member and should search // the constructor's parameter list for instance members as well. - if (modifierFlag & ModifierFlags.AccessibilityModifier) { + if (modifierFlag & (ModifierFlags.AccessibilityModifier | ModifierFlags.Readonly)) { const constructor = find(container.members, isConstructorDeclaration); if (constructor) { return [...nodes, ...constructor.parameters]; diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index fbe33df92a5..1296b3cfa88 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -709,10 +709,28 @@ namespace ts.FindAllReferences.Core { return references.length ? [{ definition: { type: DefinitionKind.Symbol, symbol }, references }] : emptyArray; } + /** As in a `readonly prop: any` or `constructor(readonly prop: any)`, not a `readonly any[]`. */ + function isReadonlyTypeOperator(node: Node): boolean { + return node.kind === SyntaxKind.ReadonlyKeyword + && isTypeOperatorNode(node.parent) + && node.parent.operator === SyntaxKind.ReadonlyKeyword; + } + /** getReferencedSymbols for special node kinds. */ function getReferencedSymbolsSpecial(node: Node, sourceFiles: ReadonlyArray, cancellationToken: CancellationToken): SymbolAndEntries[] | undefined { if (isTypeKeyword(node.kind)) { - return getAllReferencesForKeyword(sourceFiles, node.kind, cancellationToken); + // A modifier readonly (like on a property declaration) is not special; + // a readonly type keyword (like `readonly string[]`) is. + if (node.kind === SyntaxKind.ReadonlyKeyword && !isReadonlyTypeOperator(node)) { + return undefined; + } + // Likewise, when we *are* looking for a special keyword, make sure we + // *don’t* include readonly member modifiers. + return getAllReferencesForKeyword( + sourceFiles, + node.kind, + cancellationToken, + node.kind === SyntaxKind.ReadonlyKeyword ? isReadonlyTypeOperator : undefined); } // Labels @@ -1235,11 +1253,14 @@ namespace ts.FindAllReferences.Core { } } - function getAllReferencesForKeyword(sourceFiles: ReadonlyArray, keywordKind: SyntaxKind, cancellationToken: CancellationToken): SymbolAndEntries[] | undefined { + function getAllReferencesForKeyword(sourceFiles: ReadonlyArray, keywordKind: SyntaxKind, cancellationToken: CancellationToken, filter?: (node: Node) => boolean): SymbolAndEntries[] | undefined { const references = flatMap(sourceFiles, sourceFile => { cancellationToken.throwIfCancellationRequested(); - return mapDefined(getPossibleSymbolReferenceNodes(sourceFile, tokenToString(keywordKind)!, sourceFile), referenceLocation => - referenceLocation.kind === keywordKind ? nodeEntry(referenceLocation) : undefined); + return mapDefined(getPossibleSymbolReferenceNodes(sourceFile, tokenToString(keywordKind)!, sourceFile), referenceLocation => { + if (referenceLocation.kind === keywordKind && (!filter || filter(referenceLocation))) { + return nodeEntry(referenceLocation); + } + }); }); return references.length ? [{ definition: { type: DefinitionKind.Keyword, node: references[0].node }, references }] : undefined; } diff --git a/tests/cases/fourslash/documentHighlightsInvalidModifierLocations.ts b/tests/cases/fourslash/documentHighlightsInvalidModifierLocations.ts index baa203901fd..de82fb60089 100644 --- a/tests/cases/fourslash/documentHighlightsInvalidModifierLocations.ts +++ b/tests/cases/fourslash/documentHighlightsInvalidModifierLocations.ts @@ -4,7 +4,12 @@ //// m([|readonly|] p) {} ////} ////function f([|readonly|] p) {} +//// +////class D { +//// m([|public|] p) {} +////} +////function g([|public|] p) {} for (const r of test.ranges()) { - verify.documentHighlightsOf(r, test.ranges()); + verify.documentHighlightsOf(r, [r]); } diff --git a/tests/cases/fourslash/getOccurrencesReadonly1.ts b/tests/cases/fourslash/getOccurrencesReadonly1.ts new file mode 100644 index 00000000000..a01d30bed93 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesReadonly1.ts @@ -0,0 +1,7 @@ +/// + +////interface I { +//// [|readonly|] prop: string; +////} + +verify.rangesAreOccurrences(false); diff --git a/tests/cases/fourslash/getOccurrencesReadonly2.ts b/tests/cases/fourslash/getOccurrencesReadonly2.ts new file mode 100644 index 00000000000..b6485b77a37 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesReadonly2.ts @@ -0,0 +1,7 @@ +/// + +////type T = { +//// [|readonly|] prop: string; +////} + +verify.rangesAreOccurrences(false); diff --git a/tests/cases/fourslash/getOccurrencesReadonly3.ts b/tests/cases/fourslash/getOccurrencesReadonly3.ts new file mode 100644 index 00000000000..0ca487c6364 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesReadonly3.ts @@ -0,0 +1,14 @@ +/// + +////class C { +//// [|readonly|] prop: /**/readonly string[] = []; +//// constructor([|readonly|] prop2: string) { +//// class D { +//// readonly prop: string = ""; +//// } +//// } +////} + +verify.rangesAreOccurrences(false); +goTo.marker(); +verify.occurrencesAtPositionCount(1);