diff --git a/common/ast/BUILD.bazel b/common/ast/BUILD.bazel index 9b7573a7c..4d47d1a05 100644 --- a/common/ast/BUILD.bazel +++ b/common/ast/BUILD.bazel @@ -48,6 +48,11 @@ java_library( exports = ["//common/src/main/java/dev/cel/common/ast:cel_expr_visitor"], ) +cel_android_library( + name = "cel_expr_visitor_android", + exports = ["//common/src/main/java/dev/cel/common/ast:cel_expr_visitor_android"], +) + java_library( name = "expr_factory", exports = ["//common/src/main/java/dev/cel/common/ast:expr_factory"], diff --git a/common/src/main/java/dev/cel/common/BUILD.bazel b/common/src/main/java/dev/cel/common/BUILD.bazel index cb9487b8c..e2d33b0a1 100644 --- a/common/src/main/java/dev/cel/common/BUILD.bazel +++ b/common/src/main/java/dev/cel/common/BUILD.bazel @@ -350,6 +350,7 @@ java_library( "//common/ast", "//common/ast:mutable_expr", "//common/types:type_providers", + "@maven//:com_google_guava_guava", ], ) diff --git a/common/src/main/java/dev/cel/common/CelMutableAst.java b/common/src/main/java/dev/cel/common/CelMutableAst.java index dd57d8b4e..73212433b 100644 --- a/common/src/main/java/dev/cel/common/CelMutableAst.java +++ b/common/src/main/java/dev/cel/common/CelMutableAst.java @@ -14,11 +14,11 @@ package dev.cel.common; +import com.google.common.collect.ImmutableMap; import dev.cel.common.ast.CelMutableExpr; import dev.cel.common.ast.CelMutableExprConverter; import dev.cel.common.ast.CelReference; import dev.cel.common.types.CelType; -import java.util.HashMap; import java.util.Map; import java.util.Optional; @@ -32,8 +32,8 @@ public final class CelMutableAst { private final CelMutableExpr mutatedExpr; private final CelMutableSource source; - private final Map references; - private final Map types; + private final ImmutableMap references; + private final ImmutableMap types; /** Returns the underlying {@link CelMutableExpr} representation of the abstract syntax tree. */ public CelMutableExpr expr() { @@ -68,6 +68,11 @@ public Optional getType(long exprId) { return Optional.ofNullable(types.get(exprId)); } + /** Returns the type map of the abstract syntax tree. */ + public ImmutableMap getTypeMap() { + return types; + } + /** Converts this mutable AST into a parsed {@link CelAbstractSyntaxTree}. */ public CelAbstractSyntaxTree toParsedAst() { return toParsedAst(false); @@ -102,11 +107,16 @@ public static CelMutableAst fromCelAst(CelAbstractSyntaxTree ast) { * builder. */ public static CelMutableAst of(CelMutableExpr mutableExpr, CelMutableSource mutableSource) { - return new CelMutableAst(mutableExpr, mutableSource); + return new CelMutableAst(mutableExpr, mutableSource, ImmutableMap.of(), ImmutableMap.of()); } - private CelMutableAst(CelMutableExpr mutatedExpr, CelMutableSource mutableSource) { - this(mutatedExpr, mutableSource, new HashMap<>(), new HashMap<>()); + /** + * Constructs an instance of {@link CelMutableAst} with the mutable expression, its source + * builder, and node types. + */ + public static CelMutableAst of( + CelMutableExpr mutableExpr, CelMutableSource mutableSource, Map types) { + return new CelMutableAst(mutableExpr, mutableSource, ImmutableMap.of(), types); } private CelMutableAst( @@ -116,7 +126,7 @@ private CelMutableAst( Map types) { this.mutatedExpr = mutatedExpr; this.source = mutableSource; - this.references = new HashMap<>(references); - this.types = new HashMap<>(types); + this.references = ImmutableMap.copyOf(references); + this.types = ImmutableMap.copyOf(types); } } diff --git a/common/src/main/java/dev/cel/common/ast/BUILD.bazel b/common/src/main/java/dev/cel/common/ast/BUILD.bazel index 14cb75dd9..73c53e13d 100644 --- a/common/src/main/java/dev/cel/common/ast/BUILD.bazel +++ b/common/src/main/java/dev/cel/common/ast/BUILD.bazel @@ -7,6 +7,7 @@ package( ], default_visibility = [ "//common/ast:__pkg__", + "//publish:__pkg__", ], ) @@ -64,9 +65,9 @@ java_library( ], deps = [ ":ast", + ":cel_expr_visitor", "//common:cel_ast", "//common/annotations", - "//common/navigation", "@maven//:com_google_guava_guava", ], ) @@ -78,9 +79,9 @@ cel_android_library( ], deps = [ ":ast_android", + ":cel_expr_visitor_android", "//common:cel_ast_android", "//common/annotations", - "//common/navigation:navigation_android", "@maven_android//:com_google_guava_guava", ], ) @@ -143,6 +144,17 @@ java_library( ], ) +cel_android_library( + name = "cel_expr_visitor_android", + srcs = ["CelExprVisitor.java"], + tags = [ + ], + deps = [ + ":ast_android", + "//common:cel_ast_android", + ], +) + java_library( name = "expr_factory", srcs = EXPR_FACTORY_SOURCES, diff --git a/common/src/main/java/dev/cel/common/ast/CelBlock.java b/common/src/main/java/dev/cel/common/ast/CelBlock.java index 12de6d4dd..6704e2cfa 100644 --- a/common/src/main/java/dev/cel/common/ast/CelBlock.java +++ b/common/src/main/java/dev/cel/common/ast/CelBlock.java @@ -14,13 +14,14 @@ package dev.cel.common.ast; -import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.base.Preconditions.checkArgument; -import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.annotations.Internal; -import dev.cel.common.navigation.CelNavigableExpr; +import dev.cel.common.ast.CelExpr.CelCall; +import dev.cel.common.ast.CelExpr.CelIdent; +import dev.cel.common.ast.CelExpr.ExprKind.Kind; import java.util.Optional; /** @@ -36,10 +37,6 @@ public final class CelBlock { private final CelExpr blockExpr; - private CelBlock(CelExpr blockExpr) { - this.blockExpr = blockExpr; - } - public ImmutableList indices() { return blockExpr.call().args().get(0).list().elements(); } @@ -61,84 +58,79 @@ public CelExpr expr() { * @throws IllegalArgumentException if the block is malformed or its indices are invalid. */ public static Optional extract(CelAbstractSyntaxTree ast) { - CelNavigableExpr celNavigableExpr = CelNavigableExpr.fromExpr(ast.getExpr()); - - ImmutableList allCelBlocks = - celNavigableExpr - .allNodes() - .map(CelNavigableExpr::expr) - .filter(expr -> expr.callOrDefault().function().equals(FUNCTION_NAME)) - .collect(toImmutableList()); - if (allCelBlocks.isEmpty()) { + CelExpr root = ast.getExpr(); + BlockValidator validator = new BlockValidator(root); + if (!isBlockCall(root)) { + validator.validate(root, 0); return Optional.empty(); } - Preconditions.checkArgument( - allCelBlocks.size() == 1, - "Expected 1 cel.block function to be present but found %s", - allCelBlocks.size()); - Preconditions.checkArgument( - celNavigableExpr.expr().equals(allCelBlocks.get(0)), - "Expected cel.block to be present at root"); - - return Optional.of(fromExpr(allCelBlocks.get(0))); - } - - /** - * Constructs a {@link CelBlock} from a {@link CelExpr}. - * - * @throws IllegalArgumentException if the expression is not a valid block. - */ - private static CelBlock fromExpr(CelExpr expr) { - Preconditions.checkArgument( - expr.exprKind().getKind() == CelExpr.ExprKind.Kind.CALL, - "Expected cel.@block to be a call expression"); - Preconditions.checkArgument( - expr.call().function().equals(FUNCTION_NAME), "Expected function to be cel.@block"); - Preconditions.checkArgument( - expr.call().args().size() == 2, "Expected exactly 2 arguments for cel.@block"); - Preconditions.checkArgument( - expr.call().args().get(0).exprKind().getKind() == CelExpr.ExprKind.Kind.LIST, - "Expected first argument of cel.@block to be a list"); - - CelBlock block = new CelBlock(expr); - - // Assert correctness on block indices used in subexpressions + CelBlock block = new CelBlock(root); ImmutableList subexprs = block.indices(); for (int i = 0; i < subexprs.size(); i++) { - verifyBlockIndex(subexprs.get(i), i, expr); + validator.validate(subexprs.get(i), i); } - // Assert correctness on block indices used in block result - CelExpr blockResult = block.result(); - verifyBlockIndex(blockResult, subexprs.size(), expr); - boolean resultHasAtLeastOneBlockIndex = - CelNavigableExpr.fromExpr(blockResult) - .allNodes() - .map(CelNavigableExpr::expr) - .anyMatch(e -> e.identOrDefault().name().startsWith(INDEX_PREFIX)); - Preconditions.checkArgument( - resultHasAtLeastOneBlockIndex, + checkArgument( + validator.validate(block.result(), subexprs.size()), "Expected at least one reference of index in cel.block result"); - return block; + return Optional.of(block); + } + + private static boolean isBlockCall(CelExpr expr) { + return expr.getKind().equals(Kind.CALL) && expr.call().function().equals(FUNCTION_NAME); + } + + private static final class BlockValidator extends CelExprVisitor { + private final CelExpr root; + private int maxIndexValue; + private boolean hasBlockIndex; + + private boolean validate(CelExpr expr, int maxIndexValue) { + this.maxIndexValue = maxIndexValue; + this.hasBlockIndex = false; + visit(expr); + return hasBlockIndex; + } + + @Override + public void visit(CelExpr expr) { + if (!expr.getKind().equals(Kind.NOT_SET)) { + super.visit(expr); + } + } + + @Override + protected void visit(CelExpr expr, CelCall call) { + if (call.function().equals(FUNCTION_NAME)) { + throw new IllegalArgumentException( + isBlockCall(root) + ? "Expected 1 cel.block function to be present but found 2" + : "Expected cel.block to be present at root"); + } + super.visit(expr, call); + } + + @Override + protected void visit(CelExpr expr, CelIdent ident) { + if (ident.name().startsWith(INDEX_PREFIX)) { + hasBlockIndex = true; + int indexValue = Integer.parseInt(ident.name().substring(INDEX_PREFIX.length())); + checkArgument( + indexValue < maxIndexValue, + "Illegal block index found. The index value must be less than %s. Expr: %s", + maxIndexValue, + root); + } + } + + private BlockValidator(CelExpr root) { + this.root = root; + } } - private static void verifyBlockIndex(CelExpr celExpr, int maxIndexValue, CelExpr rootBlock) { - boolean areAllIndicesValid = - CelNavigableExpr.fromExpr(celExpr) - .allNodes() - .map(CelNavigableExpr::expr) - .filter(expr -> expr.identOrDefault().name().startsWith(INDEX_PREFIX)) - .map(CelExpr::ident) - .allMatch( - blockIdent -> - Integer.parseInt(blockIdent.name().substring(INDEX_PREFIX.length())) - < maxIndexValue); - Preconditions.checkArgument( - areAllIndicesValid, - "Illegal block index found. The index value must be less than %s. Expr: %s", - maxIndexValue, - rootBlock); + private CelBlock(CelExpr blockExpr) { + this.blockExpr = blockExpr; } } diff --git a/optimizer/src/main/java/dev/cel/optimizer/AstMutator.java b/optimizer/src/main/java/dev/cel/optimizer/AstMutator.java index 04db3ab99..76562f683 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/AstMutator.java +++ b/optimizer/src/main/java/dev/cel/optimizer/AstMutator.java @@ -16,6 +16,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; +import static com.google.common.collect.Streams.stream; import static java.lang.Math.max; import static java.util.stream.Collectors.toCollection; @@ -26,7 +27,6 @@ import com.google.common.collect.HashBasedTable; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Streams; import com.google.common.collect.Table; import com.google.errorprone.annotations.Immutable; import dev.cel.common.CelAbstractSyntaxTree; @@ -283,7 +283,7 @@ public MangledComprehensionAst mangleComprehensionIdentifierNames( } return MangledComprehensionAst.of( - CelMutableAst.of(mutatedComprehensionExpr, newSource), + CelMutableAst.of(mutatedComprehensionExpr, newSource, ast.getTypeMap()), ImmutableMap.copyOf(mangledIdentNamesToType)); } @@ -506,6 +506,11 @@ public CelMutableAst replaceSubtree( CelMutableExpr mutatedRoot = mutateExpr(stableIdGenerator::renumberId, ast.expr(), newAst.expr(), exprIdToReplace); + ImmutableMap remappedTypes = + ast.getTypeMap().entrySet().stream() + .filter(e -> stableIdGenerator.hasId(e.getKey())) + .collect( + toImmutableMap(e -> stableIdGenerator.renumberId(e.getKey()), Entry::getValue)); CelMutableSource newAstSource = CelMutableSource.newInstance().setDescription(ast.source().getDescription()); if (!ast.source().getMacroCalls().isEmpty()) { @@ -521,7 +526,7 @@ public CelMutableAst replaceSubtree( newAstSource = normalizeMacroSource( newAstSource, exprIdToReplace, mutatedRoot, stableIdGenerator::renumberId); - return CelMutableAst.of(mutatedRoot, newAstSource); + return CelMutableAst.of(mutatedRoot, newAstSource, remappedTypes); } /** @@ -625,7 +630,7 @@ public CelMutableAst mutateUntilFixedPoint( navAst .getRoot() .allNodes(traversalOrder) - .flatMap(node -> Streams.stream(nodeRewriter.apply(node))) + .flatMap(node -> stream(nodeRewriter.apply(node))) .findFirst()); } diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel b/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel index 694dfc04e..8fa80261a 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel @@ -71,8 +71,6 @@ java_library( "//common/navigation", "//common/navigation:common", "//common/navigation:mutable_navigation", - "//common/types", - "//common/types:type_providers", "//extensions:bindings", "//optimizer:ast_optimizer", "//optimizer:mutable_ast", diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java index 93aedd2cb..161f2bbb9 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java @@ -24,7 +24,6 @@ import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; @@ -32,32 +31,24 @@ import com.google.common.collect.Streams; import com.google.errorprone.annotations.CanIgnoreReturnValue; import dev.cel.bundle.Cel; -import dev.cel.bundle.CelBuilder; import dev.cel.common.CelAbstractSyntaxTree; -import dev.cel.common.CelFunctionDecl; import dev.cel.common.CelMutableAst; import dev.cel.common.CelMutableSource; import dev.cel.common.CelSource; import dev.cel.common.CelSource.Extension; import dev.cel.common.CelSource.Extension.Component; import dev.cel.common.CelSource.Extension.Version; -import dev.cel.common.CelValidationException; import dev.cel.common.CelVarDecl; import dev.cel.common.ast.CelBlock; 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.CelList; import dev.cel.common.ast.CelExpr.ExprKind.Kind; import dev.cel.common.ast.CelMutableExpr; import dev.cel.common.ast.CelMutableExpr.CelMutableComprehension; -import dev.cel.common.ast.CelMutableExprConverter; import dev.cel.common.navigation.CelNavigableExpr; import dev.cel.common.navigation.CelNavigableMutableAst; import dev.cel.common.navigation.CelNavigableMutableExpr; import dev.cel.common.navigation.TraversalOrder; -import dev.cel.common.types.CelType; -import dev.cel.common.types.SimpleType; import dev.cel.extensions.CelBindingsExtensions; import dev.cel.optimizer.AstMutator; import dev.cel.optimizer.AstMutator.MangledComprehensionAst; @@ -65,8 +56,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Comparator; +import java.util.HashMap; import java.util.HashSet; -import java.util.List; +import java.util.Map; +import java.util.NoSuchElementException; import java.util.Optional; import java.util.Set; import java.util.stream.Stream; @@ -96,7 +89,6 @@ public final class SubexpressionOptimizer implements CelAstOptimizer { private static final SubexpressionOptimizer INSTANCE = new SubexpressionOptimizer(SubexpressionOptimizerOptions.newBuilder().build()); - private static final String BIND_IDENTIFIER_PREFIX = "@r"; private static final Extension CEL_BLOCK_AST_EXTENSION_TAG = Extension.create("cel_block", Version.of(1L, 1L), Component.COMPONENT_RUNTIME); @@ -133,14 +125,14 @@ public static SubexpressionOptimizer newInstance(SubexpressionOptimizerOptions c */ @Override public OptimizationResult optimize(CelAbstractSyntaxTree ast, Cel cel) { - OptimizationResult result = optimizeUsingCelBlock(ast, cel); + OptimizationResult result = optimizeUsingCelBlock(ast); verifyOptimizedAstCorrectness(result.optimizedAst()); return result; } - private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel cel) { + private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast) { boolean hasSelectOptimizedNodes = CelNavigableExpr.fromExpr(ast.getExpr()) .allNodes() @@ -166,21 +158,25 @@ private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel astToModify = mangledComprehensionAst.mutableAst(); CelMutableSource sourceToModify = astToModify.source(); - int blockIdentifierIndex = 0; int iterCount; ArrayList subexpressions = new ArrayList<>(); + ImmutableList.Builder newVarDecls = ImmutableList.builder(); for (iterCount = 0; iterCount < cseOptions.iterationLimit(); iterCount++) { CelNavigableMutableAst navAst = CelNavigableMutableAst.fromAst(astToModify); - List cseCandidates = getCseCandidates(navAst); - if (cseCandidates.isEmpty()) { + CelMutableExpr cseCandidate = findCseCandidate(navAst).orElse(null); + if (cseCandidate == null) { break; } - CelMutableExpr targetCseShape = normalizeForEquality(cseCandidates.get(0)); - subexpressions.add(cseCandidates.get(0)); + CelMutableExpr targetCseShape = normalizeForEquality(cseCandidate); + subexpressions.add(cseCandidate); - String blockIdentifier = CelBlock.INDEX_PREFIX + blockIdentifierIndex++; + String blockIdentifier = CelBlock.INDEX_PREFIX + iterCount; + newVarDecls.add( + CelVarDecl.newVarDeclaration( + blockIdentifier, + astToModify.getType(cseCandidate.id()).orElseThrow(NoSuchElementException::new))); // Replace all CSE candidates with new block index identifier astToModify = @@ -193,7 +189,7 @@ private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel // Retain the existing macro calls in case if the block identifiers are replacing a subtree // that contains a comprehension. sourceToModify.addAllMacroCalls(astToModify.source().getMacroCalls()); - astToModify = CelMutableAst.of(astToModify.expr(), sourceToModify); + astToModify = CelMutableAst.of(astToModify.expr(), sourceToModify, astToModify.getTypeMap()); } if (iterCount >= cseOptions.iterationLimit()) { @@ -205,8 +201,6 @@ private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel return OptimizationResult.create(ast); } - ImmutableList.Builder newVarDecls = ImmutableList.builder(); - // Add all mangled comprehension identifiers to the environment, so that the subexpressions can // retain context to them. mangledComprehensionAst @@ -227,9 +221,6 @@ private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel newVarDecls.add(CelVarDecl.newVarDeclaration(name.resultName(), type.resultType())); }); - // Type-check all sub-expressions then create new block index identifiers. - newVarDecls.addAll(newBlockIndexVariableDeclarations(cel, newVarDecls.build(), subexpressions)); - // Wrap the optimized expression in cel.block astToModify = astMutator.wrapAstWithNewCelBlock(CelBlock.FUNCTION_NAME, astToModify, subexpressions); @@ -251,27 +242,18 @@ private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel */ @VisibleForTesting static void verifyOptimizedAstCorrectness(CelAbstractSyntaxTree ast) { - CelBlock celBlock = CelBlock.extract(ast).orElse(null); - if (celBlock == null) { - return; - } - - verifyNoInvalidScopedMangledVariables(celBlock.expr()); + CelBlock.extract(ast).ifPresent(SubexpressionOptimizer::verifyNoInvalidScopedMangledVariables); } - private static void verifyNoInvalidScopedMangledVariables(CelExpr celExpr) { - CelCall celBlockCall = celExpr.call(); - CelExpr blockBody = celBlockCall.args().get(1); - + private static void verifyNoInvalidScopedMangledVariables(CelBlock celBlock) { ImmutableSet allMangledVariablesInBlockBody = - CelNavigableExpr.fromExpr(blockBody) + CelNavigableExpr.fromExpr(celBlock.result()) .allNodes() .map(CelNavigableExpr::expr) .flatMap(SubexpressionOptimizer::extractMangledNames) .collect(toImmutableSet()); - CelList blockIndices = celBlockCall.args().get(0).list(); - for (CelExpr blockIndex : blockIndices.elements()) { + for (CelExpr blockIndex : celBlock.indices()) { ImmutableSet indexDeclaredCompVariables = CelNavigableExpr.fromExpr(blockIndex) .allNodes() @@ -297,7 +279,7 @@ private static void verifyNoInvalidScopedMangledVariables(CelExpr celExpr) { Verify.verify( !containsIllegalDeclaration, "Illegal declared reference to a comprehension variable found in block indices. Expr: %s", - celExpr); + celBlock.expr()); } } @@ -328,60 +310,26 @@ private static CelAbstractSyntaxTree tagAstExtension(CelAbstractSyntaxTree ast) return CelAbstractSyntaxTree.newParsedAst(ast.getExpr(), celSourceBuilder.build()); } - /** - * Creates a list of numbered identifiers from the subexpressions that act as an indexer to - * cel.block (ex: @index0, @index1..). Each subexpressions are type-checked, then its result type - * is used as the new identifiers' types. - */ - private static ImmutableList newBlockIndexVariableDeclarations( - Cel cel, ImmutableList mangledVarDecls, List subexpressions) { - // The resulting type of the subexpressions will likely be different from the - // entire expression's expected result type. - CelBuilder celBuilder = cel.toCelBuilder().setResultType(SimpleType.DYN); - // Add the mangled comprehension variables to the environment for type-checking subexpressions - // to succeed - celBuilder.addVarDeclarations(mangledVarDecls); - - ImmutableList.Builder varDeclBuilder = ImmutableList.builder(); - for (int i = 0; i < subexpressions.size(); i++) { - CelMutableExpr subexpression = subexpressions.get(i); - - CelAbstractSyntaxTree subAst = - CelAbstractSyntaxTree.newParsedAst( - CelMutableExprConverter.fromMutableExpr(subexpression), - CelSource.newBuilder().build()); - - try { - subAst = celBuilder.build().check(subAst).getAst(); - } catch (CelValidationException e) { - throw new IllegalStateException("Failed to type-check subexpression", e); - } - - CelVarDecl indexVar = CelVarDecl.newVarDeclaration("@index" + i, subAst.getResultType()); - celBuilder.addVarDeclarations(indexVar); - varDeclBuilder.add(indexVar); - } - - return varDeclBuilder.build(); - } - - private List getCseCandidates(CelNavigableMutableAst navAst) { + private Optional findCseCandidate(CelNavigableMutableAst navAst) { + Set ineligibleExprs = getIneligibleExprsFromComprehensionBranches(navAst); if (cseOptions.subexpressionMaxRecursionDepth() > 0) { - return getCseCandidatesWithRecursionDepth( - navAst, cseOptions.subexpressionMaxRecursionDepth()); - } else { - return getCseCandidatesWithCommonSubexpr(navAst); + return findCseCandidateWithRecursionDepth( + navAst, ineligibleExprs, cseOptions.subexpressionMaxRecursionDepth()); } + return findCseCandidateWithCommonSubexpr( + navAst + .getRoot() + .allNodes(TraversalOrder.PRE_ORDER) + .filter(node -> canEliminate(node, ineligibleExprs)) + .collect(toImmutableList())); } /** - * Retrieves all subexpr candidates based on the recursion limit even if there's no duplicate - * subexpr found. + * Retrieves a subexpr candidate based on the recursion limit even if there's no duplicate subexpr + * found. */ - private List getCseCandidatesWithRecursionDepth( - CelNavigableMutableAst navAst, int recursionLimit) { - Preconditions.checkArgument(recursionLimit > 0); - Set ineligibleExprs = getIneligibleExprsFromComprehensionBranches(navAst); + private Optional findCseCandidateWithRecursionDepth( + CelNavigableMutableAst navAst, Set ineligibleExprs, int recursionLimit) { ImmutableList descendants = navAst .getRoot() @@ -391,12 +339,12 @@ private List getCseCandidatesWithRecursionDepth( .sorted(Comparator.comparingInt(CelNavigableMutableExpr::height).reversed()) .collect(toImmutableList()); if (descendants.isEmpty()) { - return new ArrayList<>(); + return Optional.empty(); } - List cseCandidates = getCseCandidatesWithCommonSubexpr(descendants); - if (!cseCandidates.isEmpty()) { - return cseCandidates; + Optional cseCandidate = findCseCandidateWithCommonSubexpr(descendants); + if (cseCandidate.isPresent()) { + return cseCandidate; } // If there's no common subexpr, just return the one with the highest height that's still below @@ -409,64 +357,32 @@ private List getCseCandidatesWithRecursionDepth( .filter(node -> node.height() > recursionLimit) .anyMatch(node -> canEliminate(node, ineligibleExprs)); if (astHasMoreExtractableSubexprs) { - cseCandidates.add(descendants.get(0).expr()); - return cseCandidates; + return Optional.of(descendants.get(0).expr()); } // The height of the remaining subexpression is already below the recursion limit. No need to // extract. - return new ArrayList<>(); + return Optional.empty(); } - private List getCseCandidatesWithCommonSubexpr(CelNavigableMutableAst navAst) { - Set ineligibleExprs = getIneligibleExprsFromComprehensionBranches(navAst); - ImmutableList allNodes = - navAst - .getRoot() - .allNodes(TraversalOrder.PRE_ORDER) - .filter(node -> canEliminate(node, ineligibleExprs)) - .collect(toImmutableList()); - - return getCseCandidatesWithCommonSubexpr(allNodes); - } - - private List getCseCandidatesWithCommonSubexpr( + private Optional findCseCandidateWithCommonSubexpr( ImmutableList allNodes) { - CelMutableExpr normalizedCseCandidate = null; - HashSet semanticallyEqualNodes = new HashSet<>(); - for (CelNavigableMutableExpr node : allNodes) { - // Normalize the expr to test semantic equivalence. - CelMutableExpr normalizedExpr = normalizeForEquality(node.expr()); - if (semanticallyEqualNodes.contains(normalizedExpr)) { - normalizedCseCandidate = normalizedExpr; - break; - } - - semanticallyEqualNodes.add(normalizedExpr); - } - - List cseCandidates = new ArrayList<>(); - if (normalizedCseCandidate == null) { - return cseCandidates; - } - + Map seenNodes = new HashMap<>(); for (CelNavigableMutableExpr node : allNodes) { - // Normalize the expr to test semantic equivalence. - CelMutableExpr normalizedExpr = normalizeForEquality(node.expr()); - if (normalizedExpr.equals(normalizedCseCandidate)) { - cseCandidates.add(node.expr()); + CelMutableExpr expr = node.expr(); + CelMutableExpr firstSeen = seenNodes.putIfAbsent(normalizeForEquality(expr), expr); + if (firstSeen != null) { + return Optional.of(firstSeen); } } - return cseCandidates; + return Optional.empty(); } private boolean canEliminate( CelNavigableMutableExpr navigableExpr, Set ineligibleExprs) { return !navigableExpr.getKind().equals(Kind.CONSTANT) && !navigableExpr.getKind().equals(Kind.IDENT) - && !(navigableExpr.getKind().equals(Kind.IDENT) - && navigableExpr.expr().ident().name().startsWith(BIND_IDENTIFIER_PREFIX)) // Exclude empty lists (cel.bind sets this for iterRange). && !(navigableExpr.getKind().equals(Kind.LIST) && navigableExpr.expr().list().elements().isEmpty()) @@ -609,11 +525,6 @@ private CelMutableExpr normalizeForEquality(CelMutableExpr mutableExpr) { return astMutator.clearExprIds(copiedExpr); } - @VisibleForTesting - static CelFunctionDecl newCelBlockFunctionDecl(CelType unusedResultType) { - return CelBindingsExtensions.CEL_BLOCK_FUNCTION_DECL; - } - /** Options to configure how Common Subexpression Elimination behave. */ @AutoValue public abstract static class SubexpressionOptimizerOptions { diff --git a/optimizer/src/test/java/dev/cel/optimizer/optimizers/BUILD.bazel b/optimizer/src/test/java/dev/cel/optimizer/optimizers/BUILD.bazel index 3f67c49c6..13d8e1e46 100644 --- a/optimizer/src/test/java/dev/cel/optimizer/optimizers/BUILD.bazel +++ b/optimizer/src/test/java/dev/cel/optimizer/optimizers/BUILD.bazel @@ -25,6 +25,7 @@ java_library( "//common/values", "//common/values:cel_byte_string", "//extensions", + "//extensions:bindings", "//extensions:optional_library", # "//java/com/google/testing/testsize:annotations", "//optimizer", diff --git a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java index 52bc8db92..b495f9222 100644 --- a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java +++ b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java @@ -42,6 +42,7 @@ import dev.cel.common.types.SimpleType; import dev.cel.common.types.StructTypeReference; import dev.cel.expr.conformance.proto3.TestAllTypes; +import dev.cel.extensions.CelBindingsExtensions; import dev.cel.extensions.CelExtensions; import dev.cel.optimizer.CelOptimizationException; import dev.cel.optimizer.CelOptimizer; @@ -103,7 +104,7 @@ private static Cel setupCelForEvaluatingBlock(CelBuilder celBuilder) { SimpleType.DYN, ListType.create(SimpleType.DYN), SimpleType.DYN)), - SubexpressionOptimizer.newCelBlockFunctionDecl(SimpleType.DYN), + CelBindingsExtensions.CEL_BLOCK_FUNCTION_DECL, CelFunctionDecl.newFunctionDeclaration( "get_true", CelOverloadDecl.newGlobalOverload("get_true_overload", SimpleType.BOOL))) @@ -678,9 +679,9 @@ public void block_lazyEvaluationContainsError_cleansUpCycleState() throws Except CelAbstractSyntaxTree ast = compileUsingInternalFunctions( "cel.block([1/0 > 0], (index0 && false) || (index0 && true))"); + Program program = cel.createProgram(ast); - CelEvaluationException e = - assertThrows(CelEvaluationException.class, () -> cel.createProgram(ast).eval()); + CelEvaluationException e = assertThrows(CelEvaluationException.class, program::eval); assertThat(e).hasMessageThat().contains("/ by zero"); assertThat(e).hasMessageThat().doesNotContain("Cycle detected"); diff --git a/publish/BUILD.bazel b/publish/BUILD.bazel index 2fb948cea..9562e22f0 100644 --- a/publish/BUILD.bazel +++ b/publish/BUILD.bazel @@ -16,6 +16,7 @@ COMMON_TARGETS = [ "//common/src/main/java/dev/cel/common:operator", "//common/src/main/java/dev/cel/common:options", "//common/src/main/java/dev/cel/common:source", + "//common/src/main/java/dev/cel/common/ast:cel_expr_visitor", "//common/src/main/java/dev/cel/common/internal", "//common/src/main/java/dev/cel/common/internal:cel_descriptor_pools", "//common/src/main/java/dev/cel/common/internal:file_descriptor_converter",