diff --git a/parser/internal/BUILD b/parser/internal/BUILD index cebb5c329..d679e2853 100644 --- a/parser/internal/BUILD +++ b/parser/internal/BUILD @@ -117,6 +117,7 @@ cc_test( deps = [ ":antlr_parser", "//common:ast", + "//common:navigable_ast", "//common:source", "//internal:status_macros", "//internal:testing", diff --git a/parser/internal/antlr_parser.cc b/parser/internal/antlr_parser.cc index 6116d05a3..cb9e63258 100644 --- a/parser/internal/antlr_parser.cc +++ b/parser/internal/antlr_parser.cc @@ -615,7 +615,8 @@ class ParserVisitor final : public CelBaseVisitor, bool add_macro_calls = false, bool enable_optional_syntax = false, bool enable_quoted_identifiers = false, - bool enable_variadic_logical_operators = false) + bool enable_variadic_logical_operators = false, + bool fold_unary_operators = false) : source_(source), factory_(source_, max_expression_node_count), macro_registry_(macro_registry), @@ -624,7 +625,8 @@ class ParserVisitor final : public CelBaseVisitor, add_macro_calls_(add_macro_calls), enable_optional_syntax_(enable_optional_syntax), enable_quoted_identifiers_(enable_quoted_identifiers), - enable_variadic_logical_operators_(enable_variadic_logical_operators) {} + enable_variadic_logical_operators_(enable_variadic_logical_operators), + fold_unary_operators_(fold_unary_operators) {} ~ParserVisitor() override = default; @@ -700,6 +702,9 @@ class ParserVisitor final : public CelBaseVisitor, const Expr& e); std::string NormalizeIdentifier(CelParser::EscapeIdentContext* ctx); + std::any VisitUnaryOps(const std::vector& ops, + CelParser::MemberContext* member, + absl::string_view op_name); // Attempt to unnest parse context. // // Walk the parse tree to the first complex term to reduce recursive depth in @@ -716,6 +721,7 @@ class ParserVisitor final : public CelBaseVisitor, const bool enable_optional_syntax_; const bool enable_quoted_identifiers_; const bool enable_variadic_logical_operators_; + const bool fold_unary_operators_; }; template >")); } -std::any ParserVisitor::visitLogicalNot(CelParser::LogicalNotContext* ctx) { - if (ctx->ops.size() % 2 == 0) { - return visit(ctx->member()); +std::any ParserVisitor::VisitUnaryOps(const std::vector& ops, + CelParser::MemberContext* member, + absl::string_view op_name) { + if (fold_unary_operators_) { + if (ops.size() % 2 == 0) { + return visit(member); + } + int64_t op_id = factory_.NextId(SourceRangeFromToken(ops[0])); + auto target = ExprFromAny(visit(member)); + return ExprToAny(GlobalCallOrMacro(op_id, op_name, std::move(target))); } - int64_t op_id = factory_.NextId(SourceRangeFromToken(ctx->ops[0])); - auto target = ExprFromAny(visit(ctx->member())); - return ExprToAny( - GlobalCallOrMacro(op_id, CelOperator::LOGICAL_NOT, std::move(target))); + + std::vector op_ids; + op_ids.reserve(ops.size()); + for (const auto* op : ops) { + op_ids.push_back(factory_.NextId(SourceRangeFromToken(op))); + } + + auto target = ExprFromAny(visit(member)); + for (int i = static_cast(op_ids.size()) - 1; i >= 0; --i) { + target = GlobalCallOrMacro(op_ids[i], op_name, std::move(target)); + } + return ExprToAny(std::move(target)); +} + +std::any ParserVisitor::visitLogicalNot(CelParser::LogicalNotContext* ctx) { + return VisitUnaryOps(ctx->ops, ctx->member(), CelOperator::LOGICAL_NOT); } std::any ParserVisitor::visitNegate(CelParser::NegateContext* ctx) { - if (ctx->ops.size() % 2 == 0) { - return visit(ctx->member()); - } - int64_t op_id = factory_.NextId(SourceRangeFromToken(ctx->ops[0])); - auto target = ExprFromAny(visit(ctx->member())); - return ExprToAny( - GlobalCallOrMacro(op_id, CelOperator::NEGATE, std::move(target))); + return VisitUnaryOps(ctx->ops, ctx->member(), CelOperator::NEGATE); } std::string ParserVisitor::NormalizeIdentifier( @@ -1684,7 +1703,8 @@ absl::StatusOr> AntlrParseImpl( source, options.max_recursion_depth, options.expression_node_limit, registry, options.add_macro_calls, options.enable_optional_syntax, options.enable_quoted_identifiers, - options.enable_variadic_logical_operators); + options.enable_variadic_logical_operators, + options.fold_unary_operators); lexer.removeErrorListeners(); parser.removeErrorListeners(); diff --git a/parser/internal/antlr_parser_test.cc b/parser/internal/antlr_parser_test.cc index d1a99227a..4bdafc42b 100644 --- a/parser/internal/antlr_parser_test.cc +++ b/parser/internal/antlr_parser_test.cc @@ -23,6 +23,7 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "common/ast.h" +#include "common/navigable_ast.h" #include "common/source.h" #include "internal/status_macros.h" #include "internal/testing.h" @@ -72,5 +73,20 @@ TEST(AntlrParserTest, RecursionDepthExceeded) { HasSubstr("Exceeded max recursion depth of 6 when parsing.")); } +TEST(AntlrParserTest, UnaryOperatorsUnfoldedOption) { + ParserOptions options; + options.fold_unary_operators = false; + + ASSERT_OK_AND_ASSIGN(auto ast, Parse("---a", "", options)); + auto nav_ast = cel::NavigableAst::Build(ast->root_expr()); + EXPECT_EQ(nav_ast.Root().height(), 4); + + for (const auto& node : nav_ast.Root().DescendantsPostorder()) { + if (node.node_kind() == cel::NodeKind::kCall) { + EXPECT_EQ(node.expr()->call_expr().function(), "-_"); + } + } +} + } // namespace } // namespace cel::parser_internal diff --git a/parser/internal/pratt_parser_test.cc b/parser/internal/pratt_parser_test.cc index 8ebe5123e..116ae607b 100644 --- a/parser/internal/pratt_parser_test.cc +++ b/parser/internal/pratt_parser_test.cc @@ -434,9 +434,7 @@ std::vector GetParserTestCases() { TestCase{ .source = "- -1", .expected_ast = R"( - -_( - -1^#2:int64# - )^#1:Expr.Call# + 1^#3:int64# )", }, TestCase{ @@ -454,11 +452,7 @@ std::vector GetParserTestCases() { .source = "---a", .expected_ast = R"( -_( - -_( - -_( - a^#4:Expr.Ident# - )^#3:Expr.Call# - )^#2:Expr.Call# + a^#4:Expr.Ident# )^#1:Expr.Call# )", }, diff --git a/parser/internal/pratt_parser_worker.h b/parser/internal/pratt_parser_worker.h index bbd719e34..dc17f27d6 100644 --- a/parser/internal/pratt_parser_worker.h +++ b/parser/internal/pratt_parser_worker.h @@ -544,7 +544,15 @@ ExprNode PrattParserWorker::ParseUnaryOpsChain(Token first_op) { ExprNode operand; if (!ops.empty() && ops.back().type == TokenType::kMinus) { - if (peek_token_.type == TokenType::kInt) { + if (options_.fold_unary_operators && ops.size() > 1 && + ops[ops.size() - 2].type == TokenType::kMinus) { + // Match the ANTLR parser behavior where `-(-)+` prefers to match as + // repeated negate operators instead of a negation of an int literal. + // ---9223372036854775808 will fail to parse. + ops.pop_back(); + ops.pop_back(); + operand = ParseSelectorChain(); + } else if (peek_token_.type == TokenType::kInt) { int64_t op_id = ops.back().id; ops.pop_back(); operand = ParseNegativeIntLiteral(op_id); @@ -561,6 +569,13 @@ ExprNode PrattParserWorker::ParseUnaryOpsChain(Token first_op) { for (int i = static_cast(ops.size()) - 1; i >= 0; --i) { std::vector args; + if (options_.fold_unary_operators && i > 0) { + if (ops[i - 1].type == ops[i].type) { + i--; + continue; + } + } + args.push_back(std::move(operand)); absl::string_view op_name = (ops[i].type == TokenType::kExclamation) ? CelOperator::LOGICAL_NOT diff --git a/parser/options.h b/parser/options.h index eb9d2eeea..1ddce71ac 100644 --- a/parser/options.h +++ b/parser/options.h @@ -85,6 +85,22 @@ struct ParserOptions final { // early testing of the Pratt parser. // TODO(b/527638023): Remove this option once the ANTLR parser is removed. bool enable_pratt_parser = false; + + // Folds repeated unary operators (!, -). + // + // If the operator appears repeatedly, the parser will ignore every contiguous + // pair. + // + // This makes it possible to parse some semantically invalid expressions as + // valid ones, though they are not particularly harmful. + // + // Examples that parse to the same AST: + // + // `---1` : `-(1)` + // `!!!!!true` : !`true` + // `--0u` : `0u` + // `!!"hello"` : `"hello"`). + bool fold_unary_operators = true; }; } // namespace cel diff --git a/parser/parser_test.cc b/parser/parser_test.cc index e2681608d..d4a3294c3 100644 --- a/parser/parser_test.cc +++ b/parser/parser_test.cc @@ -820,11 +820,7 @@ std::vector test_cases = { "", "", "", "", // PRATT PARSER AST "-_(\n" - " -_(\n" - " -_(\n" - " a^#4:Expr.Ident#\n" - " )^#3:Expr.Call#\n" - " )^#2:Expr.Call#\n" + " a^#4:Expr.Ident#\n" ")^#1:Expr.Call#"}, {"1 + +", "", "ERROR: :1:5: Syntax error: mismatched input '+' expecting {'[', "