From a5f39aeda6d4ea4c613abdf742d59a069df5ec39 Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Fri, 9 Aug 2019 15:52:49 -0700 Subject: [PATCH] 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 | 70 +++++++++++++++++++ .../language-service/src/language_service.ts | 61 +++++----------- packages/language-service/src/ts_plugin.ts | 42 +---------- packages/language-service/src/types.ts | 4 +- .../language-service/src/typescript_host.ts | 10 +-- .../language-service/test/diagnostics_spec.ts | 12 ++-- packages/language-service/test/test_utils.ts | 29 ++++---- 7 files changed, 117 insertions(+), 111 deletions(-) diff --git a/packages/language-service/src/diagnostics.ts b/packages/language-service/src/diagnostics.ts index b3ba76546f..ecc60989a6 100644 --- a/packages/language-service/src/diagnostics.ts +++ b/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(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(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', + }; +} diff --git a/packages/language-service/src/language_service.ts b/packages/language-service/src/language_service.ts index 13866d7df4..fc86774081 100644 --- a/packages/language-service/src/language_service.ts +++ b/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( - 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( - e => ({kind: e.kind, span: e.span || template.span, message: e.message}))); - } - } - } - return results; - } } function uniqueBySpan(elements: T[]): T[] { diff --git a/packages/language-service/src/ts_plugin.ts b/packages/language-service/src/ts_plugin.ts index 3622dd9599..f828508bf7 100644 --- a/packages/language-service/src/ts_plugin.ts +++ b/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(); @@ -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; } diff --git a/packages/language-service/src/types.ts b/packages/language-service/src/types.ts index 0132f65f10..a0dcb3f330 100644 --- a/packages/language-service/src/types.ts +++ b/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. diff --git a/packages/language-service/src/typescript_host.ts b/packages/language-service/src/typescript_host.ts index 656aa46a37..f444a06798 100644 --- a/packages/language-service/src/typescript_host.ts +++ b/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 { diff --git a/packages/language-service/test/diagnostics_spec.ts b/packages/language-service/test/diagnostics_spec.ts index 5d5178b45e..f1360cd890 100644 --- a/packages/language-service/test/diagnostics_spec.ts +++ b/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(); } }); }); diff --git a/packages/language-service/test/test_utils.ts b/packages/language-service/test/test_utils.ts index ea8f67824e..80dcddadb9 100644 --- a/packages/language-service/test/test_utils.ts +++ b/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}`); } } }