From 3414ef276e706e7be30e1c6f51af491ccfad9bcf Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Thu, 23 Jan 2020 11:34:27 -0800 Subject: [PATCH] refactor(language-service): Avoid leaking host outside of LanguageService (#34941) As part of the effort to tighten the API surface of `TypeScriptServiceHost` in preparation for the migration to Ivy, I realized some recently added APIs are not strictly needed. They can be safely removed without sacrificing functionality. This allows us to clean up the code, especially in the implementation of QuickInfo, where the `TypeScriptServiceHost` is leaked outside of the `LanguageService` class. This refactoring also cleans up some duplicate code where the QuickInfo object is generated. The logic is now consolidated into a simple `createQuickInfo` method shared across two different implementations. PR Close #34941 --- packages/language-service/src/definitions.ts | 4 +- packages/language-service/src/hover.ts | 158 +++++++----------- .../language-service/src/language_service.ts | 14 +- .../language-service/src/locate_symbol.ts | 37 ++-- .../language-service/src/typescript_host.ts | 16 +- packages/language-service/test/hover_spec.ts | 37 ++-- .../test/typescript_host_spec.ts | 47 ------ 7 files changed, 108 insertions(+), 205 deletions(-) diff --git a/packages/language-service/src/definitions.ts b/packages/language-service/src/definitions.ts index a3f5f1fbf3..51dbe1ce5d 100644 --- a/packages/language-service/src/definitions.ts +++ b/packages/language-service/src/definitions.ts @@ -39,7 +39,6 @@ export function getDefinitionAndBoundSpan( return; } - const textSpan = ngSpanToTsTextSpan(symbols[0].span); const definitions: ts.DefinitionInfo[] = []; for (const symbolInfo of symbols) { const {symbol} = symbolInfo; @@ -67,7 +66,8 @@ export function getDefinitionAndBoundSpan( } return { - definitions, textSpan, + definitions, + textSpan: symbols[0].span, }; } diff --git a/packages/language-service/src/hover.ts b/packages/language-service/src/hover.ts index 9167c60433..145fdfbc6b 100644 --- a/packages/language-service/src/hover.ts +++ b/packages/language-service/src/hover.ts @@ -6,20 +6,16 @@ * found in the LICENSE file at https://angular.io/license */ -import {CompileSummaryKind, StaticSymbol} from '@angular/compiler'; +import {NgAnalyzedModules} from '@angular/compiler'; import * as ts from 'typescript'; - import {AstResult} from './common'; import {locateSymbols} from './locate_symbol'; import * as ng from './types'; -import {TypeScriptServiceHost} from './typescript_host'; -import {findTightestNode} from './utils'; - +import {inSpan} from './utils'; // Reverse mappings of enum would generate strings const SYMBOL_SPACE = ts.SymbolDisplayPartKind[ts.SymbolDisplayPartKind.space]; const SYMBOL_PUNC = ts.SymbolDisplayPartKind[ts.SymbolDisplayPartKind.punctuation]; -const SYMBOL_CLASS = ts.SymbolDisplayPartKind[ts.SymbolDisplayPartKind.className]; const SYMBOL_TEXT = ts.SymbolDisplayPartKind[ts.SymbolDisplayPartKind.text]; const SYMBOL_INTERFACE = ts.SymbolDisplayPartKind[ts.SymbolDisplayPartKind.interfaceName]; @@ -28,124 +24,92 @@ const SYMBOL_INTERFACE = ts.SymbolDisplayPartKind[ts.SymbolDisplayPartKind.inter * return the corresponding quick info. * @param info template AST * @param position location of the symbol - * @param host Language Service host to query + * @param analyzedModules all NgModules in the program. */ -export function getHover(info: AstResult, position: number, host: Readonly): - ts.QuickInfo|undefined { +export function getTemplateHover( + info: AstResult, position: number, analyzedModules: NgAnalyzedModules): ts.QuickInfo|undefined { const symbolInfo = locateSymbols(info, position)[0]; if (!symbolInfo) { return; } + const {symbol, span, staticSymbol} = symbolInfo; - const {symbol, span, compileTypeSummary} = symbolInfo; - const textSpan = {start: span.start, length: span.end - span.start}; - - if (compileTypeSummary && compileTypeSummary.summaryKind === CompileSummaryKind.Directive) { - return getDirectiveModule(compileTypeSummary.type.reference, textSpan, host, symbol); + // The container is either the symbol's container (for example, 'AppComponent' + // is the container of the symbol 'title' in its template) or the NgModule + // that the directive belongs to (the container of AppComponent is AppModule). + let containerName: string|undefined = symbol.container ?.name; + if (!containerName && staticSymbol) { + // If there is a static symbol then the target is a directive. + const ngModule = analyzedModules.ngModuleByPipeOrDirective.get(staticSymbol); + containerName = ngModule ?.type.reference.name; } - const containerDisplayParts: ts.SymbolDisplayPart[] = symbol.container ? - [ - {text: symbol.container.name, kind: symbol.container.kind}, - {text: '.', kind: SYMBOL_PUNC}, - ] : - []; - const typeDisplayParts: ts.SymbolDisplayPart[] = symbol.type ? - [ - {text: ':', kind: SYMBOL_PUNC}, - {text: ' ', kind: SYMBOL_SPACE}, - {text: symbol.type.name, kind: SYMBOL_INTERFACE}, - ] : - []; - return { - kind: symbol.kind as ts.ScriptElementKind, - kindModifiers: '', // kindModifier info not available on 'ng.Symbol' - textSpan, - documentation: symbol.documentation, - // this would generate a string like '(property) ClassX.propY: type' - // 'kind' in displayParts does not really matter because it's dropped when - // displayParts get converted to string. - displayParts: [ - {text: '(', kind: SYMBOL_PUNC}, - {text: symbol.kind, kind: symbol.kind}, - {text: ')', kind: SYMBOL_PUNC}, - {text: ' ', kind: SYMBOL_SPACE}, - ...containerDisplayParts, - {text: symbol.name, kind: symbol.kind}, - ...typeDisplayParts, - ], - }; + return createQuickInfo(symbol.name, symbol.kind, span, containerName, symbol.type?.name, symbol.documentation); } /** * Get quick info for Angular semantic entities in TypeScript files, like Directives. - * @param sf TypeScript source file an Angular symbol is in * @param position location of the symbol in the source file - * @param host Language Service host to query + * @param declarations All Directive-like declarations in the source file. + * @param analyzedModules all NgModules in the program. */ export function getTsHover( - sf: ts.SourceFile, position: number, host: Readonly): ts.QuickInfo| - undefined { - const node = findTightestNode(sf, position); - if (!node) return; - switch (node.kind) { - case ts.SyntaxKind.Identifier: - const directiveId = node as ts.Identifier; - if (ts.isClassDeclaration(directiveId.parent)) { - const directiveName = directiveId.text; - const directiveSymbol = host.getStaticSymbol(node.getSourceFile().fileName, directiveName); - if (!directiveSymbol) return; - return getDirectiveModule( - directiveSymbol, - {start: directiveId.getStart(), length: directiveId.end - directiveId.getStart()}, - host); - } - break; - default: - break; + position: number, declarations: ng.Declaration[], + analyzedModules: NgAnalyzedModules): ts.QuickInfo|undefined { + for (const {declarationSpan, metadata} of declarations) { + if (inSpan(position, declarationSpan)) { + const staticSymbol: ng.StaticSymbol = metadata.type.reference; + const directiveName = staticSymbol.name; + const kind = metadata.isComponent ? 'component' : 'directive'; + const textSpan = ts.createTextSpanFromBounds(declarationSpan.start, declarationSpan.end); + const ngModule = analyzedModules.ngModuleByPipeOrDirective.get(staticSymbol); + const moduleName = ngModule ?.type.reference.name; + return createQuickInfo( + directiveName, kind, textSpan, moduleName, ts.ScriptElementKind.classElement); + } } - return undefined; } /** - * Attempts to get quick info for the NgModule a Directive is declared in. - * @param directive identifier on a potential Directive class declaration - * @param textSpan span of the symbol - * @param host Language Service host to query - * @param symbol the internal symbol that represents the directive + * Construct a QuickInfo object taking into account its container and type. + * @param name Name of the QuickInfo target + * @param kind component, directive, pipe, etc. + * @param textSpan span of the target + * @param containerName either the Symbol's container or the NgModule that contains the directive + * @param type user-friendly name of the type + * @param documentation docstring or comment */ -function getDirectiveModule( - directive: StaticSymbol, textSpan: ts.TextSpan, host: Readonly, - symbol?: ng.Symbol): ts.QuickInfo|undefined { - const analyzedModules = host.getAnalyzedModules(false); - const ngModule = analyzedModules.ngModuleByPipeOrDirective.get(directive); - if (!ngModule) return; +function createQuickInfo( + name: string, kind: string, textSpan: ts.TextSpan, containerName?: string, type?: string, + documentation?: ts.SymbolDisplayPart[]): ts.QuickInfo { + const containerDisplayParts = containerName ? + [ + {text: containerName, kind: SYMBOL_INTERFACE}, + {text: '.', kind: SYMBOL_PUNC}, + ] : + []; - const isComponent = - host.getDeclarations(directive.filePath) - .find(decl => decl.type === directive && decl.metadata && decl.metadata.isComponent); + const typeDisplayParts = type ? + [ + {text: ':', kind: SYMBOL_PUNC}, + {text: ' ', kind: SYMBOL_SPACE}, + {text: type, kind: SYMBOL_INTERFACE}, + ] : + []; - const moduleName = ngModule.type.reference.name; return { - kind: ts.ScriptElementKind.classElement, - kindModifiers: - ts.ScriptElementKindModifier.none, // kindModifier info not available on 'ng.Symbol' - textSpan, - documentation: symbol ? symbol.documentation : undefined, - // This generates a string like '(directive) NgModule.Directive: class' - // 'kind' in displayParts does not really matter because it's dropped when - // displayParts get converted to string. + kind: kind as ts.ScriptElementKind, + kindModifiers: ts.ScriptElementKindModifier.none, + textSpan: textSpan, displayParts: [ {text: '(', kind: SYMBOL_PUNC}, - {text: isComponent ? 'component' : 'directive', kind: SYMBOL_TEXT}, + {text: kind, kind: SYMBOL_TEXT}, {text: ')', kind: SYMBOL_PUNC}, {text: ' ', kind: SYMBOL_SPACE}, - {text: moduleName, kind: SYMBOL_CLASS}, - {text: '.', kind: SYMBOL_PUNC}, - {text: directive.name, kind: SYMBOL_CLASS}, - {text: ':', kind: SYMBOL_PUNC}, - {text: ' ', kind: SYMBOL_SPACE}, - {text: ts.ScriptElementKind.classElement, kind: SYMBOL_TEXT}, + ...containerDisplayParts, + {text: name, kind: SYMBOL_INTERFACE}, + ...typeDisplayParts, ], + documentation, }; } diff --git a/packages/language-service/src/language_service.ts b/packages/language-service/src/language_service.ts index e463761896..d07c25274d 100644 --- a/packages/language-service/src/language_service.ts +++ b/packages/language-service/src/language_service.ts @@ -11,7 +11,7 @@ import * as tss from 'typescript/lib/tsserverlibrary'; import {getTemplateCompletions} from './completions'; import {getDefinitionAndBoundSpan, getTsDefinitionAndBoundSpan} from './definitions'; import {getDeclarationDiagnostics, getTemplateDiagnostics, ngDiagnosticToTsDiagnostic, uniqueBySpan} from './diagnostics'; -import {getHover, getTsHover} from './hover'; +import {getTemplateHover, getTsHover} from './hover'; import * as ng from './types'; import {TypeScriptServiceHost} from './typescript_host'; @@ -88,19 +88,15 @@ class LanguageServiceImpl implements ng.LanguageService { } getQuickInfoAtPosition(fileName: string, position: number): tss.QuickInfo|undefined { - this.host.getAnalyzedModules(); // same role as 'synchronizeHostData' + const analyzedModules = this.host.getAnalyzedModules(); // same role as 'synchronizeHostData' const templateInfo = this.host.getTemplateAstAtPosition(fileName, position); if (templateInfo) { - return getHover(templateInfo, position, this.host); + return getTemplateHover(templateInfo, position, analyzedModules); } // Attempt to get Angular-specific hover information in a TypeScript file, the NgModule a // directive belongs to. - if (fileName.endsWith('.ts')) { - const sf = this.host.getSourceFile(fileName); - if (sf) { - return getTsHover(sf, position, this.host); - } - } + const declarations = this.host.getDeclarations(fileName); + return getTsHover(position, declarations, analyzedModules); } } diff --git a/packages/language-service/src/locate_symbol.ts b/packages/language-service/src/locate_symbol.ts index 1671991687..68773e6b9f 100644 --- a/packages/language-service/src/locate_symbol.ts +++ b/packages/language-service/src/locate_symbol.ts @@ -6,8 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, Attribute, BoundDirectivePropertyAst, BoundEventAst, CompileTypeSummary, CssSelector, DirectiveAst, ElementAst, EmbeddedTemplateAst, RecursiveTemplateAstVisitor, SelectorMatcher, TemplateAst, TemplateAstPath, templateVisitAll, tokenReference} from '@angular/compiler'; - +import {AST, Attribute, BoundDirectivePropertyAst, BoundEventAst, CssSelector, DirectiveAst, ElementAst, EmbeddedTemplateAst, RecursiveTemplateAstVisitor, SelectorMatcher, StaticSymbol, TemplateAst, TemplateAstPath, templateVisitAll, tokenReference} from '@angular/compiler'; +import * as tss from 'typescript/lib/tsserverlibrary'; import {AstResult} from './common'; import {getExpressionScope} from './expression_diagnostics'; import {getExpressionSymbol} from './expressions'; @@ -16,8 +16,8 @@ import {diagnosticInfoFromTemplateInfo, findTemplateAstAt, getPathToNodeAtPositi export interface SymbolInfo { symbol: Symbol; - span: Span; - compileTypeSummary: CompileTypeSummary|undefined; + span: tss.TextSpan; + staticSymbol?: StaticSymbol; } /** @@ -52,9 +52,9 @@ function locateSymbol(ast: TemplateAst, path: TemplateAstPath, info: AstResult): undefined { const templatePosition = path.position; const position = templatePosition + info.template.span.start; - let compileTypeSummary: CompileTypeSummary|undefined = undefined; let symbol: Symbol|undefined; let span: Span|undefined; + let staticSymbol: StaticSymbol|undefined; const attributeValueSymbol = (ast: AST): boolean => { const attribute = findAttribute(info, position); if (attribute) { @@ -81,8 +81,9 @@ function locateSymbol(ast: TemplateAst, path: TemplateAstPath, info: AstResult): visitElement(ast) { const component = ast.directives.find(d => d.directive.isComponent); if (component) { - compileTypeSummary = component.directive; - symbol = info.template.query.getTypeSymbol(compileTypeSummary.type.reference); + // Need to cast because 'reference' is typed as any + staticSymbol = component.directive.type.reference as StaticSymbol; + symbol = info.template.query.getTypeSymbol(staticSymbol); symbol = symbol && new OverrideKindSymbol(symbol, DirectiveKind.COMPONENT); span = spanOf(ast); } else { @@ -90,8 +91,9 @@ function locateSymbol(ast: TemplateAst, path: TemplateAstPath, info: AstResult): const directive = ast.directives.find( d => d.directive.selector != null && d.directive.selector.indexOf(ast.name) >= 0); if (directive) { - compileTypeSummary = directive.directive; - symbol = info.template.query.getTypeSymbol(compileTypeSummary.type.reference); + // Need to cast because 'reference' is typed as any + staticSymbol = directive.directive.type.reference as StaticSymbol; + symbol = info.template.query.getTypeSymbol(staticSymbol); symbol = symbol && new OverrideKindSymbol(symbol, DirectiveKind.DIRECTIVE); span = spanOf(ast); } @@ -124,8 +126,10 @@ function locateSymbol(ast: TemplateAst, path: TemplateAstPath, info: AstResult): const attributeSelector = `[${ast.name}=${ast.value}]`; const parsedAttribute = CssSelector.parse(attributeSelector); if (!parsedAttribute.length) return; - matcher.match(parsedAttribute[0], (_, directive) => { - symbol = info.template.query.getTypeSymbol(directive.directive.type.reference); + matcher.match(parsedAttribute[0], (_, {directive}) => { + // Need to cast because 'reference' is typed as any + staticSymbol = directive.type.reference as StaticSymbol; + symbol = info.template.query.getTypeSymbol(staticSymbol); symbol = symbol && new OverrideKindSymbol(symbol, DirectiveKind.DIRECTIVE); span = spanOf(ast); }); @@ -145,8 +149,9 @@ function locateSymbol(ast: TemplateAst, path: TemplateAstPath, info: AstResult): }, visitText(ast) {}, visitDirective(ast) { - compileTypeSummary = ast.directive; - symbol = info.template.query.getTypeSymbol(compileTypeSummary.type.reference); + // Need to cast because 'reference' is typed as any + staticSymbol = ast.directive.type.reference as StaticSymbol; + symbol = info.template.query.getTypeSymbol(staticSymbol); span = spanOf(ast); }, visitDirectiveProperty(ast) { @@ -158,7 +163,11 @@ function locateSymbol(ast: TemplateAst, path: TemplateAstPath, info: AstResult): }, null); if (symbol && span) { - return {symbol, span: offsetSpan(span, info.template.span.start), compileTypeSummary}; + const {start, end} = offsetSpan(span, info.template.span.start); + return { + symbol, + span: tss.createTextSpanFromBounds(start, end), staticSymbol, + }; } } diff --git a/packages/language-service/src/typescript_host.ts b/packages/language-service/src/typescript_host.ts index 573164ac9b..e7a6e8d9ef 100644 --- a/packages/language-service/src/typescript_host.ts +++ b/packages/language-service/src/typescript_host.ts @@ -148,13 +148,9 @@ export class TypeScriptServiceHost implements LanguageServiceHost { * and templateReferences. * In addition to returning information about NgModules, this method plays the * same role as 'synchronizeHostData' in tsserver. - * @param ensureSynchronized whether or not the Language Service should make sure analyzedModules - * are synced to the last update of the project. If false, returns the set of analyzedModules - * that is already cached. This is useful if the project must not be reanalyzed, even if its - * file watchers (which are disjoint from the TypeScriptServiceHost) detect an update. */ - getAnalyzedModules(ensureSynchronized = true): NgAnalyzedModules { - if (!ensureSynchronized || this.upToDate()) { + getAnalyzedModules(): NgAnalyzedModules { + if (this.upToDate()) { return this.analyzedModules; } @@ -450,14 +446,6 @@ export class TypeScriptServiceHost implements LanguageServiceHost { return this.getTemplateAst(template); } - /** - * Gets a StaticSymbol from a file and symbol name. - * @return Angular StaticSymbol matching the file and name, if any - */ - getStaticSymbol(file: string, name: string): StaticSymbol|undefined { - return this.reflector.getStaticSymbol(file, name); - } - /** * Find the NgModule which the directive associated with the `classSymbol` * belongs to, then return its schema and transitive directives and pipes. diff --git a/packages/language-service/test/hover_spec.ts b/packages/language-service/test/hover_spec.ts index 7cc487e7f1..2342e0edd2 100644 --- a/packages/language-service/test/hover_spec.ts +++ b/packages/language-service/test/hover_spec.ts @@ -89,31 +89,24 @@ describe('hover', () => { }); it('should be able to find a reference to a component', () => { - const fileName = mockHost.addCode(` - @Component({ - template: '«<ᐱtestᐱ-comp>»' - }) - export class MyComponent { }`); - const marker = mockHost.getDefinitionMarkerFor(fileName, 'test'); - const quickInfo = ngLS.getQuickInfoAtPosition(fileName, marker.start); - expect(quickInfo).toBeTruthy(); - const {textSpan, displayParts} = quickInfo !; - expect(textSpan).toEqual(marker); - expect(toText(displayParts)).toBe('(component) AppModule.TestComponent: class'); + mockHost.override(TEST_TEMPLATE, '<~{cursor}test-comp>'); + const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor'); + const quickInfo = ngLS.getQuickInfoAtPosition(TEST_TEMPLATE, marker.start); + expect(quickInfo).toBeDefined(); + const {displayParts, documentation} = quickInfo !; + expect(toText(displayParts)).toBe('(component) AppModule.TestComponent: typeof TestComponent'); + expect(toText(documentation)).toBe('This Component provides the `test-comp` selector.'); }); it('should be able to find a reference to a directive', () => { - const fileName = mockHost.addCode(` - @Component({ - template: '' - }) - export class MyComponent { }`); - const marker = mockHost.getReferenceMarkerFor(fileName, 'string-model'); - const quickInfo = ngLS.getQuickInfoAtPosition(fileName, marker.start); - expect(quickInfo).toBeTruthy(); - const {textSpan, displayParts} = quickInfo !; - expect(textSpan).toEqual(marker); - expect(toText(displayParts)).toBe('(directive) StringModel: typeof StringModel'); + const content = mockHost.override(TEST_TEMPLATE, `
`); + const marker = mockHost.getLocationMarkerFor(TEST_TEMPLATE, 'cursor'); + const quickInfo = ngLS.getQuickInfoAtPosition(TEST_TEMPLATE, marker.start); + expect(quickInfo).toBeDefined(); + const {displayParts, textSpan} = quickInfo !; + expect(toText(displayParts)).toBe('(directive) AppModule.StringModel: typeof StringModel'); + expect(content.substring(textSpan.start, textSpan.start + textSpan.length)) + .toBe('string-model'); }); it('should be able to find an event provider', () => { diff --git a/packages/language-service/test/typescript_host_spec.ts b/packages/language-service/test/typescript_host_spec.ts index 0352a19067..dadf9e9e7c 100644 --- a/packages/language-service/test/typescript_host_spec.ts +++ b/packages/language-service/test/typescript_host_spec.ts @@ -176,51 +176,4 @@ describe('TypeScriptServiceHost', () => { // files have changed. expect(newModules).toBe(oldModules); }); - - it('should get the correct StaticSymbol for a Directive', () => { - const tsLSHost = new MockTypescriptHost(['/app/app.component.ts', '/app/main.ts']); - const tsLS = ts.createLanguageService(tsLSHost); - const ngLSHost = new TypeScriptServiceHost(tsLSHost, tsLS); - ngLSHost.getAnalyzedModules(); // modules are analyzed lazily - const sf = ngLSHost.getSourceFile('/app/app.component.ts'); - expect(sf).toBeDefined(); - const directiveDecl = sf !.forEachChild(n => { - if (ts.isClassDeclaration(n) && n.name && n.name.text === 'AppComponent') return n; - }); - - expect(directiveDecl).toBeDefined(); - expect(directiveDecl !.name).toBeDefined(); - const fileName = directiveDecl !.getSourceFile().fileName; - const symbolName = directiveDecl !.name !.getText(); - const directiveSymbol = ngLSHost.getStaticSymbol(fileName, symbolName); - expect(directiveSymbol).toBeDefined(); - expect(directiveSymbol !.name).toBe('AppComponent'); - }); - - it('should allow for retreiving analyzedModules in synchronized mode', () => { - const fileName = '/app/app.component.ts'; - const tsLSHost = new MockTypescriptHost([fileName]); - const tsLS = ts.createLanguageService(tsLSHost); - const ngLSHost = new TypeScriptServiceHost(tsLSHost, tsLS); - - // Get initial state - const originalModules = ngLSHost.getAnalyzedModules(); - - // Override app.component.ts with a different component - tsLSHost.override(fileName, ` - import {Component} from '@angular/core'; - - @Component({ - template: '
Hello!
', - }) - export class HelloComponent {} - `); - // Make sure synchronized modules match the original state - const syncModules = ngLSHost.getAnalyzedModules(false); - expect(originalModules).toEqual(syncModules); - - // Now, get modules for the updated project, which should not be synchronized - const updatedModules = ngLSHost.getAnalyzedModules(); - expect(updatedModules).not.toEqual(syncModules); - }); });