Skip to content

Commit 2eead50

Browse files
l46kokcopybara-github
authored andcommitted
Fix type-checker to respect the ordering of type providers (protobuf then user provided)
PiperOrigin-RevId: 992491032
1 parent e16c4f3 commit 2eead50

5 files changed

Lines changed: 50 additions & 9 deletions

File tree

‎bundle/src/test/java/dev/cel/bundle/CelImplTest.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1982,10 +1982,13 @@ public void program_nativeTypeUnknownsEnabled_asCallArguments() throws Exception
19821982

19831983
@Test
19841984
public void program_comprehensionDisabled_throws() throws Exception {
1985-
// TODO: Planner ExecutionFrame ignores CelOptions.enableComprehension(false).
19861985
Cel cel =
1987-
legacyCelBuilderWithMacros()
1988-
.setOptions(CelOptions.current().enableComprehension(false).build())
1986+
plannerCelBuilderWithMacros()
1987+
.setOptions(
1988+
CelOptions.current()
1989+
.enableHeterogeneousNumericComparisons(true)
1990+
.enableComprehension(false)
1991+
.build())
19891992
.build();
19901993
CelAbstractSyntaxTree ast = cel.compile("['foo', 'bar'].map(x, x)").getAst();
19911994

‎checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -446,7 +446,7 @@ public CelCheckerLegacyImpl build() {
446446
} else if (celTypeProvider != null) {
447447
messageTypeProvider =
448448
new CelTypeProvider.CombinedCelTypeProvider(
449-
ImmutableList.of(celTypeProvider, messageTypeProvider));
449+
ImmutableList.of(messageTypeProvider, celTypeProvider));
450450
}
451451

452452
// Configure the declaration set, and possibly alter the type provider if ProtoDecl values

‎checker/src/test/java/dev/cel/checker/CelCheckerLegacyImplTest.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
import com.google.common.collect.ImmutableList;
2121
import com.google.common.collect.ImmutableMap;
22+
import com.google.common.collect.ImmutableSet;
2223
import com.google.protobuf.Duration;
2324
import com.google.protobuf.FieldMask;
2425
import com.google.protobuf.Timestamp;
@@ -40,6 +41,7 @@
4041
import dev.cel.common.types.ListType;
4142
import dev.cel.common.types.MapType;
4243
import dev.cel.common.types.SimpleType;
44+
import dev.cel.common.types.StructType;
4345
import dev.cel.common.types.StructTypeReference;
4446
import dev.cel.common.types.TypeType;
4547
import dev.cel.compiler.CelCompiler;
@@ -409,6 +411,41 @@ public Optional<CelType> findType(String typeName) {
409411
assertThat(ast.getResultType()).isEqualTo(preWrappedType);
410412
}
411413

414+
@Test
415+
public void check_combinedTypeProviders_protoMessageTakesPrecedenceOverCustom() throws Exception {
416+
StructType shadowingStructType =
417+
StructType.create(
418+
TestAllTypes.getDescriptor().getFullName(),
419+
ImmutableSet.of("single_int64"),
420+
fieldName -> Optional.of(SimpleType.STRING));
421+
CelTypeProvider customTypeProvider =
422+
new CelTypeProvider() {
423+
@Override
424+
public ImmutableList<CelType> types() {
425+
return ImmutableList.of(shadowingStructType);
426+
}
427+
428+
@Override
429+
public Optional<CelType> findType(String typeName) {
430+
return typeName.equals(shadowingStructType.name())
431+
? Optional.of(shadowingStructType)
432+
: Optional.empty();
433+
}
434+
};
435+
CelCompiler celCompiler =
436+
CelCompilerFactory.standardCelCompilerBuilder()
437+
.addMessageTypes(TestAllTypes.getDescriptor())
438+
.setTypeProvider(customTypeProvider)
439+
.build();
440+
441+
CelAbstractSyntaxTree ast =
442+
celCompiler
443+
.compile("cel.expr.conformance.proto3.TestAllTypes{single_int64: 1}.single_int64")
444+
.getAst();
445+
446+
assertThat(ast.getResultType()).isEqualTo(SimpleType.INT);
447+
}
448+
412449
private enum FieldTypeTestCase {
413450
REPEATED_PRIMITIVE("msg.repeated_int64", ListType.create(SimpleType.INT)),
414451
MAP_PRIMITIVE("msg.map_string_string", MapType.create(SimpleType.STRING, SimpleType.STRING)),

‎runtime/src/main/java/dev/cel/runtime/planner/ExecutionFrame.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ static ExecutionFrame create(
4646
@Nullable CelEvaluationListener listener) {
4747
return new ExecutionFrame(
4848
functionResolver,
49-
celOptions.comprehensionMaxIterations(),
49+
getComprehensionMaxIterations(celOptions),
5050
partialVars,
5151
listener,
5252
/* asyncTracker= */ null);
@@ -61,12 +61,16 @@ static ExecutionFrame createForAsync(
6161
checkNotNull(asyncTracker, "asyncTracker");
6262
return new ExecutionFrame(
6363
functionResolver,
64-
celOptions.comprehensionMaxIterations(),
64+
getComprehensionMaxIterations(celOptions),
6565
partialVars,
6666
listener,
6767
asyncTracker);
6868
}
6969

70+
private static int getComprehensionMaxIterations(CelOptions celOptions) {
71+
return celOptions.enableComprehension() ? celOptions.comprehensionMaxIterations() : 0;
72+
}
73+
7074
Optional<CelResolvedOverload> findOverload(
7175
String functionName, Collection<String> overloadIds, Object[] args)
7276
throws CelEvaluationException {

‎runtime/src/test/java/dev/cel/runtime/DescriptorTypeResolverTest.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,6 @@ public class DescriptorTypeResolverTest {
4545
private static final Cel CEL =
4646
CelFactory.plannerCelBuilder()
4747
.setTypeProvider(PROTO_MESSAGE_TYPE_PROVIDER)
48-
// TODO: Replace setValueProvider with
49-
// addMessageTypes(TestAllTypes.getDescriptor()) once CelRuntimeImpl prioritizes custom
50-
// CelTypeProvider over its internal messageTypeProvider.
5148
.setValueProvider(
5249
(structType, fields) ->
5350
structType.equals(TestAllTypes.getDescriptor().getFullName())

0 commit comments

Comments
 (0)