Skip to content

Commit d7c8758

Browse files
l46kokcopybara-github
l46kok
authored and
copybara-github
committed
Remove capability to serially increment mangled variable indices
PiperOrigin-RevId: 803111699
1 parent 361c802 commit d7c8758

File tree

3 files changed

+35
-58
lines changed

3 files changed

+35
-58
lines changed

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

Lines changed: 8 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -203,14 +203,9 @@ public CelMutableAst renumberIdsConsecutively(CelMutableAst mutableAst) {
203203
* @param newIterVarPrefix Prefix to use for new iteration variable identifier name. For example,
204204
* providing @c will produce @c0:0, @c0:1, @c1:0, @c2:0... as new names.
205205
* @param newAccuVarPrefix Prefix to use for new accumulation variable identifier name.
206-
* @param incrementSerially If true, indices for the mangled variables are incremented serially
207-
* per occurrence regardless of their nesting level or its types.
208206
*/
209207
public MangledComprehensionAst mangleComprehensionIdentifierNames(
210-
CelMutableAst ast,
211-
String newIterVarPrefix,
212-
String newAccuVarPrefix,
213-
boolean incrementSerially) {
208+
CelMutableAst ast, String newIterVarPrefix, String newAccuVarPrefix) {
214209
CelNavigableMutableAst navigableMutableAst = CelNavigableMutableAst.fromAst(ast);
215210
Predicate<CelNavigableMutableExpr> comprehensionIdentifierPredicate = x -> true;
216211
comprehensionIdentifierPredicate =
@@ -301,29 +296,13 @@ public MangledComprehensionAst mangleComprehensionIdentifierNames(
301296
MangledComprehensionType comprehensionEntryType = comprehensionEntry.getValue();
302297

303298
CelMutableExpr comprehensionExpr = comprehensionNode.expr();
304-
MangledComprehensionName mangledComprehensionName;
305-
if (incrementSerially) {
306-
// In case of applying CSE via cascaded cel.binds, not only is mangling based on level/types
307-
// meaningless (because all comprehensions are nested anyways, thus all indices would be
308-
// uinque),
309-
// it can lead to an erroneous result due to extracting a common subexpr with accu_var at
310-
// the wrong scope.
311-
// Example: "[1].exists(k, k > 1) && [2].exists(l, l > 1). The loop step for both branches
312-
// are identical, but shouldn't be extracted.
313-
String mangledIterVarName = newIterVarPrefix + ":" + iterCount;
314-
String mangledResultName = newAccuVarPrefix + ":" + iterCount;
315-
mangledComprehensionName =
316-
MangledComprehensionName.of(mangledIterVarName, mangledResultName);
317-
mangledIdentNamesToType.put(mangledComprehensionName, comprehensionEntry.getValue());
318-
} else {
319-
mangledComprehensionName =
320-
getMangledComprehensionName(
321-
newIterVarPrefix,
322-
newAccuVarPrefix,
323-
comprehensionNode,
324-
comprehensionLevelToType,
325-
comprehensionEntryType);
326-
}
299+
MangledComprehensionName mangledComprehensionName =
300+
getMangledComprehensionName(
301+
newIterVarPrefix,
302+
newAccuVarPrefix,
303+
comprehensionNode,
304+
comprehensionLevelToType,
305+
comprehensionEntryType);
327306
mangledIdentNamesToType.put(mangledComprehensionName, comprehensionEntryType);
328307

329308
String iterVar = comprehensionExpr.comprehension().iterVar();

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,7 @@ private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel
136136
astMutator.mangleComprehensionIdentifierNames(
137137
astToModify,
138138
MANGLED_COMPREHENSION_ITER_VAR_PREFIX,
139-
MANGLED_COMPREHENSION_ACCU_VAR_PREFIX,
140-
/* incrementSerially= */ false);
139+
MANGLED_COMPREHENSION_ACCU_VAR_PREFIX);
141140
astToModify = mangledComprehensionAst.mutableAst();
142141
CelMutableSource sourceToModify = astToModify.source();
143142

‎optimizer/src/test/java/dev/cel/optimizer/AstMutatorTest.java‎

Lines changed: 26 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -666,22 +666,22 @@ public void mangleComprehensionVariable_singleMacro() throws Exception {
666666

667667
CelAbstractSyntaxTree mangledAst =
668668
AST_MUTATOR
669-
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac", true)
669+
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac")
670670
.mutableAst()
671671
.toParsedAst();
672672

673673
assertThat(mangledAst.getExpr().toString())
674674
.isEqualTo(
675675
"COMPREHENSION [13] {\n"
676-
+ " iter_var: @it:0\n"
676+
+ " iter_var: @it:0:0\n"
677677
+ " iter_range: {\n"
678678
+ " LIST [1] {\n"
679679
+ " elements: {\n"
680680
+ " CONSTANT [2] { value: false }\n"
681681
+ " }\n"
682682
+ " }\n"
683683
+ " }\n"
684-
+ " accu_var: @ac:0\n"
684+
+ " accu_var: @ac:0:0\n"
685685
+ " accu_init: {\n"
686686
+ " CONSTANT [6] { value: false }\n"
687687
+ " }\n"
@@ -693,7 +693,7 @@ public void mangleComprehensionVariable_singleMacro() throws Exception {
693693
+ " function: !_\n"
694694
+ " args: {\n"
695695
+ " IDENT [7] {\n"
696-
+ " name: @ac:0\n"
696+
+ " name: @ac:0:0\n"
697697
+ " }\n"
698698
+ " }\n"
699699
+ " }\n"
@@ -705,21 +705,21 @@ public void mangleComprehensionVariable_singleMacro() throws Exception {
705705
+ " function: _||_\n"
706706
+ " args: {\n"
707707
+ " IDENT [10] {\n"
708-
+ " name: @ac:0\n"
708+
+ " name: @ac:0:0\n"
709709
+ " }\n"
710710
+ " IDENT [5] {\n"
711-
+ " name: @it:0\n"
711+
+ " name: @it:0:0\n"
712712
+ " }\n"
713713
+ " }\n"
714714
+ " }\n"
715715
+ " }\n"
716716
+ " result: {\n"
717717
+ " IDENT [12] {\n"
718-
+ " name: @ac:0\n"
718+
+ " name: @ac:0:0\n"
719719
+ " }\n"
720720
+ " }\n"
721721
+ "}");
722-
assertThat(CEL_UNPARSER.unparse(mangledAst)).isEqualTo("[false].exists(@it:0, @it:0)");
722+
assertThat(CEL_UNPARSER.unparse(mangledAst)).isEqualTo("[false].exists(@it:0:0, @it:0:0)");
723723
assertThat(CEL.createProgram(CEL.check(mangledAst).getAst()).eval()).isEqualTo(false);
724724
assertConsistentMacroCalls(ast);
725725
}
@@ -734,7 +734,7 @@ public void mangleComprehensionVariable_adjacentMacros_sameIterVarTypes() throws
734734

735735
CelAbstractSyntaxTree mangledAst =
736736
AST_MUTATOR
737-
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac", false)
737+
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac")
738738
.mutableAst()
739739
.toParsedAst();
740740

@@ -756,7 +756,7 @@ public void mangleComprehensionVariable_adjacentMacros_differentIterVarTypes() t
756756

757757
CelAbstractSyntaxTree mangledAst =
758758
AST_MUTATOR
759-
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac", false)
759+
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac")
760760
.mutableAst()
761761
.toParsedAst();
762762

@@ -780,7 +780,7 @@ public void mangleComprehensionVariable_macroSourceDisabled_macroCallMapIsEmpty(
780780

781781
CelAbstractSyntaxTree mangledAst =
782782
AST_MUTATOR
783-
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac", true)
783+
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac")
784784
.mutableAst()
785785
.toParsedAst();
786786

@@ -793,14 +793,14 @@ public void mangleComprehensionVariable_nestedMacroWithShadowedVariables() throw
793793

794794
CelAbstractSyntaxTree mangledAst =
795795
AST_MUTATOR
796-
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac", true)
796+
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac")
797797
.mutableAst()
798798
.toParsedAst();
799799

800800
assertThat(mangledAst.getExpr().toString())
801801
.isEqualTo(
802802
"COMPREHENSION [27] {\n"
803-
+ " iter_var: @it:1\n"
803+
+ " iter_var: @it:0:0\n"
804804
+ " iter_range: {\n"
805805
+ " LIST [1] {\n"
806806
+ " elements: {\n"
@@ -810,7 +810,7 @@ public void mangleComprehensionVariable_nestedMacroWithShadowedVariables() throw
810810
+ " }\n"
811811
+ " }\n"
812812
+ " }\n"
813-
+ " accu_var: @ac:1\n"
813+
+ " accu_var: @ac:0:0\n"
814814
+ " accu_init: {\n"
815815
+ " CONSTANT [20] { value: false }\n"
816816
+ " }\n"
@@ -822,7 +822,7 @@ public void mangleComprehensionVariable_nestedMacroWithShadowedVariables() throw
822822
+ " function: !_\n"
823823
+ " args: {\n"
824824
+ " IDENT [21] {\n"
825-
+ " name: @ac:1\n"
825+
+ " name: @ac:0:0\n"
826826
+ " }\n"
827827
+ " }\n"
828828
+ " }\n"
@@ -834,20 +834,20 @@ public void mangleComprehensionVariable_nestedMacroWithShadowedVariables() throw
834834
+ " function: _||_\n"
835835
+ " args: {\n"
836836
+ " IDENT [24] {\n"
837-
+ " name: @ac:1\n"
837+
+ " name: @ac:0:0\n"
838838
+ " }\n"
839839
+ " COMPREHENSION [19] {\n"
840-
+ " iter_var: @it:0\n"
840+
+ " iter_var: @it:1:0\n"
841841
+ " iter_range: {\n"
842842
+ " LIST [5] {\n"
843843
+ " elements: {\n"
844844
+ " IDENT [6] {\n"
845-
+ " name: @it:1\n"
845+
+ " name: @it:0:0\n"
846846
+ " }\n"
847847
+ " }\n"
848848
+ " }\n"
849849
+ " }\n"
850-
+ " accu_var: @ac:0\n"
850+
+ " accu_var: @ac:1:0\n"
851851
+ " accu_init: {\n"
852852
+ " CONSTANT [12] { value: false }\n"
853853
+ " }\n"
@@ -859,7 +859,7 @@ public void mangleComprehensionVariable_nestedMacroWithShadowedVariables() throw
859859
+ " function: !_\n"
860860
+ " args: {\n"
861861
+ " IDENT [13] {\n"
862-
+ " name: @ac:0\n"
862+
+ " name: @ac:1:0\n"
863863
+ " }\n"
864864
+ " }\n"
865865
+ " }\n"
@@ -871,13 +871,13 @@ public void mangleComprehensionVariable_nestedMacroWithShadowedVariables() throw
871871
+ " function: _||_\n"
872872
+ " args: {\n"
873873
+ " IDENT [16] {\n"
874-
+ " name: @ac:0\n"
874+
+ " name: @ac:1:0\n"
875875
+ " }\n"
876876
+ " CALL [10] {\n"
877877
+ " function: _==_\n"
878878
+ " args: {\n"
879879
+ " IDENT [9] {\n"
880-
+ " name: @it:0\n"
880+
+ " name: @it:1:0\n"
881881
+ " }\n"
882882
+ " CONSTANT [11] { value: 1 }\n"
883883
+ " }\n"
@@ -887,7 +887,7 @@ public void mangleComprehensionVariable_nestedMacroWithShadowedVariables() throw
887887
+ " }\n"
888888
+ " result: {\n"
889889
+ " IDENT [18] {\n"
890-
+ " name: @ac:0\n"
890+
+ " name: @ac:1:0\n"
891891
+ " }\n"
892892
+ " }\n"
893893
+ " }\n"
@@ -896,13 +896,12 @@ public void mangleComprehensionVariable_nestedMacroWithShadowedVariables() throw
896896
+ " }\n"
897897
+ " result: {\n"
898898
+ " IDENT [26] {\n"
899-
+ " name: @ac:1\n"
899+
+ " name: @ac:0:0\n"
900900
+ " }\n"
901901
+ " }\n"
902902
+ "}");
903-
904903
assertThat(CEL_UNPARSER.unparse(mangledAst))
905-
.isEqualTo("[x].exists(@it:1, [@it:1].exists(@it:0, @it:0 == 1))");
904+
.isEqualTo("[x].exists(@it:0:0, [@it:0:0].exists(@it:1:0, @it:1:0 == 1))");
906905
assertThat(CEL.createProgram(CEL.check(mangledAst).getAst()).eval(ImmutableMap.of("x", 1)))
907906
.isEqualTo(true);
908907
assertConsistentMacroCalls(ast);
@@ -914,7 +913,7 @@ public void mangleComprehensionVariable_hasMacro_noOp() throws Exception {
914913

915914
CelAbstractSyntaxTree mangledAst =
916915
AST_MUTATOR
917-
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac", true)
916+
.mangleComprehensionIdentifierNames(CelMutableAst.fromCelAst(ast), "@it", "@ac")
918917
.mutableAst()
919918
.toParsedAst();
920919

0 commit comments

Comments
 (0)