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

@@ -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<TypeScriptServiceHost>): ng.Diagnostic[] {
   const directives = new Set<ng.StaticSymbol>();
   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<ts.LanguageServiceHost>): 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

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));
   }

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<T extends ts.Node>(
+    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));
+}

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