Sitelet https://github.com/cel-expr/cel-go/pull/262
Skip to content

Fix an unparser error on expressions like -(1 + 2) - #262

Merged
TristonianJones merged 5 commits into
cel-expr:masterfrom
timn:unparser-fix
Oct 2, 2019
Merged

TristonianJones merged 5 commits into
cel-expr:masterfrom
timn:unparser-fix

Conversation

@timn

@timn timn commented Sep 17, 2019

Copy link
Copy Markdown
Contributor

What

For an expression such as -(1 + 2), the unparser would generate - 1 + 2 , which obviously is wrong. This came up during updating the C++ CEL decompiler. Also add some more tests that were useful on the C++ side.

Why

The issue should be fixed to not produce wrong results in this case.

Tests

The parser test suite completes without error (ran bazel test ...)

Comment thread parser/unparser.go Outdated
Comment thread parser/unparser.go Outdated
For an expression such as -(1 + 2), the unparser would generate -1+2,
which obviously is wrong. This came up during updating the C++ CEL
decompiler. Also add some more tests that were useful on the C++ side.
Comment thread parser/unparser.go Outdated
Comment thread parser/unparser.go Outdated
@timn

timn commented Sep 19, 2019

Copy link
Copy Markdown
Contributor Author

Will have another look tomorrow, didn't get to it today.

FindReverse would also return logical not, and negate.
Sync with C++ decompiler, fix some more issues.
This covers several problematic areas where the unparser would
previously generate expressions which did not match the incoming proto.
Also add some (commented out) comprehension tests which are not yet
supported by the Go unparser.
@timn

timn commented Oct 2, 2019

Copy link
Copy Markdown
Contributor Author

Hi @TristonianJones. I think this is ready for another look. More test coverage, better function naming, distinction of cases, make decisions on nesting early, and alignment with C++ decompiler.

@TristonianJones
TristonianJones merged commit 8099250 into cel-expr:master Oct 2, 2019
@timn
timn deleted the unparser-fix branch October 4, 2019 08:55
TristonianJones pushed a commit that referenced this pull request Aug 24, 2026
…1434)

* parser: parenthesize unary operands that would reparse differently

visitCallUnary decides whether to parenthesize its operand with
isComplexOperator, which only returns true for a call with two or more
arguments. It was added by #262 to fix `-(1 + 2)`, and never covered an
operand that is itself a unary call or a negative numeric literal.

Two grammar rules make that unsound. `unary` parses a run of leading '-'
or '!' tokens as one expression and drops the operator when the count is
even, and `literal` binds a leading '-' into an int or double constant.
So the unparser emits text that either means something else or does not
parse:

    !(!a)                     -> !!a                     reparses as a
    -(-a)                     -> --a                     reparses as a
    -(-(-a))                  -> ---a                    reparses as -a
    -(!a)                     -> -!a                     syntax error
    !(-a)                     -> !-a                     syntax error
    -(-1)                     -> --1                     reparses as 1
    -(-9223372036854775808)   -> --9223372036854775808    invalid int literal

Since AstToString is the public way to render a checked AST back to
source, round-tripping changes evaluation: `-(-x)` with x = MinInt64 must
raise integer overflow but returns -9223372036854775808 afterwards, and
`!(!a)` on a non-bool must raise no such overload but returns a value.

Parenthesize an operand that is a single-argument logical-not or negate
call, or a negative int or double literal. math.Signbit is used for
doubles so negative zero is covered too.

* parser: simplify the unary operand check to the function name

The member-function and argument-count guards cannot change the outcome,
since the logical-not and negate operator names are unary by definition.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants