refactor(compiler-cli): move illegal template assignment check into template semantics checker (#54714)

Moves the check which ensures that there are no writes to template variables into the `TemplateSemanticsChecker` to prepare for the upcoming changes.

PR Close #54714

packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {AST, LiteralPrimitive, ParseSourceSpan, PropertyRead, SafePropertyRead, TmplAstElement, TmplAstNode, TmplAstTemplate, TmplAstTextAttribute} from '@angular/compiler';
+import {AST, LiteralPrimitive, ParseSourceSpan, PropertyRead, SafePropertyRead, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler';
 import ts from 'typescript';
 
 import {AbsoluteFsPath} from '../../../../src/ngtsc/file_system';
@@ -213,6 +213,12 @@ export interface TemplateTypeChecker {
    */
   invalidateClass(clazz: ts.ClassDeclaration): void;
 
+  /**
+   * Gets the target of a template expression, if possible.
+   */
+  getExpressionTarget(expression: AST, clazz: ts.ClassDeclaration): TmplAstReference|TmplAstVariable
+      |null;
+
   /**
    * Constructs a `ts.Diagnostic` for a given `ParseSourceSpan` within a template.
    */

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

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {AST, CssSelector, DomElementSchemaRegistry, ExternalExpr, LiteralPrimitive, ParseSourceSpan, PropertyRead, SafePropertyRead, TmplAstElement, TmplAstNode, TmplAstTemplate, TmplAstTextAttribute, WrappedNodeExpr} from '@angular/compiler';
+import {AST, CssSelector, DomElementSchemaRegistry, ExternalExpr, LiteralPrimitive, ParseSourceSpan, PropertyRead, SafePropertyRead, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable, WrappedNodeExpr} from '@angular/compiler';
 import ts from 'typescript';
 
 import {ErrorCode, ngErrorCode} from '../../diagnostics';
@@ -345,6 +345,12 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker {
     this.isComplete = false;
   }
 
+  getExpressionTarget(expression: AST, clazz: ts.ClassDeclaration): TmplAstReference|TmplAstVariable
+      |null {
+    return this.getLatestComponentState(clazz).data?.boundTarget.getExpressionTarget(expression) ||
+        null;
+  }
+
   makeTemplateDiagnostic<T extends ErrorCode>(
       clazz: ts.ClassDeclaration, sourceSpan: ParseSourceSpan, category: ts.DiagnosticCategory,
       errorCode: T, message: string, relatedInformation?: {

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

@@ -70,9 +70,6 @@ export interface OutOfBandDiagnosticRecorder {
    */
   deferredComponentUsedEagerly(templateId: TemplateId, element: TmplAstElement): void;
 
-  illegalAssignmentToTemplateVar(
-      templateId: TemplateId, assignment: PropertyWrite, target: TmplAstVariable): void;
-
   /**
    * Reports a duplicate declaration of a template variable.
    *
@@ -217,27 +214,6 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
         ngErrorCode(ErrorCode.DEFERRED_DIRECTIVE_USED_EAGERLY), errorMsg));
   }
 
-  illegalAssignmentToTemplateVar(
-      templateId: TemplateId, assignment: PropertyWrite, target: TmplAstVariable): void {
-    const mapping = this.resolver.getSourceMapping(templateId);
-    const errorMsg = `Cannot use variable '${
-        assignment
-            .name}' as the left-hand side of an assignment expression. Template variables are read-only.`;
-
-    const sourceSpan = this.resolver.toParseSourceSpan(templateId, assignment.sourceSpan);
-    if (sourceSpan === null) {
-      throw new Error(`Assertion failure: no SourceLocation found for property binding.`);
-    }
-    this._diagnostics.push(makeTemplateDiagnostic(
-        templateId, mapping, sourceSpan, ts.DiagnosticCategory.Error,
-        ngErrorCode(ErrorCode.WRITE_TO_READ_ONLY_VARIABLE), errorMsg, [{
-          text: `The variable ${assignment.name} is declared here.`,
-          start: target.valueSpan?.start.offset || target.sourceSpan.start.offset,
-          end: target.valueSpan?.end.offset || target.sourceSpan.end.offset,
-          sourceFile: mapping.node.getSourceFile(),
-        }]));
-  }
-
   duplicateTemplateVar(
       templateId: TemplateId, variable: TmplAstVariable, firstDecl: TmplAstVariable): void {
     const mapping = this.resolver.getSourceMapping(templateId);

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

@@ -1,44 +0,0 @@
-/**
- * @license
- * Copyright Google LLC All Rights Reserved.
- *
- * Use of this source code is governed by an MIT-style license that can be
- * found in the LICENSE file at https://angular.io/license
- */
-
-import {AST, BoundTarget, ImplicitReceiver, PropertyWrite, RecursiveAstVisitor, TmplAstVariable} from '@angular/compiler';
-
-import {TemplateId} from '../api';
-
-import {OutOfBandDiagnosticRecorder} from './oob';
-
-/**
- * Visits a template and records any semantic errors within its expressions.
- */
-export class ExpressionSemanticVisitor extends RecursiveAstVisitor {
-  constructor(
-      private templateId: TemplateId, private boundTarget: BoundTarget<any>,
-      private oob: OutOfBandDiagnosticRecorder) {
-    super();
-  }
-
-  override visitPropertyWrite(ast: PropertyWrite, context: any): void {
-    super.visitPropertyWrite(ast, context);
-
-    if (!(ast.receiver instanceof ImplicitReceiver)) {
-      return;
-    }
-
-    const target = this.boundTarget.getExpressionTarget(ast);
-    if (target instanceof TmplAstVariable) {
-      // Template variables are read-only.
-      this.oob.illegalAssignmentToTemplateVar(this.templateId, ast, target);
-    }
-  }
-
-  static visit(
-      ast: AST, id: TemplateId, boundTarget: BoundTarget<any>,
-      oob: OutOfBandDiagnosticRecorder): void {
-    ast.visit(new ExpressionSemanticVisitor(id, boundTarget, oob));
-  }
-}

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

@@ -20,7 +20,6 @@ import {DomSchemaChecker} from './dom';
 import {Environment} from './environment';
 import {astToTypescript, NULL_AS_ANY} from './expression';
 import {OutOfBandDiagnosticRecorder} from './oob';
-import {ExpressionSemanticVisitor} from './template_semantics';
 import {tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareVariable} from './ts_util';
 import {requiresInlineTypeCtor} from './type_constructor';
 import {TypeParameterEmitter} from './type_parameter_emitter';
@@ -1214,9 +1213,6 @@ export class TcbDirectiveOutputsOp extends TcbOp {
         const handler = tcbCreateEventHandler(output, this.tcb, this.scope, EventParamType.Any);
         this.scope.addStatement(ts.factory.createExpressionStatement(handler));
       }
-
-      ExpressionSemanticVisitor.visit(
-          output.handler, this.tcb.id, this.tcb.boundTarget, this.tcb.oobRecorder);
     }
 
     return null;
@@ -1292,9 +1288,6 @@ class TcbUnclaimedOutputsOp extends TcbOp {
         const handler = tcbCreateEventHandler(output, this.tcb, this.scope, EventParamType.Any);
         this.scope.addStatement(ts.factory.createExpressionStatement(handler));
       }
-
-      ExpressionSemanticVisitor.visit(
-          output.handler, this.tcb.id, this.tcb.boundTarget, this.tcb.oobRecorder);
     }
 
     return null;

packages/compiler-cli/src/ngtsc/typecheck/template_semantics/BUILD.bazel

@@ -10,6 +10,7 @@ ts_library(
         "//packages/compiler",
         "//packages/compiler-cli/src/ngtsc/core:api",
         "//packages/compiler-cli/src/ngtsc/diagnostics",
+        "//packages/compiler-cli/src/ngtsc/typecheck",
         "//packages/compiler-cli/src/ngtsc/typecheck/api",
         "//packages/compiler-cli/src/ngtsc/typecheck/template_semantics/api",
         "@npm//typescript",

packages/compiler-cli/src/ngtsc/typecheck/template_semantics/src/template_semantics_checker.ts

@@ -6,9 +6,10 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {ImplicitReceiver, PropertyWrite, RecursiveAstVisitor, TmplAstBoundEvent, TmplAstNode, TmplAstRecursiveVisitor, TmplAstVariable} from '@angular/compiler';
 import ts from 'typescript';
 
-import {DiagnosticCategoryLabel} from '../../../core/api';
+import {ErrorCode, ngErrorCode} from '../../../diagnostics';
 import {TemplateDiagnostic, TemplateTypeChecker} from '../../api';
 import {TemplateSemanticsChecker} from '../api/api';
 
@@ -17,12 +18,69 @@ export class TemplateSemanticsCheckerImpl implements TemplateSemanticsChecker {
 
   getDiagnosticsForComponent(component: ts.ClassDeclaration): TemplateDiagnostic[] {
     const template = this.templateTypeChecker.getTemplate(component);
-
-    if (template === null) {
-      return [];
-    }
-
-    const diagnostics: TemplateDiagnostic[] = [];
-    return diagnostics;
+    return template !== null ?
+        TemplateSemanticsVisitor.visit(template, component, this.templateTypeChecker) :
+        [];
+  }
+}
+
+/** Visitor that verifies the semantics of a template. */
+class TemplateSemanticsVisitor extends TmplAstRecursiveVisitor {
+  private constructor(private expressionVisitor: ExpressionsSemanticsVisitor) {
+    super();
+  }
+
+  static visit(
+      nodes: TmplAstNode[], component: ts.ClassDeclaration,
+      templateTypeChecker: TemplateTypeChecker) {
+    const diagnostics: TemplateDiagnostic[] = [];
+    const expressionVisitor =
+        new ExpressionsSemanticsVisitor(templateTypeChecker, component, diagnostics);
+    const templateVisitor = new TemplateSemanticsVisitor(expressionVisitor);
+    nodes.forEach(node => node.visit(templateVisitor));
+    return diagnostics;
+  }
+
+  override visitBoundEvent(event: TmplAstBoundEvent): void {
+    super.visitBoundEvent(event);
+    event.handler.visit(this.expressionVisitor, event);
+  }
+}
+
+/** Visitor that verifies the semantics of the expressions within a template. */
+class ExpressionsSemanticsVisitor extends RecursiveAstVisitor {
+  constructor(
+      private templateTypeChecker: TemplateTypeChecker, private component: ts.ClassDeclaration,
+      private diagnostics: TemplateDiagnostic[]) {
+    super();
+  }
+
+  override visitPropertyWrite(ast: PropertyWrite, context: TmplAstNode): void {
+    super.visitPropertyWrite(ast, context);
+
+    if (!(context instanceof TmplAstBoundEvent) || !(ast.receiver instanceof ImplicitReceiver)) {
+      return;
+    }
+
+    const target = this.templateTypeChecker.getExpressionTarget(ast, this.component);
+    if (target instanceof TmplAstVariable) {
+      const errorMessage = `Cannot use variable '${
+          target
+              .name}' as the left-hand side of an assignment expression. Template variables are read-only.`;
+      this.diagnostics.push(this.makeIllegalTemplateVarDiagnostic(target, context, errorMessage));
+    }
+  }
+
+  private makeIllegalTemplateVarDiagnostic(
+      target: TmplAstVariable, expressionNode: TmplAstNode,
+      errorMessage: string): TemplateDiagnostic {
+    return this.templateTypeChecker.makeTemplateDiagnostic(
+        this.component, expressionNode.sourceSpan, ts.DiagnosticCategory.Error,
+        ngErrorCode(ErrorCode.WRITE_TO_READ_ONLY_VARIABLE), errorMessage, [{
+          text: `The variable ${target.name} is declared here.`,
+          start: target.valueSpan?.start.offset || target.sourceSpan.start.offset,
+          end: target.valueSpan?.end.offset || target.sourceSpan.end.offset,
+          sourceFile: this.component.getSourceFile(),
+        }]);
   }
 }

packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts

@@ -859,7 +859,6 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
   missingPipe(): void {}
   deferredPipeUsedEagerly(templateId: TemplateId, ast: BindingPipe): void {}
   deferredComponentUsedEagerly(templateId: TemplateId, element: TmplAstElement): void {}
-  illegalAssignmentToTemplateVar(): void {}
   duplicateTemplateVar(): void {}
   requiresInlineTcb(): void {}
   requiresInlineTypeConstructors(): void {}