Skip to content

Commit ecf3ff4

Browse files
l46kokcopybara-github
authored andcommitted
Skip non-comprehension macros in macro source noramlizations
PiperOrigin-RevId: 996255870
1 parent d9cf3f9 commit ecf3ff4

2 files changed

Lines changed: 44 additions & 4 deletions

File tree

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

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -889,14 +889,35 @@ private CelMutableSource normalizeMacroSource(
889889
}
890890
}
891891

892-
if (exprIdToReplace > 0) {
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)) {
893911
long replacedId = idGenerator.generate(exprIdToReplace);
912+
CelMutableComprehension comprehension = allExprs.get(callId).comprehension();
894913
boolean isListExprBeingReplaced =
895914
allExprs.containsKey(replacedId)
896-
&& allExprs.get(replacedId).getKind().equals(ExprKind.Kind.LIST);
915+
&& allExprs.get(replacedId).getKind().equals(ExprKind.Kind.LIST)
916+
&& CelNavigableMutableExpr.fromExpr(comprehension.loopStep())
917+
.allNodes()
918+
.anyMatch(node -> node.id() == replacedId);
897919
if (isListExprBeingReplaced) {
898-
unwrapListArgumentsInMacroCallExpr(
899-
allExprs.get(callId).comprehension(), newMacroCallExpr);
920+
unwrapListArgumentsInMacroCallExpr(comprehension, newMacroCallExpr);
900921
}
901922
}
902923

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -579,6 +579,25 @@ public void list_replaceElement() throws Exception {
579579
assertThat(CEL_UNPARSER.unparse(result.toParsedAst())).isEqualTo("[2, 3, 5]");
580580
}
581581

582+
@Test
583+
public void list_replaceSubtreeWithListInAstWithHasMacro_success() throws Exception {
584+
CelAbstractSyntaxTree ast = CEL.compile("has(msg.single_int64) && 1 in [2]").getAst();
585+
CelMutableAst mutableAst = CelMutableAst.fromCelAst(ast);
586+
CelMutableExpr foldedList =
587+
CelMutableExpr.ofList(
588+
CelMutableList.create(
589+
CelMutableExpr.ofConstant(CelConstant.ofValue(1)),
590+
CelMutableExpr.ofConstant(CelConstant.ofValue(2))));
591+
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.
594+
CelAbstractSyntaxTree replacedAst =
595+
AST_MUTATOR.replaceSubtree(mutableAst, foldedList, 8).toParsedAst();
596+
597+
assertThat(CEL_UNPARSER.unparse(replacedAst)).isEqualTo("has(msg.single_int64) && 1 in [1, 2]");
598+
assertConsistentMacroCalls(replacedAst);
599+
}
600+
582601
@Test
583602
public void struct_replaceValue() throws Exception {
584603
// Tree shape (brackets are expr IDs):

0 commit comments

Comments
 (0)