From c573d912853c4566ddb6568be35c84ebd3bdb077 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 14 Jul 2020 13:20:26 -0700 Subject: [PATCH] refactor(compiler-cli): allow program strategies to opt out of inlining (#38105) The template type-checking engine relies on the abstraction interface `TypeCheckingProgramStrategy` to create updated `ts.Program`s for template type-checking. The basic API is that the type-checking engine requests changes to certain files in the program, and the strategy provides an updated `ts.Program`. Typically, such changes are made to 'ngtypecheck' shim files, but certain conditions can cause template type-checking to require "inline" operations, which change user .ts files instead. The strategy used by 'ngc' (the `ReusedProgramStrategy`) supports these kinds of updates, but other clients such as the language service might not always support modifying user files. To accommodate this, the `TypeCheckingProgramStrategy` interface was modified to include a `supportsInlineOperations` flag. If an implementation specifies `false` for inline support, the template type-checking system will return diagnostics on components which would otherwise require inline operations. Closes #38059 PR Close #38105 --- .../public-api/compiler-cli/error_code.d.ts | 2 + .../src/ngtsc/diagnostics/src/error_code.ts | 12 +++ .../src/ngtsc/typecheck/api/api.ts | 6 ++ .../ngtsc/typecheck/src/augmented_program.ts | 2 + .../src/ngtsc/typecheck/src/checker.ts | 6 +- .../src/ngtsc/typecheck/src/context.ts | 48 +++++++++- .../src/ngtsc/typecheck/src/oob.ts | 29 +++++- .../src/ngtsc/typecheck/test/BUILD.bazel | 1 + .../src/ngtsc/typecheck/test/test_utils.ts | 8 +- .../ngtsc/typecheck/test/type_checker_spec.ts | 88 ++++++++++++++++++- .../typecheck/test/type_constructor_spec.ts | 8 +- .../language-service/ivy/compiler/compiler.ts | 1 + 12 files changed, 200 insertions(+), 11 deletions(-) diff --git a/goldens/public-api/compiler-cli/error_code.d.ts b/goldens/public-api/compiler-cli/error_code.d.ts index 62a4be71ca..41ad716477 100644 --- a/goldens/public-api/compiler-cli/error_code.d.ts +++ b/goldens/public-api/compiler-cli/error_code.d.ts @@ -32,5 +32,7 @@ export declare enum ErrorCode { MISSING_PIPE = 8004, WRITE_TO_READ_ONLY_VARIABLE = 8005, DUPLICATE_VARIABLE_DECLARATION = 8006, + INLINE_TCB_REQUIRED = 8900, + INLINE_TYPE_CTOR_REQUIRED = 8901, INJECTABLE_DUPLICATE_PROV = 9001 } diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts index a67f5a8e3a..91eb812384 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -138,6 +138,18 @@ export enum ErrorCode { */ DUPLICATE_VARIABLE_DECLARATION = 8006, + /** + * The template type-checking engine would need to generate an inline type check block for a + * component, but the current type-checking environment doesn't support it. + */ + INLINE_TCB_REQUIRED = 8900, + + /** + * The template type-checking engine would need to generate an inline type constructor for a + * directive or component, but the current type-checking environment doesn't support it. + */ + INLINE_TYPE_CTOR_REQUIRED = 8901, + /** * An injectable already has a `ɵprov` property. */ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts index cc674c33c5..b7db3f0f6d 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts @@ -304,6 +304,12 @@ export interface ComponentToShimMappingStrategy { * implement efficient template type-checking using common infrastructure. */ export interface TypeCheckingProgramStrategy extends ComponentToShimMappingStrategy { + /** + * Whether this strategy supports modifying user files (inline modifications) in addition to + * modifying type-checking shims. + */ + readonly supportsInlineOperations: boolean; + /** * Retrieve the latest version of the program, containing all the updates made thus far. */ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/augmented_program.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/augmented_program.ts index b2c6f535ca..6176b36a12 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/augmented_program.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/augmented_program.ts @@ -34,6 +34,8 @@ export class ReusedProgramStrategy implements TypeCheckingProgramStrategy { private originalProgram: ts.Program, private originalHost: ts.CompilerHost, private options: ts.CompilerOptions, private shimExtensionPrefixes: string[]) {} + readonly supportsInlineOperations = true; + getProgram(): ts.Program { return this.program; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts index e4cdac6f26..53fa546853 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts @@ -16,7 +16,7 @@ import {isShim} from '../../shims'; import {getSourceFileOrNull} from '../../util/src/typescript'; import {ProgramTypeCheckAdapter, TemplateTypeChecker, TypeCheckingConfig, TypeCheckingProgramStrategy, UpdateMode} from '../api'; -import {ShimTypeCheckingData, TypeCheckContextImpl, TypeCheckingHost} from './context'; +import {InliningMode, ShimTypeCheckingData, TypeCheckContextImpl, TypeCheckingHost} from './context'; import {findTypeCheckBlock, shouldReportDiagnostic, TemplateSourceResolver, translateDiagnostic} from './diagnostics'; import {TemplateSourceManager} from './source'; @@ -145,9 +145,11 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { } private newContext(host: TypeCheckingHost): TypeCheckContextImpl { + const inlining = this.typeCheckingStrategy.supportsInlineOperations ? InliningMode.InlineOps : + InliningMode.Error; return new TypeCheckContextImpl( this.config, this.compilerHost, this.typeCheckingStrategy, this.refEmitter, this.reflector, - host); + host, inlining); } private updateFromContext(ctx: TypeCheckContextImpl): void { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index 7e92d49a4e..e5a155321d 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -114,6 +114,20 @@ export interface TypeCheckingHost { recordComplete(sfPath: AbsoluteFsPath): void; } +/** + * How a type-checking context should handle operations which would require inlining. + */ +export enum InliningMode { + /** + * Use inlining operations when required. + */ + InlineOps, + + /** + * Produce diagnostics if an operation would require inlining. + */ + Error, +} /** * A template type checking context for a program. @@ -129,7 +143,7 @@ export class TypeCheckContextImpl implements TypeCheckContext { private compilerHost: Pick, private componentMappingStrategy: ComponentToShimMappingStrategy, private refEmitter: ReferenceEmitter, private reflector: ReflectionHost, - private host: TypeCheckingHost) {} + private host: TypeCheckingHost, private inlining: InliningMode) {} /** * A `Map` of `ts.SourceFile`s that the context has seen to the operations (additions of methods @@ -161,6 +175,10 @@ export class TypeCheckContextImpl implements TypeCheckContext { return; } + // Accumulate a list of any directives which could not have type constructors generated due to + // unsupported inlining operations. + let missingInlines: ClassDeclaration[] = []; + const fileData = this.dataForFile(ref.node.getSourceFile()); const shimData = this.pendingShimForComponent(ref.node); const boundTarget = binder.bind({template}); @@ -169,6 +187,11 @@ export class TypeCheckContextImpl implements TypeCheckContext { const dirRef = dir.ref as Reference>; const dirNode = dirRef.node; if (requiresInlineTypeCtor(dirNode, this.reflector)) { + if (this.inlining === InliningMode.Error) { + missingInlines.push(dirNode); + continue; + } + // Add a type constructor operation for the directive. this.addInlineTypeCtor(fileData, dirNode.getSourceFile(), dirRef, { fnName: 'ngTypeCtor', @@ -186,13 +209,34 @@ export class TypeCheckContextImpl implements TypeCheckContext { } } + const tcbRequiresInline = requiresInlineTypeCheckBlock(ref.node); + + // If inlining is not supported, but is required for either the TCB or one of its directive + // dependencies, then exit here with an error. + if (this.inlining === InliningMode.Error && (tcbRequiresInline || missingInlines.length > 0)) { + // This template cannot be supported because the underlying strategy does not support inlining + // and inlining would be required. + + // Record diagnostics to indicate the issues with this template. + if (tcbRequiresInline) { + shimData.oobRecorder.requiresInlineTcb(ref.node); + } + + if (missingInlines.length > 0) { + shimData.oobRecorder.requiresInlineTypeConstructors(ref.node, missingInlines); + } + + // Checking this template would be unsupported, so don't try. + return; + } + const meta = { id: fileData.sourceManager.captureSource(ref.node, sourceMapping, file), boundTarget, pipes, schemas, }; - if (requiresInlineTypeCheckBlock(ref.node)) { + if (tcbRequiresInline) { // This class didn't meet the requirements for external type checking, so generate an inline // TCB for the class. this.addInlineTypeCheckBlock(fileData, shimData, ref, meta); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts index 77b0807f50..1e728be7c3 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts @@ -9,7 +9,8 @@ import {BindingPipe, PropertyWrite, TmplAstReference, TmplAstVariable} from '@angular/compiler'; import * as ts from 'typescript'; -import {ErrorCode, ngErrorCode} from '../../diagnostics'; +import {ErrorCode, makeDiagnostic, makeRelatedInformation, ngErrorCode} from '../../diagnostics'; +import {ClassDeclaration} from '../../reflection'; import {TemplateId} from '../api'; import {makeTemplateDiagnostic, TemplateSourceResolver} from './diagnostics'; @@ -61,6 +62,10 @@ export interface OutOfBandDiagnosticRecorder { */ duplicateTemplateVar( templateId: TemplateId, variable: TmplAstVariable, firstDecl: TmplAstVariable): void; + + requiresInlineTcb(node: ClassDeclaration): void; + + requiresInlineTypeConstructors(node: ClassDeclaration, directives: ClassDeclaration[]): void; } export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder { @@ -133,4 +138,26 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor span: firstDecl.sourceSpan, })); } + + requiresInlineTcb(node: ClassDeclaration): void { + this._diagnostics.push(makeDiagnostic( + ErrorCode.INLINE_TCB_REQUIRED, node.name, + `This component requires inline template type-checking, which is not supported by the current environment.`)); + } + + requiresInlineTypeConstructors(node: ClassDeclaration, directives: ClassDeclaration[]): void { + let message: string; + if (directives.length > 1) { + message = + `This component uses directives which require inline type constructors, which are not supported by the current environment.`; + } else { + message = + `This component uses a directive which requires an inline type constructor, which is not supported by the current environment.`; + } + + this._diagnostics.push(makeDiagnostic( + ErrorCode.INLINE_TYPE_CTOR_REQUIRED, node.name, message, + directives.map( + dir => makeRelatedInformation(dir.name, `Requires an inline type constructor.`)))); + } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel index bbf0bb0cf6..cf18524f63 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel @@ -11,6 +11,7 @@ ts_library( deps = [ "//packages:types", "//packages/compiler", + "//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/file_system", "//packages/compiler-cli/src/ngtsc/file_system/testing", "//packages/compiler-cli/src/ngtsc/imports", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index 7fc09368fa..a0dee2a118 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -17,7 +17,6 @@ import {ClassDeclaration, isNamedClassDeclaration, TypeScriptReflectionHost} fro import {makeProgram} from '../../testing'; import {getRootDirs} from '../../util/src/typescript'; import {ProgramTypeCheckAdapter, TemplateTypeChecker, TypeCheckContext} from '../api'; - import {TemplateId, TemplateSourceMapping, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata, TypeCheckingConfig, UpdateMode} from '../api/api'; import {ReusedProgramStrategy} from '../src/augmented_program'; import {TemplateTypeCheckerImpl} from '../src/checker'; @@ -278,6 +277,7 @@ export interface TypeCheckingTarget { export function setup(targets: TypeCheckingTarget[], overrides: { config?: Partial, options?: ts.CompilerOptions, + inlining?: boolean, } = {}): { templateTypeChecker: TemplateTypeChecker, program: ts.Program, @@ -378,6 +378,10 @@ export function setup(targets: TypeCheckingTarget[], overrides: { }); const programStrategy = new ReusedProgramStrategy(program, host, options, ['ngtypecheck']); + if (overrides.inlining !== undefined) { + (programStrategy as any).supportsInlineOperations = overrides.inlining; + } + const templateTypeChecker = new TemplateTypeCheckerImpl( program, programStrategy, checkAdapter, fullConfig, emitter, reflectionHost, host, NOOP_INCREMENTAL_BUILD); @@ -501,4 +505,6 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder { missingPipe(): void {} illegalAssignmentToTemplateVar(): void {} duplicateTemplateVar(): void {} + requiresInlineTcb(): void {} + requiresInlineTypeConstructors(): void {} } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts index 79a5a678df..2c1ee6b751 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts @@ -6,7 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ -import {absoluteFrom, getSourceFileOrError} from '../../file_system'; +import {ErrorCode, ngErrorCode} from '../../diagnostics'; +import {absoluteFrom, absoluteFromSourceFile, getSourceFileOrError} from '../../file_system'; import {runInEachFileSystem} from '../../file_system/testing'; import {getClass, setup} from './test_utils'; @@ -43,5 +44,90 @@ runInEachFileSystem(() => { expect(block!.getText()).toMatch(/: i[0-9]\.Cmp1/); expect(block!.getText()).toContain(`document.createElement("div")`); }); + + describe('when inlining is unsupported', () => { + it('should not produce errors for components that do not require inlining', () => { + const fileName = absoluteFrom('/main.ts'); + const dirFile = absoluteFrom('/dir.ts'); + const {program, templateTypeChecker} = setup( + [ + { + fileName, + source: `export class Cmp {}`, + templates: {'Cmp': '
'}, + declarations: [{ + name: 'TestDir', + selector: '[dir]', + file: dirFile, + type: 'directive', + }] + }, + { + fileName: dirFile, + source: `export class TestDir {}`, + templates: {}, + } + ], + {inlining: false}); + const sf = getSourceFileOrError(program, fileName); + const diags = templateTypeChecker.getDiagnosticsForFile(sf); + expect(diags.length).toBe(0); + }); + + it('should produce errors for components that require TCB inlining', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup( + [{ + fileName, + source: `class Cmp {} // not exported, so requires inline`, + templates: {'Cmp': '
'} + }], + {inlining: false}); + const sf = getSourceFileOrError(program, fileName); + const diags = templateTypeChecker.getDiagnosticsForFile(sf); + expect(diags.length).toBe(1); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INLINE_TCB_REQUIRED)); + }); + + it('should produce errors for components that require type constructor inlining', () => { + const fileName = absoluteFrom('/main.ts'); + const dirFile = absoluteFrom('/dir.ts'); + const {program, templateTypeChecker} = setup( + [ + { + fileName, + source: `export class Cmp {}`, + templates: {'Cmp': '
'}, + declarations: [{ + name: 'TestDir', + selector: '[dir]', + file: dirFile, + type: 'directive', + }] + }, + { + fileName: dirFile, + source: ` + // A non-exported interface used as a type bound for a generic directive causes + // an inline type constructor to be required. + interface NotExported {} + export class TestDir {}`, + templates: {}, + } + ], + {inlining: false}); + const sf = getSourceFileOrError(program, fileName); + const diags = templateTypeChecker.getDiagnosticsForFile(sf); + expect(diags.length).toBe(1); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INLINE_TYPE_CTOR_REQUIRED)); + + // The relatedInformation of the diagnostic should point to the directive which required the + // inline type constructor. + expect(diags[0].relatedInformation).not.toBeUndefined(); + expect(diags[0].relatedInformation!.length).toBe(1); + expect(diags[0].relatedInformation![0].file).not.toBeUndefined(); + expect(absoluteFromSourceFile(diags[0].relatedInformation![0].file!)).toBe(dirFile); + }); + }); }); }); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts index 57ec025f1d..54c399f1ba 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts @@ -15,7 +15,7 @@ import {getDeclaration, makeProgram} from '../../testing'; import {getRootDirs} from '../../util/src/typescript'; import {ComponentToShimMappingStrategy, UpdateMode} from '../api'; import {ReusedProgramStrategy} from '../src/augmented_program'; -import {PendingFileTypeCheckingData, TypeCheckContextImpl, TypeCheckingHost} from '../src/context'; +import {InliningMode, PendingFileTypeCheckingData, TypeCheckContextImpl, TypeCheckingHost} from '../src/context'; import {TemplateSourceManager} from '../src/source'; import {TypeCheckFile} from '../src/type_check_file'; @@ -74,7 +74,7 @@ TestClass.ngTypeCtor({value: 'test'}); ]); const ctx = new TypeCheckContextImpl( ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost, - new TestTypeCheckingHost()); + new TestTypeCheckingHost(), InliningMode.InlineOps); const TestClass = getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration); const pendingFile = makePendingFile(); @@ -113,7 +113,7 @@ TestClass.ngTypeCtor({value: 'test'}); const pendingFile = makePendingFile(); const ctx = new TypeCheckContextImpl( ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost, - new TestTypeCheckingHost()); + new TestTypeCheckingHost(), InliningMode.InlineOps); const TestClass = getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration); ctx.addInlineTypeCtor( @@ -158,7 +158,7 @@ TestClass.ngTypeCtor({value: 'test'}); const pendingFile = makePendingFile(); const ctx = new TypeCheckContextImpl( ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost, - new TestTypeCheckingHost()); + new TestTypeCheckingHost(), InliningMode.InlineOps); const TestClass = getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration); ctx.addInlineTypeCtor( diff --git a/packages/language-service/ivy/compiler/compiler.ts b/packages/language-service/ivy/compiler/compiler.ts index 91de6c4282..37f73f0efe 100644 --- a/packages/language-service/ivy/compiler/compiler.ts +++ b/packages/language-service/ivy/compiler/compiler.ts @@ -76,6 +76,7 @@ export class Compiler { function createTypeCheckingProgramStrategy(project: ts.server.Project): TypeCheckingProgramStrategy { return { + supportsInlineOperations: false, shimPathForComponent(component: ts.ClassDeclaration): AbsoluteFsPath { return TypeCheckShimGenerator.shimFor(absoluteFromSourceFile(component.getSourceFile())); },