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 (#55558)

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 #55558
  • Loading branch information
crisbeto authored and AndrewKushnir committed Apr 26, 2024
1 parent 9bb3446 commit 51ac883
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 10 deletions.
2 changes: 2 additions & 0 deletions packages/compiler-cli/src/ngtsc/core/src/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -821,6 +821,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 @@ -855,6 +856,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
Original file line number Diff line number Diff line change
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
29 changes: 19 additions & 10 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1401,8 +1401,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 @@ -1417,14 +1416,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 @@ -1516,12 +1520,14 @@ class TcbSwitchOp extends TcbOp {
private generateCase(
index: number, switchValue: ts.Expression,
defaultCase: TmplAstSwitchBlockCase|null): ts.Statement|undefined {
const checkBodies = this.tcb.env.config.checkControlFlowBodies;

// If we've reached the end, output the default case as the final `else`.
if (index >= this.block.cases.length) {
if (defaultCase !== null) {
const defaultScope = Scope.forNodes(
this.tcb, this.scope, null, defaultCase.children,
this.generateGuard(defaultCase, switchValue));
this.tcb, this.scope, null, checkBodies ? defaultCase.children : [],
checkBodies ? this.generateGuard(defaultCase, switchValue) : null);
return ts.factory.createBlock(defaultScope.render());
}
return undefined;
Expand All @@ -1535,7 +1541,8 @@ class TcbSwitchOp extends TcbOp {
}

const caseScope = Scope.forNodes(
this.tcb, this.scope, null, current.children, this.generateGuard(current, switchValue));
this.tcb, this.scope, null, checkBodies ? current.children : [],
checkBodies ? this.generateGuard(current, switchValue) : null);
const caseValue = tcbExpression(current.expression, this.tcb, caseScope);

// TODO(crisbeto): change back to a switch statement when the TS bug is resolved.
Expand Down Expand Up @@ -1611,7 +1618,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 @@ -2043,7 +2052,7 @@ class Scope {
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
Original file line number Diff line number Diff line change
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 @@ -1656,6 +1679,28 @@ describe('type check blocks', () => {
it('should handle an empty switch block', () => {
expect(tcb('@switch (expr) {}')).toContain('if (true) { ((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(
'((this).expr); if ((((this).expr)) === 1) { } ' +
'else if ((((this).expr)) === 2) { } else { } }');
});
});

describe('for loop blocks', () => {
Expand Down Expand Up @@ -1744,6 +1789,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
Original file line number Diff line number Diff line change
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 51ac883

Please sign in to comment.