fix(compiler-cli): flag two-way bindings to non-signal values in temp…

…lates (#54714)

We have a diagnostic that reports writes to template variables which worked both for regular event bindings and two-way bindings, however the latter was broken by #54154 because two-way bindings no longer had a `PropertyWrite` AST.

These changes fix the diagnostic and expand it to allow two-way bindings to template variables that are signals.

PR Close #54714

packages/compiler-cli/src/ngtsc/typecheck/extended/checks/interpolated_signal_not_invoked/index.ts

@@ -10,7 +10,7 @@ import {AST, Interpolation, PropertyRead, TmplAstNode} from '@angular/compiler';
 import ts from 'typescript';
 
 import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics';
-import {NgTemplateDiagnostic} from '../../../api';
+import {NgTemplateDiagnostic, SymbolKind} from '../../../api';
 import {isSignalReference} from '../../../src/symbol_util';
 import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api';
 
@@ -57,7 +57,7 @@ function buildDiagnosticForSignal(
     Array<NgTemplateDiagnostic<ErrorCode.INTERPOLATED_SIGNAL_NOT_INVOKED>> {
   // check for `{{ mySignal }}`
   const symbol = ctx.templateTypeChecker.getSymbolOfNode(node, component);
-  if (symbol !== null && isSignalReference(symbol)) {
+  if (symbol !== null && symbol.kind === SymbolKind.Expression && isSignalReference(symbol)) {
     const templateMapping =
         ctx.templateTypeChecker.getTemplateMappingAtTcbLocation(symbol.tcbLocation)!;
     const errorString = `${node.name} is a function and should be invoked: ${node.name}()`;
@@ -72,7 +72,8 @@ function buildDiagnosticForSignal(
   // `{{ mySignal.asReadonly }}` as these are the names of instance properties of Signal
   const symbolOfReceiver = ctx.templateTypeChecker.getSymbolOfNode(node.receiver, component);
   if ((isFunctionInstanceProperty(node.name) || isSignalInstanceProperty(node.name)) &&
-      symbolOfReceiver !== null && isSignalReference(symbolOfReceiver)) {
+      symbolOfReceiver !== null && symbolOfReceiver.kind === SymbolKind.Expression &&
+      isSignalReference(symbolOfReceiver)) {
     const templateMapping =
         ctx.templateTypeChecker.getTemplateMappingAtTcbLocation(symbolOfReceiver.tcbLocation)!;
 

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

@@ -8,7 +8,7 @@
 
 import ts from 'typescript';
 
-import {ExpressionSymbol, Symbol, SymbolKind} from '../api';
+import {Symbol, SymbolKind} from '../api';
 
 /** Names of known signal functions. */
 const SIGNAL_FNS = new Set([
@@ -20,8 +20,8 @@ const SIGNAL_FNS = new Set([
 ]);
 
 /** Returns whether a symbol is a reference to a signal. */
-export function isSignalReference(symbol: Symbol): symbol is ExpressionSymbol {
-  return symbol.kind === SymbolKind.Expression &&
+export function isSignalReference(symbol: Symbol): boolean {
+  return (symbol.kind === SymbolKind.Expression || symbol.kind === SymbolKind.Variable) &&
       // Note that `tsType.symbol` isn't optional in the typings,
       // but it appears that it can be undefined at runtime.
       (symbol.tsType.symbol !== undefined && isSignalSymbol(symbol.tsType.symbol) ||

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

@@ -6,11 +6,12 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ImplicitReceiver, PropertyWrite, RecursiveAstVisitor, TmplAstBoundEvent, TmplAstNode, TmplAstRecursiveVisitor, TmplAstVariable} from '@angular/compiler';
+import {AST, ASTWithSource, ImplicitReceiver, ParsedEventType, PropertyRead, PropertyWrite, RecursiveAstVisitor, TmplAstBoundEvent, TmplAstNode, TmplAstRecursiveVisitor, TmplAstVariable} from '@angular/compiler';
 import ts from 'typescript';
 
 import {ErrorCode, ngErrorCode} from '../../../diagnostics';
 import {TemplateDiagnostic, TemplateTypeChecker} from '../../api';
+import {isSignalReference} from '../../src/symbol_util';
 import {TemplateSemanticsChecker} from '../api/api';
 
 export class TemplateSemanticsCheckerImpl implements TemplateSemanticsChecker {
@@ -57,7 +58,15 @@ class ExpressionsSemanticsVisitor extends RecursiveAstVisitor {
 
   override visitPropertyWrite(ast: PropertyWrite, context: TmplAstNode): void {
     super.visitPropertyWrite(ast, context);
+    this.checkForIllegalWriteInEventBinding(ast, context);
+  }
+
+  override visitPropertyRead(ast: PropertyRead, context: TmplAstNode) {
+    super.visitPropertyRead(ast, context);
+    this.checkForIllegalWriteInTwoWayBinding(ast, context);
+  }
 
+  private checkForIllegalWriteInEventBinding(ast: PropertyWrite, context: TmplAstNode) {
     if (!(context instanceof TmplAstBoundEvent) || !(ast.receiver instanceof ImplicitReceiver)) {
       return;
     }
@@ -71,11 +80,33 @@ class ExpressionsSemanticsVisitor extends RecursiveAstVisitor {
     }
   }
 
+  private checkForIllegalWriteInTwoWayBinding(ast: PropertyRead, context: TmplAstNode) {
+    // Only check top-level property reads inside two-way bindings for illegal assignments.
+    if (!(context instanceof TmplAstBoundEvent) || context.type !== ParsedEventType.TwoWay ||
+        !(ast.receiver instanceof ImplicitReceiver) ||
+        ast !== unwrapAstWithSource(context.handler)) {
+      return;
+    }
+
+    const target = this.templateTypeChecker.getExpressionTarget(ast, this.component);
+    if (!(target instanceof TmplAstVariable)) {
+      return;
+    }
+
+    // Two-way bindings to template variables are only allowed if the variables are signals.
+    const symbol = this.templateTypeChecker.getSymbolOfNode(target, this.component);
+    if (symbol !== null && !isSignalReference(symbol)) {
+      const errorMessage = `Cannot use a non-signal variable '${
+          target.name}' in a two-way binding expression. Template variables are read-only.`;
+      this.diagnostics.push(this.makeIllegalTemplateVarDiagnostic(target, context, errorMessage));
+    }
+  }
+
   private makeIllegalTemplateVarDiagnostic(
-      target: TmplAstVariable, expressionNode: TmplAstNode,
+      target: TmplAstVariable, expressionNode: TmplAstBoundEvent,
       errorMessage: string): TemplateDiagnostic {
     return this.templateTypeChecker.makeTemplateDiagnostic(
-        this.component, expressionNode.sourceSpan, ts.DiagnosticCategory.Error,
+        this.component, expressionNode.handlerSpan, 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,
@@ -84,3 +115,7 @@ class ExpressionsSemanticsVisitor extends RecursiveAstVisitor {
         }]);
   }
 }
+
+function unwrapAstWithSource(ast: AST): AST {
+  return ast instanceof ASTWithSource ? ast.ast : ast;
+}

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

@@ -4868,6 +4868,70 @@ suppress
         ]);
       });
 
+      it('should not be able to write to loop template variables in a two-way binding', () => {
+        env.write('test.ts', `
+          import {Component, Directive, Input, Output, EventEmitter} from '@angular/core';
+
+          @Directive({
+            selector: '[twoWayDir]',
+            standalone: true
+          })
+          export class TwoWayDir {
+            @Input() value: number = 0;
+            @Output() valueChanges: EventEmitter<number> = new EventEmitter();
+          }
+
+          @Component({
+            template: \`
+              @for (item of items; track item) {
+                <button twoWayDir [(value)]="$index"></button>
+              }
+            \`,
+            standalone: true,
+            imports: [TwoWayDir]
+          })
+          export class Main {
+            items = [];
+          }
+        `);
+
+        const diags = env.driveDiagnostics();
+        expect(diags.map(d => ts.flattenDiagnosticMessageText(d.messageText, ''))).toEqual([
+          `Cannot use a non-signal variable '$index' in a two-way binding expression. Template variables are read-only.`,
+        ]);
+      });
+
+      it('should allow writes to signal-based template variables in two-way bindings', () => {
+        env.write('test.ts', `
+          import {Component, Directive, Input, Output, EventEmitter, signal} from '@angular/core';
+
+          @Directive({
+            selector: '[twoWayDir]',
+            standalone: true
+          })
+          export class TwoWayDir {
+            @Input() value: number = 0;
+            @Output() valueChanges: EventEmitter<number> = new EventEmitter();
+          }
+
+          @Component({
+            template: \`
+              @for (current of signals; track current) {
+                <button twoWayDir [(value)]="current"></button>
+              }
+            \`,
+            standalone: true,
+            imports: [TwoWayDir]
+          })
+          export class Main {
+            signals = [signal(1)];
+          }
+        `);
+
+        const diags = env.driveDiagnostics();
+        expect(diags).toEqual([]);
+      });
+
       it('should check the track expression of a for loop block', () => {
         env.write('test.ts', `
           import {Component} from '@angular/core';