refactor(core): template-var-assignment migration incorrectly warns (#30026)

Currently the `template-var-assignment` migration incorrectly warns if
the template writes to a property in the component that has the same
`ast.PropertyWrite´ name as a template input variable but different
receiver. e.g.

```html
<!-- "someProp.element" will be incorrectly reported as template variable assignment -->
<button *ngFor="let element of list" (click)="someProp.element = null">Reset</button>
```

Similarly if an output writes to a component property with the same name as a
template input variable, but the expression is within a different template scope,
the schematic currently incorrectly warns. e.g.

```html
<button *ngFor="let element of list">{{element}}</button>

<!-- The "element = null" expression does not refer to the "element" template input variable -->
<button (click)="element = null"></button>
```

PR Close #30026

packages/core/schematics/migrations/template-var-assignment/analyze_template.ts

@@ -7,12 +7,10 @@
  */
 
 import {PropertyWrite} from '@angular/compiler';
-import {Variable, visitAll} from '@angular/compiler/src/render3/r3_ast';
+import {visitAll} from '@angular/compiler/src/render3/r3_ast';
-
 import {ResolvedTemplate} from '../../utils/ng_component_template';
 import {parseHtmlGracefully} from '../../utils/parse_html';
-
+import {HtmlVariableAssignmentVisitor} from './angular/html_variable_assignment_visitor';
-import {PropertyAssignment, PropertyWriteHtmlVisitor} from './angular/property_write_html_visitor';
 
 export interface TemplateVariableAssignment {
   node: PropertyWrite;
@@ -32,20 +30,11 @@ export function analyzeResolvedTemplate(template: ResolvedTemplate): TemplateVar
     return null;
   }
 
-  const visitor = new PropertyWriteHtmlVisitor();
+  const visitor = new HtmlVariableAssignmentVisitor();
 
-  // Analyze the Angular Render3 HTML AST and collect all property assignments and
+  // Analyze the Angular Render3 HTML AST and collect all template variable assignments.
-  // template variables.
   visitAll(visitor, templateNodes);
 
-  return filterTemplateVariableAssignments(visitor.propertyAssignments, visitor.templateVariables)
+  return visitor.variableAssignments.map(
-      .map(({node, start, end}) => ({node, start: start + node.span.start, end}));
+      ({node, start, end}) => ({node, start: start + node.span.start, end}));
-}
-
-/**
- * Returns all template variable assignments by looking if a given property
- * assignment is setting the value for one of the specified template variables.
- */
-function filterTemplateVariableAssignments(writes: PropertyAssignment[], variables: Variable[]) {
-  return writes.filter(propertyWrite => !!variables.find(v => v.name === propertyWrite.node.name));
 }

packages/core/schematics/migrations/template-var-assignment/angular/html_variable_assignment_visitor.ts

@@ -0,0 +1,79 @@
+/**
+ * @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 {ImplicitReceiver, ParseSourceSpan, PropertyWrite, RecursiveAstVisitor} from '@angular/compiler';
+import {BoundEvent, Element, NullVisitor, Template, Variable, visitAll} from '@angular/compiler/src/render3/r3_ast';
+
+export interface TemplateVariableAssignment {
+  start: number;
+  end: number;
+  node: PropertyWrite;
+}
+
+/**
+ * HTML AST visitor that traverses the Render3 HTML AST in order to find all
+ * expressions that write to local template variables within bound events.
+ */
+export class HtmlVariableAssignmentVisitor extends NullVisitor {
+  variableAssignments: TemplateVariableAssignment[] = [];
+
+  private currentVariables: Variable[] = [];
+  private expressionAstVisitor =
+      new ExpressionAstVisitor(this.variableAssignments, this.currentVariables);
+
+  visitElement(element: Element): void {
+    visitAll(this, element.outputs);
+    visitAll(this, element.children);
+  }
+
+  visitTemplate(template: Template): void {
+    // Keep track of the template variables which can be accessed by the template
+    // child nodes through the implicit receiver.
+    this.currentVariables.push(...template.variables);
+
+    // Visit all children of the template. The template proxies the outputs of the
+    // immediate child elements, so we just ignore outputs on the "Template" in order
+    // to not visit similar bound events twice.
+    visitAll(this, template.children);
+
+    // Remove all previously added variables since all children that could access
+    // these have been visited already.
+    template.variables.forEach(v => {
+      const variableIdx = this.currentVariables.indexOf(v);
+
+      if (variableIdx !== -1) {
+        this.currentVariables.splice(variableIdx, 1);
+      }
+    });
+  }
+
+  visitBoundEvent(node: BoundEvent) {
+    node.handler.visit(this.expressionAstVisitor, node.handlerSpan);
+  }
+}
+
+/** AST visitor that resolves all variable assignments within a given expression AST. */
+class ExpressionAstVisitor extends RecursiveAstVisitor {
+  constructor(
+      private variableAssignments: TemplateVariableAssignment[],
+      private currentVariables: Variable[]) {
+    super();
+  }
+
+  visitPropertyWrite(node: PropertyWrite, span: ParseSourceSpan) {
+    if (node.receiver instanceof ImplicitReceiver &&
+        this.currentVariables.some(v => v.name === node.name)) {
+      this.variableAssignments.push({
+        node: node,
+        start: span.start.offset,
+        end: span.end.offset,
+      });
+    }
+    super.visitPropertyWrite(node, span);
+  }
+}

packages/core/schematics/migrations/template-var-assignment/angular/property_write_html_visitor.ts

@@ -1,61 +0,0 @@
-/**
- * @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 {ParseSourceSpan, PropertyWrite, RecursiveAstVisitor} from '@angular/compiler';
-import {BoundEvent, Element, NullVisitor, Template, Variable, visitAll} from '@angular/compiler/src/render3/r3_ast';
-
-export interface PropertyAssignment {
-  start: number;
-  end: number;
-  node: PropertyWrite;
-}
-
-/**
- * AST visitor that traverses the Render3 HTML AST in order to find all declared
- * template variables and property assignments within bound events.
- */
-export class PropertyWriteHtmlVisitor extends NullVisitor {
-  templateVariables: Variable[] = [];
-  propertyAssignments: PropertyAssignment[] = [];
-
-  private expressionAstVisitor = new ExpressionAstVisitor(this.propertyAssignments);
-
-  visitElement(element: Element): void {
-    visitAll(this, element.outputs);
-    visitAll(this, element.children);
-  }
-
-  visitTemplate(template: Template): void {
-    // Visit all children of the template. The template proxies the outputs of the
-    // immediate child elements, so we just ignore outputs on the "Template" in order
-    // to not visit similar bound events twice.
-    visitAll(this, template.children);
-
-    // Keep track of all declared local template variables.
-    this.templateVariables.push(...template.variables);
-  }
-
-  visitBoundEvent(node: BoundEvent) {
-    node.handler.visit(this.expressionAstVisitor, node.handlerSpan);
-  }
-}
-
-/** AST visitor that resolves all property assignments with a given expression AST. */
-class ExpressionAstVisitor extends RecursiveAstVisitor {
-  constructor(private propertyAssignments: PropertyAssignment[]) { super(); }
-
-  visitPropertyWrite(node: PropertyWrite, span: ParseSourceSpan) {
-    this.propertyAssignments.push({
-      node: node,
-      start: span.start.offset,
-      end: span.end.offset,
-    });
-
-    super.visitPropertyWrite(node, span);
-  }
-}

packages/core/schematics/test/template_var_assignment_migration_spec.ts

@@ -181,6 +181,55 @@ describe('template variable assignment migration', () => {
     expect(warnOutput.length).toBe(0);
   });
 
+  it('should not warn for property writes with template variable name but different receiver',
+     () => {
+       writeFile('/index.ts', `
+      import {Component} from '@angular/core';
+      
+      @Component({
+        templateUrl: './sub_dir/tmpl.html',
+      })
+      export class MyComp {
+        someProp = {
+          element: 'someValue',
+        };
+      }
+    `);
+
+       writeFile('/sub_dir/tmpl.html', `
+      <button *ngFor="let element of list" (click)="someProp.element = null">
+        Reset
+      </button>
+    `);
+
+       runMigration();
+
+       expect(warnOutput.length).toBe(0);
+     });
+
+  it('should not warn for property writes with template variable name but different scope', () => {
+    writeFile('/index.ts', `
+      import {Component} from '@angular/core';
+      
+      @Component({
+        templateUrl: './sub_dir/tmpl.html',
+      })
+      export class MyComp {
+        element = 'someValue';
+      }
+    `);
+
+    writeFile('/sub_dir/tmpl.html', `
+      <button *ngFor="let element of list">{{element}}</button>
+      <button (click)="element = null"></button>
+    `);
+
+    runMigration();
+
+    expect(warnOutput.length).toBe(0);
+  });
+
+
   it('should not throw an error if a detected template fails parsing', () => {
     writeFile('/index.ts', `
       import {Component} from '@angular/core';