From 9d00cf8c2c27cb636c0a2a1d90506015e0aa17a2 Mon Sep 17 00:00:00 2001 From: Dmitri Plotnikov Date: Mon, 9 Nov 2026 22:41:16 -0800 Subject: [PATCH] Support dyn and type parameters in Python function overloads. The C++ CEL runtime function dispatcher uses `cel::Kind::kAny` as its wildcard parameter kind. Normalize `kDyn` and `kTypeParam` parameter kinds to `kAny` across environment, activation, and extension function registrations, preserve `kTypeParam` through `PyCelType` and `PyObjectToCelValue`, and treat `cel.Type.DYN` as assignable from any CEL type. PiperOrigin-RevId: 1017931887 --- cel_expr_python/BUILD | 12 + cel_expr_python/cel_env_test.py | 527 ++++++++++++++++++++- cel_expr_python/cel_parallel_test.py | 85 +++- cel_expr_python/cel_test.py | 235 +++++++-- cel_expr_python/leak_check.py | 137 ++++++ cel_expr_python/py_cel_activation.cc | 7 +- cel_expr_python/py_cel_env_internal.cc | 6 +- cel_expr_python/py_cel_function.cc | 42 +- cel_expr_python/py_cel_python_extension.cc | 3 +- cel_expr_python/py_cel_type.cc | 84 +++- cel_expr_python/py_cel_value.cc | 141 ++++-- cel_expr_python/py_cel_value.h | 2 + cel_expr_python/py_message_factory.cc | 6 +- release/build_wheel.sh | 2 +- release/cel_basic_test.py | 1 + release/kokoro/release_linux.sh | 2 +- release/kokoro/release_macos.sh | 2 +- release/kokoro/release_windows.bat | 3 + release/pyproject.toml | 2 +- 19 files changed, 1180 insertions(+), 119 deletions(-) create mode 100644 cel_expr_python/leak_check.py diff --git a/cel_expr_python/BUILD b/cel_expr_python/BUILD index 02c8f9c..7cf74b6 100644 --- a/cel_expr_python/BUILD +++ b/cel_expr_python/BUILD @@ -1,5 +1,6 @@ load("@pybind11_bazel//:build_defs.bzl", "pybind_extension", "pybind_library") load("@rules_cc//cc:cc_library.bzl", "cc_library") +load("@rules_python//python:py_library.bzl", "py_library") load("@rules_python//python:py_test.bzl", "py_test") package(default_visibility = ["//visibility:public"]) @@ -150,6 +151,13 @@ cc_library( ], ) +py_library( + name = "leak_check", + testonly = True, + srcs = ["leak_check.py"], + visibility = [":__subpackages__"], +) + py_test( name = "cel_test", srcs = ["cel_test.py"], @@ -157,8 +165,10 @@ py_test( ":cel", ], deps = [ + ":leak_check", "//testing:proto2_test_all_types_py_pb2", "@com_google_absl_py//absl/testing:absltest", + "@com_google_absl_py//absl/testing:parameterized", "@com_google_protobuf//:protobuf", "@com_google_protobuf//:protobuf_python", ] + select({ @@ -174,6 +184,7 @@ py_test( ":cel", ], deps = [ + ":leak_check", "//testing:proto2_test_all_types_py_pb2", "@com_google_absl_py//absl/testing:absltest", ] + select({ @@ -196,6 +207,7 @@ py_test( deps = [ "//testing:proto2_test_all_types_py_pb2", "@com_google_absl_py//absl/testing:absltest", + "@com_google_absl_py//absl/testing:parameterized", ] + select({ "@platforms//os:windows": [], "//conditions:default": [ diff --git a/cel_expr_python/cel_env_test.py b/cel_expr_python/cel_env_test.py index 7684169..c4e3dfb 100644 --- a/cel_expr_python/cel_env_test.py +++ b/cel_expr_python/cel_env_test.py @@ -18,10 +18,12 @@ ability to be created from and serialized to YAML format. """ +import datetime import textwrap from typing import Any from absl.testing import absltest +from absl.testing import parameterized from cel.expr.conformance.proto2 import test_all_types_pb2 as test_all_types_pb from cel_expr_python import cel from cel_expr_python.ext import ext_bindings @@ -30,7 +32,7 @@ from cel_expr_python.ext import ext_strings -class CelEnvTest(absltest.TestCase): +class CelEnvTest(parameterized.TestCase): def test_env_config_from_and_to_yaml(self): config: cel.EnvConfig = cel.NewEnvConfigFromYaml(""" @@ -743,6 +745,524 @@ def test_env_options(self): default_env = cel.NewEnv() self.assertTrue(default_env.options().enable_pratt_parser) + def _new_poly_env(self) -> cel.Env: + ext = cel.CelExtension( + "poly_ext", + functions=[ + cel.FunctionDecl( + "ext_id", + [ + cel.Overload( + signature="ext_id(~T)", + return_type=cel.Type("~T"), + impl=lambda x: x, + ) + ], + ) + ], + ) + return cel.NewEnv( + extensions=[ext, ext_optional.ExtOptional()], + variables={ + "var_tp": cel.Type("~T"), + "var_type": cel.Type.TYPE, + "var_int_type": cel.Type.Type(cel.Type.INT), + "var_dyn": cel.Type.DYN, + }, + functions=[ + cel.FunctionDecl( + "to_string", + [ + cel.Overload( + signature="to_string(dyn)", + return_type=cel.Type.STRING, + impl=repr, + ) + ], + ), + cel.FunctionDecl( + "dyn_id", + [ + cel.Overload( + signature="dyn_id(dyn)", + return_type=cel.Type.DYN, + impl=lambda x: x, + ) + ], + ), + cel.FunctionDecl( + "identity", + [ + cel.Overload( + signature="identity(~T)", + return_type=cel.Type("~T"), + impl=lambda x: x, + ) + ], + ), + cel.FunctionDecl( + "tp_id", + [ + cel.Overload( + signature="tp_id(~T)", + return_type=cel.Type("~T"), + impl=lambda x: x, + ) + ], + ), + cel.FunctionDecl( + "bad_type_ret", + [ + cel.Overload( + signature="bad_type_ret()", + return_type=cel.Type.TYPE, + impl=lambda: "not_a_type", + ) + ], + ), + cel.FunctionDecl( + "late_ovl", + [ + cel.Overload( + signature="late_ovl(int)", + return_type=cel.Type.STRING, + ), + cel.Overload( + signature="late_ovl(string)", + return_type=cel.Type.STRING, + ), + ], + ), + cel.FunctionDecl( + "late_dyn", + [ + cel.Overload( + signature="late_dyn(dyn)", + return_type=cel.Type.STRING, + ), + ], + ), + cel.FunctionDecl( + "late_tp", + [ + cel.Overload( + signature="late_tp(~T)", + return_type=cel.Type("~T"), + ), + ], + ), + ], + ) + + def test_functions_with_dyn_and_type_params(self): + env = self._new_poly_env() + self.assertEqual(env.compile("to_string(42)").eval().value(), "42") + self.assertEqual(env.compile("identity('abc')").eval().value(), "abc") + self.assertEqual(env.compile("ext_id(7) + 1").eval().value(), 8) + + # Type-parameterized variables, parameterized/abstract TypeValues, invalid + # TYPE conversions, and overloaded activation functions. + self.assertEqual(env.compile("var_tp").return_type(), cel.Type.DYN) + self.assertEqual( + env.compile("tp_id(var_type)") + .eval(data={"var_type": cel.Type.Type(cel.Type.INT)}) + .value(), + cel.Type.Type(cel.Type.INT), + ) + self.assertEqual( + env.compile("tp_id(var_int_type)") + .eval(data={"var_int_type": cel.Type.INT}) + .value(), + cel.Type.INT, + ) + abstract_type = cel.Type.AbstractType("optional_type", [cel.Type.INT]) + self.assertEqual( + env.compile("tp_id(var_dyn)") + .eval(data={"var_dyn": abstract_type}) + .value(), + abstract_type, + ) + bad_type_var_res = env.compile("var_type").eval( + data={"var_type": "not_a_type"} + ) + self.assertEqual(bad_type_var_res.type(), cel.Type.ERROR) + self.assertIn( + "Unexpected value type for 'var_type': str. (Expected TYPE)", + bad_type_var_res.value(), + ) + bad_type_fn_res = env.compile("bad_type_ret()").eval() + self.assertEqual(bad_type_fn_res.type(), cel.Type.ERROR) + self.assertIn("(Expected TYPE)", bad_type_fn_res.value()) + + ovl_act = env.Activation( + {}, + functions=[ + cel.Function( + "late_ovl", + [cel.Type.INT], + False, + lambda x: f"int:{x}", + return_type=cel.Type.STRING, + ), + cel.Function( + "late_ovl", + [cel.Type.STRING], + False, + lambda x: f"str:{x}", + return_type=cel.Type.STRING, + ), + ], + ) + self.assertEqual(env.compile("late_ovl(1)").eval(ovl_act).value(), "int:1") + self.assertEqual( + env.compile("late_ovl('a')").eval(ovl_act).value(), "str:a" + ) + + poly_act = env.Activation( + {}, + functions=[ + cel.Function( + "late_dyn", + [cel.Type.DYN], + False, + repr, + return_type=cel.Type.STRING, + ), + cel.Function( + "late_tp", + [cel.Type("~T")], + False, + lambda x: x, + return_type=cel.Type("~T"), + ), + ], + ) + self.assertEqual(env.compile("late_dyn(42)").eval(poly_act).value(), "42") + self.assertEqual( + env.compile("late_dyn('abc')").eval(poly_act).value(), "'abc'" + ) + self.assertEqual(env.compile("late_tp(7)").eval(poly_act).value(), 7) + self.assertEqual( + env.compile("late_tp('abc')").eval(poly_act).value(), "abc" + ) + + @parameterized.named_parameters( + ("null_eq", "ext_id(null) == null", True), + ("bool_not", "!ext_id(false)", True), + ("int_add", "identity(7) + 1", 8), + ("uint_add", "ext_id(9223372036854775808u) + 1u", 9223372036854775809), + ("double_add", "identity(1.5) + 2.5", 4.0), + ("string_add", "ext_id('hello') + '!'", "hello!"), + ("bytes_add", "identity(b'ab') + b'cd'", bytearray(b"abcd")), + ("list_add", "ext_id([1, 2]) + [3]", [1, 2, 3]), + ("map_index", "identity({'a': 1})['a']", 1), + ("type_eq", "ext_id(type(1)) == int", True), + ("type_list_eq", "ext_id(type([1])) == list", True), + ("type_map_eq", "ext_id(type({'a': 1})) == map", True), + ( + "type_ts_eq", + ( + "ext_id(type(timestamp('2025-01-01T00:00:00Z'))) ==" + " google.protobuf.Timestamp" + ), + True, + ), + ( + "type_dur_eq", + "ext_id(type(duration('1s'))) == google.protobuf.Duration", + True, + ), + ( + "timestamp_add", + "identity(timestamp('2025-01-01T00:00:00Z')) + duration('1h')", + datetime.datetime(2025, 1, 1, 1, 0, tzinfo=datetime.timezone.utc), + ), + ( + "duration_add", + "ext_id(duration('10s')) + duration('5s')", + datetime.timedelta(seconds=15), + ), + ( + "proto_field", + ( + "ext_id(cel.expr.conformance.proto2.TestAllTypes" + "{single_string: 'hello'}).single_string" + ), + "hello", + ), + ( + "wrapper_add", + "ext_id(google.protobuf.StringValue{value: 'wrap'}) + 'ped'", + "wrapped", + ), + ) + def test_type_param_function_in_expressions( + self, + expr_str: str, + expected_val: Any, + ): + env = self._new_poly_env() + res = env.compile(expr_str).eval() + self.assertEqual(res.value(), expected_val) + + @parameterized.named_parameters( + ("null", "null", None, cel.Type.NULL), + ("bool_true", "true", True, cel.Type.BOOL), + ("bool_false", "false", False, cel.Type.BOOL), + ("int", "-42", -42, cel.Type.INT), + ( + "uint_large", + "18446744073709551615u", + 18446744073709551615, + cel.Type.UINT, + ), + ("double", "3.14", 3.14, cel.Type.DOUBLE), + ("string", "'abc'", "abc", cel.Type.STRING), + ("bytes", r"b'ab\x00c'", bytearray(b"ab\x00c"), cel.Type.BYTES), + ("list", "[1, 'two', true]", [1, "two", True], cel.Type.LIST), + ("map", "{'a': 1, 'b': 'two'}", {"a": 1, "b": "two"}, cel.Type.MAP), + ( + "timestamp", + "timestamp('2025-06-15T12:30:00.500000Z')", + datetime.datetime( + 2025, 6, 15, 12, 30, 0, 500000, datetime.timezone.utc + ), + cel.Type.TIMESTAMP, + ), + ( + "duration", + "duration('10s')", + datetime.timedelta(seconds=10), + cel.Type.DURATION, + ), + ("type_int", "type(1)", cel.Type.INT, cel.Type.TYPE), + ("type_null", "type(null)", cel.Type.NULL, cel.Type.TYPE), + ("type_type", "type(int)", cel.Type.TYPE, cel.Type.TYPE), + ( + "type_msg", + "type(cel.expr.conformance.proto2.TestAllTypes{})", + cel.Type("cel.expr.conformance.proto2.TestAllTypes"), + cel.Type.TYPE, + ), + ( + "proto_msg", + ( + "cel.expr.conformance.proto2.TestAllTypes" + "{single_string: 'hello', single_int32: 7}" + ), + test_all_types_pb.TestAllTypes(single_string="hello", single_int32=7), + cel.Type("cel.expr.conformance.proto2.TestAllTypes"), + ), + ( + "wkt_bool_value", + "google.protobuf.BoolValue{value: true}", + True, + cel.Type.BOOL, + ), + ( + "wkt_int32_value", + "google.protobuf.Int32Value{value: -12}", + -12, + cel.Type.INT, + ), + ( + "wkt_int64_value", + "google.protobuf.Int64Value{value: 42}", + 42, + cel.Type.INT, + ), + ( + "wkt_uint64_value", + "google.protobuf.UInt64Value{value: 18446744073709551615u}", + 18446744073709551615, + cel.Type.UINT, + ), + ( + "wkt_float_value", + "google.protobuf.FloatValue{value: 1.5}", + 1.5, + cel.Type.DOUBLE, + ), + ( + "wkt_double_value", + "google.protobuf.DoubleValue{value: 2.5}", + 2.5, + cel.Type.DOUBLE, + ), + ( + "wkt_string_value", + "google.protobuf.StringValue{value: 'wrapped'}", + "wrapped", + cel.Type.STRING, + ), + ( + "wkt_bytes_value", + "google.protobuf.BytesValue{value: b'bin'}", + bytearray(b"bin"), + cel.Type.BYTES, + ), + ( + "wkt_empty_wrapper", + "google.protobuf.Int64Value{}", + 0, + cel.Type.INT, + ), + ( + "wkt_unset_wrapper_field", + "cel.expr.conformance.proto2.TestAllTypes{}.single_int64_wrapper", + None, + cel.Type.NULL, + ), + ( + "wkt_empty_value", + "google.protobuf.Value{}", + None, + cel.Type.NULL, + ), + ( + "wkt_struct", + ( + "google.protobuf.Struct" + "{fields: {'k': google.protobuf.Value{string_value: 'v'}}}" + ), + {"k": "v"}, + cel.Type.MAP, + ), + ( + "wkt_list_value", + ( + "google.protobuf.ListValue" + "{values: [google.protobuf.Value{number_value: 2.0}]}" + ), + [2.0], + cel.Type.LIST, + ), + ( + "wkt_value", + "google.protobuf.Value{bool_value: true}", + True, + cel.Type.BOOL, + ), + ( + "wkt_any", + ( + "cel.expr.conformance.proto2.TestAllTypes" + "{single_any: cel.expr.conformance.proto2.TestAllTypes" + "{single_string: 'packed'}}.single_any" + ), + test_all_types_pb.TestAllTypes(single_string="packed"), + cel.Type("cel.expr.conformance.proto2.TestAllTypes"), + ), + ( + "nested_list", + "[timestamp('2025-06-15T12:30:00.500000Z'), duration('5s'), type(1)]", + [ + datetime.datetime( + 2025, 6, 15, 12, 30, 0, 500000, datetime.timezone.utc + ), + datetime.timedelta(seconds=5), + cel.Type.INT, + ], + cel.Type.LIST, + ), + ( + "nested_map", + "{1: duration('2s'), 2: type('a')}", + {1: datetime.timedelta(seconds=2), 2: cel.Type.STRING}, + cel.Type.MAP, + ), + ) + def test_dyn_function_values( + self, + expr_str: str, + expected_val: Any, + expected_type: cel.Type, + ): + env = self._new_poly_env() + res = env.compile(f"dyn_id({expr_str})").eval() + self.assertEqual(res.type(), expected_type) + self.assertEqual(res.value(), expected_val) + self.assertEqual(res.plain_value(), expected_val) + + @parameterized.named_parameters( + ( + "list", + "type([1])", + "list", + cel.Type.Type(cel.Type.List(cel.Type.INT)), + cel.Type.LIST, + ), + ( + "list_cross_elem", + "type([1])", + "type(['a'])", + cel.Type.Type(cel.Type.List(cel.Type.INT)), + cel.Type.LIST, + ), + ( + "map", + "type({'a': 1})", + "map", + cel.Type.Type(cel.Type.Map(cel.Type.STRING, cel.Type.INT)), + cel.Type.MAP, + ), + ( + "type_int", + "type(int)", + "type", + cel.Type.Type(cel.Type.Type(cel.Type.INT)), + cel.Type.TYPE, + ), + ( + "type_type_1", + "type(type(1))", + "type", + cel.Type.Type(cel.Type.Type(cel.Type.INT)), + cel.Type.TYPE, + ), + ( + "optional_of", + "type(optional.of(1))", + "optional_type", + cel.Type.Type(cel.Type.AbstractType("optional_type", [cel.Type.INT])), + cel.Type.AbstractType("optional_type", [cel.Type.DYN]), + ), + ( + "optional_of_cross_elem", + "type(optional.of(1))", + "type(optional.of('a'))", + cel.Type.Type(cel.Type.AbstractType("optional_type", [cel.Type.INT])), + cel.Type.AbstractType("optional_type", [cel.Type.DYN]), + ), + ( + "optional_none", + "type(optional.none())", + "optional_type", + cel.Type.Type(cel.Type.AbstractType("optional_type", [cel.Type.DYN])), + cel.Type.AbstractType("optional_type", [cel.Type.DYN]), + ), + ) + def test_runtime_type_erasure( + self, + type_expr: str, + expected_cel_type: str, + expected_static_type: cel.Type, + expected_runtime_val: cel.Type, + ): + env = self._new_poly_env() + expr = env.compile(type_expr) + self.assertEqual(expr.return_type(), expected_static_type) + self.assertEqual(expr.eval().value(), expected_runtime_val) + self.assertTrue( + env.compile(f"{type_expr} == {expected_cel_type}").eval().value() + ) + tp_expr = env.compile(f"tp_id({type_expr})") + self.assertEqual(tp_expr.return_type(), expected_static_type) + self.assertEqual(tp_expr.eval().value(), expected_runtime_val) + self.assertTrue( + env.compile(f"tp_id({type_expr}) == {expected_cel_type}").eval().value() + ) + class TestCelExtension(cel.CelExtension): """An example CEL extension for testing.""" @@ -755,11 +1275,8 @@ def __init__(self): "hello", [ cel.Overload( - "hello(string)", + signature="hello(string)", return_type=cel.Type.STRING, - parameters=[ - cel.Type.STRING, - ], impl=lambda arg: f"Hello, {arg}!", ) ], diff --git a/cel_expr_python/cel_parallel_test.py b/cel_expr_python/cel_parallel_test.py index 99100fd..8751687 100644 --- a/cel_expr_python/cel_parallel_test.py +++ b/cel_expr_python/cel_parallel_test.py @@ -19,12 +19,14 @@ import dataclasses import gc import logging +import threading import time from typing import Any from absl.testing import absltest from cel.expr.conformance.proto2 import test_all_types_pb2 as test_all_types_pb from cel_expr_python import cel +from cel_expr_python import leak_check @dataclasses.dataclass(frozen=True) @@ -94,6 +96,7 @@ class _TestCase: ] +@leak_check.leak_checked class CelParallelTest(absltest.TestCase): def setUp(self): @@ -119,7 +122,6 @@ def setUp(self): ) ], ) - self.object_counts_before_test = self._grab_object_counts() def tearDown(self): """Tears down the test environment.""" @@ -128,26 +130,6 @@ def tearDown(self): gc.collect() # Assert that all Arenas have been garbage-collected self.assertEqual(cel._InternalArena._get_instance_count(), 0) - self._check_for_leaks() - - def _grab_object_counts(self) -> dict[str, int]: - gc.collect() - all_objects = gc.get_objects() - type_counts = {} - for obj in all_objects: - obj_type = type(obj) - type_counts[obj_type.__name__] = type_counts.get(obj_type, 0) + 1 - return type_counts - - def _check_for_leaks(self): - type_counts = self._grab_object_counts() - for key, count in type_counts.items(): - if count != self.object_counts_before_test.get(key, 0): - self.fail( - f"Object count for {key} did not match expected count. " - f"Expected: {self.object_counts_before_test.get(key, 0)}, " - f"Actual: {count}", - ) def _test_eval(self, multi_threaded: bool): compiled_exprs = [self.env.compile(tc.expr) for tc in _TEST_CASES] @@ -277,6 +259,67 @@ def eval_custom_fn(n: int): results = list(executor.map(eval_custom_fn, range(100))) self.assertLen(results, 100) + def testConcurrentTypeToCelTypeWithDescriptorPoolLookup(self): + """Tests that PyCelType::ToCelType releases the GIL before DescriptorPool.""" + for _ in range(5): + lookup_started = threading.Event() + + class _SlowDescriptorPool: + + def __init__(self, pool: Any, event: threading.Event): + self._pool = pool + self._event = event + + def FindFileByName(self, filename: str): # pylint: disable=invalid-name + return self._pool.FindFileByName(filename) + + def FindFileContainingSymbol(self, symbol_name: str): # pylint: disable=invalid-name + if symbol_name == "cel.expr.conformance.proto2.TestAllTypes": + self._event.set() + # Releases the GIL while DescriptorPool's internal mutex is held. + time.sleep(0.1) + return self._pool.FindFileContainingSymbol(symbol_name) + + slow_pool = _SlowDescriptorPool( + test_all_types_pb.DESCRIPTOR.pool, lookup_started + ) + env = cel.NewEnv( + descriptor_pool=slow_pool, + variables={"var_type": cel.Type.TYPE}, + ) + type_expr = env.compile("var_type == var_type") + # Warm up the runtime/program so eval_type reaches PyObjectToCelValue + # immediately without paying first-time Runtime initialization cost. + self.assertTrue(type_expr.eval(data={"var_type": cel.Type.INT}).value()) + + def compile_message(): + msg_expr = env.compile("cel.expr.conformance.proto2.TestAllTypes{}") + self.assertIsNotNone(msg_expr) + + def eval_type(): + self.assertTrue(lookup_started.wait(timeout=5.0)) + res = type_expr.eval( + data={ + "var_type": cel.Type.List( + cel.Type("cel.expr.conformance.proto2.TestAllTypes") + ) + } + ) + self.assertTrue(res.value()) + + executor = concurrent.futures.ThreadPoolExecutor(max_workers=2) + timed_out = False + try: + compile_future = executor.submit(compile_message) + eval_future = executor.submit(eval_type) + compile_future.result(timeout=5.0) + eval_future.result(timeout=5.0) + except BaseException: + timed_out = True + raise + finally: + executor.shutdown(wait=not timed_out) + if __name__ == "__main__": absltest.main() diff --git a/cel_expr_python/cel_test.py b/cel_expr_python/cel_test.py index 2aca53c..26fdfff 100644 --- a/cel_expr_python/cel_test.py +++ b/cel_expr_python/cel_test.py @@ -26,12 +26,15 @@ from google.protobuf import duration_pb2 as duration_pb from google.protobuf import timestamp_pb2 as timestamp_pb from absl.testing import absltest +from absl.testing import parameterized from cel.expr.conformance.proto2 import test_all_types_pb2 as test_all_types_pb from cel_expr_python import cel +from cel_expr_python import leak_check +@leak_check.leak_checked @absltest.skipThisClass("Base class") -class _CelTestBase(absltest.TestCase): +class _CelTestBase(parameterized.TestCase): options: cel.Options = cel.Options() def setUp(self): @@ -57,7 +60,6 @@ def setUp(self): }, options=self.options, ) - self.object_counts_before_test = self._grab_object_counts() def tearDown(self): """Tears down the test environment.""" @@ -66,26 +68,6 @@ def tearDown(self): gc.collect() # Assert that all Arenas have been garbage-collected self.assertEqual(cel._InternalArena._get_instance_count(), 0) - self._check_for_leaks() - - def _grab_object_counts(self) -> dict[str, int]: - gc.collect() - all_objects = gc.get_objects() - type_counts = {} - for obj in all_objects: - obj_type = type(obj) - type_counts[obj_type.__name__] = type_counts.get(obj_type, 0) + 1 - return type_counts - - def _check_for_leaks(self): - type_counts = self._grab_object_counts() - for key, count in type_counts.items(): - if count != self.object_counts_before_test.get(key, 0): - self.fail( - f"Object count for {key} did not match expected count. " - f"Expected: {self.object_counts_before_test.get(key, 0)}, " - f"Actual: {count}", - ) def _eval( self, @@ -364,7 +346,46 @@ def testProto_unexpectedType(self): r" .*. \(Expected cel.expr.conformance.proto2.TestAllTypes\)", ) - def testProto_cannotFindMessageClass(self): + @parameterized.named_parameters( + ("message", "TestAllTypes{single_string: 'hello'}", True), + ("list", "[TestAllTypes{single_string: 'hello'}]", True), + ("nested_list", "[[1, 2], TestAllTypes{single_string: 'hello'}]", True), + ("map_value", "{'k': TestAllTypes{single_string: 'hello'}}", True), + ( + "nested_map_value", + ( + "{'a': [1, 2], 'b': [3, 4], 'z':" + " TestAllTypes{single_string: 'hello'}}" + ), + True, + ), + ("call_message", "show(TestAllTypes{single_string: 'hello'})", False), + ("call_list", "show([TestAllTypes{single_string: 'hello'}])", False), + ( + "call_nested_list", + "show([[1, 2], TestAllTypes{single_string: 'hello'}])", + False, + ), + ( + "call_map_value", + "show({'k': TestAllTypes{single_string: 'hello'}})", + False, + ), + ( + "call_nested_map_value", + ( + "show({'a': [1, 2], 'b': [3, 4], 'z':" + " TestAllTypes{single_string: 'hello'}})" + ), + False, + ), + ( + "call_second_arg", + "show([1, 2], TestAllTypes{single_string: 'hello'})", + False, + ), + ) + def testProto_cannotFindMessageClass(self, expr: str, expect_exception: bool): orig_get_message_class = message_factory.GetMessageClass def failing_get_message_class(descriptor): @@ -375,21 +396,110 @@ def failing_get_message_class(descriptor): try: message_factory.GetMessageClass = failing_get_message_class - env = cel.NewEnv(options=self.options) - expr = env.compile( - "cel.expr.conformance.proto2.TestAllTypes{single_string: 'hello'}" + called = [] + + def show(x): + called.append(x) + return "ok" + + def show_pair(a, b): + called.append((a, b)) + return "ok" + + ext = cel.CelExtension( + "show_ext", + functions=[ + cel.FunctionDecl( + "show", + [ + cel.Overload( + "show_any", cel.Type.STRING, [cel.Type.DYN], impl=show + ), + cel.Overload( + "show_pair", + cel.Type.STRING, + [cel.Type.DYN, cel.Type.DYN], + impl=show_pair, + ), + ], + ) + ], + ) + env = cel.NewEnv( + extensions=[ext], + options=self.options, + container="cel.expr.conformance.proto2", ) - res = expr.eval(env.Activation()) - with self.assertRaisesRegex( - TypeError, "Couldn't find message class for type" - ): - res.plain_value() - del res - del expr - del env + res = env.compile(expr).eval(env.Activation()) + if expect_exception: + with self.assertRaisesRegex( + TypeError, "Couldn't find message class for type" + ): + res.plain_value() + else: + # During evaluation of the function argument, the proto type will + # fail to resolve to a message class, and the error will be + # wrapped in an ERROR type. The function itself will not be called. + self.assertEqual(res.type(), cel.Type.ERROR) + self.assertIn("Couldn't find message class for type", res.value()) + self.assertEqual(called, []) finally: message_factory.GetMessageClass = orig_get_message_class + def testCustomFunction_raisesException(self): + def fail(x): + raise ValueError(f"bad value: {x}") + + ext = cel.CelExtension( + "fail_ext", + functions=[ + cel.FunctionDecl( + "fail", + [ + cel.Overload( + "fail_int", cel.Type.STRING, [cel.Type.INT], impl=fail + ) + ], + ) + ], + ) + env = cel.NewEnv(extensions=[ext], options=self.options) + res = env.compile("fail(42)").eval(env.Activation()) + self.assertEqual(res.type(), cel.Type.ERROR) + self.assertIn("bad value: 42", res.value()) + + def testCustomFunction_invalidReturnWithBrokenRepr(self): + class BrokenReprCallable: + + def __call__(self, x): + del x + return "not_an_int" + + def __repr__(self): + raise RuntimeError("repr failed") + + env = cel.NewEnv( + functions=[ + cel.FunctionDecl( + "broken_repr", + [ + cel.Overload( + "broken_repr_int", + cel.Type.INT, + [cel.Type.INT], + impl=BrokenReprCallable(), + ) + ], + ) + ], + options=self.options, + ) + res = env.compile("broken_repr(1)").eval() + self.assertEqual(res.type(), cel.Type.ERROR) + self.assertIn( + "Unexpected value type for 'Python function '''", res.value() + ) + def testEvalList(self): res: cel.Value = self._eval( "[1, 'CEL', true]", expected_return_type=cel.Type.LIST @@ -741,6 +851,17 @@ def testDynType(self): {"value": "foo", "type": cel.Type.STRING}, {"value": b"foo", "type": cel.Type.BYTES}, {"value": bytearray("bar", "ascii"), "type": cel.Type.BYTES}, + { + "value": datetime.datetime( + 2025, 1, 1, tzinfo=datetime.timezone.utc + ), + "type": cel.Type.TIMESTAMP, + }, + { + "value": datetime.timedelta(seconds=15), + "type": cel.Type.DURATION, + }, + {"value": cel.Type.INT, "type": cel.Type.TYPE}, { "value": msg, "type": cel.Type("cel.expr.conformance.proto2.TestAllTypes"), @@ -891,6 +1012,39 @@ def testTypeInitSignature(self): cel.Type("cel.expr.conformance.proto2.TestAllTypes"), cel.Type("cel.expr.conformance.proto2.TestAllTypes"), ) + self.assertTrue(cel.Type.DYN.is_assignable_from(cel.Type.INT)) + self.assertTrue( + cel.Type.LIST.is_assignable_from(cel.Type.List(cel.Type.INT)) + ) + self.assertFalse(cel.Type.INT.is_assignable_from(cel.Type.DYN)) + self.assertTrue(cel.Type("~T").is_assignable_from(cel.Type("~T"))) + self.assertFalse(cel.Type("~T").is_assignable_from(cel.Type("~U"))) + self.assertFalse( + cel.Type.TYPE.is_assignable_from( + cel.Type.AbstractType("optional_type", [cel.Type.INT]) + ) + ) + self.assertTrue( + cel.Type.AbstractType( + "optional_type", [cel.Type.INT] + ).is_assignable_from( + cel.Type.AbstractType("optional_type", [cel.Type.INT]) + ) + ) + self.assertFalse( + cel.Type.AbstractType( + "optional_type", [cel.Type.INT] + ).is_assignable_from( + cel.Type.AbstractType("optional_type", [cel.Type.STRING]) + ) + ) + self.assertFalse( + cel.Type.AbstractType( + "optional_type", [cel.Type.INT] + ).is_assignable_from( + cel.Type.AbstractType("other_type", [cel.Type.INT]) + ) + ) def testCelExpressionPersistence_checkedExpr(self): expr: cel.Expression = self.env.compile("var_msg.single_string") @@ -1064,6 +1218,19 @@ def testProtoMessageToCelValueError(self): self.assertEqual(res.type(), cel.Type.ERROR) self.assertIn("Custom pool error", str(res.value())) + def testTypeToCelValueError(self): + bad_env = cel.NewEnv( + _RaisingDescriptorPool(), + variables={"var_type": cel.Type.TYPE}, + options=self.options, + ) + expr = bad_env.compile("var_type", disable_check=True) + res = expr.eval( + data={"var_type": cel.Type("cel.expr.conformance.proto2.TestSomeTypes")} + ) + self.assertEqual(res.type(), cel.Type.ERROR) + self.assertIn("Custom pool error", str(res.value())) + def testValueOutlivesExpression(self): expr: cel.Expression = self.env.compile("'hello ' + 'world'") val: cel.Value = expr.eval() diff --git a/cel_expr_python/leak_check.py b/cel_expr_python/leak_check.py new file mode 100644 index 0000000..b8c4ea5 --- /dev/null +++ b/cel_expr_python/leak_check.py @@ -0,0 +1,137 @@ +# Copyright 2026 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Reference and memory leak detection helpers for CEL Python tests.""" + +import array +import collections.abc +import functools +import gc +import sys +import tracemalloc +from typing import Any, TypeVar + +_DEFAULT_WARMUPS = 2 +_DEFAULT_RUNS = 3 + +_F = TypeVar("_F", bound=collections.abc.Callable[..., Any]) +_T = TypeVar("_T", bound=type) + + +def _live_blocks() -> int: + """Returns the allocated Python memory block count after clearing caches.""" + # Clear CPython's internal caches (e.g., the type attribute lookup cache) + # before sampling so one-time cached entries are not mistaken for leaks. + clear_caches = getattr(sys, "_clear_internal_caches", None) or getattr( + sys, "_clear_type_cache", None + ) + if clear_caches is not None: + clear_caches() + gc.collect() + return sys.getallocatedblocks() + + +def _tracemalloc_report( + run_once: collections.abc.Callable[[], None], + iterations: int = _DEFAULT_RUNS, + limit: int = 5, +) -> str: + """Re-runs `run_once` under tracemalloc and formats top growing sites.""" + was_tracing = tracemalloc.is_tracing() + if not was_tracing: + tracemalloc.start(10) + try: + run_once() + _live_blocks() + before = tracemalloc.take_snapshot() + for _ in range(iterations): + run_once() + _live_blocks() + after = tracemalloc.take_snapshot() + finally: + if not was_tracing: + tracemalloc.stop() + ignore = [tracemalloc.Filter(False, "")] + if tracemalloc.__file__: + ignore.append(tracemalloc.Filter(False, tracemalloc.__file__)) + stats = after.filter_traces(ignore).compare_to( + before.filter_traces(ignore), "traceback" + ) + lines = [] + sites = 0 + for stat in stats: + if stat.count_diff <= 0: + continue + lines.append( + f" {stat.count_diff:+d} blocks ({stat.size_diff:+d} B) allocated at:" + ) + lines.extend( + " " + line for line in stat.traceback.format(most_recent_first=True) + ) + sites += 1 + if sites >= limit: + break + return "\n".join(lines) or " (no growth attributed by tracemalloc)" + + +def assert_no_leaks( + test_method: _F, + *, + warmups: int = _DEFAULT_WARMUPS, + runs: int = _DEFAULT_RUNS, +) -> _F: + """Wraps a `unittest.TestCase` method to fail if live blocks grow every run. + + Uses CPython's `regrtest -R` strategy: runs `warmups` unmeasured iterations + to populate lazy process-wide caches, then `runs` measured iterations, + failing only if `sys.getallocatedblocks()` increases on every measured run. + + Args: + test_method: The test method to wrap. + warmups: Number of warm-up repetitions before measuring. + runs: Number of measured repetitions. + + Returns: + The wrapped test method. + """ + + @functools.wraps(test_method) + def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any: + total_runs = warmups + runs + # Store samples in a C array so recording them keeps no Python ints alive. + blocks = array.array("q", [0]) * (total_runs + 1) + blocks[0] = _live_blocks() + for i in range(total_runs): + test_method(self, *args, **kwargs) + blocks[i + 1] = _live_blocks() + deltas = [blocks[i + 1] - blocks[i] for i in range(warmups, total_runs)] + if all(d > 0 for d in deltas): + self.fail( + f"Memory blocks grew on every one of {runs} repetitions" + f" (after {warmups} warm-up runs): deltas={deltas}." + " Top growing allocation sites:\n" + + _tracemalloc_report( + lambda: test_method(self, *args, **kwargs), iterations=runs + ) + ) + + return wrapper # type: ignore[return-value] + + +def leak_checked(cls: _T) -> _T: + """Class decorator wrapping every `test*` method with `assert_no_leaks`.""" + for name, attr in list(cls.__dict__.items()): + if name.startswith("test") and callable(attr): + setattr(cls, name, assert_no_leaks(attr)) + return cls diff --git a/cel_expr_python/py_cel_activation.cc b/cel_expr_python/py_cel_activation.cc index ef7edd2..538ef49 100644 --- a/cel_expr_python/py_cel_activation.cc +++ b/cel_expr_python/py_cel_activation.cc @@ -67,7 +67,12 @@ PyCelActivation::PyCelActivation( for (const auto& function : functions) { std::vector parameters; for (const auto& parameter : function->parameters()) { - parameters.push_back(parameter.GetKind()); + if (parameter.GetKind() == cel::Kind::kDyn || + parameter.GetKind() == cel::Kind::kTypeParam) { + parameters.push_back(cel::Kind::kAny); + } else { + parameters.push_back(parameter.GetKind()); + } } cel::FunctionDescriptor func_descriptor(function->function_name(), function->is_member(), parameters, diff --git a/cel_expr_python/py_cel_env_internal.cc b/cel_expr_python/py_cel_env_internal.cc index 7983bfe..f2dab0d 100644 --- a/cel_expr_python/py_cel_env_internal.cc +++ b/cel_expr_python/py_cel_env_internal.cc @@ -322,7 +322,11 @@ absl::StatusOr> PyCelEnvInternal::BuildRuntime( CEL_PYTHON_ASSIGN_OR_RETURN( cel::Type type, cel::TypeInfoToType(parameter, descriptor_pool_.get(), &arena)); - param_kinds.push_back(static_cast(type.kind())); + cel::Kind kind = static_cast(type.kind()); + if (kind == cel::Kind::kDyn || kind == cel::Kind::kTypeParam) { + kind = cel::Kind::kAny; + } + param_kinds.push_back(kind); } cel::FunctionDescriptor descriptor( function_config.name, overload_config.is_member_function, param_kinds, diff --git a/cel_expr_python/py_cel_function.cc b/cel_expr_python/py_cel_function.cc index f42d38c..6c5a315 100644 --- a/cel_expr_python/py_cel_function.cc +++ b/cel_expr_python/py_cel_function.cc @@ -16,6 +16,7 @@ #include +#include #include #include #include @@ -107,14 +108,34 @@ absl::StatusOr PyCelFunctionAdapter::Invoke( CEL_PYTHON_ASSIGN_OR_RETURN(auto py_arena, PyCelArena::FromProtoArena(context.arena())); PyObject* py_args = PyTuple_New(args.size()); - for (int i = 0; i < args.size(); ++i) { - PyTuple_SetItem(py_args, i, - CelValueToPyObject(args[i], env, py_arena, - /*plain_value=*/true)); + if (py_args == nullptr) { + absl::Status status = PyErr_toStatus(); + if (status.ok()) { + status = absl::ResourceExhaustedError("Failed to allocate args tuple"); + } + return cel::ErrorValue::From(status, context.arena()); + } + for (size_t i = 0; i < args.size(); ++i) { + PyObject* py_arg = CelValueToPyObject(args[i], env, py_arena, + /*plain_value=*/true); + if (py_arg == nullptr) { + absl::Status status = PyErr_toStatus(); + Py_DECREF(py_args); + if (status.ok()) { + status = absl::InternalError( + "Failed to convert CEL argument to Python object"); + } + return cel::ErrorValue::From(status, context.arena()); + } + PyTuple_SetItem(py_args, i, py_arg); } PyObject* result = PyObject_CallObject(py_function_, py_args); - Py_DECREF(py_args); absl::Status status = PyErr_toStatus(); + Py_DECREF(py_args); + if (status.ok() && result == nullptr) { + status = absl::InternalError( + "Python function returned null without setting an error"); + } if (!status.ok()) { Py_XDECREF(result); return cel::ErrorValue::From(status, context.arena()); @@ -123,8 +144,15 @@ absl::StatusOr PyCelFunctionAdapter::Invoke( absl::StatusOr cel_result = PyObjectToCelValue( result, return_type_, [this]() { - return absl::StrFormat("Python function '%s'", - PyUnicode_AsUTF8(PyObject_Repr(py_function_))); + PyObject* repr = PyObject_Repr(py_function_); + const char* repr_cstr = + repr != nullptr ? PyUnicode_AsUTF8(repr) : nullptr; + if (repr_cstr == nullptr) { + PyErr_Clear(); + } + std::string repr_str = repr_cstr != nullptr ? repr_cstr : ""; + Py_XDECREF(repr); + return absl::StrFormat("Python function '%s'", repr_str); }, env, context.arena()); Py_XDECREF(result); diff --git a/cel_expr_python/py_cel_python_extension.cc b/cel_expr_python/py_cel_python_extension.cc index 6ff92ac..50019fc 100644 --- a/cel_expr_python/py_cel_python_extension.cc +++ b/cel_expr_python/py_cel_python_extension.cc @@ -117,7 +117,8 @@ absl::Status PyCelPythonExtension::ConfigureRuntime( std::vector types; types.reserve(overload.parameters().size()); for (const PyCelType& arg : overload.parameters()) { - if (arg.GetKind() == cel::Kind::kDyn) { + if (arg.GetKind() == cel::Kind::kDyn || + arg.GetKind() == cel::Kind::kTypeParam) { // C++ runtime dispatcher historically uses kAny for wildcard type ( // not distinguishing between dyn and any) types.push_back(cel::Kind::kAny); diff --git a/cel_expr_python/py_cel_type.cc b/cel_expr_python/py_cel_type.cc index eb4bf9a..2f666ae 100644 --- a/cel_expr_python/py_cel_type.cc +++ b/cel_expr_python/py_cel_type.cc @@ -14,14 +14,17 @@ #include "cel_expr_python/py_cel_type.h" -#include // IWYU pragma: keep - Needed for PyObject +#include // IWYU pragma: keep - Needed for PyObject +#include // IWYU pragma: keep +#include #include #include #include #include #include +#include "absl/base/call_once.h" #include "absl/base/no_destructor.h" #include "absl/container/flat_hash_map.h" #include "absl/log/absl_check.h" @@ -137,7 +140,7 @@ PyCelType PyCelType::TypeType(const PyCelType& parameter) { PyCelType PyCelType::AbstractType(const std::string& name, const std::vector& params) { - return PyCelType(cel::Kind::kType, name, params); + return PyCelType(cel::Kind::kOpaque, name, params); } bool PyCelType::operator==(const PyCelType& other) const { @@ -146,14 +149,22 @@ bool PyCelType::operator==(const PyCelType& other) const { } bool PyCelType::IsAssignableFrom(const PyCelType& other) const { + if (kind_ == cel::Kind::kDyn) { + return true; + } + if (kind_ != other.kind_) { return false; } - if (kind_ == cel::Kind::kMessage) { + if (kind_ == cel::Kind::kMessage || kind_ == cel::Kind::kTypeParam) { return name_ == other.name_; } + if (kind_ == cel::Kind::kOpaque && name_ != other.name_) { + return false; + } + if (parameters_.empty()) { return true; } @@ -162,7 +173,7 @@ bool PyCelType::IsAssignableFrom(const PyCelType& other) const { return false; } - for (int i = 0; i < parameters_.size(); ++i) { + for (size_t i = 0; i < parameters_.size(); ++i) { if (!parameters_[i].IsAssignableFrom(other.parameters_[i])) { return false; } @@ -336,6 +347,9 @@ PyCelType PyCelType::FromCelType(const cel::Type& cel_type) { } case cel::TypeKind::kDyn: return PyCelType::Dyn(); + case cel::TypeKind::kTypeParam: + return PyCelType(cel::Kind::kTypeParam, + std::string(cel_type.AsTypeParam()->name())); case cel::TypeKind::kOpaque: { const auto& abstract_type = cel_type.AsOpaque(); std::vector params; @@ -401,6 +415,11 @@ PyCelType PyCelType::ForCelValue(const cel::Value& cel_value) { } } +static void EnsureDateTimeModuleImported() { + static absl::once_flag import_py_datetime_once_flag; + absl::call_once(import_py_datetime_once_flag, []() { PyDateTime_IMPORT; }); +} + absl::StatusOr PyCelType::ForPyObject( PyObject* py_object, const std::function& context) { ABSL_CHECK(PyGILState_Check()); @@ -450,6 +469,18 @@ absl::StatusOr PyCelType::ForPyObject( PyErr_Clear(); } + EnsureDateTimeModuleImported(); + if (PyDateTime_Check(py_object)) { + return PyCelType::Timestamp(); + } + if (PyDelta_Check(py_object)) { + return PyCelType::Duration(); + } + + if (py::isinstance(py::handle(py_object))) { + return PyCelType::Type(); + } + PyTypeObject* type = Py_TYPE(py_object); return absl::InvalidArgumentError(absl::StrFormat( "Non-CEL value type for '%s': %s.", context(), type->tp_name)); @@ -509,6 +540,8 @@ PyCelType PyCelType::FromTypeProto(const cel::expr::Type& type) { } case cel::expr::Type::kMessageType: return PyCelType(type.message_type()); + case cel::expr::Type::kTypeParam: + return PyCelType(cel::Kind::kTypeParam, type.type_param()); case cel::expr::Type::kType: { cel::expr::Type parameter = type.type(); if (parameter.type_kind_case()) { @@ -537,6 +570,9 @@ PyCelType PyCelType::FromTypeProto(const cel::expr::Type& type) { } PyCelType PyCelType::FromTypeInfo(const cel::Config::TypeInfo& type_info) { + if (type_info.is_type_param) { + return PyCelType(cel::Kind::kTypeParam, type_info.name); + } if (type_info.name == "null") return PyCelType::Null(); if (type_info.name == "bool") return PyCelType::Bool(); if (type_info.name == "int") return PyCelType::Int(); @@ -566,7 +602,7 @@ PyCelType PyCelType::FromTypeInfo(const cel::Config::TypeInfo& type_info) { } return PyCelType::TypeType(FromTypeInfo(type_info.params[0])); } - if (type_info.is_type_param || !type_info.params.empty()) { + if (!type_info.params.empty()) { std::vector params; params.reserve(type_info.params.size()); for (const auto& param : type_info.params) { @@ -581,6 +617,8 @@ absl::StatusOr PyCelType::ToCelType( const PyCelType& type, google::protobuf::Arena* arena, const google::protobuf::DescriptorPool& descriptor_pool) { switch (type.GetKind()) { + case cel::Kind::kNull: + return cel::NullType(); case cel::Kind::kBool: return cel::BoolType(); case cel::Kind::kInt: @@ -623,6 +661,32 @@ absl::StatusOr PyCelType::ToCelType( return cel::TimestampType(); case cel::Kind::kDuration: return cel::DurationType(); + case cel::Kind::kType: { + if (type.parameters_.empty()) { + return cel::TypeType(); + } + CEL_PYTHON_ASSIGN_OR_RETURN( + cel::Type param_type, + ToCelType(type.GetParam(0), arena, descriptor_pool)); + return cel::TypeType(arena, param_type); + } + case cel::Kind::kOpaque: { + std::vector params; + params.reserve(type.parameters_.size()); + for (const PyCelType& param : type.parameters_) { + CEL_PYTHON_ASSIGN_OR_RETURN(cel::Type param_type, + ToCelType(param, arena, descriptor_pool)); + params.push_back(param_type); + } + const std::string* arena_name = + google::protobuf::Arena::Create(arena, type.name_); + return cel::OpaqueType(arena, *arena_name, params); + } + case cel::Kind::kTypeParam: { + const std::string* arena_name = + google::protobuf::Arena::Create(arena, type.name_); + return cel::TypeParamType(*arena_name); + } default: return cel::DynType(); } @@ -674,9 +738,19 @@ cel::Config::TypeInfo PyCelType::ToTypeInfo(const PyCelType& type) { case cel::Kind::kDuration: type_info.name = "duration"; break; + case cel::Kind::kType: + type_info.name = "type"; + if (!type.parameters_.empty()) { + type_info.params.push_back(ToTypeInfo(type.GetParam(0))); + } + break; case cel::Kind::kDyn: type_info.name = "dyn"; break; + case cel::Kind::kTypeParam: + type_info.name = type.name_; + type_info.is_type_param = true; + break; default: type_info.name = type.GetName(); for (const PyCelType& param : type.parameters_) { diff --git a/cel_expr_python/py_cel_value.cc b/cel_expr_python/py_cel_value.cc index ca13f77..2d71b0c 100644 --- a/cel_expr_python/py_cel_value.cc +++ b/cel_expr_python/py_cel_value.cc @@ -55,6 +55,24 @@ namespace cel_python { namespace py = ::pybind11; +namespace { + +std::string PyObjectRepr(PyObject* obj) { + if (obj == nullptr) { + return ""; + } + PyObject* repr = PyObject_Repr(obj); + const char* repr_cstr = repr != nullptr ? PyUnicode_AsUTF8(repr) : nullptr; + if (repr_cstr == nullptr) { + PyErr_Clear(); + } + std::string repr_str = repr_cstr != nullptr ? repr_cstr : ""; + Py_XDECREF(repr); + return repr_str; +} + +} // namespace + void PyCelValue::DefinePythonBindings(py::module& m) { py::class_(m, "Value") .def("type", &PyCelValue::Type) @@ -461,8 +479,11 @@ PyObject* CelValueToPyObject(const cel::Value& cel_value, cel::ListValue list = *cel_value.AsList(); Py_ssize_t size = *list.Size(); PyObject* py_list = PyList_New(size); + if (py_list == nullptr) { + return nullptr; + } absl::Status status = absl::OkStatus(); - for (int i = 0; i < size; ++i) { + for (Py_ssize_t i = 0; i < size; ++i) { PyObject* py_item; if (plain_value) { absl::StatusOr item = @@ -479,6 +500,10 @@ PyObject* CelValueToPyObject(const cel::Value& cel_value, .release() .ptr(); } + if (py_item == nullptr) { + Py_DECREF(py_list); + return nullptr; + } // PyList_SetItem is special in that it "steals" the reference to the // item being set, so we must not decref it. // https://docs.python.org/3/c-api/list.html#:~:text=int-,pylist_setitem @@ -505,12 +530,16 @@ PyObject* CelValueToPyObject(const cel::Value& cel_value, } PyObject* py_map = PyDict_New(); + if (py_map == nullptr) { + return nullptr; + } Py_ssize_t size = *key_list.Size(); - for (int i = 0; i < size; ++i) { + for (Py_ssize_t i = 0; i < size; ++i) { absl::StatusOr key = key_list.Get(i, env->GetDescriptorPool(), env->GetMessageFactory(), arena->GetArena()); if (!key.ok()) { + Py_DECREF(py_map); PyErr_Format(PyExc_RuntimeError, "Cannot get key: %s", key.status().ToString().c_str()); return nullptr; @@ -518,6 +547,10 @@ PyObject* CelValueToPyObject(const cel::Value& cel_value, PyObject* py_key = CelValueToPyObject(*key, env, arena, /*plain_value=*/true); + if (py_key == nullptr) { + Py_DECREF(py_map); + return nullptr; + } PyObject* py_value; if (plain_value) { cel::Value element_value; @@ -525,9 +558,10 @@ PyObject* CelValueToPyObject(const cel::Value& cel_value, map.Get(*key, env->GetDescriptorPool(), env->GetMessageFactory(), arena->GetArena(), &element_value); if (!status.ok()) { + Py_DECREF(py_key); + Py_DECREF(py_map); PyErr_Format(PyExc_RuntimeError, "Cannot get value: %s", status.ToString().c_str()); - Py_DECREF(py_key); return nullptr; } py_value = CelValueToPyObject(element_value, env, arena, @@ -537,12 +571,21 @@ PyObject* CelValueToPyObject(const cel::Value& cel_value, .release() .ptr(); } + if (py_value == nullptr) { + Py_DECREF(py_key); + Py_DECREF(py_map); + return nullptr; + } // In contrast to PyList_SetItem, PyDict_SetItem does not "steal" the // reference to the key and value being set, so we must decref them. // https://docs.python.org/3/c-api/dict.html#:~:text=int-,pydict_setitem - PyDict_SetItem(py_map, py_key, py_value); + int set_status = PyDict_SetItem(py_map, py_key, py_value); Py_DECREF(py_key); Py_DECREF(py_value); + if (set_status < 0) { + Py_DECREF(py_map); + return nullptr; + } } return py_map; } @@ -674,11 +717,13 @@ static cel::Value ProtoMessageToCelValue( return *wrapped_message; } +// This should be called with the GIL held. absl::StatusOr PyObjectToCelValue( PyObject* py_object, const PyCelType& expected_type, absl::FunctionRef context, const std::shared_ptr& env, google::protobuf::Arena* arena, bool bypass_type_check) { + ABSL_CHECK(PyGILState_Check()); if (!py_object) { return cel::ErrorValue::From( absl::InvalidArgumentError( @@ -688,7 +733,8 @@ absl::StatusOr PyObjectToCelValue( } switch (expected_type.GetKind()) { - case cel::Kind::kDyn: { + case cel::Kind::kDyn: + case cel::Kind::kTypeParam: { CEL_PYTHON_ASSIGN_OR_RETURN(const PyCelType& type, PyCelType::ForPyObject(py_object, context)); return PyObjectToCelValue(py_object, type, context, env, arena, @@ -767,9 +813,9 @@ absl::StatusOr PyObjectToCelValue( time = py::cast(py_object); } catch (const py::cast_error& e) { PyErr_Clear(); - return absl::UnknownError(absl::StrFormat( - "Cannot convert %s to absl::Time: %s", - PyUnicode_AsUTF8(PyObject_Repr(py_object)), e.what())); + return absl::UnknownError( + absl::StrFormat("Cannot convert %s to absl::Time: %s", + PyObjectRepr(py_object), e.what())); } absl::StatusOr timestamp_value = cel::SafeTimestampValue(time); @@ -808,7 +854,7 @@ absl::StatusOr PyObjectToCelValue( PyErr_Clear(); return absl::UnknownError( absl::StrFormat("Cannot convert %s to cel.Type.TIMESTAMP", - PyUnicode_AsUTF8(PyObject_Repr(py_object)))); + PyObjectRepr(py_object))); } absl::Time time = absl::FromUnixSeconds(seconds) + absl::Nanoseconds(nanos); @@ -830,9 +876,9 @@ absl::StatusOr PyObjectToCelValue( duration = py::cast(py_object); } catch (const py::cast_error& e) { PyErr_Clear(); - return absl::UnknownError(absl::StrFormat( - "Cannot convert %s to absl::Duration: %s", - PyUnicode_AsUTF8(PyObject_Repr(py_object)), e.what())); + return absl::UnknownError( + absl::StrFormat("Cannot convert %s to absl::Duration: %s", + PyObjectRepr(py_object), e.what())); } absl::StatusOr duration_value = cel::SafeDurationValue(duration); @@ -869,9 +915,8 @@ absl::StatusOr PyObjectToCelValue( Py_XDECREF(py_nanos); if (!ok) { PyErr_Clear(); - return absl::UnknownError( - absl::StrFormat("Cannot convert %s to absl::Duration", - PyUnicode_AsUTF8(PyObject_Repr(py_object)))); + return absl::UnknownError(absl::StrFormat( + "Cannot convert %s to absl::Duration", PyObjectRepr(py_object))); } absl::Duration duration = absl::Seconds(seconds) + absl::Nanoseconds(nanos); @@ -953,28 +998,26 @@ absl::StatusOr PyObjectToCelValue( while (PyDict_Next(py_object, &pos, &key, &value)) { // Note: PyDict_Next returns borrowed references, so we shouldn't // DECREF them. - CEL_PYTHON_ASSIGN_OR_RETURN( - cel::Value converted_key, - PyObjectToCelValue( - key, key_type, - [context, key]() { - return absl::StrFormat( - "%s[%s]", context(), - PyUnicode_AsUTF8(PyObject_Repr(key))); - }, - env, arena)); - - CEL_PYTHON_ASSIGN_OR_RETURN( - cel::Value converted_value, - PyObjectToCelValue( - value, value_type, - [context, key, value]() { - return absl::StrFormat( - "%s[%s] -> %s", context(), - PyUnicode_AsUTF8(PyObject_Repr(key)), - PyUnicode_AsUTF8(PyObject_Repr(value))); - }, - env, arena)); + CEL_PYTHON_ASSIGN_OR_RETURN(cel::Value converted_key, + PyObjectToCelValue( + key, key_type, + [context, key]() { + return absl::StrFormat( + "%s[%s]", context(), + PyObjectRepr(key)); + }, + env, arena)); + + CEL_PYTHON_ASSIGN_OR_RETURN(cel::Value converted_value, + PyObjectToCelValue( + value, value_type, + [context, key, value]() { + return absl::StrFormat( + "%s[%s] -> %s", context(), + PyObjectRepr(key), + PyObjectRepr(value)); + }, + env, arena)); CEL_PYTHON_RETURN_IF_ERROR( builder->Put(converted_key, converted_value)); } @@ -990,6 +1033,30 @@ absl::StatusOr PyObjectToCelValue( } return InvalidTypeError(py_object, context, expected_type, arena); } + case cel::Kind::kType: { + if (py::isinstance(py::handle(py_object))) { + // Copy PyCelType while holding the GIL so it remains self-contained + // across GIL release. + PyCelType py_type = py::cast(py_object); + absl::StatusOr cel_type; + { + // Release the GIL before calling into C++ DescriptorPool to maintain + // the lock hierarchy (DescriptorPool Mutex -> Python GIL). + // PyCelType::ToCelType may resolve message types via DescriptorPool, + // which on a cache miss calls back into Python via + // PyDescriptorDatabase (re-acquiring the GIL on demand). + py::gil_scoped_release gil_release; + cel_type = + PyCelType::ToCelType(py_type, arena, *env->GetDescriptorPool()); + } + // The GIL is re-acquired here. Check if descriptor lookups raised a + // Python exception in PyDescriptorDatabase. + CEL_PYTHON_RETURN_IF_ERROR(PyErr_toStatus()); + CEL_PYTHON_RETURN_IF_ERROR(cel_type.status()); + return cel::TypeValue(*cel_type); + } + return InvalidTypeError(py_object, context, expected_type, arena); + } default: { return absl::UnimplementedError( absl::StrFormat("Unimplemented type: %d", expected_type.GetKind())); diff --git a/cel_expr_python/py_cel_value.h b/cel_expr_python/py_cel_value.h index f82d3cc..6bdbc35 100644 --- a/cel_expr_python/py_cel_value.h +++ b/cel_expr_python/py_cel_value.h @@ -146,11 +146,13 @@ class PyCelMapItemAccessor : public PyCelValue { bool resolved_ ABSL_GUARDED_BY(mutex_) = false; }; +// This should be called with the GIL held. PyObject* CelValueToPyObject(const cel::Value& cel_value, const std::shared_ptr& env, const std::shared_ptr& arena, bool plain_value); +// This should be called with the GIL held. absl::StatusOr PyObjectToCelValue( PyObject* py_object, const PyCelType& expected_type, absl::FunctionRef context, diff --git a/cel_expr_python/py_message_factory.cc b/cel_expr_python/py_message_factory.cc index 080b33b..b2d96f0 100644 --- a/cel_expr_python/py_message_factory.cc +++ b/cel_expr_python/py_message_factory.cc @@ -43,11 +43,11 @@ PyMessageFactory::PyMessageFactory(PyObject* descriptor_pool) { ABSL_LOG(FATAL) << "Cannot load module 'google.protobuf.message_factory'"; } py_func_GetMessageClass_ = PyObject_GetAttrString(pModule, "GetMessageClass"); + Py_DECREF(pModule); if (!py_func_GetMessageClass_) { ABSL_LOG(FATAL) << "Cannot find function " "'google.protobuf.message_factory.GetMessageClass'"; } - Py_INCREF(py_func_GetMessageClass_); py_func_MergeFromString_ = PyUnicode_FromString("MergeFromString"); } @@ -146,12 +146,12 @@ PyObject* PyMessageFactory::FromString(const std::string& message_type, PyObject* ret = PyObject_CallMethodObjArgs(message, py_func_MergeFromString_, serialized_proto_py, nullptr); + Py_DECREF(serialized_proto_py); if (!ret) { Py_DECREF(message); - Py_DECREF(serialized_proto_py); return nullptr; } - Py_DECREF(serialized_proto_py); + Py_DECREF(ret); return message; } diff --git a/release/build_wheel.sh b/release/build_wheel.sh index eeeef3f..6b45fb8 100755 --- a/release/build_wheel.sh +++ b/release/build_wheel.sh @@ -69,7 +69,7 @@ if [[ "$OSTYPE" == "darwin"* ]]; then else cp -r "${SRC_DIR}/." . fi -rm -rf cel_expr_python/*_test.py +rm -rf cel_expr_python/*_test.py cel_expr_python/leak_check.py PYTHON_BIN="python" if command -v python3 &> /dev/null; then diff --git a/release/cel_basic_test.py b/release/cel_basic_test.py index 7295243..902abea 100644 --- a/release/cel_basic_test.py +++ b/release/cel_basic_test.py @@ -192,6 +192,7 @@ def test_wheel_package_contents(self): ("cel_expr_python/", "cel_expr_python-", "cel_expr_python.libs/") ) or f.endswith("_test.py") + or f == "cel_expr_python/leak_check.py" or "/testdata/" in f ] self.assertEqual( diff --git a/release/kokoro/release_linux.sh b/release/kokoro/release_linux.sh index 08bfe0c..3ff4c95 100755 --- a/release/kokoro/release_linux.sh +++ b/release/kokoro/release_linux.sh @@ -243,7 +243,7 @@ pushd "${TMP_DIR}" cp -r "${SRC_DIR}"/{*,.*} . 2>/dev/null || true cp "${SRC_DIR}"/release/*.py "${SRC_DIR}"/release/*.toml . 2>/dev/null || true -rm -rf cel_expr_python/*_test.py +rm -rf cel_expr_python/*_test.py cel_expr_python/leak_check.py echo "Downloading bazelisk on host..." curl -LO https://github.com/bazelbuild/bazelisk/releases/download/v1.19.0/bazelisk-linux-amd64 diff --git a/release/kokoro/release_macos.sh b/release/kokoro/release_macos.sh index e36b555..52c1dd3 100644 --- a/release/kokoro/release_macos.sh +++ b/release/kokoro/release_macos.sh @@ -58,7 +58,7 @@ pushd "${TMP_DIR}" cp -r "${SRC_DIR}"/{*,.*} . 2>/dev/null || true cp "${SRC_DIR}"/release/*.py "${SRC_DIR}"/release/*.toml . 2>/dev/null || true -rm -rf cel_expr_python/*_test.py +rm -rf cel_expr_python/*_test.py cel_expr_python/leak_check.py # Check if pyproject.toml exists before running sed if [ -f pyproject.toml ]; then diff --git a/release/kokoro/release_windows.bat b/release/kokoro/release_windows.bat index 2931936..f0d7a37 100644 --- a/release/kokoro/release_windows.bat +++ b/release/kokoro/release_windows.bat @@ -131,6 +131,9 @@ if !ERRORLEVEL! NEQ 0 ( if exist "cel_expr_python\*_test.py" ( del /Q "cel_expr_python\*_test.py" ) +if exist "cel_expr_python\leak_check.py" ( + del /Q "cel_expr_python\leak_check.py" +) :: Substitute $VERSION in pyproject.toml with the value of VERSION. !PYTHON_EXE! -c "import sys; content = open('pyproject.toml').read(); open('pyproject.toml', 'w').write(content.replace('$VERSION', sys.argv[1]))" "%VERSION%" diff --git a/release/pyproject.toml b/release/pyproject.toml index 3b54d9e..f704343 100644 --- a/release/pyproject.toml +++ b/release/pyproject.toml @@ -42,7 +42,7 @@ exclude = ["*testdata*"] "*" = ["*.pyi"] [tool.setuptools.exclude-package-data] -"*" = ["*_test.py"] +"*" = ["*_test.py", "leak_check.py"] [tool.cibuildwheel] build = "cp310-* cp311-* cp312-* cp313-* cp314-*"