From 63a55cf45014beb0b2123cfa715674cec736cd0b Mon Sep 17 00:00:00 2001 From: Sean Huh Date: Wed, 30 Sep 2026 22:25:10 -0700 Subject: [PATCH] Fail fast when CSE is run after SelectOptimizer PiperOrigin-RevId: 991433729 --- .../dev/cel/optimizer/optimizers/BUILD.bazel | 1 + .../optimizers/DefaultOptimizerConstants.java | 3 +++ .../optimizer/optimizers/SelectOptimizer.java | 5 ++-- .../optimizers/SubexpressionOptimizer.java | 24 +++++++++++++++++++ .../SubexpressionOptimizerTest.java | 22 +++++++++++++++++ 5 files changed, 52 insertions(+), 3 deletions(-) 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 69a6addb5..dae52c1f9 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel @@ -119,6 +119,7 @@ java_library( tags = [ ], deps = [ + ":default_optimizer_constants", "//:auto_value", "//bundle:cel", "//checker:standard_decl", diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/DefaultOptimizerConstants.java b/optimizer/src/main/java/dev/cel/optimizer/optimizers/DefaultOptimizerConstants.java index a9db48391..35fee23d4 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/DefaultOptimizerConstants.java +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/DefaultOptimizerConstants.java @@ -46,5 +46,8 @@ final class DefaultOptimizerConstants { .addAll(CelExtensions.getAllFunctionNames()) .build(); + static final String CEL_ATTRIBUTE_FUNCTION_NAME = "cel.@attribute"; + static final String CEL_HAS_FIELD_FUNCTION_NAME = "cel.@hasField"; + private DefaultOptimizerConstants() {} } diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SelectOptimizer.java b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SelectOptimizer.java index 8ba175bed..16142ee20 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SelectOptimizer.java +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SelectOptimizer.java @@ -18,6 +18,8 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; +import static dev.cel.optimizer.optimizers.DefaultOptimizerConstants.CEL_ATTRIBUTE_FUNCTION_NAME; +import static dev.cel.optimizer.optimizers.DefaultOptimizerConstants.CEL_HAS_FIELD_FUNCTION_NAME; import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; @@ -139,9 +141,6 @@ public final class SelectOptimizer implements CelAstOptimizer { */ private static final long CEL_MAP_TYPE_CODE = -1L; - private static final String CEL_ATTRIBUTE_FUNCTION_NAME = "cel.@attribute"; - private static final String CEL_HAS_FIELD_FUNCTION_NAME = "cel.@hasField"; - private static final TypeParamType TYPE_PARAM_T = TypeParamType.create("T"); /** 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 6d671b162..f677b5b08 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java @@ -15,8 +15,11 @@ package dev.cel.optimizer.optimizers; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static dev.cel.optimizer.optimizers.DefaultOptimizerConstants.CEL_ATTRIBUTE_FUNCTION_NAME; +import static dev.cel.optimizer.optimizers.DefaultOptimizerConstants.CEL_HAS_FIELD_FUNCTION_NAME; import static java.util.stream.Collectors.toCollection; import com.google.auto.value.AutoValue; @@ -91,6 +94,9 @@ * @index0.startsWith("hello") && @index1.endsWith("world")) * } * + * + *

Optimizer Ordering: {@code SubexpressionOptimizer} does not support ASTs that have + * already been optimized by {@link SelectOptimizer}. */ public final class SubexpressionOptimizer implements CelAstOptimizer { @@ -124,6 +130,13 @@ public static SubexpressionOptimizer newInstance(SubexpressionOptimizerOptions c return new SubexpressionOptimizer(cseOptions); } + /** + * {@inheritDoc} + * + * @throws IllegalStateException if {@code ast} contains select-optimized nodes ({@code + * cel.@attribute} or {@code cel.@hasField}), as common subexpression elimination must run + * before {@link SelectOptimizer}. + */ @Override public OptimizationResult optimize(CelAbstractSyntaxTree ast, Cel cel) { OptimizationResult result = optimizeUsingCelBlock(ast, cel); @@ -134,6 +147,17 @@ public OptimizationResult optimize(CelAbstractSyntaxTree ast, Cel cel) { } private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel cel) { + boolean hasSelectOptimizedNodes = + CelNavigableExpr.fromExpr(ast.getExpr()) + .allNodes() + .filter(node -> node.getKind() == Kind.CALL) + .map(node -> node.expr().call().function()) + .anyMatch( + func -> + func.equals(CEL_ATTRIBUTE_FUNCTION_NAME) + || func.equals(CEL_HAS_FIELD_FUNCTION_NAME)); + checkState(!hasSelectOptimizedNodes, "SubexpressionOptimizer must run before SelectOptimizer."); + CelMutableAst astToModify = CelMutableAst.fromCelAst(ast); if (!cseOptions.populateMacroCalls()) { astToModify.source().clearMacroCalls(); 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 209dba3a5..c3e705f84 100644 --- a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java +++ b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java @@ -762,6 +762,28 @@ public void cse_nestedMacro_withOptimization_assertAstIdCorrectness() throws Exc + " @index0)"); } + @Test + public void optimize_afterSelectOptimizer_throwsException( + @TestParameter({"msg.single_int64 == 1 && msg.single_int64 == 1", "has(msg.single_int64)"}) + String expression) + throws Exception { + CelOptimizer selectThenCseOptimizer = + CelOptimizerFactory.standardCelOptimizerBuilder(cel) + .addAstOptimizers( + SelectOptimizer.newInstance(TestAllTypes.getDescriptor().getFile()), + SubexpressionOptimizer.getInstance()) + .build(); + CelAbstractSyntaxTree ast = cel.compile(expression).getAst(); + + CelOptimizationException e = + assertThrows(CelOptimizationException.class, () -> selectThenCseOptimizer.optimize(ast)); + + assertThat(e).hasCauseThat().isInstanceOf(IllegalStateException.class); + assertThat(e) + .hasMessageThat() + .contains("SubexpressionOptimizer must run before SelectOptimizer."); + } + /** * Converts AST containing cel.block related test functions to internal functions (e.g: cel.block * -> cel.@block)