Track incompatible output types for accurate error reporting

PiperOrigin-RevId: 911470209

Author: l46kok

conformance/src/test/java/dev/cel/conformance/policy/BUILD.bazel

@@ -11,8 +11,10 @@ java_library(
     srcs = glob(["*.java"]),
     deps = [
         "//:auto_value",
+        "//:java_truth",
         "//bundle:cel",
         "//policy:parser_factory",
+        "//policy:validation_exception",
         "//policy/testing:k8s_test_tag_handler",
         "//runtime:function_binding",
         "//testing/testrunner:cel_expression_source",

conformance/src/test/java/dev/cel/conformance/policy/PolicyConformanceTest.java

@@ -14,11 +14,14 @@
 
 package dev.cel.conformance.policy;
 
+import static com.google.common.truth.Truth.assertThat;
+
 import com.google.protobuf.Struct;
 import dev.cel.bundle.Cel;
 import dev.cel.bundle.CelFactory;
 import dev.cel.expr.conformance.proto3.TestAllTypes;
 import dev.cel.policy.CelPolicyParserFactory;
+import dev.cel.policy.CelPolicyValidationException;
 import dev.cel.policy.testing.K8sTagHandler;
 import dev.cel.runtime.CelFunctionBinding;
 import dev.cel.testing.testrunner.CelExpressionSource;
@@ -28,6 +31,7 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.Locale;
 import org.junit.runners.model.Statement;
 
 /** Statement representing a single CEL policy conformance test case. */
@@ -95,6 +99,16 @@ public void evaluate() throws Throwable {
       contextBuilder.setConfigFile(textprotoConfigPath.toString());
     }
 
-    TestRunnerLibrary.runTest(testCase, contextBuilder.build());
+    try {
+      TestRunnerLibrary.runTest(testCase, contextBuilder.build());
+    } catch (CelPolicyValidationException e) {
+      if (testCase.output().kind() == CelTestCase.Output.Kind.EVAL_ERROR) {
+        String expectedError = testCase.output().evalError().get(0).toString();
+        assertThat(e.getMessage().toLowerCase(Locale.US))
+            .contains(expectedError.toLowerCase(Locale.US));
+      } else {
+        throw e;
+      }
+    }
   }
 }

conformance/src/test/java/dev/cel/conformance/policy/PolicyConformanceTestRunner.java

@@ -44,6 +44,7 @@ public final class PolicyConformanceTestRunner extends ParentRunner<PolicyConfor
   private static final Splitter SPLITTER = Splitter.on(",").omitEmptyStrings();
   private static final String TESTS_YAML_FILE_NAME = "tests.yaml";
   private static final String TESTS_TEXTPROTO_FILE_NAME = "tests.textproto";
+  private static final String POLICY_YAML_FILE_NAME = "policy.yaml";
   private static final TypeRegistry TYPE_REGISTRY =
       TypeRegistry.newBuilder()
           .add(Struct.getDescriptor())
@@ -73,12 +74,40 @@ private static ImmutableList<String> discoverTestDirs(String testdataDir) {
     if (!dir.exists() || !dir.isDirectory()) {
       return ImmutableList.of();
     }
-    String[] directories = dir.list((current, name) -> new File(current, name).isDirectory());
-    if (directories == null) {
+    File[] topLevelDirs = dir.listFiles(File::isDirectory);
+    if (topLevelDirs == null) {
       return ImmutableList.of();
     }
-    Arrays.sort(directories);
-    return ImmutableList.copyOf(directories);
+
+    ImmutableList.Builder<String> testDirsBuilder = ImmutableList.builder();
+    Arrays.sort(topLevelDirs);
+    for (File topLevelDir : topLevelDirs) {
+      if (hasTestSuite(topLevelDir)) {
+        testDirsBuilder.add(topLevelDir.getName());
+        continue;
+      }
+
+      // Check one level deeper to support nested tests like compile_errors/unreachable
+      File[] subDirs = topLevelDir.listFiles(File::isDirectory);
+      if (subDirs == null) {
+        continue;
+      }
+
+      Arrays.sort(subDirs);
+      for (File subDir : subDirs) {
+        if (hasTestSuite(subDir)) {
+          testDirsBuilder.add(topLevelDir.getName() + "/" + subDir.getName());
+        }
+      }
+    }
+
+    return testDirsBuilder.build();
+  }
+
+  private static boolean hasTestSuite(File dir) {
+    return (new File(dir, TESTS_YAML_FILE_NAME).exists()
+            || new File(dir, TESTS_TEXTPROTO_FILE_NAME).exists())
+        && new File(dir, POLICY_YAML_FILE_NAME).exists();
   }
 
   private final ImmutableList<PolicyConformanceTest> tests;

policy/BUILD.bazel

@@ -59,3 +59,9 @@ java_library(
     name = "compiler_builder",
     exports = ["//policy/src/main/java/dev/cel/policy:compiler_builder"],
 )
+
+java_library(
+    name = "rule_composer",
+    visibility = ["//:internal"],
+    exports = ["//policy/src/main/java/dev/cel/policy:rule_composer"],
+)

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

@@ -244,7 +244,6 @@ java_library(
 java_library(
     name = "rule_composer",
     srcs = ["RuleComposer.java"],
-    visibility = ["//visibility:private"],
     deps = [
         ":compiled_rule",
         "//bundle:cel",
@@ -257,6 +256,8 @@ java_library(
         "//common/ast:mutable_expr",
         "//common/formats:value_string",
         "//common/navigation:mutable_navigation",
+        "//common/types",
+        "//common/types:type_providers",
         "//extensions:optional_library",
         "//optimizer:ast_optimizer",
         "//optimizer:mutable_ast",

policy/src/main/java/dev/cel/policy/RuleComposer.java

@@ -32,6 +32,8 @@
 import dev.cel.common.formats.ValueString;
 import dev.cel.common.navigation.CelNavigableMutableAst;
 import dev.cel.common.navigation.CelNavigableMutableExpr;
+import dev.cel.common.types.CelType;
+import dev.cel.common.types.OptionalType;
 import dev.cel.extensions.CelOptionalLibrary.Function;
 import dev.cel.optimizer.AstMutator;
 import dev.cel.optimizer.CelAstOptimizer;
@@ -67,10 +69,14 @@ private Step optimizeRule(Cel cel, CelCompiledRule compiledRule) {
     // If the rule has an optional output, the last result in the ternary should return
     // `optional.none`. This output is implicit and created here to reflect the desired
     // last possible output of this type of rule.
+    CelType ruleOutputType = getRuleOutputType(compiledRule);
+    // The expected output type of the rule, used to verify that all branches agree on the type.
+    CelType expectedOutputType = null;
     if (compiledRule.hasOptionalOutput()) {
       output =
           Step.newUnconditionalOptionalStep(
               newTrueLiteral(), astMutator.newGlobalCall(Function.OPTIONAL_NONE.getFunction()));
+      expectedOutputType = OptionalType.create(ruleOutputType);
     }
 
     long lastOutputId = 0;
@@ -79,47 +85,61 @@ private Step optimizeRule(Cel cel, CelCompiledRule compiledRule) {
       boolean isTriviallyTrue = match.isConditionTriviallyTrue();
       CelMutableAst condAst = CelMutableAst.fromCelAst(conditionAst);
 
+      CelType currentType;
+      Step step;
+      long currentSourceId = lastOutputId;
+
       switch (match.result().kind()) {
         case OUTPUT:
           // If the match has an output, then it is considered a non-optional output since
           // it is explicitly stated. If the rule itself is optional, then the base case value
           // of output being optional.none() will convert the non-optional value to an optional
           // one.
           OutputValue matchOutput = match.result().output();
-          CelMutableAst outAst = CelMutableAst.fromCelAst(matchOutput.ast());
-          Step step = Step.newNonOptionalStep(!isTriviallyTrue, condAst, outAst);
+          currentType = matchOutput.ast().getResultType();
+          step =
+              Step.newNonOptionalStep(
+                  !isTriviallyTrue, condAst, CelMutableAst.fromCelAst(matchOutput.ast()));
+          currentSourceId = matchOutput.sourceId();
+
+          CelType baseType = (expectedOutputType == null) ? currentType : expectedOutputType;
+          String outputFailureMessage =
+              String.format(
+                  "incompatible output types: block has output type %s, but previous outputs have"
+                      + " type %s",
+                  baseType.name(), currentType.name());
+
           output = combine(astMutator, step, output);
+          expectedOutputType = (expectedOutputType == null) ? currentType : expectedOutputType;
 
           assertComposedAstIsValid(
-              cel,
-              output.expr,
-              "conflicting output types found.",
-              matchOutput.sourceId(),
-              lastOutputId);
-          lastOutputId = matchOutput.sourceId();
+              cel, output.expr, outputFailureMessage, currentSourceId, lastOutputId);
           break;
         case RULE:
           // If the match has a nested rule, then compute the rule and whether it has
           // an optional return value.
           CelCompiledRule matchNestedRule = match.result().rule();
+          currentType = getRuleOutputType(matchNestedRule);
           Step nestedRule = optimizeRule(cel, matchNestedRule);
-          boolean nestedHasOptional = matchNestedRule.hasOptionalOutput();
+          step =
+              new Step(
+                  matchNestedRule.hasOptionalOutput(), !isTriviallyTrue, condAst, nestedRule.expr);
+          currentSourceId = matchNestedRule.sourceId();
+
+          String ruleFailureMessage =
+              String.format(
+                  "failed composing the subrule '%s' due to incompatible output types.",
+                  matchNestedRule.ruleId().map(ValueString::value).orElse(""));
 
-          Step ruleStep =
-              nestedHasOptional
-                  ? Step.newOptionalStep(!isTriviallyTrue, condAst, nestedRule.expr)
-                  : Step.newNonOptionalStep(!isTriviallyTrue, condAst, nestedRule.expr);
-          output = combine(astMutator, ruleStep, output);
+          output = combine(astMutator, step, output);
+          expectedOutputType = (expectedOutputType == null) ? currentType : expectedOutputType;
 
           assertComposedAstIsValid(
-              cel,
-              output.expr,
-              String.format(
-                  "failed composing the subrule '%s' due to conflicting output types.",
-                  matchNestedRule.ruleId().map(ValueString::value).orElse("")),
-              lastOutputId);
+              cel, output.expr, ruleFailureMessage, currentSourceId, lastOutputId);
           break;
       }
+
+      lastOutputId = currentSourceId;
     }
 
     CelMutableAst resultExpr = output.expr;
@@ -281,6 +301,20 @@ private void assertComposedAstIsValid(
     }
   }
 
+  private CelType getRuleOutputType(CelCompiledRule rule) {
+    for (CelCompiledMatch match : rule.matches()) {
+      switch (match.result().kind()) {
+        case OUTPUT:
+          return match.result().output().ast().getResultType();
+        case RULE:
+          return getRuleOutputType(match.result().rule());
+      }
+      throw new IllegalStateException("Unknown match result kind: " + match.result().kind());
+    }
+
+    throw new IllegalStateException("Policy rule contains no outputs");
+  }
+
   // Step represents an intermediate stage of rule and match expression composition.
   //
   // The CelCompiledRule and CelCompiledMatch types are meant to represent standalone tuples of
@@ -311,11 +345,6 @@ private Step(
       this.expr = expr;
     }
 
-    private static Step newOptionalStep(
-        boolean isConditional, CelMutableAst cond, CelMutableAst expr) {
-      return new Step(/* isOptional= */ true, isConditional, cond, expr);
-    }
-
     private static Step newNonOptionalStep(
         boolean isConditional, CelMutableAst cond, CelMutableAst expr) {
       return new Step(/* isOptional= */ false, isConditional, cond, expr);

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

@@ -27,9 +27,11 @@ java_library(
         "//parser:parser_factory",
         "//parser:unparser",
         "//policy",
+        "//policy:compiled_rule",
         "//policy:compiler_factory",
         "//policy:parser",
         "//policy:parser_factory",
+        "//policy:rule_composer",
         "//policy:source",
         "//policy:validation_exception",
         "//policy/testing:k8s_test_tag_handler",

policy/src/test/java/dev/cel/policy/CelPolicyCompilerImplTest.java

@@ -31,6 +31,7 @@
 import dev.cel.bundle.CelEnvironmentYamlParser;
 import dev.cel.common.CelAbstractSyntaxTree;
 import dev.cel.common.CelOptions;
+import dev.cel.common.formats.ValueString;
 import dev.cel.common.types.OptionalType;
 import dev.cel.common.types.SimpleType;
 import dev.cel.expr.conformance.proto3.TestAllTypes;
@@ -519,4 +520,25 @@ private String readExpectedErrorsBaseline() throws IOException {
       this.policyFilePath = String.format("%s/policy.yaml", name);
     }
   }
+
+  @Test
+  public void ruleComposer_ruleWithNoOutputs_throwsIllegalStateException() throws Exception {
+    Cel cel = newCel();
+    CelCompiledRule emptyRule =
+        CelCompiledRule.create(
+            1L,
+            Optional.of(ValueString.of(2L, "empty_rule")),
+            ImmutableList.of(),
+            ImmutableList.of(),
+            cel);
+
+    RuleComposer composer = RuleComposer.newInstance(emptyRule, "variables.", 1000);
+
+    IllegalStateException e =
+        assertThrows(
+            IllegalStateException.class,
+            () -> composer.optimize(cel.compile("true").getAst(), cel));
+
+    assertThat(e).hasMessageThat().isEqualTo("Policy rule contains no outputs");
+  }
 }

testing/src/test/resources/policy/compose_errors_conflicting_output/expected_errors.baseline

@@ -1,6 +1,6 @@
-ERROR: compose_errors_conflicting_output/policy.yaml:22:14: conflicting output types found.
+ERROR: compose_errors_conflicting_output/policy.yaml:22:14: incompatible output types: block has output type map, but previous outputs have type bool
  |     output: "false"
  | .............^
-ERROR: compose_errors_conflicting_output/policy.yaml:23:14: conflicting output types found.
+ERROR: compose_errors_conflicting_output/policy.yaml:23:14: incompatible output types: block has output type map, but previous outputs have type bool
  |   - output: "{'banned': true}"
  | .............^

testing/src/test/resources/policy/compose_errors_conflicting_subrule/expected_errors.baseline

@@ -1,3 +1,6 @@
-ERROR: compose_errors_conflicting_subrule/policy.yaml:36:14: failed composing the subrule 'banned regions' due to conflicting output types.
+ERROR: compose_errors_conflicting_subrule/policy.yaml:22:7: failed composing the subrule 'banned regions' due to incompatible output types.
+ |       id: "banned regions"
+ | ......^
+ERROR: compose_errors_conflicting_subrule/policy.yaml:36:14: failed composing the subrule 'banned regions' due to incompatible output types.
  |     output: "{'banned': false}"
  | .............^