Fix off-by-one error in CelExprFactory

PiperOrigin-RevId: 842335394

Author: Stephen Roberts

common/src/main/java/dev/cel/common/ast/CelExprFactory.java

@@ -266,7 +266,7 @@ protected long nextExprId() {
 
   /** Attempts to decrement the next expr ID if possible. */
   protected void maybeDeleteId(long id) {
-    if (id == exprId - 1) {
+    if (id == exprId) {
       exprId--;
     }
   }

common/src/test/java/dev/cel/common/ast/CelExprFactoryIntegrationTest.java

@@ -0,0 +1,39 @@
+// Copyright 2025 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package dev.cel.common.ast;
+
+import dev.cel.compiler.CelCompiler;
+import dev.cel.compiler.CelCompilerFactory;
+import dev.cel.parser.CelMacro;
+import java.util.Optional;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class CelExprFactoryIntegrationTest {
+
+  @Test
+  public void macroExpandingToSingleNode_doesNotCauseIdCollision() throws Exception {
+    CelMacro tcMacro =
+        CelMacro.newGlobalMacro(
+            "tc", 0, (factory, target, args) -> Optional.of(factory.newIntLiteral(0)));
+
+    CelCompiler compiler =
+        CelCompilerFactory.standardCelCompilerBuilder().addMacros(tcMacro).build();
+
+    compiler.compile("tc() == 0");
+  }
+}

common/src/test/java/dev/cel/common/ast/CelExprFactoryTest.java

@@ -37,4 +37,30 @@ public void nextExprId_startingDefaultIsOne() {
     assertThat(exprFactory.nextExprId()).isEqualTo(1L);
     assertThat(exprFactory.nextExprId()).isEqualTo(2L);
   }
+
+  @Test
+  public void maybeDeleteId_deletesLastId() {
+    CelExprFactory exprFactory = CelExprFactory.newInstance();
+    long id1 = exprFactory.nextExprId(); // 1
+    assertThat(id1).isEqualTo(1L);
+
+    exprFactory.maybeDeleteId(id1);
+
+    // Should be reused
+    assertThat(exprFactory.nextExprId()).isEqualTo(1L);
+  }
+
+  @Test
+  public void maybeDeleteId_doesNotDeletePreviouslyAllocatedId() {
+    CelExprFactory exprFactory = CelExprFactory.newInstance();
+    long id1 = exprFactory.nextExprId(); // 1
+    long id2 = exprFactory.nextExprId(); // 2
+
+    // Try to delete id1. Since id2 was allocated after, it should NOT delete id1
+    // because that would rewind the counter and cause a collision with id2.
+    exprFactory.maybeDeleteId(id1);
+
+    // Should NOT be reused. Next should be 3.
+    assertThat(exprFactory.nextExprId()).isEqualTo(3L);
+  }
 }