Remove unused replaceSubtreeWithNewBindMacro method

PiperOrigin-RevId: 797498049

Author: l46kok

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

@@ -28,7 +28,6 @@
 import dev.cel.common.CelAbstractSyntaxTree;
 import dev.cel.common.CelMutableAst;
 import dev.cel.common.CelMutableSource;
-import dev.cel.common.ast.CelConstant;
 import dev.cel.common.ast.CelExpr.ExprKind.Kind;
 import dev.cel.common.ast.CelExprIdGeneratorFactory;
 import dev.cel.common.ast.CelExprIdGeneratorFactory.ExprIdGenerator;
@@ -163,75 +162,6 @@ private CelMutableAst newCallAst(
     return CelMutableAst.of(newCallExpr, combinedSource);
   }
 
-  /**
-   * Generates a new bind macro using the provided initialization and result expression, then
-   * replaces the subtree using the new bind expr at the designated expr ID.
-   *
-   * <p>The bind call takes the format of: {@code cel.bind(varInit, varName, resultExpr)}
-   *
-   * @param ast Original AST to mutate.
-   * @param varName New variable name for the bind macro call.
-   * @param varInit Initialization expression to bind to the local variable.
-   * @param resultExpr Result expression
-   * @param exprIdToReplace Expression ID of the subtree that is getting replaced.
-   * @param populateMacroSource If true, populates the cel.bind macro source in the AST.
-   */
-  public CelMutableAst replaceSubtreeWithNewBindMacro(
-      CelMutableAst ast,
-      String varName,
-      CelMutableAst varInit,
-      CelMutableExpr resultExpr,
-      long exprIdToReplace,
-      boolean populateMacroSource) {
-    // Stabilize incoming varInit AST to avoid collision with the main AST
-    long maxId = getMaxId(ast);
-    varInit = stabilizeAst(varInit, maxId);
-    StableIdGenerator stableIdGenerator = CelExprIdGeneratorFactory.newStableIdGenerator(maxId);
-    CelMutableExpr newBindMacroExpr =
-        newBindMacroExpr(
-            varName, varInit.expr(), CelMutableExpr.newInstance(resultExpr), stableIdGenerator);
-    CelMutableSource celSource = CelMutableSource.newInstance();
-    if (populateMacroSource) {
-      CelMutableExpr newBindMacroSourceExpr =
-          newBindMacroSourceExpr(newBindMacroExpr, varName, stableIdGenerator);
-      // In situations where the existing AST already contains a macro call (ex: nested cel.binds),
-      // its macro source must be normalized to make it consistent with the newly generated bind
-      // macro.
-      celSource = combine(ast.source(), varInit.source());
-      celSource =
-          normalizeMacroSource(
-              celSource,
-              -1, // Do not replace any of the subexpr in the macro map.
-              newBindMacroSourceExpr,
-              stableIdGenerator::renumberId);
-      celSource.addMacroCalls(newBindMacroExpr.id(), newBindMacroSourceExpr);
-    }
-
-    CelMutableAst newBindAst = CelMutableAst.of(newBindMacroExpr, celSource);
-
-    return replaceSubtree(ast, newBindAst, exprIdToReplace);
-  }
-
-  /**
-   * See {@link #replaceSubtreeWithNewBindMacro(CelMutableAst, String, CelMutableAst,
-   * CelMutableExpr, long, boolean)}.
-   */
-  public CelMutableAst replaceSubtreeWithNewBindMacro(
-      CelMutableAst ast,
-      String varName,
-      CelMutableExpr varInit,
-      CelMutableExpr resultExpr,
-      long exprIdToReplace,
-      boolean populateMacroSource) {
-    return replaceSubtreeWithNewBindMacro(
-        ast,
-        varName,
-        CelMutableAst.of(varInit, CelMutableSource.newInstance()),
-        resultExpr,
-        exprIdToReplace,
-        populateMacroSource);
-  }
-
   /** Renumbers all the expr IDs in the given AST in a consecutive manner starting from 1. */
   public CelMutableAst renumberIdsConsecutively(CelMutableAst mutableAst) {
     StableIdGenerator stableIdGenerator = CelExprIdGeneratorFactory.newStableIdGenerator(0);
@@ -688,46 +618,6 @@ private CelMutableSource mangleIdentsInMacroSource(
     return newSource;
   }
 
-  private CelMutableExpr newBindMacroExpr(
-      String varName,
-      CelMutableExpr varInit,
-      CelMutableExpr resultExpr,
-      StableIdGenerator stableIdGenerator) {
-    // Renumber incoming expression IDs in the init and result expression to avoid collision with
-    // the main AST. Existing IDs are memoized for a macro source sanitization pass at the end
-    // (e.g: inserting a bind macro to an existing macro expr)
-    varInit = renumberExprIds(stableIdGenerator::nextExprId, varInit);
-    resultExpr = renumberExprIds(stableIdGenerator::nextExprId, resultExpr);
-
-    long iterRangeId = stableIdGenerator.nextExprId();
-    long loopConditionId = stableIdGenerator.nextExprId();
-    long loopStepId = stableIdGenerator.nextExprId();
-    long comprehensionId = stableIdGenerator.nextExprId();
-
-    return CelMutableExpr.ofComprehension(
-        comprehensionId,
-        CelMutableComprehension.create(
-            "#unused",
-            CelMutableExpr.ofList(iterRangeId, CelMutableList.create()),
-            varName,
-            varInit,
-            CelMutableExpr.ofConstant(loopConditionId, CelConstant.ofValue(false)),
-            CelMutableExpr.ofIdent(loopStepId, varName),
-            resultExpr));
-  }
-
-  private CelMutableExpr newBindMacroSourceExpr(
-      CelMutableExpr bindMacroExpr, String varName, StableIdGenerator stableIdGenerator) {
-    return CelMutableExpr.ofCall(
-        0, // Required sentinel value for macro call
-        CelMutableCall.create(
-            CelMutableExpr.ofIdent(stableIdGenerator.nextExprId(), "cel"),
-            "bind",
-            CelMutableExpr.ofIdent(stableIdGenerator.nextExprId(), varName),
-            bindMacroExpr.comprehension().accuInit(),
-            bindMacroExpr.comprehension().result()));
-  }
-
   private static CelMutableSource combine(
       CelMutableSource celSource1, CelMutableSource celSource2) {
     return CelMutableSource.newInstance()
@@ -920,8 +810,7 @@ private static void unwrapListArgumentsInMacroCallExpr(
     CelMutableCall existingMacroCall = newMacroCallExpr.call();
     CelMutableCall newMacroCall =
         existingMacroCall.target().isPresent()
-            ? CelMutableCall.create(existingMacroCall.target().get(),
-            existingMacroCall.function())
+            ? CelMutableCall.create(existingMacroCall.target().get(), existingMacroCall.function())
             : CelMutableCall.create(existingMacroCall.function());
     newMacroCall.addArgs(
         existingMacroCall.args().get(0)); // iter_var is first argument of the call by convention

optimizer/src/main/java/dev/cel/optimizer/optimizers/ConstantFoldingOptimizer.java

@@ -202,7 +202,9 @@ private static boolean canFoldInOperator(CelNavigableMutableExpr navigableExpr)
       CelNavigableMutableExpr parent = identNode.parent().orElse(null);
       while (parent != null) {
         if (parent.getKind().equals(Kind.COMPREHENSION)) {
-          if (parent.expr().comprehension().accuVar().equals(identNode.expr().ident().name())) {
+          String identName = identNode.expr().ident().name();
+          if (parent.expr().comprehension().accuVar().equals(identName)
+              || parent.expr().comprehension().iterVar().equals(identName)) {
             // Prevent folding a subexpression if it contains a variable declared by a
             // comprehension. The subexpression cannot be compiled without the full context of the
             // surrounding comprehension.

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

@@ -57,10 +57,8 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Comparator;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Optional;
 import java.util.Set;
 
 /**
@@ -121,8 +119,7 @@ public static SubexpressionOptimizer newInstance(SubexpressionOptimizerOptions c
 
   @Override
   public OptimizationResult optimize(CelAbstractSyntaxTree ast, Cel cel) {
-    OptimizationResult result =
-        cseOptions.enableCelBlock() ? optimizeUsingCelBlock(ast, cel) : optimizeUsingCelBind(ast);
+    OptimizationResult result = optimizeUsingCelBlock(ast, cel);
 
     verifyOptimizedAstCorrectness(result.optimizedAst());
 
@@ -330,123 +327,8 @@ private static ImmutableList<CelVarDecl> newBlockIndexVariableDeclarations(
     return varDeclBuilder.build();
   }
 
-  private OptimizationResult optimizeUsingCelBind(CelAbstractSyntaxTree ast) {
-    CelMutableAst astToModify = CelMutableAst.fromCelAst(ast);
-    if (!cseOptions.populateMacroCalls()) {
-      astToModify.source().clearMacroCalls();
-    }
-
-    astToModify =
-        astMutator
-            .mangleComprehensionIdentifierNames(
-                astToModify,
-                MANGLED_COMPREHENSION_ITER_VAR_PREFIX,
-                MANGLED_COMPREHENSION_ACCU_VAR_PREFIX,
-                /* incrementSerially= */ true)
-            .mutableAst();
-    CelMutableSource sourceToModify = astToModify.source();
-
-    int bindIdentifierIndex = 0;
-    int iterCount;
-    for (iterCount = 0; iterCount < cseOptions.iterationLimit(); iterCount++) {
-      CelNavigableMutableAst navAst = CelNavigableMutableAst.fromAst(astToModify);
-      List<CelMutableExpr> cseCandidates = getCseCandidates(navAst);
-      if (cseCandidates.isEmpty()) {
-        break;
-      }
-
-      String bindIdentifier = BIND_IDENTIFIER_PREFIX + bindIdentifierIndex;
-      bindIdentifierIndex++;
-
-      // Replace all CSE candidates with new bind identifier
-      for (CelMutableExpr cseCandidate : cseCandidates) {
-        iterCount++;
-
-        astToModify =
-            astMutator.replaceSubtree(
-                astToModify, CelMutableExpr.ofIdent(bindIdentifier), cseCandidate.id());
-      }
-
-      // Find LCA to insert the new cel.bind macro into.
-      CelNavigableMutableExpr lca = getLca(navAst, bindIdentifier);
-
-      // Insert the new bind call
-      CelMutableExpr subexpressionToBind = cseCandidates.get(0);
-      // Re-add the macro source for bind identifiers that might have been lost from previous
-      // iteration of CSE
-      astToModify.source().addAllMacroCalls(sourceToModify.getMacroCalls());
-      astToModify =
-          astMutator.replaceSubtreeWithNewBindMacro(
-              astToModify,
-              bindIdentifier,
-              subexpressionToBind,
-              lca.expr(),
-              lca.id(),
-              cseOptions.populateMacroCalls());
-
-      // Retain the existing macro calls in case if the bind identifiers are replacing a subtree
-      // that contains a comprehension.
-      sourceToModify = astToModify.source();
-    }
-
-    if (iterCount >= cseOptions.iterationLimit()) {
-      throw new IllegalStateException("Max iteration count reached.");
-    }
-
-    if (iterCount == 0) {
-      // No modification has been made.
-      return OptimizationResult.create(ast);
-    }
-
-    astToModify = astMutator.renumberIdsConsecutively(astToModify);
-
-    return OptimizationResult.create(astToModify.toParsedAst());
-  }
-
-  private static CelNavigableMutableExpr getLca(
-      CelNavigableMutableAst navAst, String boundIdentifier) {
-    CelNavigableMutableExpr root = navAst.getRoot();
-    ImmutableList<CelNavigableMutableExpr> allNodesWithIdentifier =
-        root.allNodes()
-            .filter(
-                node ->
-                    node.getKind().equals(Kind.IDENT)
-                        && node.expr().ident().name().equals(boundIdentifier))
-            .collect(toImmutableList());
-
-    if (allNodesWithIdentifier.size() < 2) {
-      throw new IllegalStateException("Expected at least 2 bound identifiers to be present.");
-    }
-
-    CelNavigableMutableExpr lca = root;
-    long lcaAncestorCount = 0;
-    HashMap<Long, Long> ancestors = new HashMap<>();
-    for (CelNavigableMutableExpr navigableExpr : allNodesWithIdentifier) {
-      Optional<CelNavigableMutableExpr> maybeParent = Optional.of(navigableExpr);
-      while (maybeParent.isPresent()) {
-        CelNavigableMutableExpr parent = maybeParent.get();
-        if (!ancestors.containsKey(parent.id())) {
-          ancestors.put(parent.id(), 1L);
-          continue;
-        }
-
-        long ancestorCount = ancestors.get(parent.id());
-        if (lcaAncestorCount < ancestorCount
-            || (lcaAncestorCount == ancestorCount && lca.depth() < parent.depth())) {
-          lca = parent;
-          lcaAncestorCount = ancestorCount;
-        }
-
-        ancestors.put(parent.id(), ancestorCount + 1);
-        maybeParent = parent.parent();
-      }
-    }
-
-    return lca;
-  }
-
   private List<CelMutableExpr> getCseCandidates(CelNavigableMutableAst navAst) {
-    if (cseOptions.enableCelBlock() && cseOptions.subexpressionMaxRecursionDepth() > 0) {
+    if (cseOptions.subexpressionMaxRecursionDepth() > 0) {
       return getCseCandidatesWithRecursionDepth(
           navAst, cseOptions.subexpressionMaxRecursionDepth());
     } else {
@@ -689,11 +571,9 @@ public abstract static class Builder {
       public abstract Builder populateMacroCalls(boolean value);
 
       /**
-       * Rewrites the optimized AST using cel.@block call instead of cascaded cel.bind macros, aimed
-       * to produce a more compact AST. {@link CelSource.Extension} field will be populated in the
-       * AST to inform that special runtime support is required to evaluate the optimized
-       * expression.
+       * @deprecated This option is a no-op. cel.@block is always enabled.
        */
+      @Deprecated
       public abstract Builder enableCelBlock(boolean value);
 
       /**
@@ -708,9 +588,8 @@ public abstract static class Builder {
        * <p>Note that expressions containing no common subexpressions may become a candidate for
        * extraction to satisfy the max depth requirement.
        *
-       * <p>This is a no-op if {@link #enableCelBlock} is set to false, the configured value is less
-       * than 1, or no subexpression needs to be extracted because the entire expression is already
-       * under the designated limit.
+       * <p>This is a no-op if the configured value is less than 1, or no subexpression needs to be
+       * extracted because the entire expression is already under the designated limit.
        *
        * <p>Examples:
        *
@@ -756,8 +635,8 @@ public Builder addEliminableFunctions(String... functions) {
     public static Builder newBuilder() {
       return new AutoValue_SubexpressionOptimizer_SubexpressionOptimizerOptions.Builder()
           .iterationLimit(500)
+          .enableCelBlock(true)
           .populateMacroCalls(false)
-          .enableCelBlock(false)
           .subexpressionMaxRecursionDepth(0);
     }