fix(compiler): fix where pipes live

Impure pipes need to live on the view
that used them and need a new instance for
each call site.

Impure pipes need to live on the component view, cached across all child views,
and need a new pure proxy for each for
each call site that lives on the view
of the call site.

Fixes #8408

This bug was introduced not long ago by 152a117

Author: tbosch

modules/@angular/compiler/src/view_compiler/compile_pipe.ts

@@ -7,17 +7,35 @@ import {Identifiers, identifierToken} from '../identifiers';
 import {injectFromViewParentInjector, createPureProxy, getPropertyInView} from './util';
 
 class _PurePipeProxy {
-  constructor(public instance: o.ReadPropExpr, public argCount: number) {}
+  constructor(public view: CompileView, public instance: o.ReadPropExpr, public argCount: number) {}
 }
 
 export class CompilePipe {
-  meta: CompilePipeMetadata;
+  static call(view: CompileView, name: string, args: o.Expression[]): o.Expression {
+    var compView = view.componentView;
+    var meta = _findPipeMeta(compView, name);
+    var pipe: CompilePipe;
+    if (meta.pure) {
+      // pure pipes live on the component view
+      pipe = compView.purePipes.get(name);
+      if (isBlank(pipe)) {
+        pipe = new CompilePipe(compView, meta);
+        compView.purePipes.set(name, pipe);
+        compView.pipes.push(pipe);
+      }
+    } else {
+      // Non pure pipes live on the view that called it
+      pipe = new CompilePipe(view, meta);
+      view.pipes.push(pipe);
+    }
+    return pipe._call(view, args);
+  }
+
   instance: o.ReadPropExpr;
   private _purePipeProxies: _PurePipeProxy[] = [];
 
-  constructor(public view: CompileView, name: string) {
-    this.meta = _findPipeMeta(view, name);
-    this.instance = o.THIS_EXPR.prop(`_pipe_${name}_${view.pipeCount++}`);
+  constructor(public view: CompileView, public meta: CompilePipeMetadata) {
+    this.instance = o.THIS_EXPR.prop(`_pipe_${meta.name}_${view.pipeCount++}`);
   }
 
   get pure(): boolean { return this.meta.pure; }
@@ -35,29 +53,33 @@ export class CompilePipe {
                                        .set(o.importExpr(this.meta.type).instantiate(deps))
                                        .toStmt());
     this._purePipeProxies.forEach((purePipeProxy) => {
-      createPureProxy(
-          this.instance.prop('transform').callMethod(o.BuiltinMethod.bind, [this.instance]),
-          purePipeProxy.argCount, purePipeProxy.instance, this.view);
+      var pipeInstanceSeenFromPureProxy =
+          getPropertyInView(this.instance, purePipeProxy.view, this.view);
+      createPureProxy(pipeInstanceSeenFromPureProxy.prop('transform')
+                          .callMethod(o.BuiltinMethod.bind, [pipeInstanceSeenFromPureProxy]),
+                      purePipeProxy.argCount, purePipeProxy.instance, purePipeProxy.view);
     });
   }
 
-  call(callingView: CompileView, args: o.Expression[]): o.Expression {
+  private _call(callingView: CompileView, args: o.Expression[]): o.Expression {
     if (this.meta.pure) {
+      // PurePipeProxies live on the view that called them.
       var purePipeProxy = new _PurePipeProxy(
-          o.THIS_EXPR.prop(`${this.instance.name}_${this._purePipeProxies.length}`), args.length);
+          callingView, o.THIS_EXPR.prop(`${this.instance.name}_${this._purePipeProxies.length}`),
+          args.length);
       this._purePipeProxies.push(purePipeProxy);
-      return getPropertyInView(
-                 o.importExpr(Identifiers.castByValue)
-                     .callFn([purePipeProxy.instance, this.instance.prop('transform')]),
-                 callingView, this.view)
+      return o.importExpr(Identifiers.castByValue)
+          .callFn([
+            purePipeProxy.instance,
+            getPropertyInView(this.instance.prop('transform'), callingView, this.view)
+          ])
           .callFn(args);
     } else {
       return getPropertyInView(this.instance, callingView, this.view).callMethod('transform', args);
     }
   }
 }
 
-
 function _findPipeMeta(view: CompileView, name: string): CompilePipeMetadata {
   var pipeMeta: CompilePipeMetadata = null;
   for (var i = view.pipeMetas.length - 1; i >= 0; i--) {

modules/@angular/compiler/src/view_compiler/compile_view.ts

@@ -127,16 +127,7 @@ export class CompileView implements NameResolver {
   }
 
   callPipe(name: string, input: o.Expression, args: o.Expression[]): o.Expression {
-    var compView = this.componentView;
-    var pipe = compView.purePipes.get(name);
-    if (isBlank(pipe)) {
-      pipe = new CompilePipe(compView, name);
-      if (pipe.pure) {
-        compView.purePipes.set(name, pipe);
-      }
-      compView.pipes.push(pipe);
-    }
-    return pipe.call(this, [input].concat(args));
+    return CompilePipe.call(this, name, [input].concat(args));
   }
 
   getLocal(name: string): o.Expression {

modules/@angular/core/test/linker/change_detection_integration_spec.ts

@@ -68,6 +68,7 @@ import {
   AfterViewInit,
   AfterViewChecked
 } from '@angular/core';
+import {NgFor} from '@angular/common';
 import {By} from '@angular/platform-browser/src/dom/debug/by';
 import {AsyncPipe} from '@angular/common';
 
@@ -544,16 +545,23 @@ export function main() {
 
         it('should call pure pipes that are used multiple times only when the arguments change',
            fakeAsync(() => {
-             var ctx = createCompFixture(`<div [someProp]="name | countingPipe"></div><div [someProp]="age | countingPipe"></div>`, Person);
+             var ctx = createCompFixture(
+                 `<div [someProp]="name | countingPipe"></div><div [someProp]="age | countingPipe"></div>` +
+                     '<div *ngFor="let x of [1,2]" [someProp]="address.city | countingPipe"></div>',
+                 Person);
              ctx.componentInstance.name = 'a';
              ctx.componentInstance.age = 10;
+             ctx.componentInstance.address = new Address('mtv');
              ctx.detectChanges(false);
-             expect(renderLog.loggedValues).toEqual(['a state:0', '10 state:1']);
+             expect(renderLog.loggedValues)
+                 .toEqual(['mtv state:0', 'mtv state:1', 'a state:2', '10 state:3']);
              ctx.detectChanges(false);
-             expect(renderLog.loggedValues).toEqual(['a state:0', '10 state:1']);
+             expect(renderLog.loggedValues)
+                 .toEqual(['mtv state:0', 'mtv state:1', 'a state:2', '10 state:3']);
              ctx.componentInstance.age = 11;
              ctx.detectChanges(false);
-             expect(renderLog.loggedValues).toEqual(['a state:0', '10 state:1', '11 state:2']);
+             expect(renderLog.loggedValues)
+                 .toEqual(['mtv state:0', 'mtv state:1', 'a state:2', '10 state:3', '11 state:4']);
            }));
 
         it('should call impure pipes on each change detection run', fakeAsync(() => {
@@ -1098,6 +1106,7 @@ const ALL_DIRECTIVES = /*@ts2dart_const*/[
   forwardRef(() => OrderCheckDirective2),
   forwardRef(() => OrderCheckDirective0),
   forwardRef(() => OrderCheckDirective1),
+  NgFor
 ];
 
 const ALL_PIPES = /*@ts2dart_const*/[

modules/@angular/core/test/linker/view_injector_integration_spec.ts

@@ -37,7 +37,7 @@ import {
   Host,
   SkipSelfMetadata
 } from '@angular/core';
-import {NgIf} from '@angular/common';
+import {NgIf, NgFor} from '@angular/common';
 import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter';
 
 const ALL_DIRECTIVES = /*@ts2dart_const*/[
@@ -71,7 +71,8 @@ const ALL_DIRECTIVES = /*@ts2dart_const*/[
   forwardRef(() => DirectiveNeedsChangeDetectorRef),
   forwardRef(() => PushComponentNeedsChangeDetectorRef),
   forwardRef(() => NeedsHostAppService),
-  NgIf
+  NgIf,
+  NgFor
 ];
 
 const ALL_PIPES = /*@ts2dart_const*/[
@@ -670,23 +671,35 @@ export function main() {
 
       it('should cache pure pipes', fakeAsync(() => {
            var el = createComp(
-               '<div [simpleDirective]="true | purePipe"></div><div *ngIf="true" [simpleDirective]="true | purePipe"></div>',
+               '<div [simpleDirective]="true | purePipe"></div><div [simpleDirective]="true | purePipe"></div>' +
+                   '<div *ngFor="let x of [1,2]" [simpleDirective]="true | purePipe"></div>',
                tcb);
            var purePipe1 = el.children[0].inject(SimpleDirective).value;
            var purePipe2 = el.children[1].inject(SimpleDirective).value;
+           var purePipe3 = el.children[2].inject(SimpleDirective).value;
+           var purePipe4 = el.children[3].inject(SimpleDirective).value;
            expect(purePipe1).toBeAnInstanceOf(PurePipe);
-           expect(purePipe1).toBe(purePipe2);
+           expect(purePipe2).toBe(purePipe1);
+           expect(purePipe3).toBe(purePipe1);
+           expect(purePipe4).toBe(purePipe1);
          }));
 
-      it('should not cache pure pipes', fakeAsync(() => {
+      it('should not cache impure pipes', fakeAsync(() => {
            var el = createComp(
-               '<div [simpleDirective]="true | impurePipe"></div><div [simpleDirective]="true | impurePipe"></div>',
+               '<div [simpleDirective]="true | impurePipe"></div><div [simpleDirective]="true | impurePipe"></div>' +
+                   '<div *ngFor="let x of [1,2]" [simpleDirective]="true | impurePipe"></div>',
                tcb);
            var purePipe1 = el.children[0].inject(SimpleDirective).value;
            var purePipe2 = el.children[1].inject(SimpleDirective).value;
+           var purePipe3 = el.children[2].inject(SimpleDirective).value;
+           var purePipe4 = el.children[3].inject(SimpleDirective).value;
            expect(purePipe1).toBeAnInstanceOf(ImpurePipe);
            expect(purePipe2).toBeAnInstanceOf(ImpurePipe);
-           expect(purePipe1).not.toBe(purePipe2);
+           expect(purePipe2).not.toBe(purePipe1);
+           expect(purePipe3).toBeAnInstanceOf(ImpurePipe);
+           expect(purePipe3).not.toBe(purePipe1);
+           expect(purePipe4).toBeAnInstanceOf(ImpurePipe);
+           expect(purePipe4).not.toBe(purePipe1);
          }));
     });
   });