fix(compiler): narrow the type of the aliased if block expression (#51952)

crisbeto · alxhub · commit 02edb4306736 · 2023-10-04T08:49:59.000-07:00
Currently the TCB for aliased `if` blocks looks something like this: ``` // Markup: `@if (expr; as alias) { {{alias}} } if (block.condition) { var alias = block.condition; "" + alias; } ``` The problem with this approach is that the type of `alias` won't be narrowed. This is something that `NgIf` currently supports. These changes resolve the issue by emitting the variable outside the `if` block and using the variable reference instead: ``` // Markup: `@if (expr; as alias) { {{alias}} } var alias = block.condition; if (alias) { "" + alias; } ``` PR Close #51952
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
@@ -85,7 +85,7 @@ export function generateTypeCheckBlock(
   const tcb = new Context(
       env, domSchemaChecker, oobRecorder, meta.id, meta.boundTarget, meta.pipes, meta.schemas,
       meta.isStandalone);
-  const scope = Scope.forNodes(tcb, null, tcb.boundTarget.target.template!, /* guard */ null);
+  const scope = Scope.forNodes(tcb, null, null, tcb.boundTarget.target.template!, /* guard */ null);
   const ctxRawType = env.referenceType(ref);
   if (!ts.isTypeReferenceNode(ctxRawType)) {
     throw new Error(
@@ -379,7 +379,8 @@ class TcbTemplateBodyOp extends TcbOp {
 
     // Create a new Scope for the template. This constructs the list of operations for the template
     // children, as well as tracks bindings within the template.
-    const tmplScope = Scope.forNodes(this.tcb, this.scope, this.template, guard);
+    const tmplScope =
+        Scope.forNodes(this.tcb, this.scope, this.template, this.template.children, guard);
 
     // Render the template's `Scope` into its statements.
     const statements = tmplScope.render();
@@ -1228,14 +1229,31 @@ class TcbIfOp extends TcbOp {
       return undefined;
     }
 
-    const branchScope = Scope.forNodes(this.tcb, this.scope, branch, null);
-
     // If the expression is null, it means that it's an `else` statement.
-    return branch.expression === null ?
-        ts.factory.createBlock(branchScope.render()) :
-        ts.factory.createIfStatement(
-            tcbExpression(branch.expression, this.tcb, branchScope),
-            ts.factory.createBlock(branchScope.render()), this.generateBranch(index + 1));
+    if (branch.expression === null) {
+      const branchScope = Scope.forNodes(this.tcb, this.scope, null, branch.children, null);
+      return ts.factory.createBlock(branchScope.render());
+    }
+
+    let branchParentScope: Scope;
+
+    if (branch.expressionAlias === null) {
+      branchParentScope = this.scope;
+    } else {
+      // If the expression is aliased, we create a scope with a variable containing the expression.
+      // Further down we'll use the variable instead of the expression itself in the `if` statement.
+      // This allows for the type of the alias to be narrowed.
+      branchParentScope = Scope.forNodes(this.tcb, this.scope, branch, [], null);
+      branchParentScope.render().forEach(stmt => this.scope.addStatement(stmt));
+    }
+
+    const branchScope = Scope.forNodes(this.tcb, branchParentScope, null, branch.children, null);
+    const expression = branch.expressionAlias === null ?
+        tcbExpression(branch.expression, this.tcb, branchScope) :
+        branchScope.resolve(branch.expressionAlias);
+
+    return ts.factory.createIfStatement(
+        expression, ts.factory.createBlock(branchScope.render()), this.generateBranch(index + 1));
   }
 }
 
@@ -1260,7 +1278,7 @@ class TcbSwitchOp extends TcbOp {
     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, current.children, null);
+      const clauseScope = Scope.forNodes(this.tcb, this.scope, null, current.children, null);
 
       if (current.expression === null) {
         clauses.push(ts.factory.createDefaultClause([...clauseScope.render(), breakStatement]));
@@ -1294,7 +1312,7 @@ class TcbForOfOp extends TcbOp {
   }
 
   override execute(): null {
-    const loopScope = Scope.forNodes(this.tcb, this.scope, this.block, null);
+    const loopScope = Scope.forNodes(this.tcb, this.scope, this.block, this.block.children, null);
     const initializer = ts.factory.createVariableDeclarationList(
         [ts.factory.createVariableDeclaration(this.block.item.name)], ts.NodeFlags.Const);
     // It's common to have a for loop over a nullable value (e.g. produced by the `async` pipe).
@@ -1431,67 +1449,60 @@ class Scope {
    * Constructs a `Scope` given either a `TmplAstTemplate` or a list of `TmplAstNode`s.
    *
    * @param tcb the overall context of TCB generation.
-   * @param parent the `Scope` of the parent template (if any) or `null` if this is the root
+   * @param parentScope the `Scope` of the parent template (if any) or `null` if this is the root
    * `Scope`.
-   * @param blockOrNodes either a `TmplAstTemplate` representing the template for which to
-   * calculate the `Scope`, or a list of nodes if no outer template object is available.
+   * @param scopedNode Node that provides the scope around the child nodes (e.g. a
+   * `TmplAstTemplate` node exposing variables to its children).
+   * @param children Child nodes that should be appended to the TCB.
    * @param guard an expression that is applied to this scope for type narrowing purposes.
    */
   static forNodes(
-      tcb: Context, parent: Scope|null,
-      blockOrNodes: TmplAstTemplate|TmplAstIfBlockBranch|TmplAstForLoopBlock|TmplAstNode[],
-      guard: ts.Expression|null): Scope {
-    const scope = new Scope(tcb, parent, guard);
+      tcb: Context, parentScope: Scope|null,
+      scopedNode: TmplAstTemplate|TmplAstIfBlockBranch|TmplAstForLoopBlock|null,
+      children: TmplAstNode[], guard: ts.Expression|null): Scope {
+    const scope = new Scope(tcb, parentScope, guard);
 
-    if (parent === null && tcb.env.config.enableTemplateTypeChecker) {
+    if (parentScope === null && tcb.env.config.enableTemplateTypeChecker) {
       // Add an autocompletion point for the component context.
       scope.opQueue.push(new TcbComponentContextCompletionOp(scope));
     }
 
-    let children: TmplAstNode[];
-
     // If given an actual `TmplAstTemplate` instance, then process any additional information it
     // has.
-    if (blockOrNodes instanceof TmplAstTemplate) {
+    if (scopedNode instanceof TmplAstTemplate) {
       // The template's variable declarations need to be added as `TcbVariableOp`s.
       const varMap = new Map<string, TmplAstVariable>();
 
-      for (const v of blockOrNodes.variables) {
+      for (const v of scopedNode.variables) {
         // Validate that variables on the `TmplAstTemplate` are only declared once.
         if (!varMap.has(v.name)) {
           varMap.set(v.name, v);
         } else {
           const firstDecl = varMap.get(v.name)!;
           tcb.oobRecorder.duplicateTemplateVar(tcb.id, v, firstDecl);
         }
-        this.registerVariable(scope, v, new TcbTemplateVariableOp(tcb, scope, blockOrNodes, v));
+        this.registerVariable(scope, v, new TcbTemplateVariableOp(tcb, scope, scopedNode, v));
       }
-      children = blockOrNodes.children;
-    } else if (blockOrNodes instanceof TmplAstIfBlockBranch) {
-      const {expression, expressionAlias} = blockOrNodes;
+    } else if (scopedNode instanceof TmplAstIfBlockBranch) {
+      const {expression, expressionAlias} = scopedNode;
       if (expression !== null && expressionAlias !== null) {
         this.registerVariable(
             scope, expressionAlias,
             new TcbBlockVariableOp(
                 tcb, scope, tcbExpression(expression, tcb, scope), expressionAlias));
       }
-      children = blockOrNodes.children;
-    } else if (blockOrNodes instanceof TmplAstForLoopBlock) {
+    } else if (scopedNode instanceof TmplAstForLoopBlock) {
       this.registerVariable(
-          scope, blockOrNodes.item,
+          scope, scopedNode.item,
           new TcbBlockVariableOp(
-              tcb, scope, ts.factory.createIdentifier(blockOrNodes.item.name), blockOrNodes.item));
+              tcb, scope, ts.factory.createIdentifier(scopedNode.item.name), scopedNode.item));
 
-      for (const variable of Object.values(blockOrNodes.contextVariables)) {
+      for (const variable of Object.values(scopedNode.contextVariables)) {
         // Note: currently all context variables are assumed to be number types.
         const type = ts.factory.createKeywordTypeNode(ts.SyntaxKind.NumberKeyword);
         this.registerVariable(
             scope, variable, new TcbBlockImplicitVariableOp(tcb, scope, type, variable));
       }
-
-      children = blockOrNodes.children;
-    } else {
-      children = blockOrNodes;
     }
     for (const node of children) {
       scope.appendNode(node);
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
@@ -1424,8 +1424,7 @@ describe('type check blocks', () => {
       }`;
 
       expect(tcb(TEMPLATE))
-          .toContain(
-              'if ((((this).expr)) === (1)) { var _t1 = (((this).expr)) === (1); "" + (_t1); }');
+          .toContain('var _t1 = (((this).expr)) === (1); if (_t1) { "" + (_t1); } }');
     });
 
     it('should generate a switch block', () => {
diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
@@ -3831,6 +3831,31 @@ suppress
         ]);
       });
 
+      it('should check narrow the type in the alias', () => {
+        env.write('test.ts', `
+          import {Component} from '@angular/core';
+
+          @Component({
+            template: \`@if (value; as alias) {
+              {{acceptsNumber(alias)}}
+            }\`,
+            standalone: true,
+          })
+          export class Main {
+            value: 'one' | 0 = 0;
+
+            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'.`,
+        ]);
+      });
+
       it('should not expose the aliased expression outside of the main block', () => {
         env.write('test.ts', `
           import {Component} from '@angular/core';
