Fix an unparser error on expressions like -(1 + 2) - #262
Merged
Merged
Conversation
TristonianJones
requested changes
Sep 17, 2019
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.
TristonianJones
requested changes
Sep 18, 2019
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.
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
approved these changes
Oct 2, 2019
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 ...)