[cppyy] Only alias size() to __len__ for container-like classes

[chrisburr]

Summary

If a C++ class has a size() method that returns a non-integer type cppyy adds a __len__ method but it always crashes. This can then affect the result of __bool__ in unexpected ways.

This PR fixes it by only pythonising size() -> __len__ if the return type is valid and it also appears to be iterable (has begin()+end() or operator[]).

Feel free to close this if you think there is a better solution.

This Pull request:

This was triggered by this issue in LHCb and Gaudi:

In order to understand the bootom issue, probably in cppyy), here is a simple reproducer, only based on Gaudi :

from GaudiPython import gbl as Cpp
intObj = Cpp.AnyDataWrapper[int](1)
print(bool(intObj))
print(bool(intObj.getData()))
dataObj = Cpp.DataObject()
print(bool(dataObj))

It will output :

False
True
True

The first one should also be True. Note how DataObject works differently from AnyDataWrapper.
One difference is that DataObject is not templated while AnyDataWrapper is.

I debugged this to find:

So this comes from unlucky interaction between two cppyy mechanisms and the signature of AnyDataWrapperBase::size().

The smoking gun is that the two objects differ in whether they have __len__ at all:

>>> hasattr(intObj, "__len__")
True
>>> hasattr(dataObj, "__len__")
False

Two pieces in ROOT's vendored CPyCppyy are relevant:

1. Any class with a method named size gets __len__ aliased to it. No check on the return type. See here:

// for STL containers, and user classes modeled after them
if (HasAttrDirect(pyclass, PyStrings::gSize, /*mustBeCPyCppyy=*/ true)) {
    Utility::AddToClass(pyclass, "__len__", "size");
}

2. So bool(x) on any cppyy object is: non-null pointer → consult len if there is one, otherwise True. See op_nonzero:

if (!self->GetObject())
    return 0;
PyObject* pylen = PyObject_CallMethodObjArgs((PyObject*)self, PyStrings::gLen, NULL);
if (!pylen) { PyErr_Clear(); return 1; }
return PyObject_IsTrue(pylen);

Put those together with AnyDataWrapperBase::size() in GaudiKernel/AnyDataWrapper.h, which is declared as std::optional<std::size_t> and returns std::nullopt for any non-container T (via the details::size fallback). AnyDataWrapper<int> inherits that, (1) aliases it to len, then (2) bool() goes through op_nonzero and calls it. You can see the mess directly:

>>> intObj.__len__.__doc__
'optional<unsigned long> AnyDataWrapper<int>::size()'
>>> len(intObj)
TypeError: 'optional<unsigned long>' object cannot be interpreted as an integer

DataObject has no size() at all, so no __len__ is installed, op_nonzero hits the PyErr_Clear(); return 1; branch, and you get True.

Changes or fixes:

Guard the automatic size() -> __len__ pythonization to require that size() returns an integer type and the class has either begin()/end() methods or accepts operator[]. This prevents bool() returning False for valid objects whose size() returns non-integer types like std::optional<std::size_t>.

Checklist:

• tested changes locally
• updated the docs (if necessary)

[guitargeek]

Thank you very much for stepping up to fix this, that's very much appreciated!

My main request is to make the integer check a part of the backend library, and a few other small things.

[guitargeek]

@chrisburr, how far do you need this packported? We need to cut somewhere, since the problem existed forever.

I also commited another fix related to this a few months ago, by the way:
e7d3bed

[github-actions]

Test Results

    22 files      22 suites   3d 1h 41m 39s ⏱️
 3 833 tests  3 830 ✅  1 💤  2 ❌
75 653 runs  75 606 ✅ 18 💤 29 ❌

For more details on these failures, see this check.

Results for commit 18ad0e0.

♻️ This comment has been updated with latest results.

[chrisburr]

how far do you need this packported? We need to cut somewhere, since the problem existed forever.

I don't know much about the issue this caused so @sponce is probably better positioned to answer.

[sponce]

No need to backport further that LCG108b, aka ROOT 6.36.08 for us

[guitargeek]

Thank you very much for improving the heuristics for the size()-to-__len__ Pythonization and addressing the review comments!

[guitargeek]

Fix was backported to 6.36 and 6.38