Skip to content

Commit

Permalink
Introduce clif.python.abc_utils with abc_utils.PyCLIFABCMeta.
Browse files Browse the repository at this point in the history
This CL makes it possible to choose between `_USE_PYTYPE_TYPE_AS_METACLASS` `True` or `False` in clif/pybind11/classes.py.

**TO BE DETERMINED:** Choice of metaclass when rolling out PyCLIF-pybind11.

Setting `_USE_PYTYPE_TYPE_AS_METACLASS = True` in this CL, for compatibility with PyCLIF-C-API.

TGP-tested **fresh** with `_USE_PYTYPE_TYPE_AS_METACLASS = False` and in combination with child CLs cl/603786594 (mox.py) and cl/603757382 (sprawling changes):

* http://tap/OCL:603885130:BASE:604107736:1707063723806:7ef9954a

TGP **rerun** with `_USE_PYTYPE_TYPE_AS_METACLASS = True`:

* http://tap/OCL:603885130:BASE:604107736:1707077805857:f8d8b6a3 (25 passing)

* b/287289622#comment59 shows the 25 passing targets

Additional manual testing with `_USE_PYTYPE_TYPE_AS_METACLASS = False`:

```
blaze test third_party/clif/testing/python:classes_test --//devtools/clif/python:pyclif_codegen_mode=pybind11
```

* http://sponge2/57affbde-047e-42d2-a04c-2acea2c5430c

PiperOrigin-RevId: 605333960
  • Loading branch information
rwgk committed Feb 19, 2024
1 parent d7a2a75 commit c5bcd50
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 1 deletion.
6 changes: 5 additions & 1 deletion clif/pybind11/classes.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

I = utils.I

_USE_PYTYPE_TYPE_AS_METACLASS = True

_KNOWN_TYPES_WITH_MULTIPLE_INHERITANCE = {
'::borg::Config': ['::borg::ConfigArgs']
}
Expand Down Expand Up @@ -100,6 +102,8 @@ def generate_from(
base.cpp_canonical_type in codegen_info.dynamic_attr_types):
enable_instance_dict = True
break
if _USE_PYTYPE_TYPE_AS_METACLASS:
definition += ', py::metaclass((PyObject*) &PyType_Type)'
definition += ', py::release_gil_before_calling_cpp_dtor()'
if mi_bases:
definition += ', py::multiple_inheritance()'
Expand All @@ -117,7 +121,7 @@ def generate_from(
for member in class_decl.members:
if member.decltype == ast_pb2.Decl.Type.CONST:
for s in consts.generate_from(class_name, member.const):
yield I + I + s
yield I + s
elif member.decltype == ast_pb2.Decl.Type.FUNC:
if member.func.name.native in ('__reduce__', '__reduce_ex__'):
reduce_or_reduce_ex_defined = True
Expand Down
23 changes: 23 additions & 0 deletions clif/python/BUILD
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# CLIF python frontend

load("@clif_python_deps//:requirements.bzl", "requirement")
load("//devtools/clif/python:clif_build_rule.bzl", "py_clif_cc")
load("//third_party/bazel_rules/rules_python/python:py_test.bzl", "py_test")

package(
Expand Down Expand Up @@ -412,3 +413,25 @@ py_library(
srcs_version = "PY2AND3",
visibility = ["//visibility:public"],
)

cc_library(
name = "meta_ext_lib",
hdrs = ["meta_ext.h"],
)

py_clif_cc(
name = "_meta_ext",
srcs = ["meta_ext.clif"],
deps = [
":meta_ext_lib",
],
)

py_library(
name = "abc_utils",
srcs = ["abc_utils.py"],
visibility = ["//visibility:public"],
deps = [
":_meta_ext",
],
)
26 changes: 26 additions & 0 deletions clif/python/abc_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
"""PyCLIF-pybind11 abc compatibility (in particular abc.ABCMeta).
This is for compatibility between the "default pybind11 metaclass"
(which is a custom metaclass) and https://docs.python.org/3/library/abc.html.
For background see:
* Description of https://github.com/pybind/pybind11/pull/5015
* Corresponding tests in clif/testing/python/classes_test.py
"""

import abc
import typing

from clif.python import _meta_ext

PyCLIFMeta = type(_meta_ext.empty)


if typing.TYPE_CHECKING:
PyCLIFABCMeta = abc.ABCMeta
else:

class PyCLIFABCMeta(abc.ABCMeta, PyCLIFMeta):
pass
4 changes: 4 additions & 0 deletions clif/python/meta_ext.clif
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
from "clif/python/meta_ext.h":
namespace `clif_meta_ext`:
class empty:
pass
10 changes: 10 additions & 0 deletions clif/python/meta_ext.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#ifndef CLIF_PYTHON_META_EXT_H_
#define CLIF_PYTHON_META_EXT_H_

namespace clif_meta_ext {

struct empty {};

} // namespace clif_meta_ext

#endif // CLIF_PYTHON_META_EXT_H_
52 changes: 52 additions & 0 deletions clif/testing/python/classes_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@

"""Tests for clif.testing.python.classes."""

import abc
from unittest import mock

from absl.testing import absltest
from absl.testing import parameterized

from clif.python import abc_utils
from clif.testing.python import classes


Expand All @@ -35,6 +37,20 @@ def testKlass(self):
# AttributeError on CPython; TypeError on PyPy.
with self.assertRaises((AttributeError, TypeError)):
k.i2 = 0
self.assertEqual(classes.Klass.C, 1)
self.assertEqual(k.C, 1)
with self.assertRaisesRegex(AttributeError, 'read-only'):
k.C = 0

# UNDESIRABLE but long established behavior.
classes.Klass.C = 0 # C++ const but not read-only in Python.
self.assertEqual(classes.Klass.C, 0)
self.assertEqual(k.C, 0)
# Restore original value, to minimize the potential for surprises,
# just in case.
classes.Klass.C = 1
self.assertEqual(classes.Klass.C, 1)
self.assertEqual(k.C, 1)

def testMockIsRejected(self):
k_inst = classes.Klass(3)
Expand Down Expand Up @@ -212,6 +228,42 @@ def testNestedUnproperty(self, attr, expected, new_value, new_expected):
ret = getattr(obj, getter)()
self.assertEqual(ret, new_expected)

def testABCMeta(self):
# Purely to help the linter.
ABCMeta = abc.ABCMeta # pylint: disable=unused-variable

if abc_utils.PyCLIFMeta is type:

class KlassABCMeta(classes.Klass, metaclass=ABCMeta):
pass

self.assertEqual(type(classes.Klass).__name__, 'type')
km = KlassABCMeta(5)
self.assertEqual(km.i, 5)

else:
self.assertEqual(classes.__pyclif_codegen_mode__, 'pybind11')
with self.assertRaisesRegex(TypeError, 'metaclass conflict'):

class KlassABCMeta(classes.Klass, metaclass=ABCMeta):
pass

def testPyCLIFABCMeta(self):
# Purely to help the linter.
PyCLIFABCMeta = abc_utils.PyCLIFABCMeta # pylint: disable=unused-variable,invalid-name

class KlassPyCLIFABCMeta(classes.Klass, metaclass=PyCLIFABCMeta):
pass

if abc_utils.PyCLIFMeta is type:
expected_type_name = 'type'
else:
self.assertEqual(classes.__pyclif_codegen_mode__, 'pybind11')
expected_type_name = 'pybind11_type'
self.assertEqual(type(classes.Klass).__name__, expected_type_name)
km = KlassPyCLIFABCMeta(5)
self.assertEqual(km.i, 5)


if __name__ == '__main__':
absltest.main()

0 comments on commit c5bcd50

Please sign in to comment.