Skip to content

Commit 002caaf

Browse files
committed
[cppyy] Only alias size() to __len__ for container-like classes
Guard the automatic size() -> __len__ pythonization to require that size() returns an integer type and the class has begin()/end() methods or operator[]. This prevents bool() returning False for valid objects whose size() returns non-integer types like std::optional<std::size_t>. Use Cppyy::IsIntegerType (backed by Clang's QualType) to check the return type, which correctly resolves typedefs like size_type. Walk the MRO when checking for size/begin/end/getitem attributes, since HasAttrDirect only checks the class's own __dict__. Skip pythonization if size() has multiple overloads. Update tests for stl_like_class2/3 which have incomplete container interfaces (missing iterators or returning non-iterator types from begin/end).
1 parent d0566ab commit 002caaf

8 files changed

Lines changed: 131 additions & 25 deletions

File tree

bindings/pyroot/cppyy/CPyCppyy/src/Cppyy.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,8 @@ namespace Cppyy {
139139
CPPYY_IMPORT
140140
bool IsAggregate(TCppType_t type);
141141
CPPYY_IMPORT
142+
bool IsIntegerType(const std::string &type_name);
143+
CPPYY_IMPORT
142144
bool IsDefaultConstructable(TCppType_t type);
143145

144146
CPPYY_IMPORT

bindings/pyroot/cppyy/CPyCppyy/src/Pythonize.cxx

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,19 @@ bool HasAttrDirect(PyObject* pyclass, PyObject* pyname, bool mustBeCPyCppyy = fa
5252
return false;
5353
}
5454

55+
bool HasAttrInMRO(PyObject *pyclass, PyObject *pyname)
56+
{
57+
// Check base classes in the MRO (skipping the class itself) for a CPyCppyy overload.
58+
PyObject *mro = ((PyTypeObject *)pyclass)->tp_mro;
59+
if (mro && PyTuple_Check(mro)) {
60+
for (Py_ssize_t i = 1; i < PyTuple_GET_SIZE(mro); ++i) {
61+
if (HasAttrDirect(PyTuple_GET_ITEM(mro, i), pyname, /*mustBeCPyCppyy=*/true))
62+
return true;
63+
}
64+
}
65+
return false;
66+
}
67+
5568
PyObject* GetAttrDirect(PyObject* pyclass, PyObject* pyname) {
5669
// get an attribute without causing getattr lookups
5770
PyObject* dct = PyObject_GetAttr(pyclass, PyStrings::gDict);
@@ -1755,12 +1768,36 @@ bool CPyCppyy::Pythonize(PyObject* pyclass, const std::string& name)
17551768
Utility::AddToClass(pyclass, pybool_name, (PyCFunction)NullCheckBool, METH_NOARGS);
17561769
}
17571770

1758-
// for STL containers, and user classes modeled after them
1759-
// the attribute must be a CPyCppyy overload, otherwise the check gives false
1760-
// positives in the case where the class has a non-function attribute that is
1761-
// called "size".
1762-
if (HasAttrDirect(pyclass, PyStrings::gSize, /*mustBeCPyCppyy=*/ true)) {
1763-
Utility::AddToClass(pyclass, "__len__", "size");
1771+
// for STL containers, and user classes modeled after them. Guard the alias to
1772+
// __len__ by verifying that size() returns an integer type and the class has
1773+
// begin()/end() or operator[] (i.e. is container-like). This prevents bool()
1774+
// returning False for valid objects whose size() returns non-integer types like
1775+
// std::optional<std::size_t>. Skip if size() has multiple overloads, as that
1776+
// indicates it is not the simple container-style size() one would map to __len__.
1777+
if (HasAttrDirect(pyclass, PyStrings::gSize, /*mustBeCPyCppyy=*/true) || HasAttrInMRO(pyclass, PyStrings::gSize)) {
1778+
bool sizeIsInteger = false;
1779+
PyObject *pySizeMethod = PyObject_GetAttr(pyclass, PyStrings::gSize);
1780+
if (pySizeMethod && CPPOverload_Check(pySizeMethod)) {
1781+
auto *ol = (CPPOverload *)pySizeMethod;
1782+
if (ol->HasMethods() && ol->fMethodInfo->fMethods.size() == 1) {
1783+
PyObject *pyrestype =
1784+
ol->fMethodInfo->fMethods[0]->Reflex(Cppyy::Reflex::RETURN_TYPE, Cppyy::Reflex::AS_STRING);
1785+
if (pyrestype) {
1786+
sizeIsInteger = Cppyy::IsIntegerType(CPyCppyy_PyText_AsString(pyrestype));
1787+
Py_DECREF(pyrestype);
1788+
}
1789+
}
1790+
}
1791+
Py_XDECREF(pySizeMethod);
1792+
1793+
if (sizeIsInteger) {
1794+
bool hasIterators = (HasAttrDirect(pyclass, PyStrings::gBegin) || HasAttrInMRO(pyclass, PyStrings::gBegin)) &&
1795+
(HasAttrDirect(pyclass, PyStrings::gEnd) || HasAttrInMRO(pyclass, PyStrings::gEnd));
1796+
bool hasSubscript = HasAttrDirect(pyclass, PyStrings::gGetItem) || HasAttrInMRO(pyclass, PyStrings::gGetItem);
1797+
if (hasIterators || hasSubscript) {
1798+
Utility::AddToClass(pyclass, "__len__", "size");
1799+
}
1800+
}
17641801
}
17651802

17661803
if (HasAttrDirect(pyclass, PyStrings::gContains)) {

bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/clingwrapper.cxx

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1247,6 +1247,18 @@ bool Cppyy::IsAggregate(TCppType_t type)
12471247
return false;
12481248
}
12491249

1250+
bool Cppyy::IsIntegerType(const std::string &type_name)
1251+
{
1252+
// Test if the named type is an integer type
1253+
TypeInfo_t *ti = gInterpreter->TypeInfo_Factory(type_name.c_str());
1254+
if (!ti)
1255+
return false;
1256+
void *qtp = gInterpreter->TypeInfo_QualTypePtr(ti);
1257+
bool result = qtp ? gInterpreter->IsIntegerType(qtp) : false;
1258+
gInterpreter->TypeInfo_Delete(ti);
1259+
return result;
1260+
}
1261+
12501262
bool Cppyy::IsDefaultConstructable(TCppType_t type)
12511263
{
12521264
// Test if this type has a default constructor or is a "plain old data" type

bindings/pyroot/cppyy/cppyy-backend/clingwrapper/src/cpp_cppyy.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,8 @@ namespace Cppyy {
136136
RPY_EXPORTED
137137
bool IsAggregate(TCppType_t type);
138138
RPY_EXPORTED
139+
bool IsIntegerType(const std::string &type_name);
140+
RPY_EXPORTED
139141
bool IsDefaultConstructable(TCppType_t type);
140142

141143
RPY_EXPORTED

bindings/pyroot/cppyy/cppyy/test/pythonizables.h

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,4 +123,43 @@ class IndexableBase {
123123

124124
class IndexableDerived : public IndexableBase {};
125125

126+
//===========================================================================
127+
// for testing size() -> __len__ pythonization guards
128+
class SizeReturnsInt {
129+
public:
130+
int size() { return 3; }
131+
int *begin() { return m_data; }
132+
int *end() { return m_data + 3; }
133+
134+
private:
135+
int m_data[3] = {1, 2, 3};
136+
};
137+
138+
class SizeReturnsNonInt {
139+
public:
140+
struct OptSize {};
141+
OptSize size() { return {}; }
142+
int *begin() { return nullptr; }
143+
int *end() { return nullptr; }
144+
};
145+
146+
class SizeWithoutIterator {
147+
public:
148+
int size() { return 5; }
149+
// no begin()/end() or operator[]
150+
};
151+
152+
// for testing __len__ with fully inherited container interface
153+
class ContainerBase {
154+
public:
155+
int size() { return 2; }
156+
int *begin() { return m_data; }
157+
int *end() { return m_data + 2; }
158+
159+
private:
160+
int m_data[2] = {10, 20};
161+
};
162+
163+
class InheritedContainer : public ContainerBase {};
164+
126165
} // namespace pyzables

bindings/pyroot/cppyy/cppyy/test/stltypes.h

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -82,15 +82,6 @@ class stl_like_class2 {
8282
value_type& operator[](ptrdiff_t i) { return fData[i]; }
8383
};
8484

85-
template<class value_type, size_t sz>
86-
class stl_like_class3 : public stl_like_class2<value_type, sz> {
87-
using stl_like_class2<value_type, sz>::fData;
88-
public:
89-
size_t size() { return sz; }
90-
value_type& begin() { return fData; }
91-
value_type& end() { return fData + sz; }
92-
};
93-
9485
class stl_like_class4 {
9586
public:
9687
struct iterator {

bindings/pyroot/cppyy/cppyy/test/test_pythonization.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,33 @@ def test10_shared_ptr_reset(self):
370370
optr.__smartptr__().reset(o2)
371371
assert optr == o2
372372

373+
def test11_size_len_pythonization_guards(self):
374+
"""Verify __len__ is only installed when size() returns int and class is iterable"""
375+
376+
import cppyy
377+
378+
# size() returns int AND has begin/end -> __len__ installed
379+
obj_int = cppyy.gbl.pyzables.SizeReturnsInt()
380+
assert hasattr(obj_int, "__len__")
381+
assert len(obj_int) == 3
382+
assert bool(obj_int)
383+
384+
# size() returns non-integer type -> __len__ NOT installed
385+
obj_opt = cppyy.gbl.pyzables.SizeReturnsNonInt()
386+
assert not hasattr(obj_opt, "__len__")
387+
assert bool(obj_opt) # should be True (valid object)
388+
389+
# size() returns int but no begin/end or operator[] -> __len__ NOT installed
390+
obj_noiter = cppyy.gbl.pyzables.SizeWithoutIterator()
391+
assert not hasattr(obj_noiter, "__len__")
392+
assert bool(obj_noiter)
393+
394+
# fully inherited container interface -> __len__ installed via MRO
395+
obj_inherited = cppyy.gbl.pyzables.InheritedContainer()
396+
assert hasattr(obj_inherited, "__len__")
397+
assert len(obj_inherited) == 2
398+
399+
373400
## actual test run
374401
if __name__ == "__main__":
375402
exit(pytest.main(args=['-sv', '-ra', __file__]))

bindings/pyroot/cppyy/cppyy/test/test_stltypes.py

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1484,16 +1484,12 @@ def test02_STL_like_class_iterators(self):
14841484

14851485
assert i == len(a)-1
14861486

1487-
for cls in [cppyy.gbl.stl_like_class2, cppyy.gbl.stl_like_class3]:
1488-
b = cls[float, 2]()
1489-
b[0] = 27; b[1] = 42
1490-
limit = len(b)+1
1491-
for x in b:
1492-
limit -= 1
1493-
assert limit and "iterated too far!"
1494-
assert x in [27, 42]
1495-
assert x == 42
1496-
del x, b
1487+
b = cppyy.gbl.stl_like_class2[float, 2]()
1488+
b[0] = 27
1489+
b[1] = 42
1490+
assert len(b) == 2
1491+
for i in range(len(b)):
1492+
assert b[i] in [27, 42]
14971493

14981494
for num in [4, 5, 6, 7]:
14991495
cls = getattr(cppyy.gbl, 'stl_like_class%d' % num)

0 commit comments

Comments
 (0)