fix(compiler): work around TypeScript bug when narrowing switch state…

…ments (#52110)

We type check `@switch` blocks by generating identical TS `switch` statements in the TCB, however TS currently has a bug where parenthesized `switch` block expressions don't narrow their types. Since we use parenthesized expressions to wrap AST nodes for diagnostics, this will bug will affect all Angular-generated `switch` statements.

These changes work around the issue by generating `if`/`else if`/`else` statements that represent the `switch`.

Some alternatives that were considered:
1. Moving the `switch` expression to a constant - this is fairly simple to implement, but it won't fully resolve the narrowing issue since the same constant will have to be used in expressions inside the different cases.
2. Removing the outer-most parenthesis from the switch expression - this works and allows us to continue using switch statements, but because we use parenthesized expressions to map diagnostics to their template locations, I wasn't sure if it won't lead to worse template dignostics.

Fixes #52077.

PR Close #52110

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {AST, BindingPipe, BindingType, BoundTarget, Call, DYNAMIC_TYPE, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, SafeCall, SafePropertyRead, SchemaMetadata, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType} from '@angular/compiler';
+import {AST, BindingPipe, BindingType, BoundTarget, Call, DYNAMIC_TYPE, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, SafeCall, SafePropertyRead, SchemaMetadata, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstSwitchBlockCase, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType} from '@angular/compiler';
 import ts from 'typescript';
 
 import {Reference} from '../../imports';
@@ -1273,27 +1273,56 @@ class TcbSwitchOp extends TcbOp {
   }
 
   override execute(): null {
-    const clauses: ts.CaseOrDefaultClause[] = [];
+    const expression = tcbExpression(this.block.expression, this.tcb, this.scope);
 
-    for (const current of this.block.cases) {
-      // Our template switch statements don't fall through so we always have a break at the end.
-      const breakStatement = ts.factory.createBreakStatement();
-      const clauseScope = Scope.forNodes(this.tcb, this.scope, null, current.children, null);
+    // Since we'll use the expression in multiple comparisons, we don't want to
+    // log the same diagnostic more than once. Ignore this expression since we already
+    // produced a `TcbExpressionOp`.
+    markIgnoreDiagnostics(expression);
+    const root = this.generateCase(0, expression, null);
 
-      if (current.expression === null) {
-        clauses.push(ts.factory.createDefaultClause([...clauseScope.render(), breakStatement]));
-      } else {
-        clauses.push(ts.factory.createCaseClause(
-            tcbExpression(current.expression, this.tcb, clauseScope),
-            [...clauseScope.render(), breakStatement]));
+    if (root !== undefined) {
+      this.scope.addStatement(root);
+    }
+
+    return null;
+  }
+
+  private generateCase(
+      index: number, switchValue: ts.Expression,
+      defaultCase: TmplAstSwitchBlockCase|null): ts.Statement|undefined {
+    // If we've reached the end, output the default case as the final `else`.
+    if (index >= this.block.cases.length) {
+      if (defaultCase !== null) {
+        // TODO(crisbeto): pass the guard once #52069 is merged.
+        const defaultScope = Scope.forNodes(this.tcb, this.scope, null, defaultCase.children, null);
+        return ts.factory.createBlock(defaultScope.render());
       }
+      return undefined;
     }
 
-    this.scope.addStatement(ts.factory.createSwitchStatement(
-        tcbExpression(this.block.expression, this.tcb, this.scope),
-        ts.factory.createCaseBlock(clauses)));
+    // If the current case is the default (could be placed arbitrarily),
+    // skip it since it needs to be generated as the final `else` at the end.
+    const current = this.block.cases[index];
+    if (current.expression === null) {
+      return this.generateCase(index + 1, switchValue, current);
+    }
 
-    return null;
+    // TODO(crisbeto): pass the guard once #52069 is merged.
+    const caseScope = Scope.forNodes(this.tcb, this.scope, null, current.children, null);
+    const caseValue = tcbExpression(current.expression, this.tcb, caseScope);
+
+    // TODO(crisbeto): change back to a switch statement when the TS bug is resolved.
+    // Note that it would be simpler to generate a `switch` statement to represent the user's
+    // code, but the current TypeScript version (at the time of writing 5.2.2) has a bug where
+    // parenthesized `switch` expressions aren't narrowed correctly:
+    // https://github.com/microsoft/TypeScript/issues/56030. We work around it by generating
+    // `if`/`else if`/`else` statements to represent the switch block instead.
+    return ts.factory.createIfStatement(
+        ts.factory.createBinaryExpression(
+            switchValue, ts.SyntaxKind.EqualsEqualsEqualsToken, caseValue),
+        ts.factory.createBlock(caseScope.render()),
+        this.generateCase(index + 1, switchValue, defaultCase));
   }
 }
 
@@ -1712,7 +1741,9 @@ class Scope {
     } else if (node instanceof TmplAstIfBlock) {
       this.opQueue.push(new TcbIfOp(this.tcb, this, node));
     } else if (node instanceof TmplAstSwitchBlock) {
-      this.opQueue.push(new TcbSwitchOp(this.tcb, this, node));
+      this.opQueue.push(
+          new TcbExpressionOp(this.tcb, this, node.expression),
+          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);

packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts

@@ -1444,8 +1444,20 @@ describe('type check blocks', () => {
 
       expect(tcb(TEMPLATE))
           .toContain(
-              'switch (((this).expr)) { case 1: "" + ((this).one()); break; ' +
-              'case 2: "" + ((this).two()); break; default: "" + ((this).default()); break; }');
+              'if (((this).expr) === 1) { "" + ((this).one()); } else if ' +
+              '(((this).expr) === 2) { "" + ((this).two()); } else { "" + ((this).default()); }');
+    });
+
+    it('should generate a switch block that only has a default case', () => {
+      const TEMPLATE = `
+        @switch (expr) {
+          @default {
+            {{default()}}
+          }
+        }
+      `;
+
+      expect(tcb(TEMPLATE)).toContain('{ "" + ((this).default()); }');
     });
 
     it('should generate a switch block inside a template', () => {
@@ -1467,9 +1479,10 @@ describe('type check blocks', () => {
 
       expect(tcb(TEMPLATE))
           .toContain(
-              'var _t1: any = null!; { var _t2 = (_t1.exp); switch (_t2()) { ' +
-              'case "one": "" + ((this).one()); break; case "two": "" + ((this).two()); break; ' +
-              'default: "" + ((this).default()); break; } } ');
+              'var _t1: any = null!; { var _t2 = (_t1.exp); _t2(); ' +
+              'if (_t2() === "one") { "" + ((this).one()); } ' +
+              'else if (_t2() === "two") { "" + ((this).two()); } ' +
+              'else { "" + ((this).default()); } }');
     });
   });
 

packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts

@@ -3996,6 +3996,62 @@ suppress
         ]);
       });
 
+      it('should only produce one diagnostic if the switch expression has an error', () => {
+        env.write('test.ts', `
+          import {Component} from '@angular/core';
+
+          @Component({
+            template: \`
+              @switch (does_not_exist_main) {
+                @case (1) {
+                  One
+                }
+
+                @case (2) {
+                  Two
+                }
+              }
+            \`,
+            standalone: true,
+          })
+          export class Main {}
+        `);
+
+        const diags = env.driveDiagnostics();
+        expect(diags.map(d => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
+          `Property 'does_not_exist_main' does not exist on type 'Main'.`,
+        ]);
+      });
+
+      it('should check a switch block that only has a default case', () => {
+        env.write('test.ts', `
+          import {Component} from '@angular/core';
+
+          @Component({
+            template: \`
+              @switch (expr) {
+                @default {
+                  {{acceptsNumber(expr)}}
+                }
+              }
+            \`,
+            standalone: true,
+          })
+          export class Main {
+            expr: 'hello' | 1 = 'hello';
+
+            acceptsNumber(value: number) {
+              return value;
+            }
+          }
+        `);
+
+        const diags = env.driveDiagnostics();
+        expect(diags.map(d => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
+          `Argument of type 'string | number' is not assignable to parameter of type 'number'.  Type 'string' is not assignable to type 'number'.`
+        ]);
+      });
+
       it('should narrow the type inside switch cases', () => {
         env.write('test.ts', `
           import {Component} from '@angular/core';
@@ -4027,6 +4083,49 @@ suppress
           `Argument of type 'string' is not assignable to parameter of type 'number'.`
         ]);
       });
+
+      it('should narrow the switch type based on a field', () => {
+        env.write('test.ts', `
+          import {Component} from '@angular/core';
+
+          export interface Base {
+            type: 'foo' | 'bar'
+          }
+
+          export interface Foo extends Base {
+            type: 'foo';
+            foo: string;
+          }
+
+          export interface Bar extends Base {
+            type: 'bar';
+            bar: number;
+          }
+
+          @Component({
+            template: \`
+              @switch (value.type) {
+                @case ('foo') {
+                  {{ acceptsNumber(value.foo) }}
+                }
+              }
+            \`,
+            standalone: true,
+          })
+          export class Main {
+            value: Foo | Bar = { type: 'foo', foo: 'foo' };
+
+            acceptsNumber(value: number) {
+              return value;
+            }
+          }
+        `);
+
+        const diags = env.driveDiagnostics();
+        expect(diags.map(d => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
+          `Argument of type 'string' is not assignable to parameter of type 'number'.`
+        ]);
+      });
     });
 
     describe('for loop blocks', () => {