diff --git a/common/src/main/java/dev/cel/common/CelOptions.java b/common/src/main/java/dev/cel/common/CelOptions.java index c4b868bf2..29ed87381 100644 --- a/common/src/main/java/dev/cel/common/CelOptions.java +++ b/common/src/main/java/dev/cel/common/CelOptions.java @@ -68,8 +68,6 @@ public enum ProtoUnsetFieldOptions { public abstract boolean retainUnbalancedLogicalExpressions(); - public abstract boolean enableHiddenAccumulatorVar(); - public abstract boolean enableQuotedIdentifierSyntax(); public abstract boolean enablePrattParser(); @@ -144,7 +142,6 @@ public static Builder newBuilder() { .populateMacroCalls(false) .retainRepeatedUnaryOperators(false) .retainUnbalancedLogicalExpressions(false) - .enableHiddenAccumulatorVar(true) .enableQuotedIdentifierSyntax(true) .enablePrattParser(false) // Type-Checker options @@ -262,16 +259,6 @@ public abstract static class Builder { */ public abstract Builder retainUnbalancedLogicalExpressions(boolean value); - /** - * Enable the use of a hidden accumulator variable name. - * - *

This is a temporary option to transition to using an internal identifier for the - * accumulator variable used by builtin comprehension macros. When enabled, parses result in a - * semantically equivalent AST, but with a different accumulator variable that can't be directly - * referenced in the source expression. - */ - public abstract Builder enableHiddenAccumulatorVar(boolean value); - /** * Enable quoted identifier syntax. * diff --git a/common/src/test/java/dev/cel/common/ast/CelExprFormatterTest.java b/common/src/test/java/dev/cel/common/ast/CelExprFormatterTest.java index 116222d68..252018890 100644 --- a/common/src/test/java/dev/cel/common/ast/CelExprFormatterTest.java +++ b/common/src/test/java/dev/cel/common/ast/CelExprFormatterTest.java @@ -21,7 +21,6 @@ import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.CelContainer; import dev.cel.common.CelFunctionDecl; -import dev.cel.common.CelOptions; import dev.cel.common.CelOverloadDecl; import dev.cel.common.types.SimpleType; import dev.cel.common.types.StructTypeReference; @@ -274,7 +273,6 @@ public void map() throws Exception { public void comprehension() throws Exception { CelCompiler celCompiler = CelCompilerFactory.standardCelCompilerBuilder() - .setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build()) .setStandardMacros(CelStandardMacro.STANDARD_MACROS) .build(); CelAbstractSyntaxTree ast = celCompiler.compile("[1, 2, 3].exists(x, x > 0)").getAst(); diff --git a/common/src/test/java/dev/cel/common/ast/CelExprVisitorTest.java b/common/src/test/java/dev/cel/common/ast/CelExprVisitorTest.java index 97f1a4a1b..5eae59e88 100644 --- a/common/src/test/java/dev/cel/common/ast/CelExprVisitorTest.java +++ b/common/src/test/java/dev/cel/common/ast/CelExprVisitorTest.java @@ -21,7 +21,6 @@ import com.google.common.collect.ImmutableList; import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.CelContainer; -import dev.cel.common.CelOptions; import dev.cel.common.Operator; import dev.cel.common.ast.CelExpr.CelCall; import dev.cel.common.ast.CelExpr.CelComprehension; @@ -327,7 +326,6 @@ public void visitList() throws Exception { public void visitComprehension() throws Exception { CelCompiler celCompiler = CelCompilerFactory.standardCelCompilerBuilder() - .setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build()) .setStandardMacros(CelStandardMacro.ALL) .build(); CelAbstractSyntaxTree ast = celCompiler.compile("[1, 1].all(x, x == 1)").getAst(); diff --git a/common/src/test/java/dev/cel/common/navigation/CelNavigableExprVisitorTest.java b/common/src/test/java/dev/cel/common/navigation/CelNavigableExprVisitorTest.java index 15c6ac620..be35bf615 100644 --- a/common/src/test/java/dev/cel/common/navigation/CelNavigableExprVisitorTest.java +++ b/common/src/test/java/dev/cel/common/navigation/CelNavigableExprVisitorTest.java @@ -811,7 +811,6 @@ public void emptyMapConstruction_allNodesReturned() throws Exception { public void comprehension_preOrder_allNodesReturned() throws Exception { CelCompiler compiler = CelCompilerFactory.standardCelCompilerBuilder() - .setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build()) .setStandardMacros(CelStandardMacro.EXISTS) .build(); CelAbstractSyntaxTree ast = compiler.compile("[true].exists(i, i)").getAst(); @@ -873,7 +872,6 @@ public void comprehension_preOrder_allNodesReturned() throws Exception { public void comprehension_postOrder_allNodesReturned() throws Exception { CelCompiler compiler = CelCompilerFactory.standardCelCompilerBuilder() - .setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build()) .setStandardMacros(CelStandardMacro.EXISTS) .build(); CelAbstractSyntaxTree ast = compiler.compile("[true].exists(i, i)").getAst(); @@ -1011,7 +1009,6 @@ public void comprehension_postOrder_maxIdsSet() throws Exception { public void comprehension_allNodes_parentsPopulated() throws Exception { CelCompiler compiler = CelCompilerFactory.standardCelCompilerBuilder() - .setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build()) .setStandardMacros(CelStandardMacro.EXISTS) .build(); CelAbstractSyntaxTree ast = compiler.compile("[true].exists(i, i)").getAst(); @@ -1070,7 +1067,6 @@ public void comprehension_allNodes_parentsPopulated() throws Exception { public void comprehension_filterComprehension_allNodesReturned() throws Exception { CelCompiler compiler = CelCompilerFactory.standardCelCompilerBuilder() - .setOptions(CelOptions.current().enableHiddenAccumulatorVar(true).build()) .setStandardMacros(CelStandardMacro.EXISTS) .build(); CelAbstractSyntaxTree ast = compiler.compile("[true].exists(i, i)").getAst(); diff --git a/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java b/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java index 7391eb16d..8a415608d 100644 --- a/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java +++ b/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java @@ -495,7 +495,8 @@ private static CelExpr validatedIterationVariable( CelExpr arg = checkNotNull(argument); if (!isSimpleIdentifier(arg)) { return reportArgumentError(exprFactory, arg); - } else if (arg.exprKind().ident().name().equals("__result__")) { + } else if (arg.exprKind().ident().name().equals(exprFactory.getAccumulatorVarName()) + || arg.exprKind().ident().name().equals("__result__")) { return reportAccumulatorOverwriteError(exprFactory, arg); } else { return arg; diff --git a/optimizer/src/main/java/dev/cel/optimizer/AstMutator.java b/optimizer/src/main/java/dev/cel/optimizer/AstMutator.java index ca9fb8bdf..c770844ff 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/AstMutator.java +++ b/optimizer/src/main/java/dev/cel/optimizer/AstMutator.java @@ -714,7 +714,7 @@ private CelMutableExpr mangleIdentsInComprehensionExpr( comprehension.setIterVar(mangledComprehensionName.iterVarName()); - // Most standard macros set accu_var as __result__, but not all (ex: cel.bind). + // Most standard macros set accu_var as @result, but not all (ex: cel.bind). if (comprehension.accuVar().equals(originalAccuVar)) { comprehension.setAccuVar(mangledComprehensionName.resultName()); } diff --git a/parser/src/main/java/dev/cel/parser/AntlrParser.java b/parser/src/main/java/dev/cel/parser/AntlrParser.java index 155fb0843..2f86d178f 100644 --- a/parser/src/main/java/dev/cel/parser/AntlrParser.java +++ b/parser/src/main/java/dev/cel/parser/AntlrParser.java @@ -129,8 +129,7 @@ final class AntlrParser extends CELBaseVisitor { "var", "void", "while"); - private static final String ACCUMULATOR_NAME = "__result__"; - private static final String HIDDEN_ACCUMULATOR_NAME = "@result"; + private static final String ACCUMULATOR_NAME = "@result"; static CelValidationResult parse( CelSource source, CelOptions options, Collection macros) { @@ -151,10 +150,7 @@ static CelValidationResult parse( sourceInfo.setDescription(source.getDescription()); ExprFactory exprFactory = new ExprFactory( - antlrParser, - sourceInfo, - options.enableHiddenAccumulatorVar() ? HIDDEN_ACCUMULATOR_NAME : ACCUMULATOR_NAME, - options.maxParseExpressionNodeCount()); + antlrParser, sourceInfo, ACCUMULATOR_NAME, options.maxParseExpressionNodeCount()); AntlrParser parserImpl = new AntlrParser(options, macros, sourceInfo, exprFactory); ErrorListener errorListener = new ErrorListener(exprFactory); antlrLexer.removeErrorListeners(); diff --git a/parser/src/main/java/dev/cel/parser/CelStandardMacro.java b/parser/src/main/java/dev/cel/parser/CelStandardMacro.java index 275159569..823f71047 100644 --- a/parser/src/main/java/dev/cel/parser/CelStandardMacro.java +++ b/parser/src/main/java/dev/cel/parser/CelStandardMacro.java @@ -307,7 +307,8 @@ private static CelExpr validatedIterationVariable( CelExpr arg = checkNotNull(argument); if (!isSimpleIdentifier(arg)) { return reportArgumentError(exprFactory, arg); - } else if (arg.exprKind().ident().name().equals("__result__")) { + } else if (arg.exprKind().ident().name().equals(exprFactory.getAccumulatorVarName()) + || arg.exprKind().ident().name().equals("__result__")) { return reportAccumulatorOverwriteError(exprFactory, arg); } else { return arg; diff --git a/parser/src/test/java/dev/cel/parser/CelMacroExprFactoryTest.java b/parser/src/test/java/dev/cel/parser/CelMacroExprFactoryTest.java index 72abf81a4..018ce97a4 100644 --- a/parser/src/test/java/dev/cel/parser/CelMacroExprFactoryTest.java +++ b/parser/src/test/java/dev/cel/parser/CelMacroExprFactoryTest.java @@ -63,7 +63,7 @@ public CelExpr reportError(CelIssue issue) { @Override public String getAccumulatorVarName() { - return "__result__"; + return "@result"; } @Override diff --git a/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java b/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java index 0f9fb36b7..72ba9aab8 100644 --- a/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java +++ b/parser/src/test/java/dev/cel/parser/CelParserParameterizedTest.java @@ -50,7 +50,6 @@ public final class CelParserParameterizedTest extends BaselineTestCase { .populateMacroCalls(true) .enableOptionalSyntax(true) .enableQuotedIdentifierSyntax(true) - .enableHiddenAccumulatorVar(true) .build(); private static final CelOptions OPTIONS_MAX_RECURSION_DEPTH_32 = @@ -74,9 +73,6 @@ public final class CelParserParameterizedTest extends BaselineTestCase { private static final CelOptions OPTIONS_MAX_ERROR_RECOVERY_LIMIT_2 = OPTIONS.toBuilder().maxParseErrorRecoveryLimit(2).build(); - private static final CelOptions OPTIONS_OLD_ACCU_VAR = - OPTIONS.toBuilder().enableHiddenAccumulatorVar(false).build(); - private static final ImmutableMap MACROS = ImmutableMap.builder() .putAll( @@ -681,17 +677,6 @@ public void parser_errors() { runTest(OPTIONS_MAX_ERROR_RECOVERY_LIMIT_2, "[1 2 3 a b c]"); } - @Test - public void parser_legacyAccuVar() { - runAntlrTest(OPTIONS_OLD_ACCU_VAR, "x * 2"); - runAntlrTest(OPTIONS_OLD_ACCU_VAR, "has(m.f)"); - runAntlrTest(OPTIONS_OLD_ACCU_VAR, "m.exists_one(v, f)"); - runAntlrTest(OPTIONS_OLD_ACCU_VAR, "m.all(v, f)"); - runAntlrTest(OPTIONS_OLD_ACCU_VAR, "m.map(v, f)"); - runAntlrTest(OPTIONS_OLD_ACCU_VAR, "m.map(v, p, f)"); - runAntlrTest(OPTIONS_OLD_ACCU_VAR, "m.filter(v, p)"); - } - private void runAntlrTest(CelOptions options, String expression) { testOutput().println("I: " + sanitizeForBaseline(expression)); testOutput().println("=====>"); diff --git a/parser/src/test/resources/parser_legacyAccuVar.baseline b/parser/src/test/resources/parser_legacyAccuVar.baseline deleted file mode 100644 index 5f9a48b31..000000000 --- a/parser/src/test/resources/parser_legacyAccuVar.baseline +++ /dev/null @@ -1,280 +0,0 @@ -I: x * 2 -=====> -P: _*_( - x^#1:Expr.Ident#, - 2^#3:int64# -)^#2:Expr.Call# -L: _*_( - x^#1[1,0]#, - 2^#3[1,4]# -)^#2[1,2]# - -I: has(m.f) -=====> -P: m^#2:Expr.Ident#.f~test-only~^#4:Expr.Select# -L: m^#2[1,4]#.f~test-only~^#4[1,3]# -M: has( - m^#2:Expr.Ident#.f^#3:Expr.Select# -)^#0:Expr.Call# - -I: m.exists_one(v, f) -=====> -P: __comprehension__( - // Variable - v, - // Target - m^#1:Expr.Ident#, - // Accumulator - __result__, - // Init - 0^#5:int64#, - // LoopCondition - true^#6:bool#, - // LoopStep - _?_:_( - f^#4:Expr.Ident#, - _+_( - __result__^#7:Expr.Ident#, - 1^#8:int64# - )^#9:Expr.Call#, - __result__^#10:Expr.Ident# - )^#11:Expr.Call#, - // Result - _==_( - __result__^#12:Expr.Ident#, - 1^#13:int64# - )^#14:Expr.Call#)^#15:Expr.Comprehension# -L: __comprehension__( - // Variable - v, - // Target - m^#1[1,0]#, - // Accumulator - __result__, - // Init - 0^#5[1,12]#, - // LoopCondition - true^#6[1,12]#, - // LoopStep - _?_:_( - f^#4[1,16]#, - _+_( - __result__^#7[1,12]#, - 1^#8[1,12]# - )^#9[1,12]#, - __result__^#10[1,12]# - )^#11[1,12]#, - // Result - _==_( - __result__^#12[1,12]#, - 1^#13[1,12]# - )^#14[1,12]#)^#15[1,12]# -M: m^#1:Expr.Ident#.exists_one( - v^#3:Expr.Ident#, - f^#4:Expr.Ident# -)^#0:Expr.Call# - -I: m.all(v, f) -=====> -P: __comprehension__( - // Variable - v, - // Target - m^#1:Expr.Ident#, - // Accumulator - __result__, - // Init - true^#5:bool#, - // LoopCondition - @not_strictly_false( - __result__^#6:Expr.Ident# - )^#7:Expr.Call#, - // LoopStep - _&&_( - __result__^#8:Expr.Ident#, - f^#4:Expr.Ident# - )^#9:Expr.Call#, - // Result - __result__^#10:Expr.Ident#)^#11:Expr.Comprehension# -L: __comprehension__( - // Variable - v, - // Target - m^#1[1,0]#, - // Accumulator - __result__, - // Init - true^#5[1,5]#, - // LoopCondition - @not_strictly_false( - __result__^#6[1,5]# - )^#7[1,5]#, - // LoopStep - _&&_( - __result__^#8[1,5]#, - f^#4[1,9]# - )^#9[1,5]#, - // Result - __result__^#10[1,5]#)^#11[1,5]# -M: m^#1:Expr.Ident#.all( - v^#3:Expr.Ident#, - f^#4:Expr.Ident# -)^#0:Expr.Call# - -I: m.map(v, f) -=====> -P: __comprehension__( - // Variable - v, - // Target - m^#1:Expr.Ident#, - // Accumulator - __result__, - // Init - []^#5:Expr.CreateList#, - // LoopCondition - true^#6:bool#, - // LoopStep - _+_( - __result__^#7:Expr.Ident#, - [ - f^#4:Expr.Ident# - ]^#8:Expr.CreateList# - )^#9:Expr.Call#, - // Result - __result__^#10:Expr.Ident#)^#11:Expr.Comprehension# -L: __comprehension__( - // Variable - v, - // Target - m^#1[1,0]#, - // Accumulator - __result__, - // Init - []^#5[1,5]#, - // LoopCondition - true^#6[1,5]#, - // LoopStep - _+_( - __result__^#7[1,5]#, - [ - f^#4[1,9]# - ]^#8[1,5]# - )^#9[1,5]#, - // Result - __result__^#10[1,5]#)^#11[1,5]# -M: m^#1:Expr.Ident#.map( - v^#3:Expr.Ident#, - f^#4:Expr.Ident# -)^#0:Expr.Call# - -I: m.map(v, p, f) -=====> -P: __comprehension__( - // Variable - v, - // Target - m^#1:Expr.Ident#, - // Accumulator - __result__, - // Init - []^#6:Expr.CreateList#, - // LoopCondition - true^#7:bool#, - // LoopStep - _?_:_( - p^#4:Expr.Ident#, - _+_( - __result__^#8:Expr.Ident#, - [ - f^#5:Expr.Ident# - ]^#9:Expr.CreateList# - )^#10:Expr.Call#, - __result__^#11:Expr.Ident# - )^#12:Expr.Call#, - // Result - __result__^#13:Expr.Ident#)^#14:Expr.Comprehension# -L: __comprehension__( - // Variable - v, - // Target - m^#1[1,0]#, - // Accumulator - __result__, - // Init - []^#6[1,5]#, - // LoopCondition - true^#7[1,5]#, - // LoopStep - _?_:_( - p^#4[1,9]#, - _+_( - __result__^#8[1,5]#, - [ - f^#5[1,12]# - ]^#9[1,5]# - )^#10[1,5]#, - __result__^#11[1,5]# - )^#12[1,5]#, - // Result - __result__^#13[1,5]#)^#14[1,5]# -M: m^#1:Expr.Ident#.map( - v^#3:Expr.Ident#, - p^#4:Expr.Ident#, - f^#5:Expr.Ident# -)^#0:Expr.Call# - -I: m.filter(v, p) -=====> -P: __comprehension__( - // Variable - v, - // Target - m^#1:Expr.Ident#, - // Accumulator - __result__, - // Init - []^#5:Expr.CreateList#, - // LoopCondition - true^#6:bool#, - // LoopStep - _?_:_( - p^#4:Expr.Ident#, - _+_( - __result__^#7:Expr.Ident#, - [ - v^#3:Expr.Ident# - ]^#8:Expr.CreateList# - )^#9:Expr.Call#, - __result__^#10:Expr.Ident# - )^#11:Expr.Call#, - // Result - __result__^#12:Expr.Ident#)^#13:Expr.Comprehension# -L: __comprehension__( - // Variable - v, - // Target - m^#1[1,0]#, - // Accumulator - __result__, - // Init - []^#5[1,8]#, - // LoopCondition - true^#6[1,8]#, - // LoopStep - _?_:_( - p^#4[1,12]#, - _+_( - __result__^#7[1,8]#, - [ - v^#3[1,9]# - ]^#8[1,8]# - )^#9[1,8]#, - __result__^#10[1,8]# - )^#11[1,8]#, - // Result - __result__^#12[1,8]#)^#13[1,8]# -M: m^#1:Expr.Ident#.filter( - v^#3:Expr.Ident#, - p^#4:Expr.Ident# -)^#0:Expr.Call# \ No newline at end of file diff --git a/testing/src/main/java/dev/cel/testing/CelBaselineTestCase.java b/testing/src/main/java/dev/cel/testing/CelBaselineTestCase.java index 8c79e5931..e62c24ada 100644 --- a/testing/src/main/java/dev/cel/testing/CelBaselineTestCase.java +++ b/testing/src/main/java/dev/cel/testing/CelBaselineTestCase.java @@ -57,7 +57,6 @@ public abstract class CelBaselineTestCase extends BaselineTestCase { protected static final CelOptions TEST_OPTIONS = CelOptions.current() .enableHeterogeneousNumericComparisons(true) - .enableHiddenAccumulatorVar(true) .enableOptionalSyntax(true) .comprehensionMaxIterations(1_000) .build();