diff --git a/modules/@angular/compiler/src/view_compiler/event_binder.ts b/modules/@angular/compiler/src/view_compiler/event_binder.ts index f48d537f5d..3ac90a88a0 100644 --- a/modules/@angular/compiler/src/view_compiler/event_binder.ts +++ b/modules/@angular/compiler/src/view_compiler/event_binder.ts @@ -59,7 +59,8 @@ export class CompileEventListener { this._method.resetDebugInfo(this.compileElement.nodeIndex, hostEvent); var context = isPresent(directiveInstance) ? directiveInstance : this.compileElement.view.componentContext; - var actionStmts = convertCdStatementToIr(this.compileElement.view, context, hostEvent.handler); + var actionStmts = convertCdStatementToIr( + this.compileElement.view, context, hostEvent.handler, this.compileElement.nodeIndex); var lastIndex = actionStmts.length - 1; if (lastIndex >= 0) { var lastStatement = actionStmts[lastIndex]; diff --git a/modules/@angular/compiler/src/view_compiler/expression_converter.ts b/modules/@angular/compiler/src/view_compiler/expression_converter.ts index f66dbe3951..3d00c9108e 100644 --- a/modules/@angular/compiler/src/view_compiler/expression_converter.ts +++ b/modules/@angular/compiler/src/view_compiler/expression_converter.ts @@ -21,25 +21,45 @@ export interface NameResolver { } export class ExpressionWithWrappedValueInfo { - constructor(public expression: o.Expression, public needsValueUnwrapper: boolean) {} + constructor( + public expression: o.Expression, public needsValueUnwrapper: boolean, + public temporaryCount: number) {} } export function convertCdExpressionToIr( nameResolver: NameResolver, implicitReceiver: o.Expression, expression: cdAst.AST, - valueUnwrapper: o.ReadVarExpr): ExpressionWithWrappedValueInfo { - const visitor = new _AstToIrVisitor(nameResolver, implicitReceiver, valueUnwrapper); + valueUnwrapper: o.ReadVarExpr, bindingIndex: number): ExpressionWithWrappedValueInfo { + const visitor = new _AstToIrVisitor(nameResolver, implicitReceiver, valueUnwrapper, bindingIndex); const irAst: o.Expression = expression.visit(visitor, _Mode.Expression); - return new ExpressionWithWrappedValueInfo(irAst, visitor.needsValueUnwrapper); + return new ExpressionWithWrappedValueInfo( + irAst, visitor.needsValueUnwrapper, visitor.temporaryCount); } export function convertCdStatementToIr( - nameResolver: NameResolver, implicitReceiver: o.Expression, stmt: cdAst.AST): o.Statement[] { - const visitor = new _AstToIrVisitor(nameResolver, implicitReceiver, null); + nameResolver: NameResolver, implicitReceiver: o.Expression, stmt: cdAst.AST, + bindingIndex: number): o.Statement[] { + const visitor = new _AstToIrVisitor(nameResolver, implicitReceiver, null, bindingIndex); let statements: o.Statement[] = []; flattenStatements(stmt.visit(visitor, _Mode.Statement), statements); + prependTemporaryDecls(visitor.temporaryCount, bindingIndex, statements); return statements; } +function temporaryName(bindingIndex: number, temporaryNumber: number): string { + return `tmp_${bindingIndex}_${temporaryNumber}`; +} + +export function temporaryDeclaration(bindingIndex: number, temporaryNumber: number): o.Statement { + return new o.DeclareVarStmt(temporaryName(bindingIndex, temporaryNumber), o.NULL_EXPR); +} + +function prependTemporaryDecls( + temporaryCount: number, bindingIndex: number, statements: o.Statement[]) { + for (let i = temporaryCount - 1; i >= 0; i--) { + statements.unshift(temporaryDeclaration(bindingIndex, i)); + } +} + enum _Mode { Statement, Expression @@ -66,13 +86,15 @@ function convertToStatementIfNeeded(mode: _Mode, expr: o.Expression): o.Expressi } class _AstToIrVisitor implements cdAst.AstVisitor { - private _map = new Map(); - + private _nodeMap = new Map(); + private _resultMap = new Map(); + private _currentTemporary: number = 0; public needsValueUnwrapper: boolean = false; + public temporaryCount: number = 0; constructor( private _nameResolver: NameResolver, private _implicitReceiver: o.Expression, - private _valueUnwrapper: o.ReadVarExpr) {} + private _valueUnwrapper: o.ReadVarExpr, private bindingIndex: number) {} visitBinary(ast: cdAst.Binary, mode: _Mode): any { var op: o.BinaryOperator; @@ -273,7 +295,9 @@ class _AstToIrVisitor implements cdAst.AstVisitor { } private visit(ast: cdAst.AST, mode: _Mode): any { - return (this._map.get(ast) || ast).visit(this, mode); + const result = this._resultMap.get(ast); + if (result) return result; + return (this._nodeMap.get(ast) || ast).visit(this, mode); } private convertSafeAccess( @@ -317,17 +341,30 @@ class _AstToIrVisitor implements cdAst.AstVisitor { // Notice that the first guard condition is the left hand of the left most safe access node // which comes in as leftMostSafe to this routine. - const condition = this.visit(leftMostSafe.receiver, mode).isBlank(); + let guardedExpression = this.visit(leftMostSafe.receiver, mode); + let temporary: o.ReadVarExpr; + if (this.needsTemporary(leftMostSafe.receiver)) { + // If the expression has method calls or pipes then we need to save the result into a + // temporary variable to avoid calling stateful or impure code more than once. + temporary = this.allocateTemporary(); + + // Preserve the result in the temporary variable + guardedExpression = temporary.set(guardedExpression); + + // Ensure all further references to the guarded expression refer to the temporary instead. + this._resultMap.set(leftMostSafe.receiver, temporary); + } + const condition = guardedExpression.isBlank(); // Convert the ast to an unguarded access to the receiver's member. The map will substitute // leftMostNode with its unguarded version in the call to `this.visit()`. if (leftMostSafe instanceof cdAst.SafeMethodCall) { - this._map.set( + this._nodeMap.set( leftMostSafe, new cdAst.MethodCall( leftMostSafe.span, leftMostSafe.receiver, leftMostSafe.name, leftMostSafe.args)); } else { - this._map.set( + this._nodeMap.set( leftMostSafe, new cdAst.PropertyRead(leftMostSafe.span, leftMostSafe.receiver, leftMostSafe.name)); } @@ -337,7 +374,12 @@ class _AstToIrVisitor implements cdAst.AstVisitor { // Remove the mapping. This is not strictly required as the converter only traverses each node // once but is safer if the conversion is changed to traverse the nodes more than once. - this._map.delete(leftMostSafe); + this._nodeMap.delete(leftMostSafe); + + // If we allcoated a temporary, release it. + if (temporary) { + this.releaseTemporary(temporary); + } // Produce the conditional return condition.conditional(o.literal(null), access); @@ -351,8 +393,8 @@ class _AstToIrVisitor implements cdAst.AstVisitor { // then to: // a == null ? null : a.b.c == null ? null : a.b.c.d.e private leftMostSafeNode(ast: cdAst.AST): cdAst.SafePropertyRead|cdAst.SafeMethodCall { - let visit = (visitor: cdAst.AstVisitor, ast: cdAst.AST): any => { - return (this._map.get(ast) || ast).visit(visitor); + const visit = (visitor: cdAst.AstVisitor, ast: cdAst.AST): any => { + return (this._nodeMap.get(ast) || ast).visit(visitor); }; return ast.visit({ visitBinary(ast: cdAst.Binary) { return null; }, @@ -378,6 +420,55 @@ class _AstToIrVisitor implements cdAst.AstVisitor { } }); } + + // Returns true of the AST includes a method or a pipe indicating that, if the + // expression is used as the target of a safe property or method access then + // the expression should be stored into a temporary variable. + private needsTemporary(ast: cdAst.AST): boolean { + const visit = (visitor: cdAst.AstVisitor, ast: cdAst.AST): boolean => { + return ast && (this._nodeMap.get(ast) || ast).visit(visitor); + }; + const visitSome = (visitor: cdAst.AstVisitor, ast: cdAst.AST[]): boolean => { + return ast.some(ast => visit(visitor, ast)); + }; + return ast.visit({ + visitBinary(ast: cdAst.Binary): + boolean{return visit(this, ast.left) || visit(this, ast.right);}, + visitChain(ast: cdAst.Chain) { return false; }, + visitConditional(ast: cdAst.Conditional): + boolean{return visit(this, ast.condition) || visit(this, ast.trueExp) || + visit(this, ast.falseExp);}, + visitFunctionCall(ast: cdAst.FunctionCall) { return true; }, + visitImplicitReceiver(ast: cdAst.ImplicitReceiver) { return false; }, + visitInterpolation(ast: cdAst.Interpolation) { return visitSome(this, ast.expressions); }, + visitKeyedRead(ast: cdAst.KeyedRead) { return false; }, + visitKeyedWrite(ast: cdAst.KeyedWrite) { return false; }, + visitLiteralArray(ast: cdAst.LiteralArray) { return true; }, + visitLiteralMap(ast: cdAst.LiteralMap) { return true; }, + visitLiteralPrimitive(ast: cdAst.LiteralPrimitive) { return false; }, + visitMethodCall(ast: cdAst.MethodCall) { return true; }, + visitPipe(ast: cdAst.BindingPipe) { return true; }, + visitPrefixNot(ast: cdAst.PrefixNot) { return visit(this, ast.expression); }, + visitPropertyRead(ast: cdAst.PropertyRead) { return false; }, + visitPropertyWrite(ast: cdAst.PropertyWrite) { return false; }, + visitQuote(ast: cdAst.Quote) { return false; }, + visitSafeMethodCall(ast: cdAst.SafeMethodCall) { return true; }, + visitSafePropertyRead(ast: cdAst.SafePropertyRead) { return false; } + }); + } + + private allocateTemporary(): o.ReadVarExpr { + const tempNumber = this._currentTemporary++; + this.temporaryCount = Math.max(this._currentTemporary, this.temporaryCount); + return new o.ReadVarExpr(temporaryName(this.bindingIndex, tempNumber)); + } + + private releaseTemporary(temporary: o.ReadVarExpr) { + this._currentTemporary--; + if (temporary.name != temporaryName(this.bindingIndex, this._currentTemporary)) { + throw new BaseException(`Temporary ${temporary.name} released out of order`); + } + } } function flattenStatements(arg: any, output: o.Statement[]) { diff --git a/modules/@angular/compiler/src/view_compiler/property_binder.ts b/modules/@angular/compiler/src/view_compiler/property_binder.ts index 9abf817d76..f4248e51a2 100644 --- a/modules/@angular/compiler/src/view_compiler/property_binder.ts +++ b/modules/@angular/compiler/src/view_compiler/property_binder.ts @@ -21,7 +21,7 @@ import {CompileElement, CompileNode} from './compile_element'; import {CompileMethod} from './compile_method'; import {CompileView} from './compile_view'; import {DetectChangesVars, ViewProperties} from './constants'; -import {convertCdExpressionToIr} from './expression_converter'; +import {convertCdExpressionToIr, temporaryDeclaration} from './expression_converter'; function createBindFieldExpr(exprIndex: number): o.ReadPropExpr { return o.THIS_EXPR.prop(`_expr_${exprIndex}`); @@ -36,14 +36,20 @@ const _animationViewCheckedFlagMap = new Map(); function bind( view: CompileView, currValExpr: o.ReadVarExpr, fieldExpr: o.ReadPropExpr, parsedExpression: cdAst.AST, context: o.Expression, actions: o.Statement[], - method: CompileMethod) { - var checkExpression = - convertCdExpressionToIr(view, context, parsedExpression, DetectChangesVars.valUnwrapper); + method: CompileMethod, bindingIndex: number) { + var checkExpression = convertCdExpressionToIr( + view, context, parsedExpression, DetectChangesVars.valUnwrapper, bindingIndex); if (isBlank(checkExpression.expression)) { // e.g. an empty expression was given return; } + if (checkExpression.temporaryCount) { + for (let i = 0; i < checkExpression.temporaryCount; i++) { + method.addStmt(temporaryDeclaration(bindingIndex, i)); + } + } + // private is fine here as no child view will reference the cached value... view.fields.push(new o.ClassField(fieldExpr.name, null, [o.StmtModifier.Private])); view.createMethod.addStmt( @@ -80,7 +86,7 @@ export function bindRenderText( [o.THIS_EXPR.prop('renderer') .callMethod('setText', [compileNode.renderNode, currValExpr]) .toStmt()], - view.detectChangesRenderPropertiesMethod); + view.detectChangesRenderPropertiesMethod, bindingIndex); } function bindAndWriteToRenderer( @@ -183,7 +189,7 @@ function bindAndWriteToRenderer( bind( view, currValExpr, fieldExpr, boundProp.value, context, updateStmts, - view.detectChangesRenderPropertiesMethod); + view.detectChangesRenderPropertiesMethod, view.bindings.length); }); } @@ -274,7 +280,7 @@ export function bindDirectiveInputs( } bind( view, currValExpr, fieldExpr, input.value, view.componentContext, statements, - detectChangesInInputsMethod); + detectChangesInInputsMethod, bindingIndex); }); if (isOnPushComp) { detectChangesInInputsMethod.addStmt(new o.IfStmt(DetectChangesVars.changed, [ diff --git a/modules/@angular/core/test/linker/regression_integration_spec.ts b/modules/@angular/core/test/linker/regression_integration_spec.ts index 379c8d66f3..1f32223de9 100644 --- a/modules/@angular/core/test/linker/regression_integration_spec.ts +++ b/modules/@angular/core/test/linker/regression_integration_spec.ts @@ -72,6 +72,39 @@ function declareTests({useJit}: {useJit: boolean}) { async.done(); }); })); + + it('should only evaluate stateful pipes once - #10639', + inject( + [TestComponentBuilder, AsyncTestCompleter], + (tcb: TestComponentBuilder, async: AsyncTestCompleter) => { + tcb.overrideView( + MyComp1, + new ViewMetadata( + {template: '{{(null|countingPipe)?.value}}', pipes: [CountingPipe]})) + .createAsync(MyComp1) + .then(fixture => { + CountingPipe.reset(); + fixture.detectChanges(/* checkNoChanges */ false); + expect(fixture.nativeElement).toHaveText('counting pipe value'); + expect(CountingPipe.calls).toBe(1); + async.done(); + }); + })); + + it('should only evaluate methods once - #10639', + inject( + [TestComponentBuilder, AsyncTestCompleter], + (tcb: TestComponentBuilder, async: AsyncTestCompleter) => { + tcb.overrideView(MyCountingComp, new ViewMetadata({template: '{{method()?.value}}'})) + .createAsync(MyCountingComp) + .then(fixture => { + MyCountingComp.reset(); + fixture.detectChanges(/* checkNoChanges */ false); + expect(fixture.nativeElement).toHaveText('counting method value'); + expect(MyCountingComp.calls).toBe(1); + async.done(); + }); + })); }); describe('providers', () => { @@ -204,6 +237,27 @@ class CustomPipe implements PipeTransform { class CmpWithNgContent { } +@Component({selector: 'counting-cmp', template: ''}) +class MyCountingComp { + method(): {value: string}|undefined { + MyCountingComp.calls++; + return {value: 'counting method value'}; + } + + static reset() { MyCountingComp.calls = 0; } + static calls = 0; +} + +@Pipe({name: 'countingPipe'}) +class CountingPipe implements PipeTransform { + transform(value: any): any { + CountingPipe.calls++; + return {value: 'counting pipe value'}; + } + static reset() { CountingPipe.calls = 0; } + static calls = 0; +} + @Component({ selector: 'left', template: `L`,