Skip to content

Commit 13f9007

Browse files
l46kokcopybara-github
l46kok
authored and
copybara-github
committed
Fix constant folding to not error when sub-asts contain unbound variables
CEL-Java fix for xref: cel-expr/cel-go#1296 PiperOrigin-RevId: 892757312
1 parent 8e9f1ac commit 13f9007

12 files changed

Lines changed: 107 additions & 26 deletions

File tree

‎optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ java_library(
3535
"//optimizer:mutable_ast",
3636
"//optimizer:optimization_exception",
3737
"//runtime",
38+
"//runtime:partial_vars",
39+
"//runtime:unknown_attributes",
3840
"@maven//:com_google_errorprone_error_prone_annotations",
3941
"@maven//:com_google_guava_guava",
4042
],

‎optimizer/src/main/java/dev/cel/optimizer/optimizers/ConstantFoldingOptimizer.java‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import dev.cel.common.CelValidationException;
3131
import dev.cel.common.Operator;
3232
import dev.cel.common.ast.CelConstant;
33-
import dev.cel.common.ast.CelExpr;
3433
import dev.cel.common.ast.CelExpr.ExprKind.Kind;
3534
import dev.cel.common.ast.CelMutableExpr;
3635
import dev.cel.common.ast.CelMutableExpr.CelMutableCall;
@@ -47,7 +46,10 @@
4746
import dev.cel.optimizer.AstMutator;
4847
import dev.cel.optimizer.CelAstOptimizer;
4948
import dev.cel.optimizer.CelOptimizationException;
49+
import dev.cel.runtime.CelAttribute.Qualifier;
50+
import dev.cel.runtime.CelAttributePattern;
5051
import dev.cel.runtime.CelEvaluationException;
52+
import dev.cel.runtime.PartialVars;
5153
import java.time.Duration;
5254
import java.time.Instant;
5355
import java.util.ArrayList;
@@ -282,7 +284,7 @@ private Optional<CelMutableAst> maybeFold(
282284
throws CelOptimizationException {
283285
Object result;
284286
try {
285-
result = evaluateExpr(cel, CelMutableExprConverter.fromMutableExpr(node.expr()));
287+
result = evaluateExpr(cel, node);
286288
} catch (CelValidationException | CelEvaluationException e) {
287289
throw new CelOptimizationException(
288290
"Constant folding failure. Failed to evaluate subtree due to: " + e.getMessage(), e);
@@ -674,13 +676,23 @@ private CelMutableAst pruneOptionalStructElements(CelMutableAst ast, CelMutableE
674676
}
675677

676678
@CanIgnoreReturnValue
677-
private static Object evaluateExpr(Cel cel, CelExpr expr)
679+
private static Object evaluateExpr(Cel cel, CelNavigableMutableExpr navigableMutableExpr)
678680
throws CelValidationException, CelEvaluationException {
681+
ImmutableList<CelAttributePattern> attributePatterns =
682+
navigableMutableExpr
683+
.allNodes()
684+
.filter(node -> node.getKind().equals(Kind.IDENT))
685+
.map(node -> node.expr().ident().name())
686+
.filter(Qualifier::isLegalIdentifier)
687+
.map(CelAttributePattern::create)
688+
.collect(toImmutableList());
679689
CelAbstractSyntaxTree ast =
680-
CelAbstractSyntaxTree.newParsedAst(expr, CelSource.newBuilder().build());
690+
CelAbstractSyntaxTree.newParsedAst(
691+
CelMutableExprConverter.fromMutableExpr(navigableMutableExpr.expr()),
692+
CelSource.newBuilder().build());
681693
ast = cel.check(ast).getAst();
682694

683-
return cel.createProgram(ast).eval();
695+
return cel.createProgram(ast).eval(PartialVars.of(attributePatterns));
684696
}
685697

686698
/** Options to configure how Constant Folding behave. */

‎optimizer/src/test/java/dev/cel/optimizer/optimizers/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ java_library(
1111
deps = [
1212
# "//java/com/google/testing/testsize:annotations",
1313
"//bundle:cel",
14+
"//bundle:cel_experimental_factory",
1415
"//common:cel_ast",
1516
"//common:cel_source",
1617
"//common:compiler_common",

‎optimizer/src/test/java/dev/cel/optimizer/optimizers/ConstantFoldingOptimizerTest.java‎

Lines changed: 40 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,12 @@
1818
import static org.junit.Assert.assertThrows;
1919

2020
import com.google.common.collect.ImmutableList;
21+
import com.google.testing.junit.testparameterinjector.TestParameter;
2122
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
2223
import com.google.testing.junit.testparameterinjector.TestParameters;
2324
import dev.cel.bundle.Cel;
25+
import dev.cel.bundle.CelBuilder;
26+
import dev.cel.bundle.CelExperimentalFactory;
2427
import dev.cel.bundle.CelFactory;
2528
import dev.cel.common.CelAbstractSyntaxTree;
2629
import dev.cel.common.CelContainer;
@@ -47,9 +50,23 @@
4750
@RunWith(TestParameterInjector.class)
4851
public class ConstantFoldingOptimizerTest {
4952
private static final CelOptions CEL_OPTIONS =
50-
CelOptions.current().populateMacroCalls(true).build();
51-
private static final Cel CEL =
52-
CelFactory.standardCelBuilder()
53+
CelOptions.current()
54+
.populateMacroCalls(true)
55+
.enableHeterogeneousNumericComparisons(true)
56+
.build();
57+
58+
private static final CelUnparser CEL_UNPARSER = CelUnparserFactory.newUnparser();
59+
60+
@SuppressWarnings("unchecked") // test only
61+
private enum RuntimeEnv {
62+
LEGACY(setupEnv(CelFactory.standardCelBuilder())),
63+
PLANNER(setupEnv(CelExperimentalFactory.plannerCelBuilder()));
64+
65+
private final Cel cel;
66+
private final CelOptimizer celOptimizer;
67+
68+
private static Cel setupEnv(CelBuilder celBuilder) {
69+
return celBuilder
5370
.addVar("x", SimpleType.DYN)
5471
.addVar("y", SimpleType.DYN)
5572
.addVar("list_var", ListType.create(SimpleType.STRING))
@@ -84,13 +101,18 @@ public class ConstantFoldingOptimizerTest {
84101
CelExtensions.sets(CEL_OPTIONS),
85102
CelExtensions.encoders(CEL_OPTIONS))
86103
.build();
104+
}
105+
106+
RuntimeEnv(Cel cel) {
107+
this.cel = cel;
108+
this.celOptimizer =
109+
CelOptimizerFactory.standardCelOptimizerBuilder(cel)
110+
.addAstOptimizers(ConstantFoldingOptimizer.getInstance())
111+
.build();
112+
}
113+
}
87114

88-
private static final CelOptimizer CEL_OPTIMIZER =
89-
CelOptimizerFactory.standardCelOptimizerBuilder(CEL)
90-
.addAstOptimizers(ConstantFoldingOptimizer.getInstance())
91-
.build();
92-
93-
private static final CelUnparser CEL_UNPARSER = CelUnparserFactory.newUnparser();
115+
@TestParameter RuntimeEnv runtimeEnv;
94116

95117
@Test
96118
@TestParameters("{source: 'null', expected: 'null'}")
@@ -238,9 +260,9 @@ public class ConstantFoldingOptimizerTest {
238260
// TODO: Support folding lists with mixed types. This requires mutable lists.
239261
// @TestParameters("{source: 'dyn([1]) + [1.0]'}")
240262
public void constantFold_success(String source, String expected) throws Exception {
241-
CelAbstractSyntaxTree ast = CEL.compile(source).getAst();
263+
CelAbstractSyntaxTree ast = runtimeEnv.cel.compile(source).getAst();
242264

243-
CelAbstractSyntaxTree optimizedAst = CEL_OPTIMIZER.optimize(ast);
265+
CelAbstractSyntaxTree optimizedAst = runtimeEnv.celOptimizer.optimize(ast);
244266

245267
assertThat(CEL_UNPARSER.unparse(optimizedAst)).isEqualTo(expected);
246268
}
@@ -378,21 +400,22 @@ public void constantFold_macros_withoutMacroCallMetadata(String source) throws E
378400
@TestParameters("{source: 'duration(\"1h\")'}")
379401
@TestParameters("{source: '[true].exists(x, x == get_true())'}")
380402
@TestParameters("{source: 'get_list([1, 2]).map(x, x * 2)'}")
403+
@TestParameters("{source: '[(x - 1 > 3) ? (x - 1) : 5].exists(x, x - 1 > 3)'}")
381404
public void constantFold_noOp(String source) throws Exception {
382-
CelAbstractSyntaxTree ast = CEL.compile(source).getAst();
405+
CelAbstractSyntaxTree ast = runtimeEnv.cel.compile(source).getAst();
383406

384-
CelAbstractSyntaxTree optimizedAst = CEL_OPTIMIZER.optimize(ast);
407+
CelAbstractSyntaxTree optimizedAst = runtimeEnv.celOptimizer.optimize(ast);
385408

386409
assertThat(CEL_UNPARSER.unparse(optimizedAst)).isEqualTo(source);
387410
}
388411

389412
@Test
390413
public void constantFold_addFoldableFunction_success() throws Exception {
391-
CelAbstractSyntaxTree ast = CEL.compile("get_true() == get_true()").getAst();
414+
CelAbstractSyntaxTree ast = runtimeEnv.cel.compile("get_true() == get_true()").getAst();
392415
ConstantFoldingOptions options =
393416
ConstantFoldingOptions.newBuilder().addFoldableFunctions("get_true").build();
394417
CelOptimizer optimizer =
395-
CelOptimizerFactory.standardCelOptimizerBuilder(CEL)
418+
CelOptimizerFactory.standardCelOptimizerBuilder(runtimeEnv.cel)
396419
.addAstOptimizers(ConstantFoldingOptimizer.newInstance(options))
397420
.build();
398421

@@ -492,9 +515,9 @@ public void constantFold_withMacroCallPopulated_comprehensionsAreReplacedWithNot
492515

493516
@Test
494517
public void constantFold_astProducesConsistentlyNumberedIds() throws Exception {
495-
CelAbstractSyntaxTree ast = CEL.compile("[1] + [2] + [3]").getAst();
518+
CelAbstractSyntaxTree ast = runtimeEnv.cel.compile("[1] + [2] + [3]").getAst();
496519

497-
CelAbstractSyntaxTree optimizedAst = CEL_OPTIMIZER.optimize(ast);
520+
CelAbstractSyntaxTree optimizedAst = runtimeEnv.celOptimizer.optimize(ast);
498521

499522
assertThat(optimizedAst.getExpr().toString())
500523
.isEqualTo(

‎runtime/src/main/java/dev/cel/runtime/PartialVars.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,12 @@ public abstract class PartialVars {
3737

3838
/** Constructs a new {@code PartialVars} from one or more {@link CelAttributePattern}s. */
3939
public static PartialVars of(CelAttributePattern... unknownAttributes) {
40-
return of((unused) -> Optional.empty(), ImmutableList.copyOf(unknownAttributes));
40+
return of(ImmutableList.copyOf(unknownAttributes));
41+
}
42+
43+
/** Constructs a new {@code PartialVars} from a list of {@link CelAttributePattern}s. */
44+
public static PartialVars of(Iterable<CelAttributePattern> unknownAttributes) {
45+
return of((unused) -> Optional.empty(), unknownAttributes);
4146
}
4247

4348
/**

‎runtime/src/main/java/dev/cel/runtime/planner/ActivationWrapper.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,4 +19,7 @@
1919
/** Identifies a resolver that can be unwrapped to bypass local variable state. */
2020
public interface ActivationWrapper extends GlobalResolver {
2121
GlobalResolver unwrap();
22+
23+
/** Returns true if the given name is bound by this local activation wrapper. */
24+
boolean isLocallyBound(String name);
2225
}

‎runtime/src/main/java/dev/cel/runtime/planner/EvalFold.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,11 @@ public GlobalResolver unwrap() {
175175
return resolver;
176176
}
177177

178+
@Override
179+
public boolean isLocallyBound(String name) {
180+
return name.equals(accuVar) || name.equals(iterVar) || name.equals(iterVar2);
181+
}
182+
178183
@Override
179184
public @Nullable Object resolve(String name) {
180185
if (name.equals(accuVar)) {

‎runtime/src/main/java/dev/cel/runtime/planner/NamespacedAttribute.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public Object resolve(long exprId, GlobalResolver ctx, ExecutionFrame frame) {
7575

7676
PartialVars partialVars = frame.partialVars().orElse(null);
7777

78-
if (partialVars != null) {
78+
if (partialVars != null && !isLocallyBound(resolver, name)) {
7979
ImmutableList<CelAttributePattern> patterns = partialVars.unknowns();
8080
// Avoid enhanced for loop to prevent UnmodifiableIterator from being allocated
8181
for (int i = 0; i < qualifiers.size(); i++) {
@@ -151,6 +151,17 @@ private static Long getEnumValue(EnumType enumType, String field) {
151151
String.format("Field %s was not found on enum %s", enumType.name(), field)));
152152
}
153153

154+
private boolean isLocallyBound(GlobalResolver resolver, String name) {
155+
while (resolver instanceof ActivationWrapper) {
156+
ActivationWrapper wrapper = (ActivationWrapper) resolver;
157+
if (wrapper.isLocallyBound(name)) {
158+
return true;
159+
}
160+
resolver = wrapper.unwrap();
161+
}
162+
return false;
163+
}
164+
154165
private GlobalResolver unwrapToNonLocal(GlobalResolver resolver) {
155166
while (resolver instanceof ActivationWrapper) {
156167
resolver = ((ActivationWrapper) resolver).unwrap();

‎runtime/src/test/java/dev/cel/runtime/PlannerInterpreterTest.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,11 @@ public void planner_unknownResultSet_success() {
283283
declareVariable("unknown_list", ListType.create(SimpleType.INT));
284284
source = "unknown_list.map(x, x)";
285285
runTest(variables, CelAttributePattern.fromQualifiedIdentifier("unknown_list"));
286+
287+
clearAllDeclarations();
288+
declareVariable("x", StructTypeReference.create(TestAllTypes.getDescriptor().getFullName()));
289+
source = "cel.bind(x, [1, 2, 3], 1 in x)";
290+
runTest(variables, CelAttributePattern.fromQualifiedIdentifier("x"));
286291
}
287292

288293
@Test

‎runtime/src/test/resources/planner_unknownResultSet_success.baseline‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -458,4 +458,16 @@ single_timestamp {
458458
seconds: 15
459459
}
460460
, unknown_attributes=[unknown_list]}
461-
result: CelUnknownSet{attributes=[unknown_list], unknownExprIds=[1]}
461+
result: CelUnknownSet{attributes=[unknown_list], unknownExprIds=[1]}
462+
463+
Source: cel.bind(x, [1, 2, 3], 1 in x)
464+
declare x {
465+
value cel.expr.conformance.proto3.TestAllTypes
466+
}
467+
=====>
468+
bindings: {x=single_string: "test"
469+
single_timestamp {
470+
seconds: 15
471+
}
472+
, unknown_attributes=[x]}
473+
result: true

0 commit comments

Comments
 (0)