Skip to content

Commit

Permalink
fix(compiler-cli): don't type check the bodies of control flow nodes …
Browse files Browse the repository at this point in the history
…in basic mode (#55360)

Angular only checks the contents of template nodes in full type checking mode. After v17, the new control flow always had its body checked, even in basic mode, which started revealing compilation errors for apps that were using the schematic to automatically switch to the new syntax.

These changes mimic the old behavior by not checking the bodies of `if`, `switch` and `for` blocks in basic mode. Note that the expressions of the blocks are still going to be checked.

Fixes #52969.

PR Close #55360
  • Loading branch information
crisbeto authored and alxhub committed Apr 19, 2024
1 parent e67a730 commit 7a16d7e
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 9 deletions.
2 changes: 2 additions & 0 deletions packages/compiler-cli/src/ngtsc/core/src/compiler.ts
Expand Up @@ -824,6 +824,7 @@ export class NgCompiler {
alwaysCheckSchemaInTemplateBodies: true,
checkTypeOfInputBindings: strictTemplates,
honorAccessModifiersForInputBindings: false,
checkControlFlowBodies: true,
strictNullInputBindings: strictTemplates,
checkTypeOfAttributes: strictTemplates,
// Even in full template type-checking mode, DOM binding checks are not quite ready yet.
Expand Down Expand Up @@ -858,6 +859,7 @@ export class NgCompiler {
applyTemplateContextGuards: false,
checkQueries: false,
checkTemplateBodies: false,
checkControlFlowBodies: false,
// Enable deep schema checking in "basic" template type-checking mode only if Closure
// compilation is requested, which is a good proxy for "only in google3".
alwaysCheckSchemaInTemplateBodies: this.closureCompilerEnabled,
Expand Down
5 changes: 5 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/api/api.ts
Expand Up @@ -335,6 +335,11 @@ export interface TypeCheckingConfig {
* Whether the type of two-way bindings should be widened to allow `WritableSignal`.
*/
allowSignalsInTwoWayBindings: boolean;

/**
* Whether to descend into the bodies of control flow blocks (`@if`, `@switch` and `@for`).
*/
checkControlFlowBodies: boolean;
}


Expand Down
25 changes: 16 additions & 9 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Expand Up @@ -1386,8 +1386,7 @@ class TcbIfOp extends TcbOp {

// If the expression is null, it means that it's an `else` statement.
if (branch.expression === null) {
const branchScope = Scope.forNodes(
this.tcb, this.scope, null, branch.children, this.generateBranchGuard(index));
const branchScope = this.getBranchScope(this.scope, branch, index);
return ts.factory.createBlock(branchScope.render());
}

Expand All @@ -1402,14 +1401,19 @@ class TcbIfOp extends TcbOp {
const expression = branch.expressionAlias === null ?
tcbExpression(branch.expression, this.tcb, expressionScope) :
expressionScope.resolve(branch.expressionAlias);

const bodyScope = Scope.forNodes(
this.tcb, expressionScope, null, branch.children, this.generateBranchGuard(index));
const bodyScope = this.getBranchScope(expressionScope, branch, index);

return ts.factory.createIfStatement(
expression, ts.factory.createBlock(bodyScope.render()), this.generateBranch(index + 1));
}

private getBranchScope(parentScope: Scope, branch: TmplAstIfBlockBranch, index: number): Scope {
const checkBody = this.tcb.env.config.checkControlFlowBodies;
return Scope.forNodes(
this.tcb, parentScope, null, checkBody ? branch.children : [],
checkBody ? this.generateBranchGuard(index) : null);
}

private generateBranchGuard(index: number): ts.Expression|null {
let guard: ts.Expression|null = null;

Expand Down Expand Up @@ -1482,9 +1486,10 @@ class TcbSwitchOp extends TcbOp {
override execute(): null {
const switchExpression = tcbExpression(this.block.expression, this.tcb, this.scope);
const clauses = this.block.cases.map(current => {
const checkBody = this.tcb.env.config.checkControlFlowBodies;
const clauseScope = Scope.forNodes(
this.tcb, this.scope, null, current.children,
this.generateGuard(current, switchExpression));
this.tcb, this.scope, null, checkBody ? current.children : [],
checkBody ? this.generateGuard(current, switchExpression) : null);
const statements = [...clauseScope.render(), ts.factory.createBreakStatement()];

return current.expression === null ?
Expand Down Expand Up @@ -1559,7 +1564,9 @@ class TcbForOfOp extends TcbOp {
}

override execute(): null {
const loopScope = Scope.forNodes(this.tcb, this.scope, this.block, this.block.children, null);
const loopScope = Scope.forNodes(
this.tcb, this.scope, this.block,
this.tcb.env.config.checkControlFlowBodies ? this.block.children : [], null);
const initializerId = loopScope.resolve(this.block.item);
if (!ts.isIdentifier(initializerId)) {
throw new Error(
Expand Down Expand Up @@ -1990,7 +1997,7 @@ class Scope {
this.opQueue.push(new TcbSwitchOp(this.tcb, this, node));
} else if (node instanceof TmplAstForLoopBlock) {
this.opQueue.push(new TcbForOfOp(this.tcb, this, node));
node.empty && this.appendChildren(node.empty);
node.empty && this.tcb.env.config.checkControlFlowBodies && this.appendChildren(node.empty);
} else if (node instanceof TmplAstBoundText) {
this.opQueue.push(new TcbExpressionOp(this.tcb, this, node.value));
} else if (node instanceof TmplAstIcu) {
Expand Down
Expand Up @@ -856,6 +856,7 @@ describe('type check blocks', () => {
applyTemplateContextGuards: true,
checkQueries: false,
checkTemplateBodies: true,
checkControlFlowBodies: true,
alwaysCheckSchemaInTemplateBodies: true,
checkTypeOfInputBindings: true,
honorAccessModifiersForInputBindings: false,
Expand Down Expand Up @@ -1572,6 +1573,28 @@ describe('type check blocks', () => {
.toContain('var _t1 = ((((this).expr)) === (1)); if (_t1) { "" + (_t1); } } }');
});

it('should not generate the body of if blocks when `checkControlFlowBodies` is disabled',
() => {
const TEMPLATE = `
@if (expr === 0) {
{{main()}}
} @else if (expr1 === 1) {
{{one()}}
} @else if (expr2 === 2) {
{{two()}}
} @else {
{{other()}}
}
`;

expect(tcb(TEMPLATE, undefined, {checkControlFlowBodies: false}))
.toContain(
'if ((((this).expr)) === (0)) { } ' +
'else if ((((this).expr1)) === (1)) { } ' +
'else if ((((this).expr2)) === (2)) { } ' +
'else { }');
});

it('should generate a switch block', () => {
const TEMPLATE = `
@switch (expr) {
Expand Down Expand Up @@ -1658,6 +1681,30 @@ describe('type check blocks', () => {
it('should handle an empty switch block', () => {
expect(tcb('@switch (expr) {}')).toContain('if (true) { switch (((this).expr)) { } }');
});

it('should not generate the body of a switch block if checkControlFlowBodies is disabled',
() => {
const TEMPLATE = `
@switch (expr) {
@case (1) {
{{one()}}
}
@case (2) {
{{two()}}
}
@default {
{{default()}}
}
}
`;

expect(tcb(TEMPLATE, undefined, {checkControlFlowBodies: false}))
.toContain(
'switch (((this).expr)) { ' +
'case 1: break; ' +
'case 2: break; ' +
'default: break; }');
});
});

describe('for loop blocks', () => {
Expand Down Expand Up @@ -1747,6 +1794,22 @@ describe('type check blocks', () => {
expect(result).toContain('for (const _t1 of ((this).items)!) { var _t2 = null! as number;');
expect(result).toContain('(this).trackingFn(_t2, _t1, ((this).prop));');
});

it('should not generate the body of a for block when checkControlFlowBodies is disabled',
() => {
const TEMPLATE = `
@for (item of items; track item) {
{{main(item)}}
} @empty {
{{empty()}}
}
`;

const result = tcb(TEMPLATE, undefined, {checkControlFlowBodies: false});
expect(result).toContain('for (const _t1 of ((this).items)!) {');
expect(result).not.toContain('.main');
expect(result).not.toContain('.empty');
});
});

describe('import generation', () => {
Expand Down
2 changes: 2 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts
Expand Up @@ -198,6 +198,7 @@ export const ALL_ENABLED_CONFIG: Readonly<TypeCheckingConfig> = {
applyTemplateContextGuards: true,
checkQueries: false,
checkTemplateBodies: true,
checkControlFlowBodies: true,
alwaysCheckSchemaInTemplateBodies: true,
checkTypeOfInputBindings: true,
honorAccessModifiersForInputBindings: true,
Expand Down Expand Up @@ -323,6 +324,7 @@ export function tcb(
checkTypeOfNonDomReferences: true,
checkTypeOfPipes: true,
checkTemplateBodies: true,
checkControlFlowBodies: true,
alwaysCheckSchemaInTemplateBodies: true,
controlFlowPreventingContentProjection: 'warning',
strictSafeNavigationTypes: true,
Expand Down

0 comments on commit 7a16d7e

Please sign in to comment.