Skip to content

Commit 2929042

Browse files
l46kokcopybara-github
authored andcommitted
Remove enableHiddenAccumulatorVar option
Hidden accumulator variable (@Result) is now unconditionally enabled across all parsers (Antlr and Pratt), matching cel-go and cel-cpp. PiperOrigin-RevId: 981327426
1 parent 6124cb0 commit 2929042

12 files changed

Lines changed: 8 additions & 327 deletions

File tree

common/src/main/java/dev/cel/common/CelOptions.java

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,6 @@ public enum ProtoUnsetFieldOptions {
6868

6969
public abstract boolean retainUnbalancedLogicalExpressions();
7070

71-
public abstract boolean enableHiddenAccumulatorVar();
72-
7371
public abstract boolean enableQuotedIdentifierSyntax();
7472

7573
public abstract boolean enablePrattParser();
@@ -144,7 +142,6 @@ public static Builder newBuilder() {
144142
.populateMacroCalls(false)
145143
.retainRepeatedUnaryOperators(false)
146144
.retainUnbalancedLogicalExpressions(false)
147-
.enableHiddenAccumulatorVar(true)
148145
.enableQuotedIdentifierSyntax(true)
149146
.enablePrattParser(false)
150147
// Type-Checker options
@@ -262,16 +259,6 @@ public abstract static class Builder {
262259
*/
263260
public abstract Builder retainUnbalancedLogicalExpressions(boolean value);
264261

265-
/**
266-
* Enable the use of a hidden accumulator variable name.
267-
*
268-
* <p>This is a temporary option to transition to using an internal identifier for the
269-
* accumulator variable used by builtin comprehension macros. When enabled, parses result in a
270-
* semantically equivalent AST, but with a different accumulator variable that can't be directly
271-
* referenced in the source expression.
272-
*/
273-
public abstract Builder enableHiddenAccumulatorVar(boolean value);
274-
275262
/**
276263
* Enable quoted identifier syntax.
277264
*

common/src/test/java/dev/cel/common/ast/CelExprFormatterTest.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import dev.cel.common.CelAbstractSyntaxTree;
2222
import dev.cel.common.CelContainer;
2323
import dev.cel.common.CelFunctionDecl;
24-
import dev.cel.common.CelOptions;
2524
import dev.cel.common.CelOverloadDecl;
2625
import dev.cel.common.types.SimpleType;
2726
import dev.cel.common.types.StructTypeReference;
@@ -274,7 +273,6 @@ public void map() throws Exception {
274273
public void comprehension() throws Exception {
275274
CelCompiler celCompiler =
276275
CelCompilerFactory.standardCelCompilerBuilder()
277-
.setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build())
278276
.setStandardMacros(CelStandardMacro.STANDARD_MACROS)
279277
.build();
280278
CelAbstractSyntaxTree ast = celCompiler.compile("[1, 2, 3].exists(x, x > 0)").getAst();

common/src/test/java/dev/cel/common/ast/CelExprVisitorTest.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import com.google.common.collect.ImmutableList;
2222
import dev.cel.common.CelAbstractSyntaxTree;
2323
import dev.cel.common.CelContainer;
24-
import dev.cel.common.CelOptions;
2524
import dev.cel.common.Operator;
2625
import dev.cel.common.ast.CelExpr.CelCall;
2726
import dev.cel.common.ast.CelExpr.CelComprehension;
@@ -327,7 +326,6 @@ public void visitList() throws Exception {
327326
public void visitComprehension() throws Exception {
328327
CelCompiler celCompiler =
329328
CelCompilerFactory.standardCelCompilerBuilder()
330-
.setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build())
331329
.setStandardMacros(CelStandardMacro.ALL)
332330
.build();
333331
CelAbstractSyntaxTree ast = celCompiler.compile("[1, 1].all(x, x == 1)").getAst();

common/src/test/java/dev/cel/common/navigation/CelNavigableExprVisitorTest.java

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -811,7 +811,6 @@ public void emptyMapConstruction_allNodesReturned() throws Exception {
811811
public void comprehension_preOrder_allNodesReturned() throws Exception {
812812
CelCompiler compiler =
813813
CelCompilerFactory.standardCelCompilerBuilder()
814-
.setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build())
815814
.setStandardMacros(CelStandardMacro.EXISTS)
816815
.build();
817816
CelAbstractSyntaxTree ast = compiler.compile("[true].exists(i, i)").getAst();
@@ -873,7 +872,6 @@ public void comprehension_preOrder_allNodesReturned() throws Exception {
873872
public void comprehension_postOrder_allNodesReturned() throws Exception {
874873
CelCompiler compiler =
875874
CelCompilerFactory.standardCelCompilerBuilder()
876-
.setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build())
877875
.setStandardMacros(CelStandardMacro.EXISTS)
878876
.build();
879877
CelAbstractSyntaxTree ast = compiler.compile("[true].exists(i, i)").getAst();
@@ -1011,7 +1009,6 @@ public void comprehension_postOrder_maxIdsSet() throws Exception {
10111009
public void comprehension_allNodes_parentsPopulated() throws Exception {
10121010
CelCompiler compiler =
10131011
CelCompilerFactory.standardCelCompilerBuilder()
1014-
.setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build())
10151012
.setStandardMacros(CelStandardMacro.EXISTS)
10161013
.build();
10171014
CelAbstractSyntaxTree ast = compiler.compile("[true].exists(i, i)").getAst();
@@ -1070,7 +1067,6 @@ public void comprehension_allNodes_parentsPopulated() throws Exception {
10701067
public void comprehension_filterComprehension_allNodesReturned() throws Exception {
10711068
CelCompiler compiler =
10721069
CelCompilerFactory.standardCelCompilerBuilder()
1073-
.setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build())
10741070
.setStandardMacros(CelStandardMacro.EXISTS)
10751071
.build();
10761072
CelAbstractSyntaxTree ast = compiler.compile("[true].exists(i, i)").getAst();

extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -495,7 +495,8 @@ private static CelExpr validatedIterationVariable(
495495
CelExpr arg = checkNotNull(argument);
496496
if (!isSimpleIdentifier(arg)) {
497497
return reportArgumentError(exprFactory, arg);
498-
} else if (arg.exprKind().ident().name().equals("__result__")) {
498+
} else if (arg.exprKind().ident().name().equals(exprFactory.getAccumulatorVarName())
499+
|| arg.exprKind().ident().name().equals("__result__")) {
499500
return reportAccumulatorOverwriteError(exprFactory, arg);
500501
} else {
501502
return arg;

optimizer/src/main/java/dev/cel/optimizer/AstMutator.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -714,7 +714,7 @@ private CelMutableExpr mangleIdentsInComprehensionExpr(
714714

715715
comprehension.setIterVar(mangledComprehensionName.iterVarName());
716716

717-
// Most standard macros set accu_var as __result__, but not all (ex: cel.bind).
717+
// Most standard macros set accu_var as @result, but not all (ex: cel.bind).
718718
if (comprehension.accuVar().equals(originalAccuVar)) {
719719
comprehension.setAccuVar(mangledComprehensionName.resultName());
720720
}

parser/src/main/java/dev/cel/parser/AntlrParser.java

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,7 @@ final class AntlrParser extends CELBaseVisitor<CelExpr> {
129129
"var",
130130
"void",
131131
"while");
132-
private static final String ACCUMULATOR_NAME = "__result__";
133-
private static final String HIDDEN_ACCUMULATOR_NAME = "@result";
132+
private static final String ACCUMULATOR_NAME = "@result";
134133

135134
static CelValidationResult parse(
136135
CelSource source, CelOptions options, Collection<CelMacro> macros) {
@@ -151,10 +150,7 @@ static CelValidationResult parse(
151150
sourceInfo.setDescription(source.getDescription());
152151
ExprFactory exprFactory =
153152
new ExprFactory(
154-
antlrParser,
155-
sourceInfo,
156-
options.enableHiddenAccumulatorVar() ? HIDDEN_ACCUMULATOR_NAME : ACCUMULATOR_NAME,
157-
options.maxParseExpressionNodeCount());
153+
antlrParser, sourceInfo, ACCUMULATOR_NAME, options.maxParseExpressionNodeCount());
158154
AntlrParser parserImpl = new AntlrParser(options, macros, sourceInfo, exprFactory);
159155
ErrorListener errorListener = new ErrorListener(exprFactory);
160156
antlrLexer.removeErrorListeners();

parser/src/main/java/dev/cel/parser/CelStandardMacro.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,8 @@ private static CelExpr validatedIterationVariable(
307307
CelExpr arg = checkNotNull(argument);
308308
if (!isSimpleIdentifier(arg)) {
309309
return reportArgumentError(exprFactory, arg);
310-
} else if (arg.exprKind().ident().name().equals("__result__")) {
310+
} else if (arg.exprKind().ident().name().equals(exprFactory.getAccumulatorVarName())
311+
|| arg.exprKind().ident().name().equals("__result__")) {
311312
return reportAccumulatorOverwriteError(exprFactory, arg);
312313
} else {
313314
return arg;

parser/src/test/java/dev/cel/parser/CelMacroExprFactoryTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ public CelExpr reportError(CelIssue issue) {
6363

6464
@Override
6565
public String getAccumulatorVarName() {
66-
return "__result__";
66+
return "@result";
6767
}
6868

6969
@Override

parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ public final class CelParserParameterizedTest extends BaselineTestCase {
5050
.populateMacroCalls(true)
5151
.enableOptionalSyntax(true)
5252
.enableQuotedIdentifierSyntax(true)
53-
.enableHiddenAccumulatorVar(true)
5453
.build();
5554

5655
private static final CelOptions OPTIONS_MAX_RECURSION_DEPTH_32 =
@@ -74,9 +73,6 @@ public final class CelParserParameterizedTest extends BaselineTestCase {
7473
private static final CelOptions OPTIONS_MAX_ERROR_RECOVERY_LIMIT_2 =
7574
OPTIONS.toBuilder().maxParseErrorRecoveryLimit(2).build();
7675

77-
private static final CelOptions OPTIONS_OLD_ACCU_VAR =
78-
OPTIONS.toBuilder().enableHiddenAccumulatorVar(false).build();
79-
8076
private static final ImmutableMap<String, CelMacro> MACROS =
8177
ImmutableMap.<String, CelMacro>builder()
8278
.putAll(
@@ -681,17 +677,6 @@ public void parser_errors() {
681677
runTest(OPTIONS_MAX_ERROR_RECOVERY_LIMIT_2, "[1 2 3 a b c]");
682678
}
683679

684-
@Test
685-
public void parser_legacyAccuVar() {
686-
runAntlrTest(OPTIONS_OLD_ACCU_VAR, "x * 2");
687-
runAntlrTest(OPTIONS_OLD_ACCU_VAR, "has(m.f)");
688-
runAntlrTest(OPTIONS_OLD_ACCU_VAR, "m.exists_one(v, f)");
689-
runAntlrTest(OPTIONS_OLD_ACCU_VAR, "m.all(v, f)");
690-
runAntlrTest(OPTIONS_OLD_ACCU_VAR, "m.map(v, f)");
691-
runAntlrTest(OPTIONS_OLD_ACCU_VAR, "m.map(v, p, f)");
692-
runAntlrTest(OPTIONS_OLD_ACCU_VAR, "m.filter(v, p)");
693-
}
694-
695680
private void runAntlrTest(CelOptions options, String expression) {
696681
testOutput().println("I: " + sanitizeForBaseline(expression));
697682
testOutput().println("=====>");

0 commit comments

Comments
 (0)