fix(compiler-cli): don't type check the bodies of control flow nodes …

…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
angular · Apr 26, 2024 · 51ac883 · 51ac883
1 parent 9bb3446
commit 51ac883
Show file tree

Hide file tree

Showing 5 changed files with 89 additions and 10 deletions.
diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts
@@ -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.
@@ -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,

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/api.ts
@@ -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;
 }
 
 

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
@@ -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());
     }
 
@@ -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;
 
@@ -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;
@@ -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.
@@ -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(
@@ -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) {

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
@@ -856,6 +856,7 @@ describe('type check blocks', () => {
       applyTemplateContextGuards: true,
       checkQueries: false,
       checkTemplateBodies: true,
+      checkControlFlowBodies: true,
       alwaysCheckSchemaInTemplateBodies: true,
       checkTypeOfInputBindings: true,
       honorAccessModifiersForInputBindings: false,
@@ -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) {
@@ -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', () => {
@@ -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', () => {

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts
@@ -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,
@@ -323,6 +324,7 @@ export function tcb(
     checkTypeOfNonDomReferences: true,
     checkTypeOfPipes: true,
     checkTemplateBodies: true,
+    checkControlFlowBodies: true,
     alwaysCheckSchemaInTemplateBodies: true,
     controlFlowPreventingContentProjection: 'warning',
     strictSafeNavigationTypes: true,