fix(Parser): Parse pipes in arguments

fixes #1680

modules/angular2/src/change_detection/parser/parser.ts

@@ -216,10 +216,21 @@ class _ParseAST {
   parsePipe() {
     var result = this.parseExpression();
     if (this.optionalOperator("|")) {
-      return this.parseInlinedPipe(result);
+      if (this.parseAction) {
-    } else {
+        this.error("Cannot have a pipe in an action expression");
-      return result;
+      }
+
+      do {
+        var name = this.expectIdentifierOrKeyword();
+        var args = [];
+        while (this.optionalCharacter($COLON)) {
+          ListWrapper.push(args, this.parsePipe());
+        }
+        result = new Pipe(result, name, args, true);
+      } while (this.optionalOperator("|"));
     }
+
+    return result;
   }
 
   parseExpression() {
@@ -249,13 +260,13 @@ class _ParseAST {
     var result = this.parseLogicalOr();
 
     if (this.optionalOperator('?')) {
-      var yes = this.parseExpression();
+      var yes = this.parsePipe();
       if (!this.optionalCharacter($COLON)) {
         var end = this.inputIndex;
         var expression = this.input.substring(start, end);
         this.error(`Conditional expression ${expression} requires all 3 expressions`);
       }
-      var no = this.parseExpression();
+      var no = this.parsePipe();
       return new Conditional(result, yes, no);
     } else {
       return result;
@@ -368,7 +379,7 @@ class _ParseAST {
         result = this.parseAccessMemberOrMethodCall(result, true);
 
       } else if (this.optionalCharacter($LBRACKET)) {
-        var key = this.parseExpression();
+        var key = this.parsePipe();
         this.expectCharacter($RBRACKET);
         result = new KeyedAccess(result, key);
 
@@ -385,9 +396,9 @@ class _ParseAST {
 
   parsePrimary() {
     if (this.optionalCharacter($LPAREN)) {
-      var result = this.parsePipe();
+      let result = this.parsePipe();
       this.expectCharacter($RPAREN);
-      return result;
+      return result
 
     } else if (this.next.isKeywordNull() || this.next.isKeywordUndefined()) {
       this.advance();
@@ -434,7 +445,7 @@ class _ParseAST {
     var result = [];
     if (!this.next.isCharacter(terminator)) {
       do {
-        ListWrapper.push(result, this.parseExpression());
+        ListWrapper.push(result, this.parsePipe());
       } while (this.optionalCharacter($COMMA));
     }
     return result;
@@ -449,7 +460,7 @@ class _ParseAST {
         var key = this.expectIdentifierOrKeywordOrString();
         ListWrapper.push(keys, key);
         this.expectCharacter($COLON);
-        ListWrapper.push(values, this.parseExpression());
+        ListWrapper.push(values, this.parsePipe());
       } while (this.optionalCharacter($COMMA));
       this.expectCharacter($RBRACE);
     }
@@ -457,50 +468,28 @@ class _ParseAST {
   }
 
   parseAccessMemberOrMethodCall(receiver, isSafe: boolean = false): AST {
-    var id = this.expectIdentifierOrKeyword();
+    let id = this.expectIdentifierOrKeyword();
 
     if (this.optionalCharacter($LPAREN)) {
-      var args = this.parseCallArguments();
+      let args = this.parseCallArguments();
       this.expectCharacter($RPAREN);
-      var fn = this.reflector.method(id);
+      let fn = this.reflector.method(id);
       return isSafe ? new SafeMethodCall(receiver, id, fn, args) :
                       new MethodCall(receiver, id, fn, args);
 
     } else {
-      var getter = this.reflector.getter(id);
+      let getter = this.reflector.getter(id);
-      var setter = this.reflector.setter(id);
+      let setter = this.reflector.setter(id);
-      var am = isSafe ? new SafeAccessMember(receiver, id, getter, setter) :
+      return isSafe ? new SafeAccessMember(receiver, id, getter, setter) :
-                        new AccessMember(receiver, id, getter, setter);
+                      new AccessMember(receiver, id, getter, setter);
-
-      if (this.optionalOperator("|")) {
-        return this.parseInlinedPipe(am);
-      } else {
-        return am;
-      }
     }
   }
 
-  parseInlinedPipe(result) {
-    do {
-      if (this.parseAction) {
-        this.error("Cannot have a pipe in an action expression");
-      }
-      var name = this.expectIdentifierOrKeyword();
-      var args = ListWrapper.create();
-      while (this.optionalCharacter($COLON)) {
-        ListWrapper.push(args, this.parseExpression());
-      }
-      result = new Pipe(result, name, args, true);
-    } while (this.optionalOperator("|"));
-
-    return result;
-  }
-
   parseCallArguments() {
     if (this.next.isCharacter($RPAREN)) return [];
     var positionals = [];
     do {
-      ListWrapper.push(positionals, this.parseExpression());
+      ListWrapper.push(positionals, this.parsePipe());
     } while (this.optionalCharacter($COMMA));
     return positionals;
   }

modules/angular2/test/change_detection/parser/parser_spec.ts

@@ -3,9 +3,10 @@ import {BaseException, isBlank, isPresent} from 'angular2/src/facade/lang';
 import {reflector} from 'angular2/src/reflection/reflection';
 import {MapWrapper, ListWrapper} from 'angular2/src/facade/collection';
 import {Parser} from 'angular2/src/change_detection/parser/parser';
+import {Unparser} from './unparser';
 import {Lexer} from 'angular2/src/change_detection/parser/lexer';
 import {Locals} from 'angular2/src/change_detection/parser/locals';
-import {Pipe, LiteralPrimitive, AccessMember} from 'angular2/src/change_detection/parser/ast';
+import {Pipe, LiteralPrimitive} from 'angular2/src/change_detection/parser/ast';
 
 class TestData {
   constructor(public a?: any, public b?: any, public fnReturnValue?: any) {}
@@ -383,34 +384,20 @@ export function main() {
     describe("parseBinding", () => {
       describe("pipes", () => {
         it("should parse pipes", () => {
-          var exp = parseBinding("'Foo'|uppercase").ast;
+          var originalExp = '"Foo" | uppercase';
-          expect(exp).toBeAnInstanceOf(Pipe);
+          var ast = parseBinding(originalExp).ast;
-          expect(exp.name).toEqual("uppercase");
+          expect(ast).toBeAnInstanceOf(Pipe);
+          expect(new Unparser().unparse(ast)).toEqual(`(${originalExp})`);
         });
 
         it("should parse pipes in the middle of a binding", () => {
-          var exp = parseBinding("user|a|b.name").ast;
+          var ast = parseBinding('(user | a | b).name').ast;
-
+          expect(new Unparser().unparse(ast)).toEqual('((user | a) | b).name');
-          expect(exp.name).toEqual("name");
-          expect(exp.receiver).toBeAnInstanceOf(Pipe);
-          expect(exp.receiver.name).toEqual("b");
-
-          expect(exp.receiver.exp).toBeAnInstanceOf(Pipe);
-          expect(exp.receiver.exp.name).toEqual("a");
         });
 
         it("should parse pipes with args", () => {
-          var exp = parseBinding("(1|a:2)|b:3").ast;
+          var ast = parseBinding("(1|a:2)|b:3").ast;
-
+          expect(new Unparser().unparse(ast)).toEqual('((1 | a:2) | b:3)');
-          expect(exp).toBeAnInstanceOf(Pipe);
-          expect(exp.name).toEqual("b");
-          expect(exp.args[0]).toBeAnInstanceOf(LiteralPrimitive);
-
-          expect(exp.exp).toBeAnInstanceOf(Pipe);
-          expect(exp.exp.name).toEqual("a");
-          expect(exp.exp.args[0]).toBeAnInstanceOf(LiteralPrimitive);
-
-          expect(exp.exp.exp).toBeAnInstanceOf(LiteralPrimitive);
         });
 
         it('should only allow identifier or keyword as formatter names', () => {
@@ -421,6 +408,25 @@ export function main() {
               .toThrowError(new RegExp('identifier or keyword'));
         });
 
+        it('should parse pipes', () => {
+          let unparser = new Unparser();
+          let exps = [
+            ['a(b | c)', 'a((b | c))'],
+            ['a.b(c.d(e) | f)', 'a.b((c.d(e) | f))'],
+            ['[1, 2, 3] | a', '([1, 2, 3] | a)'],
+            ['{a: 1} | b', '({a: 1} | b)'],
+            ['a[b] | c', '(a[b] | c)'],
+            ['a?.b | c', '(a?.b | c)'],
+            ['true | a', '(true | a)'],
+            ['a | b:c | d', '(a | b:(c | d))'],
+            ['(a | b:c) | d', '((a | b:c) | d)']
+          ];
+
+          ListWrapper.forEach(exps, e => {
+            var ast = parseBinding(e[0]).ast;
+            expect(unparser.unparse(ast)).toEqual(e[1]);
+          });
+        });
       });
 
       it('should store the source in the result',
@@ -433,7 +439,7 @@ export function main() {
         expect(() => parseBinding("1;2")).toThrowError(new RegExp("contain chained expression"));
       });
 
-      it('should throw on assignmnt', () => {
+      it('should throw on assignment', () => {
         expect(() => parseBinding("1;2")).toThrowError(new RegExp("contain chained expression"));
       });
     });
@@ -584,11 +590,9 @@ export function main() {
       });
 
       it('should parse prefix/suffix with multiple interpolation', () => {
-        var ast = parseInterpolation('before{{a}}middle{{b}}after').ast;
+        var originalExp = 'before {{ a }} middle {{ b }} after';
-        expect(ast.strings).toEqual(['before', 'middle', 'after']);
+        var ast = parseInterpolation(originalExp).ast;
-        expect(ast.expressions.length).toEqual(2);
+        expect(new Unparser().unparse(ast)).toEqual(originalExp);
-        expect(ast.expressions[0].name).toEqual('a');
-        expect(ast.expressions[1].name).toEqual('b');
       });
     });
 

modules/angular2/test/change_detection/parser/unparser.ts

@@ -68,12 +68,14 @@ export class Unparser implements AstVisitor {
   }
 
   visitPipe(ast: Pipe) {
+    this._expression += '(';
     this._visit(ast.exp);
     this._expression += ` | ${ast.name}`;
     ast.args.forEach(arg => {
       this._expression += ':';
       this._visit(arg);
-    })
+    });
+    this._expression += ')';
   }
 
   visitFunctionCall(ast: FunctionCall) {

modules/angular2/test/change_detection/parser/unparser_spec.ts

@@ -64,7 +64,7 @@ export function main() {
     it('should support Conditional', () => { check('a ? b : c', Conditional); });
 
     it('should support Pipe', () => {
-      var originalExp = 'a | b';
+      var originalExp = '(a | b)';
       var ast = parseBinding(originalExp).ast;
       expect(ast).toBeAnInstanceOf(Pipe);
       expect(unparser.unparse(ast)).toEqual(originalExp);
@@ -94,7 +94,7 @@ export function main() {
     it('should support SafeMethodCall', () => { check('a?.b(c, d)', SafeMethodCall); });
 
     it('should support complex expression', () => {
-      var originalExp = 'a + 3 * fn([c + d | e.f], {a: 3})[g].h && i';
+      var originalExp = 'a + 3 * fn([(c + d | e).f], {a: 3})[g].h && i';
       var ast = parseBinding(originalExp).ast;
       expect(unparser.unparse(ast)).toEqual(originalExp);
     });