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
This commit is contained in:
Alex Rickabaugh 2020-07-14 13:20:26 -07:00 committed by Misko Hevery
parent 16c7441c2f
commit c573d91285
12 changed files with 200 additions and 11 deletions

View File

@ -32,5 +32,7 @@ export declare enum ErrorCode {
MISSING_PIPE = 8004, MISSING_PIPE = 8004,
WRITE_TO_READ_ONLY_VARIABLE = 8005, WRITE_TO_READ_ONLY_VARIABLE = 8005,
DUPLICATE_VARIABLE_DECLARATION = 8006, DUPLICATE_VARIABLE_DECLARATION = 8006,
INLINE_TCB_REQUIRED = 8900,
INLINE_TYPE_CTOR_REQUIRED = 8901,
INJECTABLE_DUPLICATE_PROV = 9001 INJECTABLE_DUPLICATE_PROV = 9001
} }

View File

@ -138,6 +138,18 @@ export enum ErrorCode {
*/ */
DUPLICATE_VARIABLE_DECLARATION = 8006, 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. * An injectable already has a `ɵprov` property.
*/ */

View File

@ -304,6 +304,12 @@ export interface ComponentToShimMappingStrategy {
* implement efficient template type-checking using common infrastructure. * implement efficient template type-checking using common infrastructure.
*/ */
export interface TypeCheckingProgramStrategy extends ComponentToShimMappingStrategy { 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. * Retrieve the latest version of the program, containing all the updates made thus far.
*/ */

View File

@ -34,6 +34,8 @@ export class ReusedProgramStrategy implements TypeCheckingProgramStrategy {
private originalProgram: ts.Program, private originalHost: ts.CompilerHost, private originalProgram: ts.Program, private originalHost: ts.CompilerHost,
private options: ts.CompilerOptions, private shimExtensionPrefixes: string[]) {} private options: ts.CompilerOptions, private shimExtensionPrefixes: string[]) {}
readonly supportsInlineOperations = true;
getProgram(): ts.Program { getProgram(): ts.Program {
return this.program; return this.program;
} }

View File

@ -16,7 +16,7 @@ import {isShim} from '../../shims';
import {getSourceFileOrNull} from '../../util/src/typescript'; import {getSourceFileOrNull} from '../../util/src/typescript';
import {ProgramTypeCheckAdapter, TemplateTypeChecker, TypeCheckingConfig, TypeCheckingProgramStrategy, UpdateMode} from '../api'; 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 {findTypeCheckBlock, shouldReportDiagnostic, TemplateSourceResolver, translateDiagnostic} from './diagnostics';
import {TemplateSourceManager} from './source'; import {TemplateSourceManager} from './source';
@ -145,9 +145,11 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
} }
private newContext(host: TypeCheckingHost): TypeCheckContextImpl { private newContext(host: TypeCheckingHost): TypeCheckContextImpl {
const inlining = this.typeCheckingStrategy.supportsInlineOperations ? InliningMode.InlineOps :
InliningMode.Error;
return new TypeCheckContextImpl( return new TypeCheckContextImpl(
this.config, this.compilerHost, this.typeCheckingStrategy, this.refEmitter, this.reflector, this.config, this.compilerHost, this.typeCheckingStrategy, this.refEmitter, this.reflector,
host); host, inlining);
} }
private updateFromContext(ctx: TypeCheckContextImpl): void { private updateFromContext(ctx: TypeCheckContextImpl): void {

View File

@ -114,6 +114,20 @@ export interface TypeCheckingHost {
recordComplete(sfPath: AbsoluteFsPath): void; 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. * A template type checking context for a program.
@ -129,7 +143,7 @@ export class TypeCheckContextImpl implements TypeCheckContext {
private compilerHost: Pick<ts.CompilerHost, 'getCanonicalFileName'>, private compilerHost: Pick<ts.CompilerHost, 'getCanonicalFileName'>,
private componentMappingStrategy: ComponentToShimMappingStrategy, private componentMappingStrategy: ComponentToShimMappingStrategy,
private refEmitter: ReferenceEmitter, private reflector: ReflectionHost, 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 * 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; 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 fileData = this.dataForFile(ref.node.getSourceFile());
const shimData = this.pendingShimForComponent(ref.node); const shimData = this.pendingShimForComponent(ref.node);
const boundTarget = binder.bind({template}); const boundTarget = binder.bind({template});
@ -169,6 +187,11 @@ export class TypeCheckContextImpl implements TypeCheckContext {
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>; const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
const dirNode = dirRef.node; const dirNode = dirRef.node;
if (requiresInlineTypeCtor(dirNode, this.reflector)) { if (requiresInlineTypeCtor(dirNode, this.reflector)) {
if (this.inlining === InliningMode.Error) {
missingInlines.push(dirNode);
continue;
}
// Add a type constructor operation for the directive. // Add a type constructor operation for the directive.
this.addInlineTypeCtor(fileData, dirNode.getSourceFile(), dirRef, { this.addInlineTypeCtor(fileData, dirNode.getSourceFile(), dirRef, {
fnName: 'ngTypeCtor', 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 = { const meta = {
id: fileData.sourceManager.captureSource(ref.node, sourceMapping, file), id: fileData.sourceManager.captureSource(ref.node, sourceMapping, file),
boundTarget, boundTarget,
pipes, pipes,
schemas, schemas,
}; };
if (requiresInlineTypeCheckBlock(ref.node)) { if (tcbRequiresInline) {
// This class didn't meet the requirements for external type checking, so generate an inline // This class didn't meet the requirements for external type checking, so generate an inline
// TCB for the class. // TCB for the class.
this.addInlineTypeCheckBlock(fileData, shimData, ref, meta); this.addInlineTypeCheckBlock(fileData, shimData, ref, meta);

View File

@ -9,7 +9,8 @@
import {BindingPipe, PropertyWrite, TmplAstReference, TmplAstVariable} from '@angular/compiler'; import {BindingPipe, PropertyWrite, TmplAstReference, TmplAstVariable} from '@angular/compiler';
import * as ts from 'typescript'; 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 {TemplateId} from '../api';
import {makeTemplateDiagnostic, TemplateSourceResolver} from './diagnostics'; import {makeTemplateDiagnostic, TemplateSourceResolver} from './diagnostics';
@ -61,6 +62,10 @@ export interface OutOfBandDiagnosticRecorder {
*/ */
duplicateTemplateVar( duplicateTemplateVar(
templateId: TemplateId, variable: TmplAstVariable, firstDecl: TmplAstVariable): void; templateId: TemplateId, variable: TmplAstVariable, firstDecl: TmplAstVariable): void;
requiresInlineTcb(node: ClassDeclaration): void;
requiresInlineTypeConstructors(node: ClassDeclaration, directives: ClassDeclaration[]): void;
} }
export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder { export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
@ -133,4 +138,26 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
span: firstDecl.sourceSpan, 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.`))));
}
} }

View File

@ -11,6 +11,7 @@ ts_library(
deps = [ deps = [
"//packages:types", "//packages:types",
"//packages/compiler", "//packages/compiler",
"//packages/compiler-cli/src/ngtsc/diagnostics",
"//packages/compiler-cli/src/ngtsc/file_system", "//packages/compiler-cli/src/ngtsc/file_system",
"//packages/compiler-cli/src/ngtsc/file_system/testing", "//packages/compiler-cli/src/ngtsc/file_system/testing",
"//packages/compiler-cli/src/ngtsc/imports", "//packages/compiler-cli/src/ngtsc/imports",

View File

@ -17,7 +17,6 @@ import {ClassDeclaration, isNamedClassDeclaration, TypeScriptReflectionHost} fro
import {makeProgram} from '../../testing'; import {makeProgram} from '../../testing';
import {getRootDirs} from '../../util/src/typescript'; import {getRootDirs} from '../../util/src/typescript';
import {ProgramTypeCheckAdapter, TemplateTypeChecker, TypeCheckContext} from '../api'; import {ProgramTypeCheckAdapter, TemplateTypeChecker, TypeCheckContext} from '../api';
import {TemplateId, TemplateSourceMapping, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata, TypeCheckingConfig, UpdateMode} from '../api/api'; import {TemplateId, TemplateSourceMapping, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata, TypeCheckingConfig, UpdateMode} from '../api/api';
import {ReusedProgramStrategy} from '../src/augmented_program'; import {ReusedProgramStrategy} from '../src/augmented_program';
import {TemplateTypeCheckerImpl} from '../src/checker'; import {TemplateTypeCheckerImpl} from '../src/checker';
@ -278,6 +277,7 @@ export interface TypeCheckingTarget {
export function setup(targets: TypeCheckingTarget[], overrides: { export function setup(targets: TypeCheckingTarget[], overrides: {
config?: Partial<TypeCheckingConfig>, config?: Partial<TypeCheckingConfig>,
options?: ts.CompilerOptions, options?: ts.CompilerOptions,
inlining?: boolean,
} = {}): { } = {}): {
templateTypeChecker: TemplateTypeChecker, templateTypeChecker: TemplateTypeChecker,
program: ts.Program, program: ts.Program,
@ -378,6 +378,10 @@ export function setup(targets: TypeCheckingTarget[], overrides: {
}); });
const programStrategy = new ReusedProgramStrategy(program, host, options, ['ngtypecheck']); const programStrategy = new ReusedProgramStrategy(program, host, options, ['ngtypecheck']);
if (overrides.inlining !== undefined) {
(programStrategy as any).supportsInlineOperations = overrides.inlining;
}
const templateTypeChecker = new TemplateTypeCheckerImpl( const templateTypeChecker = new TemplateTypeCheckerImpl(
program, programStrategy, checkAdapter, fullConfig, emitter, reflectionHost, host, program, programStrategy, checkAdapter, fullConfig, emitter, reflectionHost, host,
NOOP_INCREMENTAL_BUILD); NOOP_INCREMENTAL_BUILD);
@ -501,4 +505,6 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
missingPipe(): void {} missingPipe(): void {}
illegalAssignmentToTemplateVar(): void {} illegalAssignmentToTemplateVar(): void {}
duplicateTemplateVar(): void {} duplicateTemplateVar(): void {}
requiresInlineTcb(): void {}
requiresInlineTypeConstructors(): void {}
} }

View File

@ -6,7 +6,8 @@
* found in the LICENSE file at https://angular.io/license * 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 {runInEachFileSystem} from '../../file_system/testing';
import {getClass, setup} from './test_utils'; import {getClass, setup} from './test_utils';
@ -43,5 +44,90 @@ runInEachFileSystem(() => {
expect(block!.getText()).toMatch(/: i[0-9]\.Cmp1/); expect(block!.getText()).toMatch(/: i[0-9]\.Cmp1/);
expect(block!.getText()).toContain(`document.createElement("div")`); 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': '<div dir></div>'},
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': '<div></div>'}
}],
{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': '<div dir></div>'},
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<T extends NotExported> {}`,
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);
});
});
}); });
}); });

View File

@ -15,7 +15,7 @@ import {getDeclaration, makeProgram} from '../../testing';
import {getRootDirs} from '../../util/src/typescript'; import {getRootDirs} from '../../util/src/typescript';
import {ComponentToShimMappingStrategy, UpdateMode} from '../api'; import {ComponentToShimMappingStrategy, UpdateMode} from '../api';
import {ReusedProgramStrategy} from '../src/augmented_program'; 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 {TemplateSourceManager} from '../src/source';
import {TypeCheckFile} from '../src/type_check_file'; import {TypeCheckFile} from '../src/type_check_file';
@ -74,7 +74,7 @@ TestClass.ngTypeCtor({value: 'test'});
]); ]);
const ctx = new TypeCheckContextImpl( const ctx = new TypeCheckContextImpl(
ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost, ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost,
new TestTypeCheckingHost()); new TestTypeCheckingHost(), InliningMode.InlineOps);
const TestClass = const TestClass =
getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration); getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration);
const pendingFile = makePendingFile(); const pendingFile = makePendingFile();
@ -113,7 +113,7 @@ TestClass.ngTypeCtor({value: 'test'});
const pendingFile = makePendingFile(); const pendingFile = makePendingFile();
const ctx = new TypeCheckContextImpl( const ctx = new TypeCheckContextImpl(
ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost, ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost,
new TestTypeCheckingHost()); new TestTypeCheckingHost(), InliningMode.InlineOps);
const TestClass = const TestClass =
getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration); getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration);
ctx.addInlineTypeCtor( ctx.addInlineTypeCtor(
@ -158,7 +158,7 @@ TestClass.ngTypeCtor({value: 'test'});
const pendingFile = makePendingFile(); const pendingFile = makePendingFile();
const ctx = new TypeCheckContextImpl( const ctx = new TypeCheckContextImpl(
ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost, ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost,
new TestTypeCheckingHost()); new TestTypeCheckingHost(), InliningMode.InlineOps);
const TestClass = const TestClass =
getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration); getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration);
ctx.addInlineTypeCtor( ctx.addInlineTypeCtor(

View File

@ -76,6 +76,7 @@ export class Compiler {
function createTypeCheckingProgramStrategy(project: ts.server.Project): function createTypeCheckingProgramStrategy(project: ts.server.Project):
TypeCheckingProgramStrategy { TypeCheckingProgramStrategy {
return { return {
supportsInlineOperations: false,
shimPathForComponent(component: ts.ClassDeclaration): AbsoluteFsPath { shimPathForComponent(component: ts.ClassDeclaration): AbsoluteFsPath {
return TypeCheckShimGenerator.shimFor(absoluteFromSourceFile(component.getSourceFile())); return TypeCheckShimGenerator.shimFor(absoluteFromSourceFile(component.getSourceFile()));
}, },