Implement standalone validator and general interfaces to perform cust…

…om AST validation. TimestampLiteralValidator is added as a canonical validator demonstrating its usage. PiperOrigin-RevId: 561076631
google · Aug 29, 2023 · 5593808 · 5593808
1 parent 1fa54c9
commit 5593808
Show file tree

Hide file tree

Showing 32 changed files with 968 additions and 48 deletions.
diff --git a/bundle/src/main/java/dev/cel/bundle/CelFactory.java b/bundle/src/main/java/dev/cel/bundle/CelFactory.java
@@ -16,7 +16,9 @@
 
 import dev.cel.checker.CelCheckerLegacyImpl;
 import dev.cel.common.CelOptions;
+import dev.cel.compiler.CelCompiler;
 import dev.cel.parser.CelParserImpl;
+import dev.cel.runtime.CelRuntime;
 
 /** Helper class to configure the entire CEL stack in a common interface. */
 public final class CelFactory {
@@ -35,4 +37,9 @@ public static CelBuilder standardCelBuilder() {
         .setOptions(CelOptions.current().build())
         .setStandardEnvironmentEnabled(true);
   }
+
+  /** Combines a prebuilt {@link CelCompiler} and {@link CelRuntime} into {@link Cel}. */
+  public static Cel combine(CelCompiler celCompiler, CelRuntime celRuntime) {
+    return CelImpl.combine(celCompiler, celRuntime);
+  }
 }
diff --git a/bundle/src/main/java/dev/cel/bundle/CelImpl.java b/bundle/src/main/java/dev/cel/bundle/CelImpl.java
@@ -100,6 +100,11 @@ public void accept(EnvVisitor envVisitor) {
     }
   }
 
+  /** Combines a prebuilt {@link CelCompiler} and {@link CelRuntime} into {@link CelImpl}. */
+  static CelImpl combine(CelCompiler compiler, CelRuntime runtime) {
+    return new CelImpl(Suppliers.memoize(() -> compiler), Suppliers.memoize(() -> runtime));
+  }
+
   /**
    * Create a new builder for constructing a {@code CelImpl} instance.
    *

diff --git a/checker/src/main/java/dev/cel/checker/ExprChecker.java b/checker/src/main/java/dev/cel/checker/ExprChecker.java
@@ -137,7 +137,8 @@ public static CelAbstractSyntaxTree typecheck(
     // by DYN.
     Map<Long, CelType> typeMap =
         Maps.transformValues(env.getTypeMap(), checker.inferenceContext::finalize);
-    return new CelAbstractSyntaxTree(expr, ast.getSource(), env.getRefMap(), typeMap);
+
+    return CelAbstractSyntaxTree.newCheckedAst(expr, ast.getSource(), env.getRefMap(), typeMap);
   }
 
   private final Env env;

diff --git a/common/src/main/java/dev/cel/common/BUILD.bazel b/common/src/main/java/dev/cel/common/BUILD.bazel
@@ -108,7 +108,6 @@ java_library(
     ],
     deps = [
         ":common",
-        "//:auto_value",
         "//common/ast:expr_v1alpha1_converter",
         "//common/types:cel_v1alpha1_types",
         "@com_google_googleapis//google/api/expr/v1alpha1:expr_java_proto",

diff --git a/common/src/main/java/dev/cel/common/CelAbstractSyntaxTree.java b/common/src/main/java/dev/cel/common/CelAbstractSyntaxTree.java
@@ -14,8 +14,6 @@
 
 package dev.cel.common;
 
-import static com.google.common.collect.ImmutableMap.toImmutableMap;
-
 import dev.cel.expr.CheckedExpr;
 import dev.cel.expr.Expr;
 import dev.cel.expr.ParsedExpr;
@@ -28,13 +26,11 @@
 import dev.cel.common.annotations.Internal;
 import dev.cel.common.ast.CelConstant;
 import dev.cel.common.ast.CelExpr;
-import dev.cel.common.ast.CelExprConverter;
 import dev.cel.common.ast.CelReference;
 import dev.cel.common.types.CelType;
 import dev.cel.common.types.CelTypes;
 import dev.cel.common.types.SimpleType;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.NoSuchElementException;
 import java.util.Optional;
 
@@ -56,46 +52,44 @@ public final class CelAbstractSyntaxTree {
 
   private final ImmutableMap<Long, CelType> types;
 
-  /** Internal: Consumers should not be creating an instance of this class directly. */
-  @Internal
-  public CelAbstractSyntaxTree(CelExpr celExpr, CelSource celSource) {
-    this(celExpr, celSource, ImmutableMap.of(), ImmutableMap.of());
+  /**
+   * Constructs a new instance of CelAbstractSyntaxTree that represent a parsed expression.
+   *
+   * <p>Note that ASTs should not be manually constructed except for special circumstances such as
+   * validating or optimizing an AST.
+   */
+  public static CelAbstractSyntaxTree newParsedAst(CelExpr celExpr, CelSource celSource) {
+    return new CelAbstractSyntaxTree(celExpr, celSource);
   }
 
-  /** Internal: Consumers should not be creating an instance of this class directly. */
+  /**
+   * Constructs a new instance of CelAbstractSyntaxTree that represent a checked expression.
+   *
+   * <p>CEL Library Internals. Do not construct a type-checked AST by hand. Use a CelCompiler to
+   * type-check a parsed AST instead.
+   */
   @Internal
-  public CelAbstractSyntaxTree(
+  public static CelAbstractSyntaxTree newCheckedAst(
       CelExpr celExpr,
       CelSource celSource,
       Map<Long, CelReference> references,
       Map<Long, CelType> types) {
-    this.celExpr = celExpr;
-    this.celSource = celSource;
-    this.references = ImmutableMap.copyOf(references);
-    this.types = ImmutableMap.copyOf(types);
+    return new CelAbstractSyntaxTree(celExpr, celSource, references, types);
   }
 
-  CelAbstractSyntaxTree(ParsedExpr parsedExpr, CelSource celSource) {
-    this(
-        CheckedExpr.newBuilder()
-            .setExpr(parsedExpr.getExpr())
-            .setSourceInfo(parsedExpr.getSourceInfo())
-            .build(),
-        celSource);
+  private CelAbstractSyntaxTree(CelExpr celExpr, CelSource celSource) {
+    this(celExpr, celSource, ImmutableMap.of(), ImmutableMap.of());
   }
 
-  CelAbstractSyntaxTree(CheckedExpr checkedExpr, CelSource celSource) {
-    this.celExpr = CelExprConverter.fromExpr(checkedExpr.getExpr());
+  private CelAbstractSyntaxTree(
+      CelExpr celExpr,
+      CelSource celSource,
+      Map<Long, CelReference> references,
+      Map<Long, CelType> types) {
+    this.celExpr = celExpr;
     this.celSource = celSource;
-    this.references =
-        checkedExpr.getReferenceMapMap().entrySet().stream()
-            .collect(
-                toImmutableMap(
-                    Entry::getKey,
-                    v -> CelExprConverter.exprReferenceToCelReference(v.getValue())));
-    this.types =
-        checkedExpr.getTypeMapMap().entrySet().stream()
-            .collect(toImmutableMap(Entry::getKey, v -> CelTypes.typeToCelType(v.getValue())));
+    this.references = ImmutableMap.copyOf(references);
+    this.types = ImmutableMap.copyOf(types);
   }
 
   /**

diff --git a/common/src/main/java/dev/cel/common/CelIssue.java b/common/src/main/java/dev/cel/common/CelIssue.java
@@ -29,9 +29,10 @@
 public abstract class CelIssue {
 
   /** Severity of a CelIssue. */
-  public static enum Severity {
+  public enum Severity {
     ERROR,
     WARNING,
+    INFORMATION,
     DEPRECATED;
   }
 

diff --git a/common/src/main/java/dev/cel/common/CelProtoAbstractSyntaxTree.java b/common/src/main/java/dev/cel/common/CelProtoAbstractSyntaxTree.java
@@ -41,7 +41,7 @@ public final class CelProtoAbstractSyntaxTree {
   private CelProtoAbstractSyntaxTree(CheckedExpr checkedExpr) {
     this.checkedExpr = checkedExpr;
     this.ast =
-        new CelAbstractSyntaxTree(
+        CelAbstractSyntaxTree.newCheckedAst(
             CelExprConverter.fromExpr(checkedExpr.getExpr()),
             CelSource.newBuilder()
                 .addAllLineOffsets(checkedExpr.getSourceInfo().getLineOffsetsList())

diff --git a/common/src/main/java/dev/cel/common/CelProtoV1Alpha1AbstractSyntaxTree.java b/common/src/main/java/dev/cel/common/CelProtoV1Alpha1AbstractSyntaxTree.java
@@ -44,7 +44,7 @@ public final class CelProtoV1Alpha1AbstractSyntaxTree {
   private CelProtoV1Alpha1AbstractSyntaxTree(CheckedExpr checkedExpr) {
     this.checkedExpr = checkedExpr;
     this.ast =
-        new CelAbstractSyntaxTree(
+        CelAbstractSyntaxTree.newCheckedAst(
             CelExprV1Alpha1Converter.fromExpr(checkedExpr.getExpr()),
             CelSource.newBuilder()
                 .addAllLineOffsets(checkedExpr.getSourceInfo().getLineOffsetsList())

diff --git a/common/src/main/java/dev/cel/common/CelValidationResult.java b/common/src/main/java/dev/cel/common/CelValidationResult.java
@@ -22,6 +22,7 @@
 import com.google.common.collect.Iterables;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.errorprone.annotations.Immutable;
+import com.google.errorprone.annotations.InlineMe;
 import dev.cel.common.annotations.Internal;
 import org.jspecify.nullness.Nullable;
 
@@ -112,11 +113,22 @@ public ImmutableList<CelIssue> getAllIssues() {
     return issues;
   }
 
-  /** Convert the {@code CelIssue} set to a debug string. */
-  public String getDebugString() {
+  /** Convert all issues to a human-readable string. */
+  public String getIssueString() {
     return JOINER.join(Iterables.transform(issues, iss -> iss.toDisplayString(source)));
   }
 
+  /**
+   * Convert the {@code CelIssue} set to a debug string.
+   *
+   * @deprecated Use {@link #getIssueString()} instead.
+   */
+  @Deprecated
+  @InlineMe(replacement = "this.getIssueString()")
+  public String getDebugString() {
+    return getIssueString();
+  }
+
   /** Convert the {@code CelIssue}s with {@code ERROR} severity to an error string. */
   public String getErrorString() {
     return JOINER.join(Iterables.transform(getErrors(), error -> error.toDisplayString(source)));

diff --git a/common/src/test/java/dev/cel/common/BUILD.bazel b/common/src/test/java/dev/cel/common/BUILD.bazel
@@ -13,11 +13,9 @@ java_library(
         "//common",
         "//common:features",
         "//common:options",
-        "//common:proto_ast",
         "//common:proto_v1alpha1_ast",
         "//common/ast",
         "//common/internal",
-        "//common/resources/testdata/proto3:test_all_types_java_proto",
         "//common/types",
         "//common/types:cel_types",
         "//common/types:cel_v1alpha1_types",

diff --git a/common/src/test/java/dev/cel/common/CelAbstractSyntaxTreeTest.java b/common/src/test/java/dev/cel/common/CelAbstractSyntaxTreeTest.java
@@ -190,7 +190,7 @@ public void parsedExpression_createAst() {
             .addLineOffsets(10)
             .build();
 
-    CelAbstractSyntaxTree ast = new CelAbstractSyntaxTree(celExpr, celSource);
+    CelAbstractSyntaxTree ast = CelAbstractSyntaxTree.newParsedAst(celExpr, celSource);
 
     assertThat(ast).isNotNull();
     assertThat(ast.getExpr()).isEqualTo(celExpr);

diff --git a/compiler/src/main/java/dev/cel/compiler/BUILD.bazel b/compiler/src/main/java/dev/cel/compiler/BUILD.bazel
@@ -28,7 +28,6 @@ java_library(
     ],
     deps = [
         ":compiler_builder",
-        "//:auto_value",
         "//checker",
         "//checker:checker_builder",
         "//checker:checker_legacy_environment",

diff --git a/compiler/src/main/java/dev/cel/compiler/CelCompilerFactory.java b/compiler/src/main/java/dev/cel/compiler/CelCompilerFactory.java
@@ -14,9 +14,11 @@
 
 package dev.cel.compiler;
 
+import dev.cel.checker.CelChecker;
 import dev.cel.checker.CelCheckerBuilder;
 import dev.cel.checker.CelCheckerLegacyImpl;
 import dev.cel.common.CelOptions;
+import dev.cel.parser.CelParser;
 import dev.cel.parser.CelParserImpl;
 
 /** Factory class for creating builders for type-checker and compiler instances. */
@@ -41,11 +43,15 @@ public static CelCheckerBuilder standardCelCheckerBuilder() {
    * configured by default.
    */
   public static CelCompilerBuilder standardCelCompilerBuilder() {
-
     return CelCompilerImpl.newBuilder(CelParserImpl.newBuilder(), CelCheckerLegacyImpl.newBuilder())
         .setOptions(CelOptions.current().build())
         .setStandardEnvironmentEnabled(true);
   }
 
+  /** Combines a prebuilt {@link CelParser} and {@link CelChecker} into {@link CelCompiler}. */
+  public static CelCompiler combine(CelParser celParser, CelChecker celChecker) {
+    return CelCompilerImpl.combine(celParser, celChecker);
+  }
+
   private CelCompilerFactory() {}
 }
diff --git a/compiler/src/main/java/dev/cel/compiler/CelCompilerImpl.java b/compiler/src/main/java/dev/cel/compiler/CelCompilerImpl.java
@@ -48,7 +48,7 @@
 
 /**
  * CelCompiler implementation which uses either the legacy or modernized CEL-Java stack to offer a
- * stream-lined expression parse/type-check experience, via a single {@code compile} method. *
+ * stream-lined expression parse/type-check experience, via a single {@code compile} method.
  *
  * <p>CEL Library Internals. Do Not Use. Consumers should use factories, such as {@link
  * CelCompilerFactory} instead to instantiate a compiler.
@@ -82,6 +82,11 @@ public void accept(EnvVisitor envVisitor) {
     }
   }
 
+  /** Combines a prebuilt {@link CelParser} and {@link CelChecker} into {@link CelCompilerImpl}. */
+  static CelCompilerImpl combine(CelParser parser, CelChecker checker) {
+    return new CelCompilerImpl(parser, checker);
+  }
+
   /**
    * Create a new builder for constructing a {@code CelCompiler} instance.
    *

diff --git a/parser/src/main/java/dev/cel/parser/BUILD.bazel b/parser/src/main/java/dev/cel/parser/BUILD.bazel
@@ -47,7 +47,6 @@ java_library(
         ":macro",
         ":operator",
         ":parser_builder",
-        "//:auto_value",
         "//common",
         "//common:compiler_common",
         "//common:options",

diff --git a/parser/src/main/java/dev/cel/parser/Parser.java b/parser/src/main/java/dev/cel/parser/Parser.java
@@ -162,7 +162,7 @@ static CelValidationResult parse(CelParserImpl parser, CelSource source, CelOpti
           sourceInfo.build(), parseFailure, ImmutableList.copyOf(exprFactory.getIssuesList()));
     }
     return new CelValidationResult(
-        new CelAbstractSyntaxTree(expr, sourceInfo.build()),
+        CelAbstractSyntaxTree.newParsedAst(expr, sourceInfo.build()),
         ImmutableList.copyOf(exprFactory.getIssuesList()));
   }
 

diff --git a/parser/src/test/java/dev/cel/parser/CelUnparserImplTest.java b/parser/src/test/java/dev/cel/parser/CelUnparserImplTest.java
@@ -228,7 +228,8 @@ public void unparse_fails(
             Throwable.class,
             () ->
                 unparser.unparse(
-                    new CelAbstractSyntaxTree(invalidExpr, CelSource.newBuilder().build())));
+                    CelAbstractSyntaxTree.newParsedAst(
+                        invalidExpr, CelSource.newBuilder().build())));
 
     assertThat(thrown).hasMessageThat().contains("unexpected");
   }

diff --git a/validator/BUILD.bazel b/validator/BUILD.bazel
@@ -0,0 +1,26 @@
+package(
+    default_applicable_licenses = ["//:license"],
+    default_visibility = ["//visibility:public"],  # TODO: Expose when ready
+)
+
+java_library(
+    name = "validator",
+    exports = ["//validator/src/main/java/dev/cel/validator"],
+)
+
+java_library(
+    name = "validator_builder",
+    exports = ["//validator/src/main/java/dev/cel/validator:validator_builder"],
+)
+
+java_library(
+    name = "ast_validator",
+    exports = ["//validator/src/main/java/dev/cel/validator:ast_validator"],
+)
+
+java_library(
+    name = "validator_impl",
+    testonly = 1,
+    visibility = ["//validator/src/test/java/dev/cel/validator:__pkg__"],
+    exports = ["//validator/src/main/java/dev/cel/validator:validator_impl"],
+)