From 63624a2d4630f5c724841aad649a7d4db13526e2 Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Wed, 30 Sep 2020 10:59:38 -0700 Subject: [PATCH] feat(language-service): [Ivy] getSemanticDiagnostics for external templates (#39065) This PR enables `getSemanticDiagnostics()` to be called on external templates. Several changes are needed to land this feature: 1. The adapter needs to implement two additional methods: a. `readResource()` Load the template from snapshot instead of reading from disk b. `getModifiedResourceFiles()` Inform the compiler that external templates have changed so that the loader could invalidate its internal cache. 2. Create `ScriptInfo` for external templates in MockHost. Prior to this, MockHost only track changes in TypeScript files. Now it needs to create `ScriptInfo` for external templates as well. For (1), in order to make sure we don't reload the template if it hasn't changed, we need to keep track of its version. Since the complexity has increased, the adapter is refactored into its own class. PR Close #39065 --- .../language-service/ivy/language_service.ts | 73 +++++------- .../ivy/language_service_adapter.ts | 111 ++++++++++++++++++ .../ivy/test/diagnostic_spec.ts | 35 +++++- .../ivy/test/language_service_adapter_spec.ts | 49 ++++++++ .../language-service/ivy/test/mock_host.ts | 19 ++- 5 files changed, 241 insertions(+), 46 deletions(-) create mode 100644 packages/language-service/ivy/language_service_adapter.ts create mode 100644 packages/language-service/ivy/test/language_service_adapter_spec.ts diff --git a/packages/language-service/ivy/language_service.ts b/packages/language-service/ivy/language_service.ts index ba7ceb7c2da63..54efb64ece453 100644 --- a/packages/language-service/ivy/language_service.ts +++ b/packages/language-service/ivy/language_service.ts @@ -8,60 +8,74 @@ import {CompilerOptions, createNgCompilerOptions} from '@angular/compiler-cli'; import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core'; -import {NgCompilerAdapter} from '@angular/compiler-cli/src/ngtsc/core/api'; -import {absoluteFrom, absoluteFromSourceFile, AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system'; +import {absoluteFromSourceFile, AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system'; import {PatchedProgramIncrementalBuildStrategy} from '@angular/compiler-cli/src/ngtsc/incremental'; -import {isShim} from '@angular/compiler-cli/src/ngtsc/shims'; import {TypeCheckShimGenerator} from '@angular/compiler-cli/src/ngtsc/typecheck'; import {OptimizeFor, TypeCheckingProgramStrategy} from '@angular/compiler-cli/src/ngtsc/typecheck/api'; import * as ts from 'typescript/lib/tsserverlibrary'; import {DefinitionBuilder} from './definitions'; +import {isExternalTemplate, isTypeScriptFile, LanguageServiceAdapter} from './language_service_adapter'; import {QuickInfoBuilder} from './quick_info'; export class LanguageService { private options: CompilerOptions; private lastKnownProgram: ts.Program|null = null; private readonly strategy: TypeCheckingProgramStrategy; - private readonly adapter: NgCompilerAdapter; + private readonly adapter: LanguageServiceAdapter; constructor(project: ts.server.Project, private readonly tsLS: ts.LanguageService) { this.options = parseNgCompilerOptions(project); this.strategy = createTypeCheckingProgramStrategy(project); - this.adapter = createNgCompilerAdapter(project); + this.adapter = new LanguageServiceAdapter(project); this.watchConfigFile(project); } getSemanticDiagnostics(fileName: string): ts.Diagnostic[] { const program = this.strategy.getProgram(); - const compiler = this.createCompiler(program); - if (fileName.endsWith('.ts')) { + const compiler = this.createCompiler(program, fileName); + const ttc = compiler.getTemplateTypeChecker(); + const diagnostics: ts.Diagnostic[] = []; + if (isTypeScriptFile(fileName)) { const sourceFile = program.getSourceFile(fileName); - if (!sourceFile) { - return []; + if (sourceFile) { + diagnostics.push(...ttc.getDiagnosticsForFile(sourceFile, OptimizeFor.SingleFile)); + } + } else { + const components = compiler.getComponentsWithTemplateFile(fileName); + for (const component of components) { + if (ts.isClassDeclaration(component)) { + diagnostics.push(...ttc.getDiagnosticsForComponent(component)); + } } - const ttc = compiler.getTemplateTypeChecker(); - const diagnostics = ttc.getDiagnosticsForFile(sourceFile, OptimizeFor.SingleFile); - this.lastKnownProgram = compiler.getNextProgram(); - return diagnostics; } - throw new Error('Ivy LS currently does not support external template'); + this.lastKnownProgram = compiler.getNextProgram(); + return diagnostics; } getDefinitionAndBoundSpan(fileName: string, position: number): ts.DefinitionInfoAndBoundSpan |undefined { const program = this.strategy.getProgram(); - const compiler = this.createCompiler(program); + const compiler = this.createCompiler(program, fileName); return new DefinitionBuilder(this.tsLS, compiler).getDefinitionAndBoundSpan(fileName, position); } getQuickInfoAtPosition(fileName: string, position: number): ts.QuickInfo|undefined { const program = this.strategy.getProgram(); - const compiler = this.createCompiler(program); + const compiler = this.createCompiler(program, fileName); return new QuickInfoBuilder(this.tsLS, compiler).get(fileName, position); } - private createCompiler(program: ts.Program): NgCompiler { + /** + * Create a new instance of Ivy compiler. + * If the specified `fileName` refers to an external template, check if it has + * changed since the last time it was read. If it has changed, signal the + * compiler to reload the file via the adapter. + */ + private createCompiler(program: ts.Program, fileName: string): NgCompiler { + if (isExternalTemplate(fileName)) { + this.adapter.registerTemplateUpdate(fileName); + } return new NgCompiler( this.adapter, this.options, @@ -107,31 +121,6 @@ export function parseNgCompilerOptions(project: ts.server.Project): CompilerOpti return createNgCompilerOptions(basePath, config, project.getCompilationSettings()); } -function createNgCompilerAdapter(project: ts.server.Project): NgCompilerAdapter { - return { - entryPoint: null, // entry point is only needed if code is emitted - constructionDiagnostics: [], - ignoreForEmit: new Set(), - factoryTracker: null, // no .ngfactory shims - unifiedModulesHost: null, // only used in Bazel - rootDirs: project.getCompilationSettings().rootDirs?.map(absoluteFrom) || [], - isShim, - fileExists(fileName: string): boolean { - return project.fileExists(fileName); - }, - readFile(fileName: string): string | - undefined { - return project.readFile(fileName); - }, - getCurrentDirectory(): string { - return project.getCurrentDirectory(); - }, - getCanonicalFileName(fileName: string): string { - return project.projectService.toCanonicalFileName(fileName); - }, - }; -} - function createTypeCheckingProgramStrategy(project: ts.server.Project): TypeCheckingProgramStrategy { return { diff --git a/packages/language-service/ivy/language_service_adapter.ts b/packages/language-service/ivy/language_service_adapter.ts new file mode 100644 index 0000000000000..2b3eb77bd44e9 --- /dev/null +++ b/packages/language-service/ivy/language_service_adapter.ts @@ -0,0 +1,111 @@ +/** + * @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 {NgCompilerAdapter} from '@angular/compiler-cli/src/ngtsc/core/api'; +import {absoluteFrom, AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system'; +import {isShim} from '@angular/compiler-cli/src/ngtsc/shims'; +import * as ts from 'typescript/lib/tsserverlibrary'; + +export class LanguageServiceAdapter implements NgCompilerAdapter { + readonly entryPoint = null; + readonly constructionDiagnostics: ts.Diagnostic[] = []; + readonly ignoreForEmit: Set = new Set(); + readonly factoryTracker = null; // no .ngfactory shims + readonly unifiedModulesHost = null; // only used in Bazel + readonly rootDirs: AbsoluteFsPath[]; + private readonly templateVersion = new Map(); + private readonly modifiedTemplates = new Set(); + + constructor(private readonly project: ts.server.Project) { + this.rootDirs = project.getCompilationSettings().rootDirs?.map(absoluteFrom) || []; + } + + isShim(sf: ts.SourceFile): boolean { + return isShim(sf); + } + + fileExists(fileName: string): boolean { + return this.project.fileExists(fileName); + } + + readFile(fileName: string): string|undefined { + return this.project.readFile(fileName); + } + + getCurrentDirectory(): string { + return this.project.getCurrentDirectory(); + } + + getCanonicalFileName(fileName: string): string { + return this.project.projectService.toCanonicalFileName(fileName); + } + + /** + * readResource() is an Angular-specific method for reading files that are not + * managed by the TS compiler host, namely templates and stylesheets. + * It is a method on ExtendedTsCompilerHost, see + * packages/compiler-cli/src/ngtsc/core/api/src/interfaces.ts + */ + readResource(fileName: string): string { + if (isTypeScriptFile(fileName)) { + throw new Error(`readResource() should not be called on TS file: ${fileName}`); + } + // Calling getScriptSnapshot() will actually create a ScriptInfo if it does + // not exist! The same applies for getScriptVersion(). + // getScriptInfo() will not create one if it does not exist. + // In this case, we *want* a script info to be created so that we could + // keep track of its version. + const snapshot = this.project.getScriptSnapshot(fileName); + if (!snapshot) { + // This would fail if the file does not exist, or readFile() fails for + // whatever reasons. + throw new Error(`Failed to get script snapshot while trying to read ${fileName}`); + } + const version = this.project.getScriptVersion(fileName); + this.templateVersion.set(fileName, version); + this.modifiedTemplates.delete(fileName); + return snapshot.getText(0, snapshot.getLength()); + } + + /** + * getModifiedResourceFiles() is an Angular-specific method for notifying + * the Angular compiler templates that have changed since it last read them. + * It is a method on ExtendedTsCompilerHost, see + * packages/compiler-cli/src/ngtsc/core/api/src/interfaces.ts + */ + getModifiedResourceFiles(): Set { + return this.modifiedTemplates; + } + + /** + * Check whether the specified `fileName` is newer than the last time it was + * read. If it is newer, register it and return true, otherwise do nothing and + * return false. + * @param fileName path to external template + */ + registerTemplateUpdate(fileName: string): boolean { + if (!isExternalTemplate(fileName)) { + return false; + } + const lastVersion = this.templateVersion.get(fileName); + const latestVersion = this.project.getScriptVersion(fileName); + if (lastVersion !== latestVersion) { + this.modifiedTemplates.add(fileName); + return true; + } + return false; + } +} + +export function isTypeScriptFile(fileName: string): boolean { + return fileName.endsWith('.ts'); +} + +export function isExternalTemplate(fileName: string): boolean { + return !isTypeScriptFile(fileName); +} diff --git a/packages/language-service/ivy/test/diagnostic_spec.ts b/packages/language-service/ivy/test/diagnostic_spec.ts index 45eeb72e2f777..20d37c8351351 100644 --- a/packages/language-service/ivy/test/diagnostic_spec.ts +++ b/packages/language-service/ivy/test/diagnostic_spec.ts @@ -10,9 +10,9 @@ import * as ts from 'typescript/lib/tsserverlibrary'; import {LanguageService} from '../language_service'; -import {APP_COMPONENT, setup} from './mock_host'; +import {APP_COMPONENT, setup, TEST_TEMPLATE} from './mock_host'; -describe('diagnostic', () => { +describe('getSemanticDiagnostics', () => { const {project, service, tsLS} = setup(); const ngLS = new LanguageService(project, tsLS); @@ -35,4 +35,35 @@ describe('diagnostic', () => { expect(text.substring(start!, start! + length!)).toBe('nope'); expect(messageText).toBe(`Property 'nope' does not exist on type 'AppComponent'.`); }); + + it('should process external template', () => { + const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); + expect(diags).toEqual([]); + }); + + it('should report member does not exist in external template', () => { + const {text} = service.overwrite(TEST_TEMPLATE, `{{ nope }}`); + const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); + expect(diags.length).toBe(1); + const {category, file, start, length, messageText} = diags[0]; + expect(category).toBe(ts.DiagnosticCategory.Error); + expect(file?.fileName).toBe(TEST_TEMPLATE); + expect(text.substring(start!, start! + length!)).toBe('nope'); + expect(messageText).toBe(`Property 'nope' does not exist on type 'TemplateReference'.`); + }); + + it('should retrieve external template from latest snapshot', () => { + // This test is to make sure we are reading from snapshot instead of disk + // if content from snapshot is newer. It also makes sure the internal cache + // of the resource loader is invalidated on content change. + service.overwrite(TEST_TEMPLATE, `{{ foo }}`); + const d1 = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); + expect(d1.length).toBe(1); + expect(d1[0].messageText).toBe(`Property 'foo' does not exist on type 'TemplateReference'.`); + + service.overwrite(TEST_TEMPLATE, `{{ bar }}`); + const d2 = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); + expect(d2.length).toBe(1); + expect(d2[0].messageText).toBe(`Property 'bar' does not exist on type 'TemplateReference'.`); + }); }); diff --git a/packages/language-service/ivy/test/language_service_adapter_spec.ts b/packages/language-service/ivy/test/language_service_adapter_spec.ts new file mode 100644 index 0000000000000..81769c1202494 --- /dev/null +++ b/packages/language-service/ivy/test/language_service_adapter_spec.ts @@ -0,0 +1,49 @@ +/** + * @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 {LanguageServiceAdapter} from '../language_service_adapter'; +import {setup, TEST_TEMPLATE} from './mock_host'; + +const {project, service} = setup(); + +describe('Language service adapter', () => { + it('should register update if it has not seen the template before', () => { + const adapter = new LanguageServiceAdapter(project); + // Note that readResource() has never been called, so the adapter has no + // knowledge of the template at all. + const isRegistered = adapter.registerTemplateUpdate(TEST_TEMPLATE); + expect(isRegistered).toBeTrue(); + expect(adapter.getModifiedResourceFiles().size).toBe(1); + }); + + it('should not register update if template has not changed', () => { + const adapter = new LanguageServiceAdapter(project); + adapter.readResource(TEST_TEMPLATE); + const isRegistered = adapter.registerTemplateUpdate(TEST_TEMPLATE); + expect(isRegistered).toBeFalse(); + expect(adapter.getModifiedResourceFiles().size).toBe(0); + }); + + it('should register update if template has changed', () => { + const adapter = new LanguageServiceAdapter(project); + adapter.readResource(TEST_TEMPLATE); + service.overwrite(TEST_TEMPLATE, '

Hello World

'); + const isRegistered = adapter.registerTemplateUpdate(TEST_TEMPLATE); + expect(isRegistered).toBe(true); + expect(adapter.getModifiedResourceFiles().size).toBe(1); + }); + + it('should clear template updates on read', () => { + const adapter = new LanguageServiceAdapter(project); + const isRegistered = adapter.registerTemplateUpdate(TEST_TEMPLATE); + expect(isRegistered).toBeTrue(); + expect(adapter.getModifiedResourceFiles().size).toBe(1); + adapter.readResource(TEST_TEMPLATE); + expect(adapter.getModifiedResourceFiles().size).toBe(0); + }); +}); diff --git a/packages/language-service/ivy/test/mock_host.ts b/packages/language-service/ivy/test/mock_host.ts index 19041952db613..134748489e07b 100644 --- a/packages/language-service/ivy/test/mock_host.ts +++ b/packages/language-service/ivy/test/mock_host.ts @@ -8,6 +8,7 @@ import {join} from 'path'; import * as ts from 'typescript/lib/tsserverlibrary'; +import {isTypeScriptFile} from '../language_service_adapter'; const logger: ts.server.Logger = { close(): void{}, @@ -161,10 +162,24 @@ class MockService { getScriptInfo(fileName: string): ts.server.ScriptInfo { const scriptInfo = this.ps.getScriptInfo(fileName); - if (!scriptInfo) { + if (scriptInfo) { + return scriptInfo; + } + // There is no script info for external template, so create one. + // But we need to make sure it's not a TS file. + if (isTypeScriptFile(fileName)) { throw new Error(`No existing script info for ${fileName}`); } - return scriptInfo; + const newScriptInfo = this.ps.getOrCreateScriptInfoForNormalizedPath( + ts.server.toNormalizedPath(fileName), + true, // openedByClient + '', // fileContent + ts.ScriptKind.External, // scriptKind + ); + if (!newScriptInfo) { + throw new Error(`Failed to create new script info for ${fileName}`); + } + return newScriptInfo; } /**