Draft: Comprehension fix

.bazelrc

@@ -5,3 +5,7 @@ build --java_language_version=11
 
 # Hide Java 8 deprecation warnings.
 common --javacopt=-Xlint:-options
+
+# MacOS Fix https://github.com/protocolbuffers/protobuf/issues/16944
+build --host_cxxopt=-std=c++14
+build --cxxopt=-std=c++14

runtime/BUILD.bazel

@@ -105,6 +105,11 @@ java_library(
     exports = ["//runtime/src/main/java/dev/cel/runtime:interpreter"],
 )
 
+java_library(
+    name = "interpretable",
+    exports = ["//runtime/src/main/java/dev/cel/runtime:interpretable"],
+)
+
 java_library(
     name = "runtime_helpers",
     visibility = ["//:internal"],

runtime/src/main/java/dev/cel/runtime/BUILD.bazel

@@ -506,7 +506,6 @@ java_library(
 java_library(
     name = "interpretable",
     srcs = INTERPRABLE_SOURCES,
-    visibility = ["//visibility:private"],
     deps = [
         ":evaluation_exception",
         ":evaluation_listener",

runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java

@@ -17,6 +17,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
@@ -111,7 +112,8 @@ public Interpretable createInterpretable(CelAbstractSyntaxTree ast) {
   }
 
   @Immutable
-  private static final class DefaultInterpretable
+  @VisibleForTesting
+  static final class DefaultInterpretable
       implements Interpretable, UnknownTrackingInterpretable {
     private final TypeResolver typeResolver;
     private final RuntimeTypeProvider typeProvider;
@@ -165,12 +167,38 @@ public Object evalTrackingUnknowns(
         Optional<? extends FunctionResolver> functionResolver,
         CelEvaluationListener listener)
         throws CelEvaluationException {
+      ExecutionFrame frame = newExecutionFrame(resolver, functionResolver, listener);
+      IntermediateResult internalResult = evalInternal(frame, ast.getExpr());
+      return internalResult.value();
+    }
+
+    /**
+     * Evaluates this interpretable and returns the resulting execution frame populated with evaluation state.
+     * This method is specifically designed for testing the interpreter's internal invariants.
+     *
+     * <p><b>Do not expose to public. This method is strictly for internal testing purposes only.</b>
+     */
+    @VisibleForTesting
+    ExecutionFrame populateExecutionFrame(ExecutionFrame frame) throws CelEvaluationException {
+      evalInternal(frame, ast.getExpr());
+
+      return frame;
+    }
+
+    @VisibleForTesting
+    ExecutionFrame newTestExecutionFrame(GlobalResolver resolver) throws CelEvaluationException {
+      return newExecutionFrame(RuntimeUnknownResolver.fromResolver(resolver), Optional.empty(), CelEvaluationListener.noOpListener());
+    }
+
+
+    private ExecutionFrame newExecutionFrame(
+        RuntimeUnknownResolver resolver,
+        Optional<? extends FunctionResolver> functionResolver,
+        CelEvaluationListener listener) throws CelEvaluationException {
       int comprehensionMaxIterations =
           celOptions.enableComprehension() ? celOptions.comprehensionMaxIterations() : 0;
-      ExecutionFrame frame =
+      return
           new ExecutionFrame(listener, resolver, functionResolver, comprehensionMaxIterations);
-      IntermediateResult internalResult = evalInternal(frame, ast.getExpr());
-      return internalResult.value();
     }
 
     private IntermediateResult evalInternal(ExecutionFrame frame, CelExpr expr)
@@ -916,6 +944,7 @@ private IntermediateResult evalComprehension(
             iterVar, IntermediateResult.create(iterAttr, RuntimeHelpers.maybeAdaptPrimitive(elem)));
         loopVars.put(accuVar, accuValue);
 
+        System.out.println("Push scope");
         frame.pushScope(Collections.unmodifiableMap(loopVars));
         IntermediateResult evalObject = evalBooleanStrict(frame, compre.loopCondition());
         if (!isUnknownValue(evalObject.value()) && !(boolean) evalObject.value()) {
@@ -927,8 +956,13 @@ private IntermediateResult evalComprehension(
       }
 
       frame.pushScope(Collections.singletonMap(accuVar, accuValue));
-      IntermediateResult result = evalInternal(frame, compre.result());
-      frame.popScope();
+      IntermediateResult result;
+      try {
+        result = evalInternal(frame, compre.result());
+      }
+      finally {
+        frame.popScope();
+      }
       return result;
     }
 
@@ -975,14 +1009,17 @@ private LazyExpression(CelExpr celExpr) {
     }
   }
 
+
   /** This class tracks the state meaningful to a single evaluation pass. */
-  private static class ExecutionFrame {
+  static class ExecutionFrame {
     private final CelEvaluationListener evaluationListener;
     private final int maxIterations;
     private final ArrayDeque<RuntimeUnknownResolver> resolvers;
     private final Optional<? extends FunctionResolver> lateBoundFunctionResolver;
     private RuntimeUnknownResolver currentResolver;
     private int iterations;
+    @VisibleForTesting
+    int scopeLevel;
 
     private ExecutionFrame(
         CelEvaluationListener evaluationListener,
@@ -1040,12 +1077,14 @@ private void cacheLazilyEvaluatedResult(
 
     /** Note: we utilize a HashMap instead of ImmutableMap to make lookups faster on string keys. */
     private void pushScope(Map<String, IntermediateResult> scope) {
+      scopeLevel++;
       RuntimeUnknownResolver scopedResolver = currentResolver.withScope(scope);
       currentResolver = scopedResolver;
       resolvers.addLast(scopedResolver);
     }
 
     private void popScope() {
+      scopeLevel--;
       if (resolvers.isEmpty()) {
         throw new IllegalStateException("Execution frame error: more scopes popped than pushed");
       }

runtime/src/test/java/dev/cel/runtime/BUILD.bazel

@@ -72,6 +72,7 @@ java_library(
         "//common:cel_descriptors",
         "//common:cel_exception",
         "//common:cel_source",
+        "//common:compiler_common",
         "//common:error_codes",
         "//common:options",
         "//common:proto_ast",
@@ -100,6 +101,8 @@ java_library(
         "//runtime:evaluation_listener",
         "//runtime:function_binding",
         "//runtime:function_overload_impl",
+        "//runtime:interpretable",
+        "//runtime:interpreter",
         "//runtime:interpreter_util",
         "//runtime:lite_runtime",
         "//runtime:lite_runtime_factory",

runtime/src/test/java/dev/cel/runtime/DefaultInterpreterTest.java

@@ -0,0 +1,62 @@
+package dev.cel.runtime;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
+
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
+import dev.cel.common.CelAbstractSyntaxTree;
+import dev.cel.common.CelFunctionDecl;
+import dev.cel.common.CelOptions;
+import dev.cel.common.CelOverloadDecl;
+import dev.cel.common.types.SimpleType;
+import dev.cel.compiler.CelCompiler;
+import dev.cel.compiler.CelCompilerFactory;
+import dev.cel.parser.CelStandardMacro;
+import dev.cel.runtime.DefaultInterpreter.DefaultInterpretable;
+import dev.cel.runtime.DefaultInterpreter.ExecutionFrame;
+import java.util.Map;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+@RunWith(TestParameterInjector.class)
+public class DefaultInterpreterTest {
+
+  @Test
+  public void nestedComprehensions_accuVarContainsErrors_scopeLevelInvariantNotViolated() throws Exception {
+    CelCompiler celCompiler = CelCompilerFactory.standardCelCompilerBuilder()
+        .addFunctionDeclarations(CelFunctionDecl.newFunctionDeclaration(
+            "error", CelOverloadDecl.newGlobalOverload("error_overload", SimpleType.DYN)
+        ))
+        .setStandardMacros(CelStandardMacro.STANDARD_MACROS).build();
+    RuntimeTypeProvider emptyProvider = new RuntimeTypeProvider() {
+      @Override
+      public Object createMessage(String messageName, Map<String, Object> values) {
+        return null;
+      }
+      @Override
+      public Object selectField(Object message, String fieldName) {
+        return null;
+      }
+      @Override
+      public Object hasField(Object message, String fieldName) {
+        return null;
+      }
+      @Override
+      public Object adapt(Object message) {
+        return message;
+      }
+    };
+    CelAbstractSyntaxTree ast =
+        celCompiler.compile("[1].all(x, [2].all(y, error()))").getAst();
+    DefaultDispatcher dispatcher = DefaultDispatcher.create();
+    dispatcher.add("error", long.class, (args) -> new IllegalArgumentException("Always throws"));
+    DefaultInterpreter defaultInterpreter = new DefaultInterpreter(
+        new TypeResolver(), emptyProvider, dispatcher, CelOptions.DEFAULT);
+    DefaultInterpretable interpretable = (DefaultInterpretable) defaultInterpreter.createInterpretable(ast);
+
+    ExecutionFrame frame = interpretable.newTestExecutionFrame(GlobalResolver.EMPTY);
+
+    assertThrows(CelEvaluationException.class, () -> interpretable.populateExecutionFrame(frame));
+    assertThat(frame.scopeLevel).isEqualTo(0);
+  }
+}