From 70ad91ed8bac3cacc1c5f8a04bd8196294bb0c1c Mon Sep 17 00:00:00 2001
From: Paul Gschwendtner <paulgschwendtner@gmail.com>
Date: Mon, 17 Jun 2019 11:41:16 +0200
Subject: [PATCH] refactor(compiler): recursive ast expression visitor not
 passing context (#31085)

Currently the `RecursiveAstVisitor` that is part of the template expression
parser does not _always_ properly pass through the context that can be
specified when visting a given expression. Only a handful of AST types
pass through the context while others are accidentally left out. This causes
unexpected and inconsistent behavior and basically makes the `context`
parameter not usable if the type of template expression is not known.

e.g. the template variable assignment migration currently depends on
the `RecursiveAstVisitor` but sometimes breaks if developers use
things like conditionals in their template variable assignments.

Fixes #31043

PR Close #31085
---
 .../compiler/src/expression_parser/ast.ts     | 40 +++++++++----------
 .../template_var_assignment_migration_spec.ts | 24 +++++++++++
 2 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/packages/compiler/src/expression_parser/ast.ts b/packages/compiler/src/expression_parser/ast.ts
index 208cda0ca2..89b8d4669e 100644
--- a/packages/compiler/src/expression_parser/ast.ts
+++ b/packages/compiler/src/expression_parser/ast.ts
@@ -268,24 +268,24 @@ export class NullAstVisitor implements AstVisitor {
 
 export class RecursiveAstVisitor implements AstVisitor {
   visitBinary(ast: Binary, context: any): any {
-    ast.left.visit(this);
-    ast.right.visit(this);
+    ast.left.visit(this, context);
+    ast.right.visit(this, context);
     return null;
   }
   visitChain(ast: Chain, context: any): any { return this.visitAll(ast.expressions, context); }
   visitConditional(ast: Conditional, context: any): any {
-    ast.condition.visit(this);
-    ast.trueExp.visit(this);
-    ast.falseExp.visit(this);
+    ast.condition.visit(this, context);
+    ast.trueExp.visit(this, context);
+    ast.falseExp.visit(this, context);
     return null;
   }
   visitPipe(ast: BindingPipe, context: any): any {
-    ast.exp.visit(this);
+    ast.exp.visit(this, context);
     this.visitAll(ast.args, context);
     return null;
   }
   visitFunctionCall(ast: FunctionCall, context: any): any {
-    ast.target !.visit(this);
+    ast.target !.visit(this, context);
     this.visitAll(ast.args, context);
     return null;
   }
@@ -294,14 +294,14 @@ export class RecursiveAstVisitor implements AstVisitor {
     return this.visitAll(ast.expressions, context);
   }
   visitKeyedRead(ast: KeyedRead, context: any): any {
-    ast.obj.visit(this);
-    ast.key.visit(this);
+    ast.obj.visit(this, context);
+    ast.key.visit(this, context);
     return null;
   }
   visitKeyedWrite(ast: KeyedWrite, context: any): any {
-    ast.obj.visit(this);
-    ast.key.visit(this);
-    ast.value.visit(this);
+    ast.obj.visit(this, context);
+    ast.key.visit(this, context);
+    ast.value.visit(this, context);
     return null;
   }
   visitLiteralArray(ast: LiteralArray, context: any): any {
@@ -310,32 +310,32 @@ export class RecursiveAstVisitor implements AstVisitor {
   visitLiteralMap(ast: LiteralMap, context: any): any { return this.visitAll(ast.values, context); }
   visitLiteralPrimitive(ast: LiteralPrimitive, context: any): any { return null; }
   visitMethodCall(ast: MethodCall, context: any): any {
-    ast.receiver.visit(this);
+    ast.receiver.visit(this, context);
     return this.visitAll(ast.args, context);
   }
   visitPrefixNot(ast: PrefixNot, context: any): any {
-    ast.expression.visit(this);
+    ast.expression.visit(this, context);
     return null;
   }
   visitNonNullAssert(ast: NonNullAssert, context: any): any {
-    ast.expression.visit(this);
+    ast.expression.visit(this, context);
     return null;
   }
   visitPropertyRead(ast: PropertyRead, context: any): any {
-    ast.receiver.visit(this);
+    ast.receiver.visit(this, context);
     return null;
   }
   visitPropertyWrite(ast: PropertyWrite, context: any): any {
-    ast.receiver.visit(this);
-    ast.value.visit(this);
+    ast.receiver.visit(this, context);
+    ast.value.visit(this, context);
     return null;
   }
   visitSafePropertyRead(ast: SafePropertyRead, context: any): any {
-    ast.receiver.visit(this);
+    ast.receiver.visit(this, context);
     return null;
   }
   visitSafeMethodCall(ast: SafeMethodCall, context: any): any {
-    ast.receiver.visit(this);
+    ast.receiver.visit(this, context);
     return this.visitAll(ast.args, context);
   }
   visitAll(asts: AST[], context: any): any {
diff --git a/packages/core/schematics/test/template_var_assignment_migration_spec.ts b/packages/core/schematics/test/template_var_assignment_migration_spec.ts
index f0af511a57..c21a6666b5 100644
--- a/packages/core/schematics/test/template_var_assignment_migration_spec.ts
+++ b/packages/core/schematics/test/template_var_assignment_migration_spec.ts
@@ -211,6 +211,30 @@ describe('template variable assignment migration', () => {
        expect(warnOutput.length).toBe(0);
      });
 
+  it('should warn for template variable assignments in expression conditional', async() => {
+    writeFile('/index.ts', `
+      import {Component} from '@angular/core';
+
+      @Component({
+        templateUrl: './sub_dir/tmpl.html',
+      })
+      export class MyComp {
+        otherVar = false;
+      }
+    `);
+
+    writeFile('/sub_dir/tmpl.html', `
+      <ng-template let-tmplVar>
+        <p (click)="enabled ? tmplVar = true : otherVar = true"></p>
+      </ng-template>
+    `);
+
+    await runMigration();
+
+    expect(warnOutput.length).toBe(1);
+    expect(warnOutput[0]).toMatch(/^⮑ {3}sub_dir\/tmpl.html@3:31: Found assignment/);
+  });
+
   it('should not warn for property writes with template variable name but different scope',
      async() => {
        writeFile('/index.ts', `