From 3a1d36cce3b60e40ca7f887790a4c84f503ff355 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Wed, 21 Oct 2020 11:01:10 -0700 Subject: [PATCH] refactor(language-service): Use compiler APIs in Ivy to get definitions for external resources (#39476) Rather than re-reading component metadata that was already interpreted by the Ivy compiler, the Language Service should instead use the compiler APIs to get information it needs about the metadata. PR Close #39476 --- .../compiler-cli/src/ngtsc/metadata/index.ts | 2 +- .../ngtsc/metadata/src/resource_registry.ts | 8 + packages/language-service/ivy/BUILD.bazel | 2 +- packages/language-service/ivy/definitions.ts | 167 +++++++----------- .../language-service/ivy/language_service.ts | 8 +- .../ivy/language_service_adapter.ts | 9 +- .../ivy/test/definitions_spec.ts | 24 ++- packages/language-service/ivy/ts_utils.ts | 30 ++++ packages/language-service/ivy/utils.ts | 146 ++++----------- .../language-service/src/typescript_host.ts | 8 +- 10 files changed, 168 insertions(+), 236 deletions(-) create mode 100644 packages/language-service/ivy/ts_utils.ts diff --git a/packages/compiler-cli/src/ngtsc/metadata/index.ts b/packages/compiler-cli/src/ngtsc/metadata/index.ts index 653465c772..30a12ab42a 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/index.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/index.ts @@ -10,6 +10,6 @@ export * from './src/api'; export {DtsMetadataReader} from './src/dts'; export {flattenInheritedDirectiveMetadata} from './src/inheritance'; export {CompoundMetadataRegistry, LocalMetadataRegistry, InjectableClassRegistry} from './src/registry'; -export {ResourceRegistry, Resource, ComponentResources} from './src/resource_registry'; +export {ResourceRegistry, Resource, ComponentResources, isExternalResource, ExternalResource} from './src/resource_registry'; export {extractDirectiveTypeCheckMeta, CompoundMetadataReader} from './src/util'; export {BindingPropertyName, ClassPropertyMapping, ClassPropertyName, InputOrOutput} from './src/property_mapping'; diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/resource_registry.ts b/packages/compiler-cli/src/ngtsc/metadata/src/resource_registry.ts index 24547bd968..28a50e1263 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/resource_registry.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/resource_registry.ts @@ -23,6 +23,14 @@ export interface Resource { expression: ts.Expression; } +export interface ExternalResource extends Resource { + path: AbsoluteFsPath; +} + +export function isExternalResource(resource: Resource): resource is ExternalResource { + return resource.path !== null; +} + /** * Represents the either inline or external resources of a component. * diff --git a/packages/language-service/ivy/BUILD.bazel b/packages/language-service/ivy/BUILD.bazel index 6a48cdcde5..8b584217c6 100644 --- a/packages/language-service/ivy/BUILD.bazel +++ b/packages/language-service/ivy/BUILD.bazel @@ -12,8 +12,8 @@ ts_library( "//packages/compiler-cli/src/ngtsc/core:api", "//packages/compiler-cli/src/ngtsc/file_system", "//packages/compiler-cli/src/ngtsc/incremental", + "//packages/compiler-cli/src/ngtsc/metadata", "//packages/compiler-cli/src/ngtsc/reflection", - "//packages/compiler-cli/src/ngtsc/resource", "//packages/compiler-cli/src/ngtsc/shims", "//packages/compiler-cli/src/ngtsc/typecheck", "//packages/compiler-cli/src/ngtsc/typecheck/api", diff --git a/packages/language-service/ivy/definitions.ts b/packages/language-service/ivy/definitions.ts index e9215eec18..5e59d651fe 100644 --- a/packages/language-service/ivy/definitions.ts +++ b/packages/language-service/ivy/definitions.ts @@ -8,108 +8,13 @@ import {AST, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstElement, TmplAstNode, TmplAstTemplate, TmplAstTextAttribute} from '@angular/compiler'; import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core'; +import {isExternalResource} from '@angular/compiler-cli/src/ngtsc/metadata'; import {DirectiveSymbol, DomBindingSymbol, ElementSymbol, ShimLocation, Symbol, SymbolKind, TemplateSymbol} from '@angular/compiler-cli/src/ngtsc/typecheck/api'; import * as ts from 'typescript'; + import {getTargetAtPosition} from './template_target'; - -import {findTightestNode, flatMap, getClassDeclFromDecoratorProp, getDirectiveMatchesForAttribute, getDirectiveMatchesForElementTag, getPropertyAssignmentFromValue, getTemplateInfoAtPosition, getTextSpanOfNode, isDollarEvent, isTypeScriptFile, TemplateInfo, toTextSpan} from './utils'; - - -export interface ResourceResolver { - /** - * Resolve the url of a resource relative to the file that contains the reference to it. - * - * @param file The, possibly relative, url of the resource. - * @param basePath The path to the file that contains the URL of the resource. - * @returns A resolved url of resource. - * @throws An error if the resource cannot be resolved. - */ - resolve(file: string, basePath: string): string; -} - -/** - * Gets an Angular-specific definition in a TypeScript source file. - */ -export function getTsDefinitionAndBoundSpan( - sf: ts.SourceFile, position: number, - resourceResolver: ResourceResolver): ts.DefinitionInfoAndBoundSpan|undefined { - const node = findTightestNode(sf, position); - if (!node) return; - switch (node.kind) { - case ts.SyntaxKind.StringLiteral: - case ts.SyntaxKind.NoSubstitutionTemplateLiteral: - // Attempt to extract definition of a URL in a property assignment. - return getUrlFromProperty(node as ts.StringLiteralLike, resourceResolver); - default: - return undefined; - } -} - -/** - * Attempts to get the definition of a file whose URL is specified in a property assignment in a - * directive decorator. - * Currently applies to `templateUrl` and `styleUrls` properties. - */ -function getUrlFromProperty(urlNode: ts.StringLiteralLike, resourceResolver: ResourceResolver): - ts.DefinitionInfoAndBoundSpan|undefined { - // Get the property assignment node corresponding to the `templateUrl` or `styleUrls` assignment. - // These assignments are specified differently; `templateUrl` is a string, and `styleUrls` is - // an array of strings: - // { - // templateUrl: './template.ng.html', - // styleUrls: ['./style.css', './other-style.css'] - // } - // `templateUrl`'s property assignment can be found from the string literal node; - // `styleUrls`'s property assignment can be found from the array (parent) node. - // - // First search for `templateUrl`. - let asgn = getPropertyAssignmentFromValue(urlNode, 'templateUrl'); - if (!asgn) { - // `templateUrl` assignment not found; search for `styleUrls` array assignment. - asgn = getPropertyAssignmentFromValue(urlNode.parent, 'styleUrls'); - if (!asgn) { - // Nothing found, bail. - return; - } - } - - // If the property assignment is not a property of a class decorator, don't generate definitions - // for it. - if (!getClassDeclFromDecoratorProp(asgn)) { - return; - } - - const sf = urlNode.getSourceFile(); - let url: string; - try { - url = resourceResolver.resolve(urlNode.text, sf.fileName); - } catch { - // If the file does not exist, bail. - return; - } - - const templateDefinitions: ts.DefinitionInfo[] = [{ - kind: ts.ScriptElementKind.externalModuleName, - name: url, - containerKind: ts.ScriptElementKind.unknown, - containerName: '', - // Reading the template is expensive, so don't provide a preview. - // TODO(ayazhafiz): Consider providing an actual span: - // 1. We're likely to read the template anyway - // 2. We could show just the first 100 chars or so - textSpan: {start: 0, length: 0}, - fileName: url, - }]; - - return { - definitions: templateDefinitions, - textSpan: { - // Exclude opening and closing quotes in the url span. - start: urlNode.getStart() + 1, - length: urlNode.getWidth() - 2, - }, - }; -} +import {findTightestNode, getParentClassDeclaration} from './ts_utils'; +import {flatMap, getDirectiveMatchesForAttribute, getDirectiveMatchesForElementTag, getTemplateInfoAtPosition, getTextSpanOfNode, isDollarEvent, isTypeScriptFile, TemplateInfo, toTextSpan} from './utils'; interface DefinitionMeta { node: AST|TmplAstNode; @@ -122,9 +27,7 @@ interface HasShimLocation { } export class DefinitionBuilder { - constructor( - private readonly tsLS: ts.LanguageService, private readonly compiler: NgCompiler, - private readonly resourceResolver: ResourceResolver) {} + constructor(private readonly tsLS: ts.LanguageService, private readonly compiler: NgCompiler) {} getDefinitionAndBoundSpan(fileName: string, position: number): ts.DefinitionInfoAndBoundSpan |undefined { @@ -136,11 +39,7 @@ export class DefinitionBuilder { if (!isTypeScriptFile(fileName)) { return; } - const sf = this.compiler.getNextProgram().getSourceFile(fileName); - if (!sf) { - return; - } - return getTsDefinitionAndBoundSpan(sf, position, this.resourceResolver); + return getDefinitionForExpressionAtPosition(fileName, position, this.compiler); } const definitionMeta = this.getDefinitionMetaAtPosition(templateInfo, position); // The `$event` of event handlers would point to the $event parameter in the shim file, as in @@ -315,3 +214,57 @@ export class DefinitionBuilder { return {node, parent, symbol}; } } + +/** + * Gets an Angular-specific definition in a TypeScript source file. + */ +function getDefinitionForExpressionAtPosition( + fileName: string, position: number, compiler: NgCompiler): ts.DefinitionInfoAndBoundSpan| + undefined { + const sf = compiler.getNextProgram().getSourceFile(fileName); + if (sf === undefined) { + return; + } + + const expression = findTightestNode(sf, position); + if (expression === undefined) { + return; + } + const classDeclaration = getParentClassDeclaration(expression); + if (classDeclaration === undefined) { + return; + } + const componentResources = compiler.getComponentResources(classDeclaration); + if (componentResources === null) { + return; + } + + const allResources = [...componentResources.styles, componentResources.template]; + + const resourceForExpression = allResources.find(resource => resource.expression === expression); + if (resourceForExpression === undefined || !isExternalResource(resourceForExpression)) { + return; + } + + const templateDefinitions: ts.DefinitionInfo[] = [{ + kind: ts.ScriptElementKind.externalModuleName, + name: resourceForExpression.path, + containerKind: ts.ScriptElementKind.unknown, + containerName: '', + // Reading the template is expensive, so don't provide a preview. + // TODO(ayazhafiz): Consider providing an actual span: + // 1. We're likely to read the template anyway + // 2. We could show just the first 100 chars or so + textSpan: {start: 0, length: 0}, + fileName: resourceForExpression.path, + }]; + + return { + definitions: templateDefinitions, + textSpan: { + // Exclude opening and closing quotes in the url span. + start: expression.getStart() + 1, + length: expression.getWidth() - 2, + }, + }; +} diff --git a/packages/language-service/ivy/language_service.ts b/packages/language-service/ivy/language_service.ts index a7538a8dfe..4af86f7061 100644 --- a/packages/language-service/ivy/language_service.ts +++ b/packages/language-service/ivy/language_service.ts @@ -58,8 +58,8 @@ export class LanguageService { getDefinitionAndBoundSpan(fileName: string, position: number): ts.DefinitionInfoAndBoundSpan |undefined { const compiler = this.compilerFactory.getOrCreateWithChangedFile(fileName, this.options); - const results = new DefinitionBuilder(this.tsLS, compiler, this.adapter) - .getDefinitionAndBoundSpan(fileName, position); + const results = + new DefinitionBuilder(this.tsLS, compiler).getDefinitionAndBoundSpan(fileName, position); this.compilerFactory.registerLastKnownProgram(); return results; } @@ -67,8 +67,8 @@ export class LanguageService { getTypeDefinitionAtPosition(fileName: string, position: number): readonly ts.DefinitionInfo[]|undefined { const compiler = this.compilerFactory.getOrCreateWithChangedFile(fileName, this.options); - const results = new DefinitionBuilder(this.tsLS, compiler, this.adapter) - .getTypeDefinitionsAtPosition(fileName, position); + const results = + new DefinitionBuilder(this.tsLS, compiler).getTypeDefinitionsAtPosition(fileName, position); this.compilerFactory.registerLastKnownProgram(); return results; } diff --git a/packages/language-service/ivy/language_service_adapter.ts b/packages/language-service/ivy/language_service_adapter.ts index 377eea2779..2aeb1e76a0 100644 --- a/packages/language-service/ivy/language_service_adapter.ts +++ b/packages/language-service/ivy/language_service_adapter.ts @@ -8,14 +8,12 @@ import {NgCompilerAdapter} from '@angular/compiler-cli/src/ngtsc/core/api'; import {absoluteFrom, AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system'; -import {AdapterResourceLoader} from '@angular/compiler-cli/src/ngtsc/resource'; import {isShim} from '@angular/compiler-cli/src/ngtsc/shims'; import * as ts from 'typescript/lib/tsserverlibrary'; -import {ResourceResolver} from './definitions'; import {isTypeScriptFile} from './utils'; -export class LanguageServiceAdapter implements NgCompilerAdapter, ResourceResolver { +export class LanguageServiceAdapter implements NgCompilerAdapter { readonly entryPoint = null; readonly constructionDiagnostics: ts.Diagnostic[] = []; readonly ignoreForEmit: Set = new Set(); @@ -79,9 +77,4 @@ export class LanguageServiceAdapter implements NgCompilerAdapter, ResourceResolv const latestVersion = this.project.getScriptVersion(fileName); return lastVersion !== latestVersion; } - - resolve(file: string, basePath: string): string { - const loader = new AdapterResourceLoader(this, this.project.getCompilationSettings()); - return loader.resolve(file, basePath); - } } diff --git a/packages/language-service/ivy/test/definitions_spec.ts b/packages/language-service/ivy/test/definitions_spec.ts index 23b84f61d4..dcc52ca9f4 100644 --- a/packages/language-service/ivy/test/definitions_spec.ts +++ b/packages/language-service/ivy/test/definitions_spec.ts @@ -459,7 +459,7 @@ describe('definitions', () => { @Component({ templateUrl: './tes¦t.ng', }) - export class MyComponent {}`); + export class AppComponent {}`); const result = ngLS.getDefinitionAndBoundSpan(APP_COMPONENT, position); expect(result).toBeDefined(); @@ -481,7 +481,7 @@ describe('definitions', () => { template: 'empty', styleUrls: ['./te¦st.css'] }) - export class MyComponent {}`); + export class AppComponent {}`); const result = ngLS.getDefinitionAndBoundSpan(APP_COMPONENT, position); @@ -497,6 +497,26 @@ describe('definitions', () => { expect(def.fileName).toContain('/app/test.css'); expect(def.textSpan).toEqual({start: 0, length: 0}); }); + + xit('should be able to find a resource url with malformed component meta', () => { + const {position, text} = service.overwrite(APP_COMPONENT, ` + import {Component} from '@angular/core'; + @Component({ + invalidProperty: '', + styleUrls: ['./te¦st.css'] + }) + export class AppComponent {}`); + const result = ngLS.getDefinitionAndBoundSpan(APP_COMPONENT, position); + + + expect(result).toBeDefined(); + const {textSpan, definitions} = result!; + + expect(text.substring(textSpan.start, textSpan.start + textSpan.length)) + .toEqual('./test.css'); + expect(definitions).toBeDefined(); + expect(definitions![0].fileName).toContain('/app/test.css'); + }); }); function getDefinitionsAndAssertBoundSpan( diff --git a/packages/language-service/ivy/ts_utils.ts b/packages/language-service/ivy/ts_utils.ts new file mode 100644 index 0000000000..4ebb791870 --- /dev/null +++ b/packages/language-service/ivy/ts_utils.ts @@ -0,0 +1,30 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import * as ts from 'typescript'; + +/** + * Return the node that most tightly encompasses the specified `position`. + * @param node The starting node to start the top-down search. + * @param position The target position within the `node`. + */ +export function findTightestNode(node: ts.Node, position: number): ts.Node|undefined { + if (node.getStart() <= position && position < node.getEnd()) { + return node.forEachChild(c => findTightestNode(c, position)) ?? node; + } + return undefined; +} + +export function getParentClassDeclaration(startNode: ts.Node): ts.ClassDeclaration|undefined { + while (startNode) { + if (ts.isClassDeclaration(startNode)) { + return startNode; + } + startNode = startNode.parent; + } + return undefined; +} diff --git a/packages/language-service/ivy/utils.ts b/packages/language-service/ivy/utils.ts index 266c43f1f4..1c54df46a1 100644 --- a/packages/language-service/ivy/utils.ts +++ b/packages/language-service/ivy/utils.ts @@ -7,6 +7,7 @@ */ import {AbsoluteSourceSpan, CssSelector, ParseSourceSpan, SelectorMatcher} from '@angular/compiler'; import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core'; +import {isExternalResource} from '@angular/compiler-cli/src/ngtsc/metadata'; import {DeclarationNode} from '@angular/compiler-cli/src/ngtsc/reflection'; import {DirectiveSymbol} from '@angular/compiler-cli/src/ngtsc/typecheck/api'; import * as e from '@angular/compiler/src/expression_parser/ast'; // e for expression AST @@ -14,87 +15,7 @@ import * as t from '@angular/compiler/src/render3/r3_ast'; // t for temp import * as ts from 'typescript'; import {ALIAS_NAME, SYMBOL_PUNC} from './display_parts'; - - - -/** - * Return the node that most tightly encompass the specified `position`. - * @param node - * @param position - */ -export function findTightestNode(node: ts.Node, position: number): ts.Node|undefined { - if (node.getStart() <= position && position < node.getEnd()) { - return node.forEachChild(c => findTightestNode(c, position)) || node; - } -} - -/** - * Returns a property assignment from the assignment value if the property name - * matches the specified `key`, or `undefined` if there is no match. - */ -export function getPropertyAssignmentFromValue(value: ts.Node, key: string): ts.PropertyAssignment| - undefined { - const propAssignment = value.parent; - if (!propAssignment || !ts.isPropertyAssignment(propAssignment) || - propAssignment.name.getText() !== key) { - return; - } - return propAssignment; -} - -/** - * Given a decorator property assignment, return the ClassDeclaration node that corresponds to the - * directive class the property applies to. - * If the property assignment is not on a class decorator, no declaration is returned. - * - * For example, - * - * @Component({ - * template: '
' - * ^^^^^^^^^^^^^^^^^^^^^^^---- property assignment - * }) - * class AppComponent {} - * ^---- class declaration node - * - * @param propAsgnNode property assignment - */ -export function getClassDeclFromDecoratorProp(propAsgnNode: ts.PropertyAssignment): - ts.ClassDeclaration|undefined { - if (!propAsgnNode.parent || !ts.isObjectLiteralExpression(propAsgnNode.parent)) { - return; - } - const objLitExprNode = propAsgnNode.parent; - if (!objLitExprNode.parent || !ts.isCallExpression(objLitExprNode.parent)) { - return; - } - const callExprNode = objLitExprNode.parent; - if (!callExprNode.parent || !ts.isDecorator(callExprNode.parent)) { - return; - } - const decorator = callExprNode.parent; - if (!decorator.parent || !ts.isClassDeclaration(decorator.parent)) { - return; - } - const classDeclNode = decorator.parent; - return classDeclNode; -} - -/** - * Given the node which is the string of the inline template for a component, returns the - * `ts.ClassDeclaration` for the component. - */ -export function getClassDeclOfInlineTemplateNode(templateStringNode: ts.Node): ts.ClassDeclaration| - undefined { - if (!ts.isStringLiteralLike(templateStringNode)) { - return; - } - const tmplAsgn = getPropertyAssignmentFromValue(templateStringNode, 'template'); - if (!tmplAsgn) { - return; - } - return getClassDeclFromDecoratorProp(tmplAsgn); -} - +import {findTightestNode, getParentClassDeclaration} from './ts_utils'; export function getTextSpanOfNode(node: t.Node|e.AST): ts.TextSpan { if (isTemplateNodeWithKeyAndValue(node)) { @@ -146,19 +67,50 @@ export interface TemplateInfo { component: ts.ClassDeclaration; } +function getInlineTemplateInfoAtPosition( + sf: ts.SourceFile, position: number, compiler: NgCompiler): TemplateInfo|undefined { + const expression = findTightestNode(sf, position); + if (expression === undefined) { + return undefined; + } + const classDecl = getParentClassDeclaration(expression); + if (classDecl === undefined) { + return undefined; + } + + // Return `undefined` if the position is not on the template expression or the template resource + // is not inline. + const resources = compiler.getComponentResources(classDecl); + if (resources === null || isExternalResource(resources.template) || + expression !== resources.template.expression) { + return undefined; + } + + const template = compiler.getTemplateTypeChecker().getTemplate(classDecl); + if (template === null) { + return undefined; + } + + return {template, component: classDecl}; +} + /** * Retrieves the `ts.ClassDeclaration` at a location along with its template nodes. */ export function getTemplateInfoAtPosition( fileName: string, position: number, compiler: NgCompiler): TemplateInfo|undefined { if (isTypeScriptFile(fileName)) { - return getTemplateInfoFromClassMeta(fileName, position, compiler); + const sf = compiler.getNextProgram().getSourceFile(fileName); + if (sf === undefined) { + return undefined; + } + + return getInlineTemplateInfoAtPosition(sf, position, compiler); } else { return getFirstComponentForTemplateFile(fileName, compiler); } } - /** * First, attempt to sort component declarations by file name. * If the files are the same, sort by start location of the declaration. @@ -194,34 +146,6 @@ function getFirstComponentForTemplateFile(fileName: string, compiler: NgCompiler return undefined; } -/** - * Retrieves the `ts.ClassDeclaration` at a location along with its template nodes. - */ -function getTemplateInfoFromClassMeta( - fileName: string, position: number, compiler: NgCompiler): TemplateInfo|undefined { - const classDecl = getClassDeclForInlineTemplateAtPosition(fileName, position, compiler); - if (!classDecl || !classDecl.name) { // Does not handle anonymous class - return; - } - const template = compiler.getTemplateTypeChecker().getTemplate(classDecl); - if (template === null) { - return; - } - - return {template, component: classDecl}; -} - -function getClassDeclForInlineTemplateAtPosition( - fileName: string, position: number, compiler: NgCompiler): ts.ClassDeclaration|undefined { - const sourceFile = compiler.getNextProgram().getSourceFile(fileName); - if (!sourceFile) { - return undefined; - } - const node = findTightestNode(sourceFile, position); - if (!node) return; - return getClassDeclOfInlineTemplateNode(node); -} - /** * Given an attribute node, converts it to string form. */ diff --git a/packages/language-service/src/typescript_host.ts b/packages/language-service/src/typescript_host.ts index 1ce6b56f8b..ae927fd77d 100644 --- a/packages/language-service/src/typescript_host.ts +++ b/packages/language-service/src/typescript_host.ts @@ -13,7 +13,7 @@ import * as tss from 'typescript/lib/tsserverlibrary'; import {createLanguageService} from './language_service'; import {ReflectorHost} from './reflector_host'; import {ExternalTemplate, InlineTemplate} from './template'; -import {findTightestNode, getClassDeclOfInlineTemplateNode, getDirectiveClassLike} from './ts_utils'; +import {findTightestNode, getClassDeclFromDecoratorProp, getDirectiveClassLike, getPropertyAssignmentFromValue} from './ts_utils'; import {AstResult, Declaration, DeclarationError, DiagnosticMessageChain, LanguageService, LanguageServiceHost, Span, TemplateSource} from './types'; /** @@ -382,7 +382,11 @@ export class TypeScriptServiceHost implements LanguageServiceHost { if (!tss.isStringLiteralLike(node)) { return; } - const classDecl = getClassDeclOfInlineTemplateNode(node); + const tmplAsgn = getPropertyAssignmentFromValue(node, 'template'); + if (!tmplAsgn) { + return; + } + const classDecl = getClassDeclFromDecoratorProp(tmplAsgn); if (!classDecl || !classDecl.name) { // Does not handle anonymous class return; }