Skip to content

Commit a2135f5

Browse files
l46kokcopybara-github
l46kok
authored and
copybara-github
committed
Remove unwrapListArgumentsInMacroCallExpr from AstMutator.
PiperOrigin-RevId: 996680420
1 parent 40d7f3a commit a2135f5

2 files changed

Lines changed: 15 additions & 160 deletions

File tree

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

Lines changed: 0 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -889,38 +889,6 @@ private CelMutableSource normalizeMacroSource(
889889
}
890890
}
891891

892-
// Handle replacing the synthetic single-element list inside a `map` or `filter` loop_step.
893-
//
894-
// Example: `[1].map(x, 10)`
895-
// - In the main AST, `loop_step` is `@result + [10]` (where `[10]` is a synthetic LIST
896-
// node wrapping the body `10`).
897-
// - In `macro_calls`, the call is `[1].map(x, 10)`, which only references the inner `10`
898-
// node—NOT the synthetic `[10]` LIST node.
899-
//
900-
// If a mutation replaces the `[10]` LIST node itself (e.g. with `[20]`), the loop above
901-
// won't find `[10]`'s ID in `macro_calls`, so we unwrap `20` from `[20]` into the macro
902-
// call (`[1].map(x, 20)`).
903-
//
904-
// We must first verify that:
905-
// 1. This macro is actually a COMPREHENSION (e.g. `has(msg.f)` is in `macro_calls` too,
906-
// but expands to a SELECT node, not a COMPREHENSION).
907-
// 2. The replaced LIST node is inside *this* comprehension's `loop_step` (not an unrelated
908-
// list replacement elsewhere in the AST, such as folding `[1, 2] + [3, 4]`).
909-
if (exprIdToReplace > 0
910-
&& allExprs.get(callId).getKind().equals(ExprKind.Kind.COMPREHENSION)) {
911-
long replacedId = idGenerator.generate(exprIdToReplace);
912-
CelMutableComprehension comprehension = allExprs.get(callId).comprehension();
913-
boolean isListExprBeingReplaced =
914-
allExprs.containsKey(replacedId)
915-
&& allExprs.get(replacedId).getKind().equals(ExprKind.Kind.LIST)
916-
&& CelNavigableMutableExpr.fromExpr(comprehension.loopStep())
917-
.allNodes()
918-
.anyMatch(node -> node.id() == replacedId);
919-
if (isListExprBeingReplaced) {
920-
unwrapListArgumentsInMacroCallExpr(comprehension, newMacroCallExpr);
921-
}
922-
}
923-
924892
newMacroSource.addMacroCalls(callId, newMacroCallExpr);
925893
}
926894

@@ -963,57 +931,6 @@ private CelMutableSource normalizeMacroSource(
963931
return newMacroSource;
964932
}
965933

966-
/**
967-
* Unwraps the arguments in the extraneous list_expr which is present in the AST but does not
968-
* exist in the macro call map. `map`, `filter` are examples of such.
969-
*
970-
* <p>This method inspects the comprehension's accumulator initializer to infer that the list_expr
971-
* solely exists to match the expected result type of the macro call signature.
972-
*
973-
* @param comprehension Comprehension in the main AST to extract the macro call arguments from
974-
* (loop step).
975-
* @param newMacroCallExpr (Output parameter) Modified macro call expression with the call
976-
* arguments unwrapped.
977-
*/
978-
private static void unwrapListArgumentsInMacroCallExpr(
979-
CelMutableComprehension comprehension, CelMutableExpr newMacroCallExpr) {
980-
CelMutableExpr accuInit = comprehension.accuInit();
981-
if (!accuInit.getKind().equals(ExprKind.Kind.LIST) || !accuInit.list().elements().isEmpty()) {
982-
// Does not contain an extraneous list.
983-
return;
984-
}
985-
986-
CelMutableExpr loopStepExpr = comprehension.loopStep();
987-
List<CelMutableExpr> loopStepArgs = loopStepExpr.call().args();
988-
if (loopStepArgs.size() != 2 && loopStepArgs.size() != 3) {
989-
throw new IllegalArgumentException(
990-
String.format(
991-
"Expected exactly 2 or 3 arguments but got %d instead on expr id: %d",
992-
loopStepArgs.size(), loopStepExpr.id()));
993-
}
994-
995-
CelMutableCall existingMacroCall = newMacroCallExpr.call();
996-
CelMutableCall newMacroCall =
997-
existingMacroCall.target().isPresent()
998-
? CelMutableCall.create(existingMacroCall.target().get(), existingMacroCall.function())
999-
: CelMutableCall.create(existingMacroCall.function());
1000-
newMacroCall.addArgs(
1001-
existingMacroCall.args().get(0)); // iter_var is first argument of the call by convention
1002-
1003-
CelMutableList extraneousList;
1004-
if (loopStepArgs.size() == 2) {
1005-
extraneousList = loopStepArgs.get(1).list();
1006-
} else {
1007-
newMacroCall.addArgs(loopStepArgs.get(0));
1008-
// For map(x,y,z), z is wrapped in a _+_(@result, [z])
1009-
extraneousList = loopStepArgs.get(1).call().args().get(1).list();
1010-
}
1011-
1012-
newMacroCall.addArgs(extraneousList.elements());
1013-
1014-
newMacroCallExpr.setCall(newMacroCall);
1015-
}
1016-
1017934
private CelMutableExpr mutateExpr(
1018935
ExprIdGenerator idGenerator,
1019936
CelMutableExpr root,

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

Lines changed: 15 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -308,77 +308,6 @@ public void replaceSubtree_macroReplacedWithConstExpr_macroCallCleared() throws
308308
assertThat(CEL.createProgram(CEL.check(mutatedAst).getAst()).eval()).isEqualTo(1);
309309
}
310310

311-
@Test
312-
@SuppressWarnings("unchecked") // Test only
313-
public void replaceSubtree_replaceExtraneousListCreatedByMacro_unparseSuccess() throws Exception {
314-
// Certain macros such as `map` or `filter` generates an extraneous list_expr in the loop step's
315-
// argument that does not exist in the original expression.
316-
// For example, the loop step of this expression looks like:
317-
// CALL [10] {
318-
// function: _+_
319-
// args: {
320-
// IDENT [8] {
321-
// name: __result__
322-
// }
323-
// LIST [9] {
324-
// elements: {
325-
// CONSTANT [5] { value: 1 }
326-
// }
327-
// }
328-
// }
329-
// }
330-
CelAbstractSyntaxTree ast = CEL.compile("[1].map(x, 1)").getAst();
331-
CelMutableAst mutableAst = CelMutableAst.fromCelAst(ast);
332-
CelMutableAst mutableAst2 = CelMutableAst.fromCelAst(ast);
333-
334-
// These two mutation are equivalent.
335-
CelAbstractSyntaxTree mutatedAstWithList =
336-
AST_MUTATOR
337-
.replaceSubtree(
338-
mutableAst,
339-
CelMutableExpr.ofList(
340-
CelMutableList.create(CelMutableExpr.ofConstant(CelConstant.ofValue(2L)))),
341-
9L)
342-
.toParsedAst();
343-
CelAbstractSyntaxTree mutatedAstWithConstant =
344-
AST_MUTATOR
345-
.replaceSubtree(mutableAst2, CelMutableExpr.ofConstant(CelConstant.ofValue(2L)), 5L)
346-
.toParsedAst();
347-
348-
assertThat(CEL_UNPARSER.unparse(mutatedAstWithList)).isEqualTo("[1].map(x, 2)");
349-
assertThat(CEL_UNPARSER.unparse(mutatedAstWithConstant)).isEqualTo("[1].map(x, 2)");
350-
assertThat((List<Long>) CEL.createProgram(CEL.check(mutatedAstWithList).getAst()).eval())
351-
.containsExactly(2L);
352-
}
353-
354-
@Test
355-
@SuppressWarnings("unchecked") // Test only
356-
public void replaceSubtree_replaceExtraneousListCreatedByThreeArgMacro_unparseSuccess()
357-
throws Exception {
358-
CelAbstractSyntaxTree ast = CEL.compile("[1].map(x, true, 1)").getAst();
359-
CelMutableAst mutableAst = CelMutableAst.fromCelAst(ast);
360-
CelMutableAst mutableAst2 = CelMutableAst.fromCelAst(ast);
361-
362-
// These two mutation are equivalent.
363-
CelAbstractSyntaxTree mutatedAstWithList =
364-
AST_MUTATOR
365-
.replaceSubtree(
366-
mutableAst,
367-
CelMutableExpr.ofList(
368-
CelMutableList.create(CelMutableExpr.ofConstant(CelConstant.ofValue(2L)))),
369-
10L)
370-
.toParsedAst();
371-
CelAbstractSyntaxTree mutatedAstWithConstant =
372-
AST_MUTATOR
373-
.replaceSubtree(mutableAst2, CelMutableExpr.ofConstant(CelConstant.ofValue(2L)), 6L)
374-
.toParsedAst();
375-
376-
assertThat(CEL_UNPARSER.unparse(mutatedAstWithList)).isEqualTo("[1].map(x, true, 2)");
377-
assertThat(CEL_UNPARSER.unparse(mutatedAstWithConstant)).isEqualTo("[1].map(x, true, 2)");
378-
assertThat((List<Long>) CEL.createProgram(CEL.check(mutatedAstWithList).getAst()).eval())
379-
.containsExactly(2L);
380-
}
381-
382311
@Test
383312
public void globalCallExpr_replaceRoot() throws Exception {
384313
// Tree shape (brackets are expr IDs):
@@ -580,21 +509,30 @@ public void list_replaceElement() throws Exception {
580509
}
581510

582511
@Test
583-
public void list_replaceSubtreeWithListInAstWithHasMacro_success() throws Exception {
584-
CelAbstractSyntaxTree ast = CEL.compile("has(msg.single_int64) && 1 in [2]").getAst();
512+
@TestParameters(
513+
"{source: 'has(msg.single_int64) && 1 in [2]', exprIdToReplace: 8,"
514+
+ " expected: 'has(msg.single_int64) && 1 in [1, 2]'}")
515+
@TestParameters(
516+
"{source: '[1].filter(x, x in [2])', exprIdToReplace: 7,"
517+
+ " expected: '[1].filter(x, x in [1, 2])'}")
518+
@TestParameters("{source: '[1].map(x, [2])', exprIdToReplace: 5, expected: '[1].map(x, [1, 2])'}")
519+
@TestParameters(
520+
"{source: '[1].map(x, true, [2])', exprIdToReplace: 6,"
521+
+ " expected: '[1].map(x, true, [1, 2])'}")
522+
public void list_replaceSubtreeWithListInAstWithMacro_success(
523+
String source, long exprIdToReplace, String expected) throws Exception {
524+
CelAbstractSyntaxTree ast = CEL.compile(source).getAst();
585525
CelMutableAst mutableAst = CelMutableAst.fromCelAst(ast);
586526
CelMutableExpr foldedList =
587527
CelMutableExpr.ofList(
588528
CelMutableList.create(
589529
CelMutableExpr.ofConstant(CelConstant.ofValue(1)),
590530
CelMutableExpr.ofConstant(CelConstant.ofValue(2))));
591531

592-
// Node 8 is `[2]`; replacing it with a LIST triggers normalizeMacroSource while `has(...)` is
593-
// present in macroCalls as a SELECT node rather than a COMPREHENSION node.
594532
CelAbstractSyntaxTree replacedAst =
595-
AST_MUTATOR.replaceSubtree(mutableAst, foldedList, 8).toParsedAst();
533+
AST_MUTATOR.replaceSubtree(mutableAst, foldedList, exprIdToReplace).toParsedAst();
596534

597-
assertThat(CEL_UNPARSER.unparse(replacedAst)).isEqualTo("has(msg.single_int64) && 1 in [1, 2]");
535+
assertThat(CEL_UNPARSER.unparse(replacedAst)).isEqualTo(expected);
598536
assertConsistentMacroCalls(replacedAst);
599537
}
600538

0 commit comments

Comments
 (0)