parser: parenthesize unary operands that would reparse differently (#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.

Author: nileshpatil6

parser/unparser.go

@@ -17,6 +17,7 @@ package parser
 import (
 	"errors"
 	"fmt"
+	"math"
 	"regexp"
 	"strconv"
 	"strings"
@@ -270,7 +271,7 @@ func (un *unparser) visitCallUnary(expr ast.Expr) error {
 		return fmt.Errorf("cannot unmangle operator: %s", fun)
 	}
 	un.str.WriteString(unmangled)
-	nested := isComplexOperator(args[0])
+	nested := isComplexOperator(args[0]) || isAmbiguousUnaryOperand(args[0])
 	return un.visitMaybeNested(args[0], nested)
 }
 
@@ -507,6 +508,30 @@ func isComplexOperator(expr ast.Expr) bool {
 	return false
 }
 
+// Indicates whether the operand of a unary operator must be wrapped in parentheses in order for
+// the unparsed expression to parse back to an equivalent AST.
+//
+// The CEL grammar parses a run of leading '!' or '-' tokens as a single unary expression and drops
+// the operators when their count is even, and it also treats a leading '-' as part of an int or
+// double literal. Emitting such an operand without parentheses either changes the expression, e.g.
+// `!(!a)` would unparse to `!!a` which parses back to `a`, or produces output which does not parse
+// at all, e.g. `-(!a)` would unparse to `-!a`.
+func isAmbiguousUnaryOperand(expr ast.Expr) bool {
+	switch expr.Kind() {
+	case ast.CallKind:
+		fun := expr.AsCall().FunctionName()
+		return fun == operators.LogicalNot || fun == operators.Negate
+	case ast.LiteralKind:
+		switch lit := expr.AsLiteral().(type) {
+		case types.Int:
+			return lit < 0
+		case types.Double:
+			return math.Signbit(float64(lit))
+		}
+	}
+	return false
+}
+
 // Indicates whether it is a complex operation compared to another.
 // expr is *not* considered complex if it is not a call expression or has
 // less than two arguments, or if it has a higher precedence than op.

parser/unparser_test.go

@@ -113,6 +113,19 @@ func TestUnparse(t *testing.T) {
 		{name: "select_quoted", in: "a.`b-c`"},
 		{name: "opt_select_quoted", in: "a.?`b.c`"},
 		{name: "message_create_quoted", in: "MyType{`in`: false}"},
+		// A unary operator whose operand is another unary operator, or a negative numeric
+		// literal, must keep the operand parenthesized. The grammar parses a run of leading
+		// '!' or '-' tokens as a single unary expression and cancels out pairs of them, and
+		// it also treats a leading '-' as part of an int or double literal.
+		{name: "call_not_not", in: `!(!a)`},
+		{name: "call_neg_neg", in: `-(-a)`},
+		{name: "call_neg_not", in: `-(!a)`},
+		{name: "call_not_neg", in: `!(-a)`},
+		{name: "call_neg_neg_neg", in: `-(-(-a))`},
+		{name: "call_neg_lit_int", in: `-(-1)`},
+		{name: "call_neg_lit_int_min", in: `-(-9223372036854775808)`},
+		{name: "call_neg_lit_double", in: `-(-1.5)`},
+		{name: "call_neg_lit_int_nested", in: `1 + -(-1)`},
 
 		// Equivalent expressions form unparse which do not match the originals.
 		{name: "call_add_equiv", in: `a+b-c`, out: `a + b - c`},