refactor(language-service): Return ts.Diagnostic[] for getDiagnostics (#32115)

Part 2/3 of language service refactoring:
Now that the language service is a proper tsserver plugin, all LS
interfaces should return TS values. This PR refactors the
ng.getDiagnostics() API to return ts.Diagnostic[] instead of
ng.Diagnostic[].

PR Close #32115

packages/language-service/src/diagnostics.ts

@@ -7,13 +7,52 @@
  */
 
 import {NgAnalyzedModules, StaticSymbol} from '@angular/compiler';
+import {DiagnosticTemplateInfo, getTemplateExpressionDiagnostics} from '@angular/compiler-cli/src/language_services';
+import * as ts from 'typescript';
+
 import {AstResult} from './common';
 import {Declarations, Diagnostic, DiagnosticKind, DiagnosticMessageChain, Diagnostics, Span, TemplateSource} from './types';
+import {offsetSpan, spanOf} from './utils';
 
 export interface AstProvider {
   getTemplateAst(template: TemplateSource, fileName: string): AstResult;
 }
 
+export function getTemplateDiagnostics(template: TemplateSource, ast: AstResult): Diagnostics {
+  const results: Diagnostics = [];
+
+  if (ast.parseErrors && ast.parseErrors.length) {
+    results.push(...ast.parseErrors.map<Diagnostic>(e => {
+      return {
+        kind: DiagnosticKind.Error,
+        span: offsetSpan(spanOf(e.span), template.span.start),
+        message: e.msg,
+      };
+    }));
+  } else if (ast.templateAst && ast.htmlAst) {
+    const info: DiagnosticTemplateInfo = {
+      templateAst: ast.templateAst,
+      htmlAst: ast.htmlAst,
+      offset: template.span.start,
+      query: template.query,
+      members: template.members,
+    };
+    const expressionDiagnostics = getTemplateExpressionDiagnostics(info);
+    results.push(...expressionDiagnostics);
+  }
+  if (ast.errors) {
+    results.push(...ast.errors.map<Diagnostic>(e => {
+      return {
+        kind: e.kind,
+        span: e.span || template.span,
+        message: e.message,
+      };
+    }));
+  }
+
+  return results;
+}
+
 export function getDeclarationDiagnostics(
     declarations: Declarations, modules: NgAnalyzedModules): Diagnostics {
   const results: Diagnostics = [];
@@ -60,3 +99,34 @@ export function getDeclarationDiagnostics(
 
   return results;
 }
+
+function diagnosticChainToDiagnosticChain(chain: DiagnosticMessageChain):
+    ts.DiagnosticMessageChain {
+  return {
+    messageText: chain.message,
+    category: ts.DiagnosticCategory.Error,
+    code: 0,
+    next: chain.next ? diagnosticChainToDiagnosticChain(chain.next) : undefined
+  };
+}
+
+function diagnosticMessageToDiagnosticMessageText(message: string | DiagnosticMessageChain): string|
+    ts.DiagnosticMessageChain {
+  if (typeof message === 'string') {
+    return message;
+  }
+  return diagnosticChainToDiagnosticChain(message);
+}
+
+export function ngDiagnosticToTsDiagnostic(
+    d: Diagnostic, file: ts.SourceFile | undefined): ts.Diagnostic {
+  return {
+    file,
+    start: d.span.start,
+    length: d.span.end - d.span.start,
+    messageText: diagnosticMessageToDiagnosticMessageText(d.message),
+    category: ts.DiagnosticCategory.Error,
+    code: 0,
+    source: 'ng',
+  };
+}

packages/language-service/src/language_service.ts

@@ -6,16 +6,15 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {CompileMetadataResolver, CompilePipeSummary} from '@angular/compiler';
-import {DiagnosticTemplateInfo, getTemplateExpressionDiagnostics} from '@angular/compiler-cli/src/language_services';
+import {CompilePipeSummary} from '@angular/compiler';
 import * as tss from 'typescript/lib/tsserverlibrary';
+
 import {getTemplateCompletions} from './completions';
 import {getDefinitionAndBoundSpan} from './definitions';
-import {getDeclarationDiagnostics} from './diagnostics';
+import {getDeclarationDiagnostics, getTemplateDiagnostics, ngDiagnosticToTsDiagnostic} from './diagnostics';
 import {getHover} from './hover';
-import {Completion, Diagnostic, DiagnosticKind, Diagnostics, Hover, LanguageService, LanguageServiceHost, Location, Span, TemplateSource} from './types';
-import {offsetSpan, spanOf} from './utils';
-
+import {Completion, Diagnostic, LanguageService, Span} from './types';
+import {TypeScriptServiceHost} from './typescript_host';
 
 
 /**
@@ -23,20 +22,21 @@ import {offsetSpan, spanOf} from './utils';
  *
  * @publicApi
  */
-export function createLanguageService(host: LanguageServiceHost): LanguageService {
+export function createLanguageService(host: TypeScriptServiceHost): LanguageService {
   return new LanguageServiceImpl(host);
 }
 
 class LanguageServiceImpl implements LanguageService {
-  constructor(private host: LanguageServiceHost) {}
+  constructor(private readonly host: TypeScriptServiceHost) {}
 
   getTemplateReferences(): string[] { return this.host.getTemplateReferences(); }
 
-  getDiagnostics(fileName: string): Diagnostic[] {
+  getDiagnostics(fileName: string): tss.Diagnostic[] {
     const results: Diagnostic[] = [];
     const templates = this.host.getTemplates(fileName);
-    if (templates && templates.length) {
-      results.push(...this.getTemplateDiagnostics(fileName, templates));
+    for (const template of templates) {
+      const ast = this.host.getTemplateAst(template, fileName);
+      results.push(...getTemplateDiagnostics(template, ast));
     }
 
     const declarations = this.host.getDeclarations(fileName);
@@ -44,8 +44,11 @@ class LanguageServiceImpl implements LanguageService {
       const summary = this.host.getAnalyzedModules();
       results.push(...getDeclarationDiagnostics(declarations, summary));
     }
-
-    return uniqueBySpan(results);
+    if (!results.length) {
+      return [];
+    }
+    const sourceFile = fileName.endsWith('.ts') ? this.host.getSourceFile(fileName) : undefined;
+    return uniqueBySpan(results).map(d => ngDiagnosticToTsDiagnostic(d, sourceFile));
   }
 
   getPipesAt(fileName: string, position: number): CompilePipeSummary[] {
@@ -76,38 +79,6 @@ class LanguageServiceImpl implements LanguageService {
       return getHover(templateInfo);
     }
   }
-
-  private getTemplateDiagnostics(fileName: string, templates: TemplateSource[]): Diagnostics {
-    const results: Diagnostics = [];
-    for (const template of templates) {
-      const ast = this.host.getTemplateAst(template, fileName);
-      if (ast) {
-        if (ast.parseErrors && ast.parseErrors.length) {
-          results.push(...ast.parseErrors.map<Diagnostic>(
-              e => ({
-                kind: DiagnosticKind.Error,
-                span: offsetSpan(spanOf(e.span), template.span.start),
-                message: e.msg
-              })));
-        } else if (ast.templateAst && ast.htmlAst) {
-          const info: DiagnosticTemplateInfo = {
-            templateAst: ast.templateAst,
-            htmlAst: ast.htmlAst,
-            offset: template.span.start,
-            query: template.query,
-            members: template.members
-          };
-          const expressionDiagnostics = getTemplateExpressionDiagnostics(info);
-          results.push(...expressionDiagnostics);
-        }
-        if (ast.errors) {
-          results.push(...ast.errors.map<Diagnostic>(
-              e => ({kind: e.kind, span: e.span || template.span, message: e.message})));
-        }
-      }
-    }
-    return results;
-  }
 }
 
 function uniqueBySpan<T extends{span: Span}>(elements: T[]): T[] {

packages/language-service/src/ts_plugin.ts

@@ -10,7 +10,7 @@ import * as ts from 'typescript'; // used as value, passed in by tsserver at run
 import * as tss from 'typescript/lib/tsserverlibrary'; // used as type only
 
 import {createLanguageService} from './language_service';
-import {Completion, Diagnostic, DiagnosticMessageChain} from './types';
+import {Completion} from './types';
 import {TypeScriptServiceHost} from './typescript_host';
 
 const projectHostMap = new WeakMap<tss.server.Project, TypeScriptServiceHost>();
@@ -33,36 +33,6 @@ function completionToEntry(c: Completion): tss.CompletionEntry {
   };
 }
 
-function diagnosticChainToDiagnosticChain(chain: DiagnosticMessageChain):
-    ts.DiagnosticMessageChain {
-  return {
-    messageText: chain.message,
-    category: ts.DiagnosticCategory.Error,
-    code: 0,
-    next: chain.next ? diagnosticChainToDiagnosticChain(chain.next) : undefined
-  };
-}
-
-function diagnosticMessageToDiagnosticMessageText(message: string | DiagnosticMessageChain): string|
-    tss.DiagnosticMessageChain {
-  if (typeof message === 'string') {
-    return message;
-  }
-  return diagnosticChainToDiagnosticChain(message);
-}
-
-function diagnosticToDiagnostic(d: Diagnostic, file: tss.SourceFile | undefined): tss.Diagnostic {
-  return {
-    file,
-    start: d.span.start,
-    length: d.span.end - d.span.start,
-    messageText: diagnosticMessageToDiagnosticMessageText(d.message),
-    category: ts.DiagnosticCategory.Error,
-    code: 0,
-    source: 'ng'
-  };
-}
-
 export function create(info: tss.server.PluginCreateInfo): tss.LanguageService {
   const {project, languageService: tsLS, languageServiceHost: tsLSHost, config} = info;
   // This plugin could operate under two different modes:
@@ -115,16 +85,10 @@ export function create(info: tss.server.PluginCreateInfo): tss.LanguageService {
   function getSemanticDiagnostics(fileName: string): tss.Diagnostic[] {
     const results: tss.Diagnostic[] = [];
     if (!angularOnly) {
-      const tsResults = tsLS.getSemanticDiagnostics(fileName);
-      results.push(...tsResults);
+      results.push(...tsLS.getSemanticDiagnostics(fileName));
     }
     // For semantic diagnostics we need to combine both TS + Angular results
-    const ngResults = ngLS.getDiagnostics(fileName);
-    if (!ngResults.length) {
-      return results;
-    }
-    const sourceFile = fileName.endsWith('.ts') ? ngLSHost.getSourceFile(fileName) : undefined;
-    results.push(...ngResults.map(d => diagnosticToDiagnostic(d, sourceFile)));
+    results.push(...ngLS.getDiagnostics(fileName));
     return results;
   }
 

packages/language-service/src/types.ts

@@ -190,7 +190,7 @@ export interface LanguageServiceHost {
    * Return the template source information for all templates in `fileName` or for `fileName` if
    * it is a template file.
    */
-  getTemplates(fileName: string): TemplateSources;
+  getTemplates(fileName: string): TemplateSource[];
 
   /**
    * Returns the Angular declarations in the given file.
@@ -386,7 +386,7 @@ export interface LanguageService {
   /**
    * Returns a list of all error for all templates in the given file.
    */
-  getDiagnostics(fileName: string): Diagnostic[];
+  getDiagnostics(fileName: string): tss.Diagnostic[];
 
   /**
    * Return the completions at the given position.

packages/language-service/src/typescript_host.ts

@@ -166,16 +166,16 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
     return analyzedModules;
   }
 
-  getTemplates(fileName: string): TemplateSources {
+  getTemplates(fileName: string): TemplateSource[] {
+    const results: TemplateSource[] = [];
     if (fileName.endsWith('.ts')) {
       let version = this.host.getScriptVersion(fileName);
-      let result: TemplateSource[] = [];
 
       // Find each template string in the file
       let visit = (child: ts.Node) => {
         let templateSource = this.getSourceFromNode(fileName, version, child);
         if (templateSource) {
-          result.push(templateSource);
+          results.push(templateSource);
         } else {
           ts.forEachChild(child, visit);
         }
@@ -185,17 +185,17 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
       if (sourceFile) {
         ts.forEachChild(sourceFile, visit);
       }
-      return result.length ? result : undefined;
     } else {
       this.ensureTemplateMap();
       const componentSymbol = this.fileToComponent.get(fileName);
       if (componentSymbol) {
         const templateSource = this.getTemplateAt(fileName, 0);
         if (templateSource) {
-          return [templateSource];
+          results.push(templateSource);
         }
       }
     }
+    return results;
   }
 
   getDeclarations(fileName: string): Declarations {

packages/language-service/test/diagnostics_spec.ts

@@ -34,7 +34,7 @@ describe('diagnostics', () => {
   describe('for semantic errors', () => {
     const fileName = '/app/test.ng';
 
-    function diagnostics(template: string): Diagnostics {
+    function diagnostics(template: string): ts.Diagnostic[] {
       try {
         mockHost.override(fileName, template);
         return ngService.getDiagnostics(fileName) !;
@@ -185,7 +185,7 @@ describe('diagnostics', () => {
       addCode(code, fileName => {
         const diagnostic = findDiagnostic(ngService.getDiagnostics(fileName) !, 'pipe') !;
         expect(diagnostic).not.toBeUndefined();
-        expect(diagnostic.span.end - diagnostic.span.start).toBeLessThan(11);
+        expect(diagnostic.length).toBeLessThan(11);
       });
     });
 
@@ -379,13 +379,13 @@ describe('diagnostics', () => {
       }
     }
 
-    function expectOnlyModuleDiagnostics(diagnostics: Diagnostics | undefined) {
+    function expectOnlyModuleDiagnostics(diagnostics: ts.Diagnostic[] | undefined) {
       // Expect only the 'MyComponent' diagnostic
       if (!diagnostics) throw new Error('Expecting Diagnostics');
       if (diagnostics.length > 1) {
         const unexpectedDiagnostics =
-            diagnostics.filter(diag => !diagnosticMessageContains(diag.message, 'MyComponent'))
-                .map(diag => `(${diag.span.start}:${diag.span.end}): ${diag.message}`);
+            diagnostics.filter(diag => !diagnosticMessageContains(diag.messageText, 'MyComponent'))
+                .map(diag => `(${diag.start}:${diag.start! + diag.length!}): ${diag.messageText}`);
 
         if (unexpectedDiagnostics.length) {
           fail(`Unexpected diagnostics:\n  ${unexpectedDiagnostics.join('\n  ')}`);
@@ -393,7 +393,7 @@ describe('diagnostics', () => {
         }
       }
       expect(diagnostics.length).toBe(1);
-      expect(diagnosticMessageContains(diagnostics[0].message, 'MyComponent')).toBeTruthy();
+      expect(diagnosticMessageContains(diagnostics[0].messageText, 'MyComponent')).toBeTruthy();
     }
   });
 });

packages/language-service/test/test_utils.ts

@@ -331,18 +331,19 @@ function removeReferenceMarkers(value: string): string {
   return value.replace(referenceMarker, (match, text) => text.replace(/ᐱ/g, ''));
 }
 
-export function noDiagnostics(diagnostics: Diagnostics) {
+export function noDiagnostics(diagnostics: ts.Diagnostic[]) {
   if (diagnostics && diagnostics.length) {
-    throw new Error(`Unexpected diagnostics: \n  ${diagnostics.map(d => d.message).join('\n  ')}`);
+    throw new Error(
+        `Unexpected diagnostics: \n  ${diagnostics.map(d => d.messageText).join('\n  ')}`);
   }
 }
 
 export function diagnosticMessageContains(
-    message: string | DiagnosticMessageChain, messageFragment: string): boolean {
+    message: string | ts.DiagnosticMessageChain, messageFragment: string): boolean {
   if (typeof message == 'string') {
     return message.indexOf(messageFragment) >= 0;
   }
-  if (message.message.indexOf(messageFragment) >= 0) {
+  if (message.messageText.indexOf(messageFragment) >= 0) {
     return true;
   }
   if (message.next) {
@@ -351,16 +352,17 @@ export function diagnosticMessageContains(
   return false;
 }
 
-export function findDiagnostic(diagnostics: Diagnostic[], messageFragment: string): Diagnostic|
-    undefined {
-  return diagnostics.find(d => diagnosticMessageContains(d.message, messageFragment));
+export function findDiagnostic(
+    diagnostics: ts.Diagnostic[], messageFragment: string): ts.Diagnostic|undefined {
+  return diagnostics.find(d => diagnosticMessageContains(d.messageText, messageFragment));
 }
 
 export function includeDiagnostic(
-    diagnostics: Diagnostics, message: string, text?: string, len?: string): void;
+    diagnostics: ts.Diagnostic[], message: string, text?: string, len?: string): void;
 export function includeDiagnostic(
-    diagnostics: Diagnostics, message: string, at?: number, len?: number): void;
-export function includeDiagnostic(diagnostics: Diagnostics, message: string, p1?: any, p2?: any) {
+    diagnostics: ts.Diagnostic[], message: string, at?: number, len?: number): void;
+export function includeDiagnostic(
+    diagnostics: ts.Diagnostic[], message: string, p1?: any, p2?: any) {
   expect(diagnostics).toBeDefined();
   if (diagnostics) {
     const diagnostic = findDiagnostic(diagnostics, message);
@@ -368,13 +370,12 @@ export function includeDiagnostic(diagnostics: Diagnostics, message: string, p1?
     if (diagnostic && p1 != null) {
       const at = typeof p1 === 'number' ? p1 : p2.indexOf(p1);
       const len = typeof p2 === 'number' ? p2 : p1.length;
-      expect(diagnostic.span.start)
+      expect(diagnostic.start)
           .toEqual(
               at,
-              `expected message '${message}' was reported at ${diagnostic.span.start} but should be ${at}`);
+              `expected message '${message}' was reported at ${diagnostic.start} but should be ${at}`);
       if (len != null) {
-        expect(diagnostic.span.end - diagnostic.span.start)
-            .toEqual(len, `expected '${message}'s span length to be ${len}`);
+        expect(diagnostic.length).toEqual(len, `expected '${message}'s span length to be ${len}`);
       }
     }
   }