Unparser doesn't preserve operation precedence

[3cp]

I'm submitting a bug report

• Library Version: 1.2.1

Please tell us about your environment:

• Operating System: IRRELEVANT

• Node Version: IRRELEVANT

• NPM Version: IRRELEVANT

• JSPM OR Webpack AND Version IRRELEVANT

• Browser: IRRELEVANT

• Language: IRRELEVANT

Current behavior:

const parser = new Parser();
parser.parse('a&&(b||c)').toString() // 'a&&b||c' wrong logic
parser.parse('a&&(b=1)').toString()  // 'a&&b=1' syntactically wrong
parser.parse('2*(2+3)').toString() // '2*2+3' oops

Expected/desired behavior:

• What is the expected behavior? Unparser should preserve operation precedence

• What is the motivation / use case for changing the behavior? It's a bug to be fixed. unparser.spec.js should cover this.

I tried fixing it, but failed. It looks like the unparser needs to understand the position of the expression in the tree, and also the default javascript operator precedence, in order to decide whether needs to wrap the expression in extra parenthesis.

[jdanyow]

a || (b && c) and a || b && c are the same because && has higher operator precedence than ||. Same goes for the other example. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence

[3cp]

@jdanyow sorry I was showing wrong example, try 'a&&(b||c)' will show you the bug.

I am not wrong on 2nd sample 'a&&(b=1)', I think you missed it in glance, it is assignment not equal operator.

This is a valid bug.

[3cp]

@jdanyow One more solid error, please reopen the issue.

parser.parse('2*(2+3)').toString() // '2*2+3' oops

[jdanyow]

basically every node of type Binary needs to wrap it's output in parens

[3cp]

More than that, by looking at the operator precedence, I found Conditional also failed.

parser.parse('!(true?true:true)').toString() // '(!true?true:true)' evaluated value reversed

There might be more.

Adding extra explicit parens could solve the logic bug, but the appearance of the unparsed expression is in the most verbose form.

[3cp]

I have a suggestion to use original tokens to unparse, not using AST.

Tokens stripped all unneeded white spaces, the final unparsed form is based on original expression, not minimum, but at least not the most verbose form.

Here is a quick working hack on Parser to prove my suggestion.

export class Parser {
  constructor() {
    this.cache = {};
    this.lexer = new Lexer();
  }

  parse(input, opts = {}) {
    input = input || '';

    if (!this.cache[input]) {
      const parserImp = new ParserImplementation(this.lexer, input, opts);
      this.cache[input] = parserImp.parseExpression();

      let exp = '';
      for (let i = 0, length = parserImp.tokens.length; i < length; ++i) {
        exp += parserImp.tokens[i].text;
      }

      this.cache[input].toString = function() {
        return exp;
      };
    }

    return this.cache[input];
  }
}

[fkleuver]

@huochunpeng I actually stumbled upon this last week as well. A quick-n-dirty fix for this (without impacting the performance of the parser) would be to change visitAssign, visitConditional and visitBinary in Unparser:

visitAssign(assign) {
  this.write('(');
  assign.target.accept(this);
  this.write('=');
  assign.value.accept(this);
  this.write(')');
}

visitConditional(conditional) {
  this.write('(');
  conditional.condition.accept(this);
  this.write('?');
  conditional.yes.accept(this);
  this.write(':');
  conditional.no.accept(this);
  this.write(')');
}

visitBinary(binary) {
  this.write('(');
  binary.left.accept(this);
  this.write(binary.operation);
  binary.right.accept(this);
  this.write(')');
}

visitUnary is already parenthesized so this should guarantee correctness in all expressions where precedence may apply. It adds a lot of unnecessary parens though.

If preferred, I could make the unparser a little smarter and let it apply parens whenever the inner expression has a lower precedence than the outer one (in a similar fashion to how I'm currently handling precedence in binary parsing). Might add 20 LOC give or take. @bigopon what do you think? Worth it?

[3cp]

Thx for looking into this. Currently no other part of aurelia actually uses unparser. The logic is never exercised, hence no bug has been exposed so far.

Verbose parenthesis might be not that bad. Because both verbose and optimised unparsed expression does not guarantee matching original expression. But personally I like the optimised version.

[bigopon]

A PR will be nice, but I don't know if we need to do any thing beyond your patch above. It's the most untouched part of Aurelia

[fkleuver]

I'm inclined to agree @bigopon , all I'm worried about is backwards compatibility. If some users turn out to depend somehow on the Unparser, parentheses suddenly popping up everywhere might break that. Though on the flipside of that coin, the Unparser currently not behaving as it should is unlikely to have made any form of depending on the Unparser very successful

@huochunpeng In general my preference would be the optimized version as well, but ultimately these sort of things are a tradeoff in performance vs file size. The Unparser is so very rarely used that it might indeed not even be worth adding another 100-200 bytes just for this.

The change above would be a merge conflict with my current PR so I'll do it after that one is through :)

[qraynaud]

As stated in my bug above aurelia/templating-binding#127, this is not only unparser that has a problem.

4/2*10 is equal to 0.2 according to aurelia now. This is new! And 4-2+10 is equal to -8. That is a real problem. A lot of things behave like right associativity or like + and * have a higher precedence than - and / (respectively).

Here is a gist demonstrating that: https://gist.run/?id=86a169dd2dea36afc86745cbe781d4dd

[fkleuver]

What exact version of binding is that?

[qraynaud]

@fkleuver : I'm on 2.1.0. On the gist I don't know, I forked the official gist today.

[fkleuver]

This is actually a separate issue from the unparser issue you're replying to. It somehow slipped through the cracks in my refactoring - it's quite a nasty one indeed. I fixed it right away. Thanks a lot for pointing it out!

[3cp]

@EisenbergEffect pls reopen this issue. It was auto-closed by github since #696 mentioned, but that fix is for a new issue raised by @qraynaud here.

[EisenbergEffect]

Sorry about that folks! Reopening.