Skip to content

Commit 63f5167

Browse files
alxhubprofanis
alxhub
authored and
profanis
committed
refactor(compiler-cli): allow program strategies to opt out of inlining (angular#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 angular#38059 PR Close angular#38105
1 parent 982dfc2 commit 63f5167

File tree

12 files changed

+200
-11
lines changed

12 files changed

+200
-11
lines changed

‎goldens/public-api/compiler-cli/error_code.d.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,5 +32,7 @@ export declare enum ErrorCode {
3232
MISSING_PIPE = 8004,
3333
WRITE_TO_READ_ONLY_VARIABLE = 8005,
3434
DUPLICATE_VARIABLE_DECLARATION = 8006,
35+
INLINE_TCB_REQUIRED = 8900,
36+
INLINE_TYPE_CTOR_REQUIRED = 8901,
3537
INJECTABLE_DUPLICATE_PROV = 9001
3638
}

‎packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,18 @@ export enum ErrorCode {
138138
*/
139139
DUPLICATE_VARIABLE_DECLARATION = 8006,
140140

141+
/**
142+
* The template type-checking engine would need to generate an inline type check block for a
143+
* component, but the current type-checking environment doesn't support it.
144+
*/
145+
INLINE_TCB_REQUIRED = 8900,
146+
147+
/**
148+
* The template type-checking engine would need to generate an inline type constructor for a
149+
* directive or component, but the current type-checking environment doesn't support it.
150+
*/
151+
INLINE_TYPE_CTOR_REQUIRED = 8901,
152+
141153
/**
142154
* An injectable already has a `ɵprov` property.
143155
*/

‎packages/compiler-cli/src/ngtsc/typecheck/api/api.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,12 @@ export interface ComponentToShimMappingStrategy {
304304
* implement efficient template type-checking using common infrastructure.
305305
*/
306306
export interface TypeCheckingProgramStrategy extends ComponentToShimMappingStrategy {
307+
/**
308+
* Whether this strategy supports modifying user files (inline modifications) in addition to
309+
* modifying type-checking shims.
310+
*/
311+
readonly supportsInlineOperations: boolean;
312+
307313
/**
308314
* Retrieve the latest version of the program, containing all the updates made thus far.
309315
*/

‎packages/compiler-cli/src/ngtsc/typecheck/src/augmented_program.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ export class ReusedProgramStrategy implements TypeCheckingProgramStrategy {
3434
private originalProgram: ts.Program, private originalHost: ts.CompilerHost,
3535
private options: ts.CompilerOptions, private shimExtensionPrefixes: string[]) {}
3636

37+
readonly supportsInlineOperations = true;
38+
3739
getProgram(): ts.Program {
3840
return this.program;
3941
}

‎packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import {isShim} from '../../shims';
1616
import {getSourceFileOrNull} from '../../util/src/typescript';
1717
import {ProgramTypeCheckAdapter, TemplateTypeChecker, TypeCheckingConfig, TypeCheckingProgramStrategy, UpdateMode} from '../api';
1818

19-
import {ShimTypeCheckingData, TypeCheckContextImpl, TypeCheckingHost} from './context';
19+
import {InliningMode, ShimTypeCheckingData, TypeCheckContextImpl, TypeCheckingHost} from './context';
2020
import {findTypeCheckBlock, shouldReportDiagnostic, TemplateSourceResolver, translateDiagnostic} from './diagnostics';
2121
import {TemplateSourceManager} from './source';
2222

@@ -145,9 +145,11 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
145145
}
146146

147147
private newContext(host: TypeCheckingHost): TypeCheckContextImpl {
148+
const inlining = this.typeCheckingStrategy.supportsInlineOperations ? InliningMode.InlineOps :
149+
InliningMode.Error;
148150
return new TypeCheckContextImpl(
149151
this.config, this.compilerHost, this.typeCheckingStrategy, this.refEmitter, this.reflector,
150-
host);
152+
host, inlining);
151153
}
152154

153155
private updateFromContext(ctx: TypeCheckContextImpl): void {

‎packages/compiler-cli/src/ngtsc/typecheck/src/context.ts

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,20 @@ export interface TypeCheckingHost {
114114
recordComplete(sfPath: AbsoluteFsPath): void;
115115
}
116116

117+
/**
118+
* How a type-checking context should handle operations which would require inlining.
119+
*/
120+
export enum InliningMode {
121+
/**
122+
* Use inlining operations when required.
123+
*/
124+
InlineOps,
125+
126+
/**
127+
* Produce diagnostics if an operation would require inlining.
128+
*/
129+
Error,
130+
}
117131

118132
/**
119133
* A template type checking context for a program.
@@ -129,7 +143,7 @@ export class TypeCheckContextImpl implements TypeCheckContext {
129143
private compilerHost: Pick<ts.CompilerHost, 'getCanonicalFileName'>,
130144
private componentMappingStrategy: ComponentToShimMappingStrategy,
131145
private refEmitter: ReferenceEmitter, private reflector: ReflectionHost,
132-
private host: TypeCheckingHost) {}
146+
private host: TypeCheckingHost, private inlining: InliningMode) {}
133147

134148
/**
135149
* 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 {
161175
return;
162176
}
163177

178+
// Accumulate a list of any directives which could not have type constructors generated due to
179+
// unsupported inlining operations.
180+
let missingInlines: ClassDeclaration[] = [];
181+
164182
const fileData = this.dataForFile(ref.node.getSourceFile());
165183
const shimData = this.pendingShimForComponent(ref.node);
166184
const boundTarget = binder.bind({template});
@@ -169,6 +187,11 @@ export class TypeCheckContextImpl implements TypeCheckContext {
169187
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
170188
const dirNode = dirRef.node;
171189
if (requiresInlineTypeCtor(dirNode, this.reflector)) {
190+
if (this.inlining === InliningMode.Error) {
191+
missingInlines.push(dirNode);
192+
continue;
193+
}
194+
172195
// Add a type constructor operation for the directive.
173196
this.addInlineTypeCtor(fileData, dirNode.getSourceFile(), dirRef, {
174197
fnName: 'ngTypeCtor',
@@ -186,13 +209,34 @@ export class TypeCheckContextImpl implements TypeCheckContext {
186209
}
187210
}
188211

212+
const tcbRequiresInline = requiresInlineTypeCheckBlock(ref.node);
213+
214+
// If inlining is not supported, but is required for either the TCB or one of its directive
215+
// dependencies, then exit here with an error.
216+
if (this.inlining === InliningMode.Error && (tcbRequiresInline || missingInlines.length > 0)) {
217+
// This template cannot be supported because the underlying strategy does not support inlining
218+
// and inlining would be required.
219+
220+
// Record diagnostics to indicate the issues with this template.
221+
if (tcbRequiresInline) {
222+
shimData.oobRecorder.requiresInlineTcb(ref.node);
223+
}
224+
225+
if (missingInlines.length > 0) {
226+
shimData.oobRecorder.requiresInlineTypeConstructors(ref.node, missingInlines);
227+
}
228+
229+
// Checking this template would be unsupported, so don't try.
230+
return;
231+
}
232+
189233
const meta = {
190234
id: fileData.sourceManager.captureSource(ref.node, sourceMapping, file),
191235
boundTarget,
192236
pipes,
193237
schemas,
194238
};
195-
if (requiresInlineTypeCheckBlock(ref.node)) {
239+
if (tcbRequiresInline) {
196240
// This class didn't meet the requirements for external type checking, so generate an inline
197241
// TCB for the class.
198242
this.addInlineTypeCheckBlock(fileData, shimData, ref, meta);

‎packages/compiler-cli/src/ngtsc/typecheck/src/oob.ts

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@
99
import {BindingPipe, PropertyWrite, TmplAstReference, TmplAstVariable} from '@angular/compiler';
1010
import * as ts from 'typescript';
1111

12-
import {ErrorCode, ngErrorCode} from '../../diagnostics';
12+
import {ErrorCode, makeDiagnostic, makeRelatedInformation, ngErrorCode} from '../../diagnostics';
13+
import {ClassDeclaration} from '../../reflection';
1314
import {TemplateId} from '../api';
1415

1516
import {makeTemplateDiagnostic, TemplateSourceResolver} from './diagnostics';
@@ -61,6 +62,10 @@ export interface OutOfBandDiagnosticRecorder {
6162
*/
6263
duplicateTemplateVar(
6364
templateId: TemplateId, variable: TmplAstVariable, firstDecl: TmplAstVariable): void;
65+
66+
requiresInlineTcb(node: ClassDeclaration): void;
67+
68+
requiresInlineTypeConstructors(node: ClassDeclaration, directives: ClassDeclaration[]): void;
6469
}
6570

6671
export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
@@ -133,4 +138,26 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
133138
span: firstDecl.sourceSpan,
134139
}));
135140
}
141+
142+
requiresInlineTcb(node: ClassDeclaration): void {
143+
this._diagnostics.push(makeDiagnostic(
144+
ErrorCode.INLINE_TCB_REQUIRED, node.name,
145+
`This component requires inline template type-checking, which is not supported by the current environment.`));
146+
}
147+
148+
requiresInlineTypeConstructors(node: ClassDeclaration, directives: ClassDeclaration[]): void {
149+
let message: string;
150+
if (directives.length > 1) {
151+
message =
152+
`This component uses directives which require inline type constructors, which are not supported by the current environment.`;
153+
} else {
154+
message =
155+
`This component uses a directive which requires an inline type constructor, which is not supported by the current environment.`;
156+
}
157+
158+
this._diagnostics.push(makeDiagnostic(
159+
ErrorCode.INLINE_TYPE_CTOR_REQUIRED, node.name, message,
160+
directives.map(
161+
dir => makeRelatedInformation(dir.name, `Requires an inline type constructor.`))));
162+
}
136163
}

‎packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ ts_library(
1111
deps = [
1212
"//packages:types",
1313
"//packages/compiler",
14+
"//packages/compiler-cli/src/ngtsc/diagnostics",
1415
"//packages/compiler-cli/src/ngtsc/file_system",
1516
"//packages/compiler-cli/src/ngtsc/file_system/testing",
1617
"//packages/compiler-cli/src/ngtsc/imports",

‎packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import {ClassDeclaration, isNamedClassDeclaration, TypeScriptReflectionHost} fro
1717
import {makeProgram} from '../../testing';
1818
import {getRootDirs} from '../../util/src/typescript';
1919
import {ProgramTypeCheckAdapter, TemplateTypeChecker, TypeCheckContext} from '../api';
20-
2120
import {TemplateId, TemplateSourceMapping, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata, TypeCheckingConfig, UpdateMode} from '../api/api';
2221
import {ReusedProgramStrategy} from '../src/augmented_program';
2322
import {TemplateTypeCheckerImpl} from '../src/checker';
@@ -278,6 +277,7 @@ export interface TypeCheckingTarget {
278277
export function setup(targets: TypeCheckingTarget[], overrides: {
279278
config?: Partial<TypeCheckingConfig>,
280279
options?: ts.CompilerOptions,
280+
inlining?: boolean,
281281
} = {}): {
282282
templateTypeChecker: TemplateTypeChecker,
283283
program: ts.Program,
@@ -378,6 +378,10 @@ export function setup(targets: TypeCheckingTarget[], overrides: {
378378
});
379379

380380
const programStrategy = new ReusedProgramStrategy(program, host, options, ['ngtypecheck']);
381+
if (overrides.inlining !== undefined) {
382+
(programStrategy as any).supportsInlineOperations = overrides.inlining;
383+
}
384+
381385
const templateTypeChecker = new TemplateTypeCheckerImpl(
382386
program, programStrategy, checkAdapter, fullConfig, emitter, reflectionHost, host,
383387
NOOP_INCREMENTAL_BUILD);
@@ -501,4 +505,6 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
501505
missingPipe(): void {}
502506
illegalAssignmentToTemplateVar(): void {}
503507
duplicateTemplateVar(): void {}
508+
requiresInlineTcb(): void {}
509+
requiresInlineTypeConstructors(): void {}
504510
}

‎packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts

Lines changed: 87 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {absoluteFrom, getSourceFileOrError} from '../../file_system';
9+
import {ErrorCode, ngErrorCode} from '../../diagnostics';
10+
import {absoluteFrom, absoluteFromSourceFile, getSourceFileOrError} from '../../file_system';
1011
import {runInEachFileSystem} from '../../file_system/testing';
1112

1213
import {getClass, setup} from './test_utils';
@@ -43,5 +44,90 @@ runInEachFileSystem(() => {
4344
expect(block!.getText()).toMatch(/: i[0-9]\.Cmp1/);
4445
expect(block!.getText()).toContain(`document.createElement("div")`);
4546
});
47+
48+
describe('when inlining is unsupported', () => {
49+
it('should not produce errors for components that do not require inlining', () => {
50+
const fileName = absoluteFrom('/main.ts');
51+
const dirFile = absoluteFrom('/dir.ts');
52+
const {program, templateTypeChecker} = setup(
53+
[
54+
{
55+
fileName,
56+
source: `export class Cmp {}`,
57+
templates: {'Cmp': '<div dir></div>'},
58+
declarations: [{
59+
name: 'TestDir',
60+
selector: '[dir]',
61+
file: dirFile,
62+
type: 'directive',
63+
}]
64+
},
65+
{
66+
fileName: dirFile,
67+
source: `export class TestDir {}`,
68+
templates: {},
69+
}
70+
],
71+
{inlining: false});
72+
const sf = getSourceFileOrError(program, fileName);
73+
const diags = templateTypeChecker.getDiagnosticsForFile(sf);
74+
expect(diags.length).toBe(0);
75+
});
76+
77+
it('should produce errors for components that require TCB inlining', () => {
78+
const fileName = absoluteFrom('/main.ts');
79+
const {program, templateTypeChecker} = setup(
80+
[{
81+
fileName,
82+
source: `class Cmp {} // not exported, so requires inline`,
83+
templates: {'Cmp': '<div></div>'}
84+
}],
85+
{inlining: false});
86+
const sf = getSourceFileOrError(program, fileName);
87+
const diags = templateTypeChecker.getDiagnosticsForFile(sf);
88+
expect(diags.length).toBe(1);
89+
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INLINE_TCB_REQUIRED));
90+
});
91+
92+
it('should produce errors for components that require type constructor inlining', () => {
93+
const fileName = absoluteFrom('/main.ts');
94+
const dirFile = absoluteFrom('/dir.ts');
95+
const {program, templateTypeChecker} = setup(
96+
[
97+
{
98+
fileName,
99+
source: `export class Cmp {}`,
100+
templates: {'Cmp': '<div dir></div>'},
101+
declarations: [{
102+
name: 'TestDir',
103+
selector: '[dir]',
104+
file: dirFile,
105+
type: 'directive',
106+
}]
107+
},
108+
{
109+
fileName: dirFile,
110+
source: `
111+
// A non-exported interface used as a type bound for a generic directive causes
112+
// an inline type constructor to be required.
113+
interface NotExported {}
114+
export class TestDir<T extends NotExported> {}`,
115+
templates: {},
116+
}
117+
],
118+
{inlining: false});
119+
const sf = getSourceFileOrError(program, fileName);
120+
const diags = templateTypeChecker.getDiagnosticsForFile(sf);
121+
expect(diags.length).toBe(1);
122+
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INLINE_TYPE_CTOR_REQUIRED));
123+
124+
// The relatedInformation of the diagnostic should point to the directive which required the
125+
// inline type constructor.
126+
expect(diags[0].relatedInformation).not.toBeUndefined();
127+
expect(diags[0].relatedInformation!.length).toBe(1);
128+
expect(diags[0].relatedInformation![0].file).not.toBeUndefined();
129+
expect(absoluteFromSourceFile(diags[0].relatedInformation![0].file!)).toBe(dirFile);
130+
});
131+
});
46132
});
47133
});

‎packages/compiler-cli/src/ngtsc/typecheck/test/type_constructor_spec.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import {getDeclaration, makeProgram} from '../../testing';
1515
import {getRootDirs} from '../../util/src/typescript';
1616
import {ComponentToShimMappingStrategy, UpdateMode} from '../api';
1717
import {ReusedProgramStrategy} from '../src/augmented_program';
18-
import {PendingFileTypeCheckingData, TypeCheckContextImpl, TypeCheckingHost} from '../src/context';
18+
import {InliningMode, PendingFileTypeCheckingData, TypeCheckContextImpl, TypeCheckingHost} from '../src/context';
1919
import {TemplateSourceManager} from '../src/source';
2020
import {TypeCheckFile} from '../src/type_check_file';
2121

@@ -74,7 +74,7 @@ TestClass.ngTypeCtor({value: 'test'});
7474
]);
7575
const ctx = new TypeCheckContextImpl(
7676
ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost,
77-
new TestTypeCheckingHost());
77+
new TestTypeCheckingHost(), InliningMode.InlineOps);
7878
const TestClass =
7979
getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration);
8080
const pendingFile = makePendingFile();
@@ -113,7 +113,7 @@ TestClass.ngTypeCtor({value: 'test'});
113113
const pendingFile = makePendingFile();
114114
const ctx = new TypeCheckContextImpl(
115115
ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost,
116-
new TestTypeCheckingHost());
116+
new TestTypeCheckingHost(), InliningMode.InlineOps);
117117
const TestClass =
118118
getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration);
119119
ctx.addInlineTypeCtor(
@@ -158,7 +158,7 @@ TestClass.ngTypeCtor({value: 'test'});
158158
const pendingFile = makePendingFile();
159159
const ctx = new TypeCheckContextImpl(
160160
ALL_ENABLED_CONFIG, host, new TestMappingStrategy(), emitter, reflectionHost,
161-
new TestTypeCheckingHost());
161+
new TestTypeCheckingHost(), InliningMode.InlineOps);
162162
const TestClass =
163163
getDeclaration(program, _('/main.ts'), 'TestClass', isNamedClassDeclaration);
164164
ctx.addInlineTypeCtor(

‎packages/language-service/ivy/compiler/compiler.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ export class Compiler {
7676
function createTypeCheckingProgramStrategy(project: ts.server.Project):
7777
TypeCheckingProgramStrategy {
7878
return {
79+
supportsInlineOperations: false,
7980
shimPathForComponent(component: ts.ClassDeclaration): AbsoluteFsPath {
8081
return TypeCheckShimGenerator.shimFor(absoluteFromSourceFile(component.getSourceFile()));
8182
},

0 commit comments

Comments
 (0)