diff --git a/parser/src/main/java/dev/cel/parser/BUILD.bazel b/parser/src/main/java/dev/cel/parser/BUILD.bazel index 99e5de89..c1e62df3 100644 --- a/parser/src/main/java/dev/cel/parser/BUILD.bazel +++ b/parser/src/main/java/dev/cel/parser/BUILD.bazel @@ -98,7 +98,7 @@ java_library( tags = [ ], deps = [ - "@cel_spec//proto/cel/expr:expr_java_proto", + "//common/ast", "@maven//:com_google_guava_guava", ], ) @@ -127,9 +127,22 @@ java_library( tags = [ ], deps = [ - "//checker:proto_expr_visitor", - "//parser:operator", - "@cel_spec//proto/cel/expr:expr_java_proto", + ":unparser_visitor", + "//common", + "//common:options", + ], +) + +java_library( + name = "unparser_visitor", + srcs = ["CelUnparserVisitor.java"], + tags = [ + ], + deps = [ + ":operator", + "//common", + "//common/ast", + "//common/ast:cel_expr_visitor", "@maven//:com_google_protobuf_protobuf_java", ], ) diff --git a/parser/src/main/java/dev/cel/parser/CelUnparser.java b/parser/src/main/java/dev/cel/parser/CelUnparser.java index 384f4a8f..ba3338e5 100644 --- a/parser/src/main/java/dev/cel/parser/CelUnparser.java +++ b/parser/src/main/java/dev/cel/parser/CelUnparser.java @@ -14,22 +14,23 @@ package dev.cel.parser; -import dev.cel.expr.ParsedExpr; +import dev.cel.common.CelAbstractSyntaxTree; +import dev.cel.common.CelOptions; /** - * Provides an unparsing utility that converts a ParsedExpr back into a human readable format. + * Provides an unparsing utility that converts an AST back into a human-readable format. * - *

Input to the unparser is a ParsedExpr. The unparser does not do any checks to see if the - * ParsedExpr is syntactically or semantically correct but does checks enough to prevent its crash - * and might return errors in such cases. + *

Input to the unparser is a {@link CelAbstractSyntaxTree}. The unparser does not do any checks + * to see if the AST is syntactically or semantically correct but does check enough to prevent its + * crash and might return errors in such cases. */ public interface CelUnparser { /** - * Unparses the {@link ParsedExpr} value to a human-readable string. + * Unparses the {@link CelAbstractSyntaxTree} value to a human-readable string. * - *

For the best results ensure that the expression is parsed with ParserOptions.add_macro_calls - * = true. + *

To reconstruct an expression that originally contained a macro call, ensure the expression + * was parsed with {@link CelOptions#populateMacroCalls()} enabled. */ - String unparse(ParsedExpr parsedExpr); + String unparse(CelAbstractSyntaxTree ast); } diff --git a/parser/src/main/java/dev/cel/parser/CelUnparserImpl.java b/parser/src/main/java/dev/cel/parser/CelUnparserImpl.java index 576842ba..deffd9d5 100644 --- a/parser/src/main/java/dev/cel/parser/CelUnparserImpl.java +++ b/parser/src/main/java/dev/cel/parser/CelUnparserImpl.java @@ -14,419 +14,14 @@ package dev.cel.parser; -import dev.cel.expr.Constant; -import dev.cel.expr.Expr; -import dev.cel.expr.Expr.CreateStruct.Entry; -import dev.cel.expr.Expr.ExprKindCase; -import dev.cel.expr.ParsedExpr; -import dev.cel.expr.SourceInfo; -import com.google.protobuf.ByteString; -import dev.cel.checker.CelProtoExprVisitor; -import java.util.Optional; +import dev.cel.common.CelAbstractSyntaxTree; /** Unparser implementation for CEL. */ final class CelUnparserImpl implements CelUnparser { - private static final String LEFT_PAREN = "("; - private static final String RIGHT_PAREN = ")"; - private static final String DOT = "."; - private static final String COMMA = ","; - private static final String SPACE = " "; - private static final String LEFT_BRACKET = "["; - private static final String RIGHT_BRACKET = "]"; - private static final String LEFT_BRACCE = "{"; - private static final String RIGHT_BRACE = "}"; - private static final String COLON = ":"; - private static final String QUESTION_MARK = "?"; - @Override - public String unparse(ParsedExpr parsedExpr) { - return CelUnparserExprVisitor.unparse(parsedExpr); - } - - static final class CelUnparserExprVisitor extends CelProtoExprVisitor { - private final Expr expr; - private final SourceInfo sourceInfo; - private final StringBuilder stringBuilder; - - /** Creates a new {@link CelUnparserExprVisitor}. */ - public CelUnparserExprVisitor(Expr expr, SourceInfo sourceInfo) { - this.expr = expr; - this.sourceInfo = sourceInfo; - this.stringBuilder = new StringBuilder(); - } - - public static String unparse(ParsedExpr parsedExpr) { - CelUnparserExprVisitor unparserVisitor = - new CelUnparserExprVisitor(parsedExpr.getExpr(), parsedExpr.getSourceInfo()); - return unparserVisitor.doParse(); - } - - private String doParse() { - visit(expr); - return stringBuilder.toString(); - } - - @Override - public void visit(Expr expr) { - if (sourceInfo.getMacroCallsMap().containsKey(expr.getId())) { - visit(sourceInfo.getMacroCallsMap().get(expr.getId())); - return; - } - super.visit(expr); - } - - @Override - protected void visit(Expr expr, Constant constant) { - switch (constant.getConstantKindCase()) { - case STRING_VALUE: - stringBuilder.append("\"").append(constant.getStringValue()).append("\""); - break; - case INT64_VALUE: - stringBuilder.append(constant.getInt64Value()); - break; - case UINT64_VALUE: - stringBuilder.append(constant.getUint64Value()).append("u"); - break; - case BOOL_VALUE: - stringBuilder.append(constant.getBoolValue() ? "true" : "false"); - break; - case DOUBLE_VALUE: - stringBuilder.append(constant.getDoubleValue()); - break; - case NULL_VALUE: - stringBuilder.append("null"); - break; - case BYTES_VALUE: - stringBuilder.append("b\"").append(bytesToOctets(constant.getBytesValue())).append("\""); - break; - default: - throw new IllegalArgumentException("unexpected expr kind"); - } - } - - @Override - protected void visit(Expr expr, Expr.Ident ident) { - stringBuilder.append(ident.getName()); - } - - @Override - protected void visit(Expr expr, Expr.Select select) { - if (select.getTestOnly()) { - stringBuilder.append(Operator.HAS.getFunction()).append(LEFT_PAREN); - } - Expr operand = select.getOperand(); - boolean nested = !select.getTestOnly() && isBinaryOrTernaryOperator(operand); - visitMaybeNested(operand, nested); - stringBuilder.append(DOT).append(select.getField()); - if (select.getTestOnly()) { - stringBuilder.append(RIGHT_PAREN); - } - } - - @Override - protected void visit(Expr expr, Expr.Call call) { - String fun = call.getFunction(); - - Optional op = Operator.lookupUnaryOperator(fun); - if (op.isPresent()) { - visitUnary(call, op.get()); - return; - } - - op = Operator.lookupBinaryOperator(fun); - if (op.isPresent()) { - visitBinary(call, op.get()); - return; - } - - if (fun.equals(Operator.INDEX.getFunction())) { - visitIndex(call); - return; - } - - if (fun.equals(Operator.CONDITIONAL.getFunction())) { - visitTernary(call); - return; - } - - if (call.hasTarget()) { - boolean nested = isBinaryOrTernaryOperator(call.getTarget()); - visitMaybeNested(call.getTarget(), nested); - stringBuilder.append(DOT); - } - - stringBuilder.append(fun).append(LEFT_PAREN); - for (int i = 0; i < call.getArgsCount(); i++) { - if (i > 0) { - stringBuilder.append(COMMA).append(SPACE); - } - visit(call.getArgs(i)); - } - stringBuilder.append(RIGHT_PAREN); - } - - @Override - protected void visit(Expr expr, Expr.CreateList createList) { - stringBuilder.append(LEFT_BRACKET); - for (int i = 0; i < createList.getElementsCount(); i++) { - if (i > 0) { - stringBuilder.append(COMMA).append(SPACE); - } - visit(createList.getElements(i)); - } - stringBuilder.append(RIGHT_BRACKET); - } - - @Override - protected void visit(Expr expr, Expr.CreateStruct createStruct) { - if (!createStruct.getMessageName().isEmpty()) { - stringBuilder.append(createStruct.getMessageName()); - } - stringBuilder.append(LEFT_BRACCE); - for (int i = 0; i < createStruct.getEntriesCount(); i++) { - if (i > 0) { - stringBuilder.append(COMMA).append(SPACE); - } - - Entry e = createStruct.getEntries(i); - switch (e.getKeyKindCase()) { - case FIELD_KEY: - stringBuilder.append(e.getFieldKey()); - break; - case MAP_KEY: - visit(e.getMapKey()); - break; - default: - throw new IllegalArgumentException( - String.format("unexpected struct: %s", createStruct)); - } - stringBuilder.append(COLON).append(SPACE); - visit(e.getValue()); - } - stringBuilder.append(RIGHT_BRACE); - } - - // TODO: comprehension unparsing should use macro call metadata - @Override - protected void visit(Expr expr, Expr.Comprehension comprehension) { - boolean nested = isComplexOperator(comprehension.getIterRange()); - visitMaybeNested(comprehension.getIterRange(), nested); - stringBuilder.append(DOT); - - if (comprehension - .getLoopStep() - .getCallExpr() - .getFunction() - .equals(Operator.LOGICAL_AND.getFunction())) { - visitAllMacro(comprehension); - return; - } - - if (comprehension - .getLoopStep() - .getCallExpr() - .getFunction() - .equals(Operator.LOGICAL_OR.getFunction())) { - visitExistsMacro(comprehension); - return; - } - - if (comprehension.getResult().getExprKindCase() == ExprKindCase.CALL_EXPR) { - visitExistsOneMacro(comprehension); - return; - } - - visitMapMacro(comprehension); - } - - private void visitUnary(Expr.Call expr, String op) { - if (expr.getArgsCount() != 1) { - throw new IllegalArgumentException(String.format("unexpected unary: %s", expr)); - } - stringBuilder.append(op); - boolean nested = isComplexOperator(expr.getArgs(0)); - visitMaybeNested(expr.getArgs(0), nested); - } - - private void visitBinary(Expr.Call expr, String op) { - if (expr.getArgsCount() != 2) { - throw new IllegalArgumentException(String.format("unexpected binary: %s", expr)); - } - - Expr lhs = expr.getArgs(0); - Expr rhs = expr.getArgs(1); - String fun = expr.getFunction(); - - // add parens if the current operator is lower precedence than the lhs expr - // operator. - boolean lhsParen = isComplexOperatorWithRespectTo(lhs, fun); - // add parens if the current operator is lower precedence than the rhs expr - // operator, or the same precedence and the operator is left recursive. - boolean rhsParen = isComplexOperatorWithRespectTo(rhs, fun); - if (!rhsParen && Operator.isOperatorLeftRecursive(fun)) { - rhsParen = isOperatorSamePrecedence(fun, rhs); - } - - visitMaybeNested(lhs, lhsParen); - stringBuilder.append(SPACE).append(op).append(SPACE); - visitMaybeNested(rhs, rhsParen); - } - - private void visitTernary(Expr.Call expr) { - if (expr.getArgsCount() != 3) { - throw new IllegalArgumentException(String.format("unexpected ternary: %s", expr)); - } - - boolean nested = - isOperatorSamePrecedence(Operator.CONDITIONAL.getFunction(), expr.getArgs(0)) - || isComplexOperator(expr.getArgs(0)); - visitMaybeNested(expr.getArgs(0), nested); - - stringBuilder.append(SPACE).append(QUESTION_MARK).append(SPACE); - - nested = - isOperatorSamePrecedence(Operator.CONDITIONAL.getFunction(), expr.getArgs(1)) - || isComplexOperator(expr.getArgs(1)); - visitMaybeNested(expr.getArgs(1), nested); - - stringBuilder.append(SPACE).append(COLON).append(SPACE); - - nested = - isOperatorSamePrecedence(Operator.CONDITIONAL.getFunction(), expr.getArgs(2)) - || isComplexOperator(expr.getArgs(2)); - visitMaybeNested(expr.getArgs(2), nested); - } - - private void visitIndex(Expr.Call expr) { - if (expr.getArgsCount() != 2) { - throw new IllegalArgumentException(String.format("unexpected index call: %s", expr)); - } - boolean nested = isBinaryOrTernaryOperator(expr.getArgs(0)); - visitMaybeNested(expr.getArgs(0), nested); - stringBuilder.append(LEFT_BRACKET); - visit(expr.getArgs(1)); - stringBuilder.append(RIGHT_BRACKET); - } - - private void visitMaybeNested(Expr expr, boolean nested) { - if (nested) { - stringBuilder.append(LEFT_PAREN); - } - visit(expr); - if (nested) { - stringBuilder.append(RIGHT_PAREN); - } - } - - private void visitAllMacro(Expr.Comprehension expr) { - if (expr.getLoopStep().getCallExpr().getArgsCount() != 2) { - throw new IllegalArgumentException(String.format("unexpected all macro: %s", expr)); - } - stringBuilder - .append(Operator.ALL.getFunction()) - .append(LEFT_PAREN) - .append(expr.getIterVar()) - .append(COMMA) - .append(SPACE); - visit(expr.getLoopStep().getCallExpr().getArgs(1)); - stringBuilder.append(RIGHT_PAREN); - } - - private void visitExistsMacro(Expr.Comprehension expr) { - if (expr.getLoopStep().getCallExpr().getArgsCount() != 2) { - throw new IllegalArgumentException(String.format("unexpected exists macro %s", expr)); - } - stringBuilder - .append(Operator.EXISTS.getFunction()) - .append(LEFT_PAREN) - .append(expr.getIterVar()) - .append(COMMA) - .append(SPACE); - visit(expr.getLoopStep().getCallExpr().getArgs(1)); - stringBuilder.append(RIGHT_PAREN); - } - - private void visitExistsOneMacro(Expr.Comprehension expr) { - if (expr.getLoopStep().getCallExpr().getArgsCount() != 3) { - throw new IllegalArgumentException(String.format("unexpected exists one macro: %s", expr)); - } - stringBuilder - .append(Operator.EXISTS_ONE.getFunction()) - .append(LEFT_PAREN) - .append(expr.getIterVar()) - .append(COMMA) - .append(SPACE); - visit(expr.getLoopStep().getCallExpr().getArgs(0)); - stringBuilder.append(RIGHT_PAREN); - } - - private void visitMapMacro(Expr.Comprehension expr) { - stringBuilder - .append(Operator.MAP.getFunction()) - .append(LEFT_PAREN) - .append(expr.getIterVar()) - .append(COMMA) - .append(SPACE); - Expr step = expr.getLoopStep(); - if (step.getCallExpr().getFunction().equals(Operator.CONDITIONAL.getFunction())) { - if (step.getCallExpr().getArgsCount() != 3) { - throw new IllegalArgumentException( - String.format("unexpected exists map macro filter step: %s", expr)); - } - visit(step.getCallExpr().getArgs(0)); - stringBuilder.append(COMMA).append(SPACE); - - Expr temp = step.getCallExpr().getArgs(1); - step = temp; - } - - if (step.getCallExpr().getArgsCount() != 2 - || step.getCallExpr().getArgs(1).getListExpr().getElementsCount() != 1) { - throw new IllegalArgumentException(String.format("unexpected exists map macro: %s", expr)); - } - visit(step.getCallExpr().getArgs(1).getListExpr().getElements(0)); - stringBuilder.append(RIGHT_PAREN); - } - - private boolean isBinaryOrTernaryOperator(Expr expr) { - if (!isComplexOperator(expr)) { - return false; - } - return Operator.lookupBinaryOperator(expr.getCallExpr().getFunction()).isPresent() - || isOperatorSamePrecedence(Operator.CONDITIONAL.getFunction(), expr); - } - - private boolean isComplexOperator(Expr expr) { - // If the arg is a call with more than one arg, return true - return expr.hasCallExpr() && expr.getCallExpr().getArgsCount() >= 2; - } - - private boolean isOperatorSamePrecedence(String op, Expr expr) { - if (!expr.hasCallExpr()) { - return false; - } - return Operator.lookupPrecedence(op) - == Operator.lookupPrecedence(expr.getCallExpr().getFunction()); - } - - private boolean isComplexOperatorWithRespectTo(Expr expr, String op) { - // If the arg is not a call with more than one arg, return false. - if (!expr.hasCallExpr() || expr.getCallExpr().getArgsCount() < 2) { - return false; - } - // Otherwise, return whether the given op has lower precedence than expr - return Operator.isOperatorLowerPrecedence(op, expr); - } - - // bytesToOctets converts byte sequences to a string using a three digit octal encoded value - // per byte. - private String bytesToOctets(ByteString bytes) { - StringBuilder sb = new StringBuilder(); - for (byte b : bytes.toByteArray()) { - sb.append(String.format("\\%03o", b)); - } - return sb.toString(); - } + public String unparse(CelAbstractSyntaxTree ast) { + CelUnparserVisitor unparserVisitor = new CelUnparserVisitor(ast); + return unparserVisitor.unparse(); } } diff --git a/parser/src/main/java/dev/cel/parser/CelUnparserVisitor.java b/parser/src/main/java/dev/cel/parser/CelUnparserVisitor.java new file mode 100644 index 00000000..04dd4b0e --- /dev/null +++ b/parser/src/main/java/dev/cel/parser/CelUnparserVisitor.java @@ -0,0 +1,322 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package dev.cel.parser; + +import com.google.protobuf.ByteString; +import dev.cel.common.CelAbstractSyntaxTree; +import dev.cel.common.CelSource; +import dev.cel.common.ast.CelConstant; +import dev.cel.common.ast.CelExpr; +import dev.cel.common.ast.CelExpr.CelCall; +import dev.cel.common.ast.CelExpr.CelComprehension; +import dev.cel.common.ast.CelExpr.CelCreateList; +import dev.cel.common.ast.CelExpr.CelCreateStruct; +import dev.cel.common.ast.CelExpr.CelIdent; +import dev.cel.common.ast.CelExpr.CelSelect; +import dev.cel.common.ast.CelExpr.ExprKind.Kind; +import dev.cel.common.ast.CelExprVisitor; +import java.util.Optional; + +/** Visitor implementation to unparse an AST. */ +public class CelUnparserVisitor extends CelExprVisitor { + protected static final String LEFT_PAREN = "("; + protected static final String RIGHT_PAREN = ")"; + protected static final String DOT = "."; + protected static final String COMMA = ","; + protected static final String SPACE = " "; + protected static final String LEFT_BRACKET = "["; + protected static final String RIGHT_BRACKET = "]"; + protected static final String LEFT_BRACE = "{"; + protected static final String RIGHT_BRACE = "}"; + protected static final String COLON = ":"; + protected static final String QUESTION_MARK = "?"; + + protected final CelAbstractSyntaxTree ast; + protected final CelSource sourceInfo; + protected final StringBuilder stringBuilder; + + /** Creates a new {@link CelUnparserVisitor}. */ + public CelUnparserVisitor(CelAbstractSyntaxTree ast) { + this.ast = ast; + this.sourceInfo = ast.getSource(); + this.stringBuilder = new StringBuilder(); + } + + public String unparse() { + visit(ast.getExpr()); + return stringBuilder.toString(); + } + + @Override + public void visit(CelExpr expr) { + if (sourceInfo.getMacroCalls().containsKey(expr.id())) { + visit(sourceInfo.getMacroCalls().get(expr.id())); + return; + } + super.visit(expr); + } + + @Override + protected void visit(CelExpr expr, CelConstant constant) { + switch (constant.getKind()) { + case STRING_VALUE: + stringBuilder.append("\"").append(constant.stringValue()).append("\""); + break; + case INT64_VALUE: + stringBuilder.append(constant.int64Value()); + break; + case UINT64_VALUE: + stringBuilder.append(constant.uint64Value()).append("u"); + break; + case BOOLEAN_VALUE: + stringBuilder.append(constant.booleanValue() ? "true" : "false"); + break; + case DOUBLE_VALUE: + stringBuilder.append(constant.doubleValue()); + break; + case NULL_VALUE: + stringBuilder.append("null"); + break; + case BYTES_VALUE: + stringBuilder.append("b\"").append(bytesToOctets(constant.bytesValue())).append("\""); + break; + default: + throw new IllegalArgumentException("unexpected expr kind"); + } + } + + @Override + protected void visit(CelExpr expr, CelIdent ident) { + stringBuilder.append(ident.name()); + } + + @Override + protected void visit(CelExpr expr, CelSelect select) { + if (select.testOnly()) { + stringBuilder.append(Operator.HAS.getFunction()).append(LEFT_PAREN); + } + CelExpr operand = select.operand(); + boolean nested = !select.testOnly() && isBinaryOrTernaryOperator(operand); + visitMaybeNested(operand, nested); + stringBuilder.append(DOT).append(select.field()); + if (select.testOnly()) { + stringBuilder.append(RIGHT_PAREN); + } + } + + @Override + protected void visit(CelExpr expr, CelCall call) { + String fun = call.function(); + + Optional op = Operator.lookupUnaryOperator(fun); + if (op.isPresent()) { + visitUnary(call, op.get()); + return; + } + + op = Operator.lookupBinaryOperator(fun); + if (op.isPresent()) { + visitBinary(call, op.get()); + return; + } + + if (fun.equals(Operator.INDEX.getFunction())) { + visitIndex(call); + return; + } + + if (fun.equals(Operator.CONDITIONAL.getFunction())) { + visitTernary(call); + return; + } + + if (call.target().isPresent()) { + boolean nested = isBinaryOrTernaryOperator(call.target().get()); + visitMaybeNested(call.target().get(), nested); + stringBuilder.append(DOT); + } + + stringBuilder.append(fun).append(LEFT_PAREN); + for (int i = 0; i < call.args().size(); i++) { + if (i > 0) { + stringBuilder.append(COMMA).append(SPACE); + } + visit(call.args().get(i)); + } + stringBuilder.append(RIGHT_PAREN); + } + + @Override + protected void visit(CelExpr expr, CelCreateList createList) { + stringBuilder.append(LEFT_BRACKET); + for (int i = 0; i < createList.elements().size(); i++) { + if (i > 0) { + stringBuilder.append(COMMA).append(SPACE); + } + visit(createList.elements().get(i)); + } + stringBuilder.append(RIGHT_BRACKET); + } + + @Override + protected void visit(CelExpr expr, CelCreateStruct createStruct) { + if (!createStruct.messageName().isEmpty()) { + stringBuilder.append(createStruct.messageName()); + } + stringBuilder.append(LEFT_BRACE); + for (int i = 0; i < createStruct.entries().size(); i++) { + if (i > 0) { + stringBuilder.append(COMMA).append(SPACE); + } + + CelCreateStruct.Entry e = createStruct.entries().get(i); + switch (e.keyKind().getKind()) { + case FIELD_KEY: + stringBuilder.append(e.keyKind().fieldKey()); + break; + case MAP_KEY: + visit(e.keyKind().mapKey()); + break; + } + stringBuilder.append(COLON).append(SPACE); + visit(e.value()); + } + stringBuilder.append(RIGHT_BRACE); + } + + @Override + protected void visit(CelExpr expr, CelComprehension comprehension) { + throw new UnsupportedOperationException( + "Comprehension unparsing requires macro calls to be populated. Ensure the option is" + + " enabled."); + } + + private void visitUnary(CelCall expr, String op) { + if (expr.args().size() != 1) { + throw new IllegalArgumentException(String.format("unexpected unary: %s", expr)); + } + stringBuilder.append(op); + boolean nested = isComplexOperator(expr.args().get(0)); + visitMaybeNested(expr.args().get(0), nested); + } + + private void visitBinary(CelCall expr, String op) { + if (expr.args().size() != 2) { + throw new IllegalArgumentException(String.format("unexpected binary: %s", expr)); + } + + CelExpr lhs = expr.args().get(0); + CelExpr rhs = expr.args().get(1); + String fun = expr.function(); + + // add parens if the current operator is lower precedence than the lhs expr + // operator. + boolean lhsParen = isComplexOperatorWithRespectTo(lhs, fun); + // add parens if the current operator is lower precedence than the rhs expr + // operator, or the same precedence and the operator is left recursive. + boolean rhsParen = isComplexOperatorWithRespectTo(rhs, fun); + if (!rhsParen && Operator.isOperatorLeftRecursive(fun)) { + rhsParen = isOperatorSamePrecedence(fun, rhs); + } + + visitMaybeNested(lhs, lhsParen); + stringBuilder.append(SPACE).append(op).append(SPACE); + visitMaybeNested(rhs, rhsParen); + } + + private void visitTernary(CelCall expr) { + if (expr.args().size() != 3) { + throw new IllegalArgumentException(String.format("unexpected ternary: %s", expr)); + } + + boolean nested = + isOperatorSamePrecedence(Operator.CONDITIONAL.getFunction(), expr.args().get(0)) + || isComplexOperator(expr.args().get(0)); + visitMaybeNested(expr.args().get(0), nested); + + stringBuilder.append(SPACE).append(QUESTION_MARK).append(SPACE); + + nested = + isOperatorSamePrecedence(Operator.CONDITIONAL.getFunction(), expr.args().get(1)) + || isComplexOperator(expr.args().get(1)); + visitMaybeNested(expr.args().get(1), nested); + + stringBuilder.append(SPACE).append(COLON).append(SPACE); + + nested = + isOperatorSamePrecedence(Operator.CONDITIONAL.getFunction(), expr.args().get(2)) + || isComplexOperator(expr.args().get(2)); + visitMaybeNested(expr.args().get(2), nested); + } + + private void visitIndex(CelCall expr) { + if (expr.args().size() != 2) { + throw new IllegalArgumentException(String.format("unexpected index call: %s", expr)); + } + boolean nested = isBinaryOrTernaryOperator(expr.args().get(0)); + visitMaybeNested(expr.args().get(0), nested); + stringBuilder.append(LEFT_BRACKET); + visit(expr.args().get(1)); + stringBuilder.append(RIGHT_BRACKET); + } + + private void visitMaybeNested(CelExpr expr, boolean nested) { + if (nested) { + stringBuilder.append(LEFT_PAREN); + } + visit(expr); + if (nested) { + stringBuilder.append(RIGHT_PAREN); + } + } + + private boolean isBinaryOrTernaryOperator(CelExpr expr) { + if (!isComplexOperator(expr)) { + return false; + } + return Operator.lookupBinaryOperator(expr.call().function()).isPresent() + || isOperatorSamePrecedence(Operator.CONDITIONAL.getFunction(), expr); + } + + private boolean isComplexOperator(CelExpr expr) { + // If the arg is a call with more than one arg, return true + return expr.exprKind().getKind().equals(Kind.CALL) && expr.call().args().size() >= 2; + } + + private boolean isOperatorSamePrecedence(String op, CelExpr expr) { + if (!expr.exprKind().getKind().equals(Kind.CALL)) { + return false; + } + return Operator.lookupPrecedence(op) == Operator.lookupPrecedence(expr.call().function()); + } + + private boolean isComplexOperatorWithRespectTo(CelExpr expr, String op) { + // If the arg is not a call with more than one arg, return false. + if (!expr.exprKind().getKind().equals(Kind.CALL) || expr.call().args().size() < 2) { + return false; + } + // Otherwise, return whether the given op has lower precedence than expr + return Operator.isOperatorLowerPrecedence(op, expr); + } + + // bytesToOctets converts byte sequences to a string using a three digit octal encoded value + // per byte. + private String bytesToOctets(ByteString bytes) { + StringBuilder sb = new StringBuilder(); + for (byte b : bytes.toByteArray()) { + sb.append(String.format("\\%03o", b)); + } + return sb.toString(); + } +} diff --git a/parser/src/main/java/dev/cel/parser/Operator.java b/parser/src/main/java/dev/cel/parser/Operator.java index f804f86c..8ae1b546 100644 --- a/parser/src/main/java/dev/cel/parser/Operator.java +++ b/parser/src/main/java/dev/cel/parser/Operator.java @@ -14,8 +14,8 @@ package dev.cel.parser; -import dev.cel.expr.Expr; import com.google.common.collect.ImmutableMap; +import dev.cel.common.ast.CelExpr; import java.util.Objects; import java.util.Optional; @@ -60,11 +60,11 @@ public enum Operator { private final String functionName; private final String displayName; - private Operator(String functionName) { + Operator(String functionName) { this(functionName, ""); } - private Operator(String functionName, String displayName) { + Operator(String functionName, String displayName) { this.functionName = functionName; this.displayName = displayName; } @@ -193,11 +193,11 @@ static Optional lookupBinaryOperator(String op) { return Optional.ofNullable(BINARY_OPERATORS.get(op)); } - static boolean isOperatorLowerPrecedence(String op, Expr expr) { - if (!expr.hasCallExpr()) { + static boolean isOperatorLowerPrecedence(String op, CelExpr expr) { + if (!expr.exprKind().getKind().equals(CelExpr.ExprKind.Kind.CALL)) { return false; } - return lookupPrecedence(op) < lookupPrecedence(expr.getCallExpr().getFunction()); + return lookupPrecedence(op) < lookupPrecedence(expr.call().function()); } static boolean isOperatorLeftRecursive(String op) { diff --git a/parser/src/test/java/dev/cel/parser/BUILD.bazel b/parser/src/test/java/dev/cel/parser/BUILD.bazel index 9d5598d4..270c412c 100644 --- a/parser/src/test/java/dev/cel/parser/BUILD.bazel +++ b/parser/src/test/java/dev/cel/parser/BUILD.bazel @@ -15,14 +15,8 @@ java_library( "//common", "//common:compiler_common", "//common:options", - "//common:proto_ast", "//common/ast", "//common/internal", - "//common/resources/testdata/proto2:messages_extensions_proto2_java_proto", - "//common/resources/testdata/proto2:messages_proto2_java_proto", - "//common/resources/testdata/proto2:test_all_types_java_proto", - "//common/resources/testdata/proto3:standalone_global_enum_java_proto", - "//common/resources/testdata/proto3:test_all_types_java_proto", "//extensions:optional_library", "//parser", "//parser:macro", @@ -37,9 +31,7 @@ java_library( "@maven//:com_google_guava_guava_testlib", "@maven//:com_google_protobuf_protobuf_java", "@maven//:com_google_testparameterinjector_test_parameter_injector", - "@maven//:com_google_truth_extensions_truth_proto_extension", "@maven//:junit_junit", - "@maven//:org_jspecify_jspecify", ], ) diff --git a/parser/src/test/java/dev/cel/parser/CelUnparserImplTest.java b/parser/src/test/java/dev/cel/parser/CelUnparserImplTest.java index dfea979a..b8d1cae7 100644 --- a/parser/src/test/java/dev/cel/parser/CelUnparserImplTest.java +++ b/parser/src/test/java/dev/cel/parser/CelUnparserImplTest.java @@ -15,20 +15,18 @@ package dev.cel.parser; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; import static org.junit.Assert.assertThrows; -import dev.cel.expr.Constant; -import dev.cel.expr.Expr; -import dev.cel.expr.Expr.Call; -import dev.cel.expr.Expr.CreateStruct; -import dev.cel.expr.Expr.CreateStruct.Entry; -import dev.cel.expr.ParsedExpr; import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameter.TestParameterValuesProvider; import com.google.testing.junit.testparameterinjector.TestParameterInjector; +import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.CelOptions; import dev.cel.common.CelProtoAbstractSyntaxTree; +import dev.cel.common.CelSource; +import dev.cel.common.ast.CelExpr; +import dev.cel.common.ast.CelExpr.CelCall; +import dev.cel.common.ast.CelExpr.CelCreateStruct; import java.util.Arrays; import java.util.List; import org.junit.Test; @@ -162,51 +160,61 @@ public List provideValues() { public void unparse_succeeds( @TestParameter(valuesProvider = ValidExprDataProvider.class) String originalExpr) throws Exception { - ParsedExpr parsedExprOne = - CelProtoAbstractSyntaxTree.fromCelAst(parser.parse(originalExpr, "unparser").getAst()) - .toParsedExpr(); + CelAbstractSyntaxTree astOne = parser.parse(originalExpr, "unparser").getAst(); - String unparsedResult = unparser.unparse(parsedExprOne); + String unparsedResult = unparser.unparse(astOne); assertThat(originalExpr).isEqualTo(unparsedResult); // parse again, confirm it's the same result - ParsedExpr parsedExprTwo = - CelProtoAbstractSyntaxTree.fromCelAst(parser.parse(unparsedResult, "unparser").getAst()) - .toParsedExpr(); - assertThat(parsedExprTwo).isEqualTo(parsedExprOne); + CelAbstractSyntaxTree astTwo = parser.parse(unparsedResult, "unparser").getAst(); + + assertThat(CelProtoAbstractSyntaxTree.fromCelAst(astTwo).toParsedExpr()) + .isEqualTo(CelProtoAbstractSyntaxTree.fromCelAst(astOne).toParsedExpr()); } private static final class InvalidExprDataProvider implements TestParameterValuesProvider { @Override - public List provideValues() { + public List provideValues() { return Arrays.asList( - Expr.getDefaultInstance(), // empty expr - Expr.newBuilder().setConstExpr(Constant.getDefaultInstance()).build(), // bad_constant - Expr.newBuilder() - .setCallExpr( - Call.newBuilder() + CelExpr.newBuilder().build(), // empty expr + CelExpr.newBuilder() + .setCall( + CelCall.newBuilder() .setFunction("_&&_") - .addArgs(Expr.getDefaultInstance()) - .addArgs(Expr.getDefaultInstance()) + .addArgs(CelExpr.newBuilder().build()) + .addArgs(CelExpr.newBuilder().build()) .build()) .build(), // bad args - Expr.newBuilder() - .setStructExpr( - CreateStruct.newBuilder() + CelExpr.newBuilder() + .setCreateStruct( + CelCreateStruct.newBuilder() .setMessageName("Msg") - .addEntries(Entry.newBuilder().setFieldKey("field").build()) + .addEntries( + CelCreateStruct.Entry.newBuilder() + .setId(0) + .setValue(CelExpr.newBuilder().build()) + .setFieldKey("field") + .build()) .build()) .build(), // bad struct - Expr.newBuilder() - .setStructExpr( - CreateStruct.newBuilder() + CelExpr.newBuilder() + .setCreateStruct( + CelCreateStruct.newBuilder() .setMessageName("Msg") - .addEntries(Entry.newBuilder().setMapKey(Expr.getDefaultInstance()).build()) + .addEntries( + CelCreateStruct.Entry.newBuilder() + .setId(0) + .setValue(CelExpr.newBuilder().build()) + .setMapKey(CelExpr.newBuilder().build()) + .build()) .build()) .build(), // bad map - Expr.newBuilder() - .setCallExpr( - Call.newBuilder().setFunction("_[_]").addArgs(Expr.getDefaultInstance()).build()) + CelExpr.newBuilder() + .setCall( + CelCall.newBuilder() + .setFunction("_[_]") + .addArgs(CelExpr.newBuilder().build()) + .build()) .build() // bad index ); } @@ -214,12 +222,45 @@ public List provideValues() { @Test public void unparse_fails( - @TestParameter(valuesProvider = InvalidExprDataProvider.class) Expr invalidExpr) { + @TestParameter(valuesProvider = InvalidExprDataProvider.class) CelExpr invalidExpr) { Throwable thrown = assertThrows( Throwable.class, - () -> unparser.unparse(ParsedExpr.newBuilder().setExpr(invalidExpr).build())); + () -> + unparser.unparse( + new CelAbstractSyntaxTree(invalidExpr, CelSource.newBuilder().build()))); assertThat(thrown).hasMessageThat().contains("unexpected"); } + + @Test + public void unparse_comprehensionWithoutMacroCallTracking_presenceTestSucceeds() + throws Exception { + CelParserImpl parser = + CelParserImpl.newBuilder() + .setOptions(CelOptions.newBuilder().populateMacroCalls(false).build()) + .addMacros(CelMacro.STANDARD_MACROS) + .build(); + CelAbstractSyntaxTree ast = parser.parse("has(hello.world)").getAst(); + + assertThat(unparser.unparse(ast)).isEqualTo("has(hello.world)"); + } + + @Test + public void unparse_comprehensionWithoutMacroCallTracking_throwsException() throws Exception { + CelParserImpl parser = + CelParserImpl.newBuilder() + .setOptions(CelOptions.newBuilder().populateMacroCalls(false).build()) + .addMacros(CelMacro.STANDARD_MACROS) + .build(); + CelAbstractSyntaxTree ast = parser.parse("[1, 2, 3].all(x, x > 0)").getAst(); + + UnsupportedOperationException e = + assertThrows(UnsupportedOperationException.class, () -> unparser.unparse(ast)); + assertThat(e) + .hasMessageThat() + .contains( + "Comprehension unparsing requires macro calls to be populated. Ensure the option is" + + " enabled."); + } } diff --git a/parser/src/test/java/dev/cel/parser/OperatorTest.java b/parser/src/test/java/dev/cel/parser/OperatorTest.java index 9967751a..93346e63 100644 --- a/parser/src/test/java/dev/cel/parser/OperatorTest.java +++ b/parser/src/test/java/dev/cel/parser/OperatorTest.java @@ -19,10 +19,10 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import dev.cel.expr.Expr; -import dev.cel.expr.Expr.Call; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import com.google.testing.junit.testparameterinjector.TestParameters; +import dev.cel.common.ast.CelExpr; +import dev.cel.common.ast.CelExpr.CelCall; import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; @@ -155,7 +155,8 @@ public void lookupBinaryOperator_empty(String operator) { "{operator1: '_!=_', operator2: '_?_:_'}", }) public void operatorLowerPrecedence(String operator1, String operator2) { - Expr expr = Expr.newBuilder().setCallExpr(Call.newBuilder().setFunction(operator2)).build(); + CelExpr expr = + CelExpr.newBuilder().setCall(CelCall.newBuilder().setFunction(operator2).build()).build(); assertTrue(Operator.isOperatorLowerPrecedence(operator1, expr)); } @@ -170,18 +171,12 @@ public void operatorLowerPrecedence(String operator1, String operator2) { "{operator1: '_!=_', operator2: '_-_'}", }) public void operatorNotLowerPrecedence(String operator1, String operator2) { - Expr expr = Expr.newBuilder().setCallExpr(Call.newBuilder().setFunction(operator2)).build(); + CelExpr expr = + CelExpr.newBuilder().setCall(CelCall.newBuilder().setFunction(operator2).build()).build(); assertFalse(Operator.isOperatorLowerPrecedence(operator1, expr)); } - @Test - public void operatorNotLowerPrecedenceWhenNoCallExpr() { - assertFalse( - Operator.isOperatorLowerPrecedence( - "_?_:_", Expr.newBuilder().setCallExpr(Call.getDefaultInstance()).build())); - } - @Test @TestParameters({ "{operator: '_[_]'}",