fix(ivy): generate a better error for template var writes (#34339)

In Ivy it's illegal for a template to write to a template variable. So the
template:

```html
<ng-template let-somevar>
  <button (click)="somevar = 3">Set var to 3</button>
</ng-template>
```

is erroneous and previously would fail to compile with an assertion error
from the `TemplateDefinitionBuilder`. This error wasn't particularly user-
friendly, though, as it lacked the context of which template or where the
error occurred.

In this commit, a new check in template type-checking is added which detects
such erroneous writes and produces a true diagnostic with the appropriate
context information.

Closes #33674

PR Close #34339

packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts

@@ -95,6 +95,20 @@ export enum ErrorCode {
    */
   MISSING_PIPE = 8004,
 
+  /**
+   * The left-hand side of an assignment expression was a template variable. Effectively, the
+   * template looked like:
+   *
+   * ```
+   * <ng-template let-something>
+   *   <button (click)="something = ...">...</button>
+   * </ng-template>
+   * ```
+   *
+   * Template variables are read-only.
+   */
+  WRITE_TO_READ_ONLY_VARIABLE = 8005,
+
   /**
    * An injectable already has a `ɵprov` property.
    */

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

@@ -158,8 +158,22 @@ export function translateDiagnostic(
  */
 export function makeTemplateDiagnostic(
     mapping: TemplateSourceMapping, span: ParseSourceSpan, category: ts.DiagnosticCategory,
-    code: number, messageText: string | ts.DiagnosticMessageChain): ts.Diagnostic {
+    code: number, messageText: string | ts.DiagnosticMessageChain, relatedMessage?: {
+      text: string,
+      span: ParseSourceSpan,
+    }): ts.Diagnostic {
   if (mapping.type === 'direct') {
+    let relatedInformation: ts.DiagnosticRelatedInformation[]|undefined = undefined;
+    if (relatedMessage !== undefined) {
+      relatedInformation = [{
+        category: ts.DiagnosticCategory.Message,
+        code: 0,
+        file: mapping.node.getSourceFile(),
+        start: relatedMessage.span.start.offset,
+        length: relatedMessage.span.end.offset - relatedMessage.span.start.offset,
+        messageText: relatedMessage.text,
+      }];
+    }
     // For direct mappings, the error is shown inline as ngtsc was able to pinpoint a string
     // constant within the `@Component` decorator for the template. This allows us to map the error
     // directly into the bytes of the source file.
@@ -170,7 +184,7 @@ export function makeTemplateDiagnostic(
       messageText,
       file: mapping.node.getSourceFile(),
       start: span.start.offset,
-      length: span.end.offset - span.start.offset,
+      length: span.end.offset - span.start.offset, relatedInformation,
     };
   } else if (mapping.type === 'indirect' || mapping.type === 'external') {
     // For indirect mappings (template was declared inline, but ngtsc couldn't map it directly
@@ -189,6 +203,29 @@ export function makeTemplateDiagnostic(
     const sf = ts.createSourceFile(
         fileName, mapping.template, ts.ScriptTarget.Latest, false, ts.ScriptKind.JSX);
 
+    let relatedInformation: ts.DiagnosticRelatedInformation[] = [];
+    if (relatedMessage !== undefined) {
+      relatedInformation.push({
+        category: ts.DiagnosticCategory.Message,
+        code: 0,
+        file: sf,
+        start: relatedMessage.span.start.offset,
+        length: relatedMessage.span.end.offset - relatedMessage.span.start.offset,
+        messageText: relatedMessage.text,
+      });
+    }
+
+    relatedInformation.push({
+      category: ts.DiagnosticCategory.Message,
+      code: 0,
+      file: componentSf,
+      // mapping.node represents either the 'template' or 'templateUrl' expression. getStart()
+      // and getEnd() are used because they don't include surrounding whitespace.
+      start: mapping.node.getStart(),
+      length: mapping.node.getEnd() - mapping.node.getStart(),
+      messageText: `Error occurs in the template of component ${componentName}.`,
+    });
+
     return {
       source: 'ngtsc',
       category,
@@ -198,16 +235,7 @@ export function makeTemplateDiagnostic(
       start: span.start.offset,
       length: span.end.offset - span.start.offset,
       // Show a secondary message indicating the component whose template contains the error.
-      relatedInformation: [{
+      relatedInformation,
-        category: ts.DiagnosticCategory.Message,
-        code: 0,
-        file: componentSf,
-        // mapping.node represents either the 'template' or 'templateUrl' expression. getStart()
-        // and getEnd() are used because they don't include surrounding whitespace.
-        start: mapping.node.getStart(),
-        length: mapping.node.getEnd() - mapping.node.getStart(),
-        messageText: `Error occurs in the template of component ${componentName}.`,
-      }],
     };
   } else {
     throw new Error(`Unexpected source mapping type: ${(mapping as {type: string}).type}`);

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

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {AbsoluteSourceSpan, BindingPipe, TmplAstReference} from '@angular/compiler';
+import {AbsoluteSourceSpan, BindingPipe, PropertyWrite, TmplAstReference, TmplAstVariable} from '@angular/compiler';
 import * as ts from 'typescript';
 
 import {ErrorCode, ngErrorCode} from '../../diagnostics';
@@ -49,6 +49,10 @@ export interface OutOfBandDiagnosticRecorder {
    * plus span of the larger expression context.
    */
   missingPipe(templateId: string, ast: BindingPipe, sourceSpan: AbsoluteSourceSpan): void;
+
+  illegalAssignmentToTemplateVar(
+      templateId: string, assignment: PropertyWrite, assignmentSpan: AbsoluteSourceSpan,
+      target: TmplAstVariable): void;
 }
 
 export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecorder {
@@ -82,4 +86,24 @@ export class OutOfBandDiagnosticRecorderImpl implements OutOfBandDiagnosticRecor
         mapping, sourceSpan, ts.DiagnosticCategory.Error, ngErrorCode(ErrorCode.MISSING_PIPE),
         errorMsg));
   }
+
+  illegalAssignmentToTemplateVar(
+      templateId: string, assignment: PropertyWrite, assignmentSpan: AbsoluteSourceSpan,
+      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 location = absoluteSourceSpanToSourceLocation(templateId, assignmentSpan);
+    const sourceSpan = this.resolver.sourceLocationToSpan(location);
+    if (sourceSpan === null) {
+      throw new Error(`Assertion failure: no SourceLocation found for property binding.`);
+    }
+    this._diagnostics.push(makeTemplateDiagnostic(
+        mapping, sourceSpan, ts.DiagnosticCategory.Error,
+        ngErrorCode(ErrorCode.WRITE_TO_READ_ONLY_VARIABLE), errorMsg, {
+          text: `The variable ${assignment.name} is declared here.`,
+          span: target.valueSpan || target.sourceSpan,
+        }));
+  }
 }

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

@@ -0,0 +1,44 @@
+/**
+ * @license
+ * Copyright Google Inc. 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, ParseSourceSpan, PropertyWrite, RecursiveAstVisitor, TmplAstVariable} from '@angular/compiler';
+
+import {toAbsoluteSpan} from './diagnostics';
+import {OutOfBandDiagnosticRecorder} from './oob';
+
+/**
+ * Visits a template and records any semantic errors within its expressions.
+ */
+export class ExpressionSemanticVisitor extends RecursiveAstVisitor {
+  constructor(
+      private templateId: string, private boundTarget: BoundTarget<any>,
+      private oob: OutOfBandDiagnosticRecorder, private sourceSpan: ParseSourceSpan) {
+    super();
+  }
+
+  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.
+      const astSpan = toAbsoluteSpan(ast.span, this.sourceSpan);
+      this.oob.illegalAssignmentToTemplateVar(this.templateId, ast, astSpan, target);
+    }
+  }
+
+  static visit(
+      ast: AST, sourceSpan: ParseSourceSpan, id: string, boundTarget: BoundTarget<any>,
+      oob: OutOfBandDiagnosticRecorder): void {
+    ast.visit(new ExpressionSemanticVisitor(id, boundTarget, oob, sourceSpan));
+  }
+}

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, DYNAMIC_TYPE, ImplicitReceiver, MethodCall, ParseSourceSpan, ParseSpan, ParsedEventType, PropertyRead, SchemaMetadata, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler';
+import {AST, BindingPipe, BindingType, BoundTarget, DYNAMIC_TYPE, ImplicitReceiver, MethodCall, ParseSourceSpan, ParseSpan, ParsedEventType, PropertyRead, PropertyWrite, SchemaMetadata, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstElement, TmplAstNode, TmplAstReference, TmplAstTemplate, TmplAstTextAttribute, TmplAstVariable} from '@angular/compiler';
 import * as ts from 'typescript';
 
 import {Reference} from '../../imports';
@@ -18,6 +18,7 @@ import {DomSchemaChecker} from './dom';
 import {Environment} from './environment';
 import {NULL_AS_ANY, astToTypescript} from './expression';
 import {OutOfBandDiagnosticRecorder} from './oob';
+import {ExpressionSemanticVisitor} from './template_semantics';
 import {checkIfClassIsExported, checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateVariable, tsDeclareVariable} from './ts_util';
 
 
@@ -491,6 +492,10 @@ class TcbDirectiveOutputsOp extends TcbOp {
         const handler = tcbCreateEventHandler(output, this.tcb, this.scope, EventParamType.Any);
         this.scope.addStatement(ts.createExpressionStatement(handler));
       }
+
+      ExpressionSemanticVisitor.visit(
+          output.handler, output.handlerSpan, this.tcb.id, this.tcb.boundTarget,
+          this.tcb.oobRecorder);
     }
 
     return null;
@@ -549,6 +554,10 @@ class TcbUnclaimedOutputsOp extends TcbOp {
         const handler = tcbCreateEventHandler(output, this.tcb, this.scope, EventParamType.Any);
         this.scope.addStatement(ts.createExpressionStatement(handler));
       }
+
+      ExpressionSemanticVisitor.visit(
+          output.handler, output.handlerSpan, this.tcb.id, this.tcb.boundTarget,
+          this.tcb.oobRecorder);
     }
 
     return null;
@@ -972,6 +981,16 @@ class TcbExpressionTranslator {
       // returned here to let it fall through resolution so it will be caught when the
       // `ImplicitReceiver` is resolved in the branch below.
       return this.resolveTarget(ast);
+    } else if (ast instanceof PropertyWrite && ast.receiver instanceof ImplicitReceiver) {
+      const target = this.resolveTarget(ast);
+      if (target === null) {
+        return null;
+      }
+
+      const expr = this.translate(ast.value);
+      const result = ts.createParen(ts.createBinary(target, ts.SyntaxKind.EqualsToken, expr));
+      addParseSpanInfo(result, toAbsoluteSpan(ast.span, this.sourceSpan));
+      return result;
     } else if (ast instanceof ImplicitReceiver) {
       // AST instances representing variables and references look very similar to property reads
       // or method calls from the component context: both have the shape

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

@@ -389,4 +389,5 @@ export class NoopOobRecorder implements OutOfBandDiagnosticRecorder {
   get diagnostics(): ReadonlyArray<ts.Diagnostic> { return []; }
   missingReferenceTarget(): void {}
   missingPipe(): void {}
+  illegalAssignmentToTemplateVar(): void {}
 }

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

@@ -280,6 +280,12 @@ describe('type check blocks', () => {
           '_t1.addEventListener("event", ($event): any => (ctx).foo(($event as any)));');
     });
 
+    it('should detect writes to template variables', () => {
+      const TEMPLATE = `<ng-template let-v><div (event)="v = 3"></div></ng-template>`;
+      const block = tcb(TEMPLATE);
+      expect(block).toContain('_t3.addEventListener("event", ($event): any => (_t2 = 3))');
+    });
+
   });
 
   describe('config', () => {

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

@@ -1121,6 +1121,34 @@ export declare class AnimationEvent {
       expect(getSourceCodeForDiagnostic(diags[2])).toEqual('[fromChild]="4"');
     });
 
+    it('should detect an illegal write to a template variable', () => {
+      env.write('test.ts', `
+        import {Component, NgModule} from '@angular/core';
+        import {CommonModule} from '@angular/common';
+
+        @Component({
+          selector: 'test',
+          template: \`
+            <div *ngIf="x as y">
+              <button (click)="y = !y">Toggle</button>
+            </div>
+          \`,
+        })
+        export class TestCmp {
+          x!: boolean;
+        }
+
+        @NgModule({
+          declarations: [TestCmp],
+          imports: [CommonModule],
+        })
+        export class Module {}
+      `);
+      const diags = env.driveDiagnostics();
+      expect(diags.length).toEqual(1);
+      expect(getSourceCodeForDiagnostic(diags[0])).toEqual('y = !y');
+    });
+
     describe('input coercion', () => {
       beforeEach(() => {
         env.tsconfig({fullTemplateTypeCheck: true, strictInputTypes: true});