Skip to content

Add the capability to inline constants during type-check #938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
copybara-service wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add the capability to inline constants during type-check #938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bundle/src/main/java/dev/cel/bundle/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ java_library(
"//common:compiler_common",
"//common:container",
"//common:options",
"//common/ast",
"//common/internal:env_visitor",
"//common/internal:file_descriptor_converter",
"//common/types:cel_proto_types",
Expand Down
8 changes: 8 additions & 0 deletions bundle/src/main/java/dev/cel/bundle/CelBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import dev.cel.common.CelFunctionDecl;
import dev.cel.common.CelOptions;
import dev.cel.common.CelVarDecl;
import dev.cel.common.ast.CelConstant;
import dev.cel.common.types.CelType;
import dev.cel.common.types.CelTypeProvider;
import dev.cel.common.values.CelValueProvider;
Expand Down Expand Up @@ -101,6 +102,13 @@ public interface CelBuilder {
@CanIgnoreReturnValue
CelBuilder addVar(String name, CelType type);

/**
* Adds a named constant declaration to the CEL environment. The declared constant will be inlined
* into the expression during type-check.
*/
@CanIgnoreReturnValue
CelBuilder addConstant(String name, CelConstant celConstant);

/** Add variable and function {@code declarations} to the CEL environment. */
@CanIgnoreReturnValue
CelBuilder addDeclarations(Decl... declarations);
Expand Down
7 changes: 7 additions & 0 deletions bundle/src/main/java/dev/cel/bundle/CelImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import dev.cel.common.CelSource;
import dev.cel.common.CelValidationResult;
import dev.cel.common.CelVarDecl;
import dev.cel.common.ast.CelConstant;
import dev.cel.common.internal.EnvVisitable;
import dev.cel.common.internal.EnvVisitor;
import dev.cel.common.internal.FileDescriptorSetConverter;
Expand Down Expand Up @@ -214,6 +215,12 @@ public CelBuilder addVar(String name, CelType type) {
return this;
}

@Override
public CelBuilder addConstant(String name, CelConstant celConstant) {
compilerBuilder.addConstant(name, celConstant);
return this;
}

@Override
public CelBuilder addDeclarations(Decl... declarations) {
compilerBuilder.addDeclarations(declarations);
Expand Down
2 changes: 2 additions & 0 deletions checker/src/main/java/dev/cel/checker/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ java_library(
"//common:options",
"//common:source_location",
"//common/annotations",
"//common/ast",
"//common/ast:expr_converter",
"//common/internal:env_visitor",
"//common/internal:errors",
Expand Down Expand Up @@ -108,6 +109,7 @@ java_library(
"//common:compiler_common",
"//common:container",
"//common:options",
"//common/ast",
"//common/types:type_providers",
"@cel_spec//proto/cel/expr:checked_java_proto",
"@maven//:com_google_errorprone_error_prone_annotations",
Expand Down
8 changes: 8 additions & 0 deletions checker/src/main/java/dev/cel/checker/CelCheckerBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import dev.cel.common.CelFunctionDecl;
import dev.cel.common.CelOptions;
import dev.cel.common.CelVarDecl;
import dev.cel.common.ast.CelConstant;
import dev.cel.common.types.CelType;
import dev.cel.common.types.CelTypeProvider;

Expand Down Expand Up @@ -69,6 +70,13 @@ public interface CelCheckerBuilder {
@CanIgnoreReturnValue
CelCheckerBuilder addVarDeclarations(Iterable<CelVarDecl> celVarDecls);

/**
* Adds a named constant declaration to the CEL environment. The declared constant will be inlined
* into the expression during type-check.
*/
@CanIgnoreReturnValue
CelCheckerBuilder addConstant(String name, CelConstant celConstant);

/**
* Add one or more {@link ProtoTypeMask} values. The {@code ProtoTypeMask} values will be used to
* compute a set of {@code Decl} values using a protobuf message's fields as the names and types
Expand Down
23 changes: 23 additions & 0 deletions checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import dev.cel.common.CelValidationResult;
import dev.cel.common.CelVarDecl;
import dev.cel.common.annotations.Internal;
import dev.cel.common.ast.CelConstant;
import dev.cel.common.ast.CelExprConverter;
import dev.cel.common.internal.EnvVisitable;
import dev.cel.common.internal.EnvVisitor;
Expand All @@ -50,6 +51,7 @@
import dev.cel.common.types.CelType;
import dev.cel.common.types.CelTypeProvider;
import dev.cel.common.types.ProtoMessageTypeProvider;
import dev.cel.common.types.SimpleType;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -285,6 +287,27 @@ public CelCheckerBuilder addVarDeclarations(Iterable<CelVarDecl> celVarDecls) {
return this;
}

@Override
public CelCheckerBuilder addConstant(String name, CelConstant celConstant) {
switch (celConstant.getKind()) {
case NOT_SET:
case TIMESTAMP_VALUE:
case DURATION_VALUE:
throw new IllegalArgumentException("Unsupported constant: " + celConstant.getKind());
default:
break;
}

this.identDeclarations.add(
CelIdentDecl.newBuilder()
.setName(name)
.setType(SimpleType.DYN)
.setConstant(celConstant)
.setIsInlinable(true)
.build());
return this;
}

@Override
public CelCheckerBuilder addProtoTypeMasks(ProtoTypeMask... typeMasks) {
checkNotNull(typeMasks);
Expand Down
7 changes: 6 additions & 1 deletion checker/src/main/java/dev/cel/checker/CelIdentDecl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ public abstract class CelIdentDecl {
/** Documentation string for the identifier. */
public abstract String doc();

/** If set, the identifier will get inlined as a constant value during type-check. */
abstract boolean isInlinable();

/** Converts a {@link CelIdentDecl} to a protobuf equivalent form {@code Decl} */
public static Decl celIdentToDecl(CelIdentDecl identDecl) {
IdentDecl.Builder identBuilder =
Expand All @@ -71,7 +74,7 @@ public static CelIdentDecl newIdentDeclaration(String name, CelType type) {
}

public static Builder newBuilder() {
return new AutoValue_CelIdentDecl.Builder().setDoc("");
return new AutoValue_CelIdentDecl.Builder().setDoc("").setIsInlinable(false);
}

/** Builder for configuring the {@link CelIdentDecl}. */
Expand All @@ -92,6 +95,8 @@ public abstract static class Builder {
@CanIgnoreReturnValue
public abstract Builder setDoc(String value);

public abstract Builder setIsInlinable(boolean value);

@CanIgnoreReturnValue
public Builder clearConstant() {
return setConstant(Optional.empty());
Expand Down
14 changes: 2 additions & 12 deletions checker/src/main/java/dev/cel/checker/Env.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,13 +919,8 @@ public static class DeclGroup {

/** Construct an empty {@code DeclGroup}. */
public DeclGroup() {
this(new HashMap<>(), new HashMap<>());
}

/** Construct a new {@code DeclGroup} from the input {@code idents} and {@code functions}. */
public DeclGroup(Map<String, CelIdentDecl> idents, Map<String, CelFunctionDecl> functions) {
this.functions = functions;
this.idents = idents;
this.functions = new HashMap<>();
this.idents = new HashMap<>();
}

/**
Expand Down Expand Up @@ -959,11 +954,6 @@ public void putIdent(CelIdentDecl ident) {
public void putFunction(CelFunctionDecl function) {
functions.put(function.name(), function);
}

/** Create a copy of the {@code DeclGroup} with immutable identifier and function maps. */
public DeclGroup immutableCopy() {
return new DeclGroup(getIdents(), getFunctions());
}
}

/**
Expand Down
14 changes: 12 additions & 2 deletions checker/src/main/java/dev/cel/checker/ExprChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,8 +264,18 @@ private void visit(CelMutableExpr expr, CelMutableIdent ident) {
// Overwrite the identifier with its fully qualified name.
expr.setIdent(CelMutableIdent.create(refName));
}
env.setType(expr, decl.type());
env.setRef(expr, makeReference(refName, decl));

if (decl.isInlinable()) {
decl.constant()
.ifPresent(
constant -> {
expr.setConstant(constant);
visit(expr, expr.constant());
});
} else {
env.setType(expr, decl.type());
env.setRef(expr, makeReference(refName, decl));
}
}

private void visit(CelMutableExpr expr, CelMutableSelect select) {
Expand Down
3 changes: 3 additions & 0 deletions checker/src/test/java/dev/cel/checker/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,12 @@ java_library(
"//common/types:json",
"//common/types:message_type_provider",
"//common/types:type_providers",
"//common/values",
"//common/values:cel_byte_string",
"//compiler",
"//compiler:compiler_builder",
"//parser:macro",
"//parser:unparser",
"//testing:adorner",
"//testing:cel_baseline_test_case",
"@maven//:junit_junit",
Expand Down
113 changes: 111 additions & 2 deletions checker/src/test/java/dev/cel/checker/CelCompilerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,34 @@
package dev.cel.checker;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;

import com.google.common.collect.ImmutableList;
import com.google.common.primitives.UnsignedLong;
import com.google.protobuf.Duration;
import com.google.protobuf.Timestamp;
import com.google.testing.junit.testparameterinjector.TestParameter;
import com.google.testing.junit.testparameterinjector.TestParameterInjector;
import com.google.testing.junit.testparameterinjector.TestParameterValuesProvider;
import dev.cel.common.CelAbstractSyntaxTree;
import dev.cel.common.CelFunctionDecl;
import dev.cel.common.CelOverloadDecl;
import dev.cel.common.CelValidationException;
import dev.cel.common.ast.CelConstant;
import dev.cel.common.ast.CelReference;
import dev.cel.common.types.CelType;
import dev.cel.common.types.SimpleType;
import dev.cel.common.values.CelByteString;
import dev.cel.common.values.NullValue;
import dev.cel.compiler.CelCompiler;
import dev.cel.compiler.CelCompilerBuilder;
import dev.cel.compiler.CelCompilerFactory;
import dev.cel.compiler.CelCompilerImpl;
import dev.cel.parser.CelUnparserFactory;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
@RunWith(TestParameterInjector.class)
public class CelCompilerImplTest {

@Test
Expand All @@ -42,4 +58,97 @@ public void toCompilerBuilder_isImmutable() {

assertThat(newCompilerBuilder).isNotEqualTo(celCompilerBuilder);
}

@Test
public void addConstant_constantsInlined(@TestParameter ConstantTestCase testCase)
throws Exception {
CelCompiler compiler =
CelCompilerFactory.standardCelCompilerBuilder()
.addConstant("const_ident", testCase.constant)
.build();

CelAbstractSyntaxTree ast = compiler.compile("const_ident").getAst();

String unparsed = CelUnparserFactory.newUnparser().unparse(ast);
assertThat(unparsed).isEqualTo(testCase.unparsed);
assertThat(ast.getResultType()).isEqualTo(testCase.celType);
}

@Test
public void addConstant_unsupportedConstants_throws(
@TestParameter(valuesProvider = UnsupportedConstantsProvider.class)
CelConstant unsupportedConstant) {
CelCompilerBuilder builder = CelCompilerFactory.standardCelCompilerBuilder();

IllegalArgumentException e =
assertThrows(
IllegalArgumentException.class,
() -> builder.addConstant("const_ident", unsupportedConstant));
assertThat(e).hasMessageThat().contains("Unsupported constant");
}

@Test
public void addConstant_collidesWithVariables_throws() {
CelValidationException e =
assertThrows(
CelValidationException.class,
() ->
CelCompilerFactory.standardCelCompilerBuilder()
.addVar("const_ident", SimpleType.INT)
.addConstant("const_ident", CelConstant.ofValue(2L))
.build()
.compile("const_ident")
.getAst());
assertThat(e).hasMessageThat().contains("overlapping declaration name 'const_ident'");
}

@Test
public void addConstant_withinAnExpression_containsValidAstMetadata() throws Exception {
CelCompiler compiler =
CelCompilerFactory.standardCelCompilerBuilder()
.addConstant("const_ident", CelConstant.ofValue(42L))
.build();

CelAbstractSyntaxTree ast = compiler.compile("const_ident + 2").getAst();

String unparsed = CelUnparserFactory.newUnparser().unparse(ast);
assertThat(unparsed).isEqualTo("42 + 2");
assertThat(ast.getReferenceMap())
.containsExactly(2L, CelReference.newBuilder().addOverloadIds("add_int64").build());
assertThat(ast.getTypeMap())
.containsExactly(1L, SimpleType.INT, 2L, SimpleType.INT, 3L, SimpleType.INT);
}

private enum ConstantTestCase {
BOOL(CelConstant.ofValue(true), SimpleType.BOOL, "true"),
INT(CelConstant.ofValue(2L), SimpleType.INT, "2"),
UINT(CelConstant.ofValue(UnsignedLong.valueOf(3L)), SimpleType.UINT, "3u"),
DOUBLE(CelConstant.ofValue(2.5d), SimpleType.DOUBLE, "2.5"),
STRING(CelConstant.ofValue("hello"), SimpleType.STRING, "\"hello\""),
BYTES(
CelConstant.ofValue(CelByteString.copyFromUtf8("hello")),
SimpleType.BYTES,
"b\"\\150\\145\\154\\154\\157\""),
NULL(CelConstant.ofValue(NullValue.NULL_VALUE), SimpleType.NULL_TYPE, "null");

private final CelConstant constant;
private final CelType celType;
private final String unparsed;

ConstantTestCase(CelConstant constant, CelType celType, String unparsed) {
this.constant = constant;
this.celType = celType;
this.unparsed = unparsed;
}
}

private static final class UnsupportedConstantsProvider extends TestParameterValuesProvider {
@Override
protected ImmutableList<?> provideValues(Context context) {
return ImmutableList.of(
CelConstant.ofNotSet(),
CelConstant.ofValue(Timestamp.getDefaultInstance()),
CelConstant.ofValue(Duration.getDefaultInstance()));
}
}
}
14 changes: 0 additions & 14 deletions common/src/main/java/dev/cel/common/ast/CelConstant.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,9 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.primitives.UnsignedLong;
import com.google.errorprone.annotations.Immutable;
import com.google.errorprone.annotations.InlineMe;
import com.google.protobuf.ByteString;
import com.google.protobuf.Duration;
import com.google.protobuf.Timestamp;
import dev.cel.common.annotations.Internal;
import dev.cel.common.values.CelByteString;
import dev.cel.common.values.NullValue;

Expand All @@ -33,7 +31,6 @@
* <p>This is the CEL-Java native type equivalent of Constant message type from syntax.proto.
*/
@AutoOneOf(CelConstant.Kind.class)
@Internal
@Immutable
public abstract class CelConstant {
private static final ImmutableSet<Class<?>> CONSTANT_CLASSES =
Expand Down Expand Up @@ -140,17 +137,6 @@ public static CelConstant ofValue(CelByteString value) {
return AutoOneOf_CelConstant.bytesValue(value);
}

/**
* @deprecated Use native type equivalent {@link #ofValue(NullValue)} instead.
*/
@InlineMe(
replacement = "CelConstant.ofValue(NullValue.NULL_VALUE)",
imports = {"dev.cel.common.ast.CelConstant", "dev.cel.common.values.NullValue"})
@Deprecated
public static CelConstant ofValue(com.google.protobuf.NullValue unused) {
return ofValue(NullValue.NULL_VALUE);
}

/**
* @deprecated Use native type equivalent {@link #ofValue(CelByteString)} instead.
*/
Expand Down
Loading
Loading