binding: Unparser doesn't preserve operation precedence

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.

About this issue

Original URL
State: open
Created 7 years ago
Comments: 16 (14 by maintainers)

Commits related to this issue

temporary hack to fix unparser bug // https://github.com/aurelia/binding/issues/586 — committed to buttonwoodcx/bcx-expression-evaluator by 3cp 7 years ago

Most upvoted comments

@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?

fkleuver on May 25, 2018