From adeee0fa7f63cde67f0bbfa25d99227a37b9f1b2 Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Tue, 10 Sep 2019 10:55:12 -0500 Subject: [PATCH] feat(language-service): provide diagnostic for invalid templateUrls (#32586) `templateUrls` that do not point to actual files are now diagnosed as such by the Language Service. Support for `styleUrls` will come in a next PR. This introduces a utility method `getPropertyValueOfType` that scans TypeScript ASTs until a property assignment whose initializer of a certain type is found. This PR also notices a couple of things that could be improved in the language-service implementation, such as enumerating directive properties and unifying common logic, that will be fixed in future PRs. Part of #32564. PR Close #32586 --- packages/language-service/src/diagnostics.ts | 97 +++++++++++++++++-- .../language-service/src/language_service.ts | 5 +- packages/language-service/src/utils.ts | 18 ++++ .../language-service/test/diagnostics_spec.ts | 34 +++++++ 4 files changed, 145 insertions(+), 9 deletions(-) diff --git a/packages/language-service/src/diagnostics.ts b/packages/language-service/src/diagnostics.ts index 90850f0a4a..5b6dc6f348 100644 --- a/packages/language-service/src/diagnostics.ts +++ b/packages/language-service/src/diagnostics.ts @@ -8,11 +8,13 @@ import {NgAnalyzedModules} from '@angular/compiler'; import {getTemplateExpressionDiagnostics} from '@angular/compiler-cli/src/language_services'; +import * as path from 'path'; import * as ts from 'typescript'; import {AstResult} from './common'; import * as ng from './types'; -import {offsetSpan, spanOf} from './utils'; +import {TypeScriptServiceHost} from './typescript_host'; +import {findPropertyValueOfType, findTightestNode, offsetSpan, spanOf} from './utils'; /** * Return diagnostic information for the parsed AST of the template. @@ -53,8 +55,24 @@ function missingDirective(name: string, isComponent: boolean) { 'available inside a template. Consider adding it to a NgModule declaration.'; } +/** + * Logs an error for an impossible state with a certain message. + */ +function logImpossibleState(message: string) { + console.error(`Impossible state: ${message}`); +} + +/** + * Performs a variety diagnostics on directive declarations. + * + * @param declarations Angular directive declarations + * @param modules NgModules in the project + * @param host TypeScript service host used to perform TypeScript queries + * @return diagnosed errors, if any + */ export function getDeclarationDiagnostics( - declarations: ng.Declaration[], modules: NgAnalyzedModules): ng.Diagnostic[] { + declarations: ng.Declaration[], modules: NgAnalyzedModules, + host: Readonly): ng.Diagnostic[] { const directives = new Set(); for (const ngModule of modules.ngModules) { for (const directive of ngModule.declaredDirectives) { @@ -66,6 +84,20 @@ export function getDeclarationDiagnostics( for (const declaration of declarations) { const {errors, metadata, type, declarationSpan} = declaration; + + const sf = host.getSourceFile(type.filePath); + if (!sf) { + logImpossibleState(`directive ${type.name} exists but has no source file`); + return []; + } + // TypeScript identifier of the directive declaration annotation (e.g. "Component" or + // "Directive") on a directive class. + const directiveIdentifier = findTightestNode(sf, declarationSpan.start); + if (!directiveIdentifier) { + logImpossibleState(`directive ${type.name} exists but has no identifier`); + return []; + } + for (const error of errors) { results.push({ kind: ng.DiagnosticKind.Error, @@ -91,12 +123,28 @@ export function getDeclarationDiagnostics( message: `Component '${type.name}' must have a template or templateUrl`, span: declarationSpan, }); - } else if (template && templateUrl) { - results.push({ - kind: ng.DiagnosticKind.Error, - message: `Component '${type.name}' must not have both template and templateUrl`, - span: declarationSpan, - }); + } else if (templateUrl) { + if (template) { + results.push({ + kind: ng.DiagnosticKind.Error, + message: `Component '${type.name}' must not have both template and templateUrl`, + span: declarationSpan, + }); + } + + // Find templateUrl value from the directive call expression, which is the parent of the + // directive identifier. + // + // TODO: We should create an enum of the various properties a directive can have to use + // instead of string literals. We can then perform a mass migration of all literal usages. + const templateUrlNode = findPropertyValueOfType( + directiveIdentifier.parent, 'templateUrl', ts.isStringLiteralLike); + if (!templateUrlNode) { + logImpossibleState(`templateUrl ${templateUrl} exists but its TypeScript node doesn't`); + return []; + } + + results.push(...validateUrls([templateUrlNode], host.tsLsHost)); } } else if (!directives.has(declaration.type)) { results.push({ @@ -110,6 +158,39 @@ export function getDeclarationDiagnostics( return results; } +/** + * Checks that URLs on a directive point to a valid file. + * Note that this diagnostic check may require a filesystem hit, and thus may be slower than other + * checks. + * + * @param urls urls to check for validity + * @param tsLsHost TS LS host used for querying filesystem information + * @return diagnosed url errors, if any + */ +function validateUrls( + urls: ts.StringLiteralLike[], tsLsHost: Readonly): ng.Diagnostic[] { + if (!tsLsHost.fileExists) { + return []; + } + + const allErrors: ng.Diagnostic[] = []; + // TODO(ayazhafiz): most of this logic can be unified with the logic in + // definitions.ts#getUrlFromProperty. Create a utility function to be used by both. + for (const urlNode of urls) { + const curPath = urlNode.getSourceFile().fileName; + const url = path.join(path.dirname(curPath), urlNode.text); + if (tsLsHost.fileExists(url)) continue; + + allErrors.push({ + kind: ng.DiagnosticKind.Error, + message: `URL does not point to a valid file`, + // Exclude opening and closing quotes in the url span. + span: {start: urlNode.getStart() + 1, end: urlNode.end - 1}, + }); + } + return allErrors; +} + /** * Return a recursive data structure that chains diagnostic messages. * @param chain diff --git a/packages/language-service/src/language_service.ts b/packages/language-service/src/language_service.ts index 22e72d80cb..37df7d4d69 100644 --- a/packages/language-service/src/language_service.ts +++ b/packages/language-service/src/language_service.ts @@ -37,6 +37,7 @@ class LanguageServiceImpl implements LanguageService { const analyzedModules = this.host.getAnalyzedModules(); // same role as 'synchronizeHostData' const results: Diagnostic[] = []; const templates = this.host.getTemplates(fileName); + for (const template of templates) { const astOrDiagnostic = this.host.getTemplateAst(template); if (isAstResult(astOrDiagnostic)) { @@ -45,10 +46,12 @@ class LanguageServiceImpl implements LanguageService { results.push(astOrDiagnostic); } } + const declarations = this.host.getDeclarations(fileName); if (declarations && declarations.length) { - results.push(...getDeclarationDiagnostics(declarations, analyzedModules)); + results.push(...getDeclarationDiagnostics(declarations, analyzedModules, this.host)); } + const sourceFile = fileName.endsWith('.ts') ? this.host.getSourceFile(fileName) : undefined; return uniqueBySpan(results).map(d => ngDiagnosticToTsDiagnostic(d, sourceFile)); } diff --git a/packages/language-service/src/utils.ts b/packages/language-service/src/utils.ts index 933d87514d..6b8ca94b27 100644 --- a/packages/language-service/src/utils.ts +++ b/packages/language-service/src/utils.ts @@ -214,3 +214,21 @@ export function getDirectiveClassLike(node: ts.Node): DirectiveClassLike|undefin } } } + +/** + * Finds the value of a property assignment that is nested in a TypeScript node and is of a certain + * type T. + * + * @param startNode node to start searching for nested property assignment from + * @param propName property assignment name + * @param predicate function to verify that a node is of type T. + * @return node property assignment value of type T, or undefined if none is found + */ +export function findPropertyValueOfType( + startNode: ts.Node, propName: string, predicate: (node: ts.Node) => node is T): T|undefined { + if (ts.isPropertyAssignment(startNode) && startNode.name.getText() === propName) { + const {initializer} = startNode; + if (predicate(initializer)) return initializer; + } + return startNode.forEachChild(c => findPropertyValueOfType(c, propName, predicate)); +} diff --git a/packages/language-service/test/diagnostics_spec.ts b/packages/language-service/test/diagnostics_spec.ts index dc1409fb24..39aa793d04 100644 --- a/packages/language-service/test/diagnostics_spec.ts +++ b/packages/language-service/test/diagnostics_spec.ts @@ -475,6 +475,40 @@ describe('diagnostics', () => { `Module '"../node_modules/@angular/core/core"' has no exported member 'OpaqueToken'.`); }); + describe('URL diagnostics', () => { + it('should report errors for invalid templateUrls', () => { + const fileName = mockHost.addCode(` + @Component({ + templateUrl: '«notAFile»', + }) + export class MyComponent {}`); + + const marker = mockHost.getReferenceMarkerFor(fileName, 'notAFile'); + + const diagnostics = ngLS.getDiagnostics(fileName) !; + const urlDiagnostic = + diagnostics.find(d => d.messageText === 'URL does not point to a valid file'); + expect(urlDiagnostic).toBeDefined(); + + const {start, length} = urlDiagnostic !; + expect(start).toBe(marker.start); + expect(length).toBe(marker.length); + }); + + it('should not report errors for valid templateUrls', () => { + const fileName = mockHost.addCode(` + @Component({ + templateUrl: './test.ng', + }) + export class MyComponent {}`); + + const diagnostics = ngLS.getDiagnostics(fileName) !; + const urlDiagnostic = + diagnostics.find(d => d.messageText === 'URL does not point to a valid file'); + expect(urlDiagnostic).toBeUndefined(); + }); + }); + // https://github.com/angular/vscode-ng-language-service/issues/235 // There is no easy fix for this issue currently due to the way template // tokenization is done. In the example below, the whole string