Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always call PyNumber_Index when casting from Python to a C++ integral type, also pre-3.8 #2801

Merged
40 changes: 25 additions & 15 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -1040,14 +1040,31 @@ struct type_caster<T, enable_if_t<std::is_arithmetic<T>::value && !is_std_char_t
return false;
} else if (PyFloat_Check(src.ptr())) {
return false;
} else if (!convert && !index_check(src.ptr()) && !PYBIND11_LONG_CHECK(src.ptr())) {
} else if (!convert && !PYBIND11_LONG_CHECK(src.ptr()) && !index_check(src.ptr())) {
return false;
} else if (std::is_unsigned<py_type>::value) {
py_value = as_unsigned<py_type>(src.ptr());
} else { // signed integer:
py_value = sizeof(T) <= sizeof(long)
? (py_type) PyLong_AsLong(src.ptr())
: (py_type) PYBIND11_LONG_AS_LONGLONG(src.ptr());
} else {
handle src_or_index = src;
#if PY_VERSION_HEX < 0x03080000
object index;
if (!PYBIND11_LONG_CHECK(src.ptr())) { // So: index_check(src.ptr())
index = reinterpret_steal<object>(PyNumber_Index(src.ptr()));
if (!index) {
PyErr_Clear();
if (!convert)
return false;
}
else {
src_or_index = index;
}
}
#endif
if (std::is_unsigned<py_type>::value) {
py_value = as_unsigned<py_type>(src_or_index.ptr());
} else { // signed integer:
py_value = sizeof(T) <= sizeof(long)
? (py_type) PyLong_AsLong(src_or_index.ptr())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, another note: this results in warnings, and I think that's correct. Because it's not just getting out the long from the PyLong object, but it's also trying to do the conversion.

Yes, the C API is quite messy here. And it's only made worse by the structure of this caster.

: (py_type) PYBIND11_LONG_AS_LONGLONG(src_or_index.ptr());
}
}

// Python API reported an error
Expand All @@ -1056,15 +1073,8 @@ struct type_caster<T, enable_if_t<std::is_arithmetic<T>::value && !is_std_char_t
// Check to see if the conversion is valid (integers should match exactly)
// Signed/unsigned checks happen elsewhere
if (py_err || (std::is_integral<T>::value && sizeof(py_type) != sizeof(T) && py_value != (py_type) (T) py_value)) {
bool type_error = py_err && PyErr_ExceptionMatches(
#if PY_VERSION_HEX < 0x03000000 && !defined(PYPY_VERSION)
PyExc_SystemError
#else
PyExc_TypeError
#endif
);
PyErr_Clear();
if (type_error && convert && PyNumber_Check(src.ptr())) {
if (py_err && convert && PyNumber_Check(src.ptr())) {
auto tmp = reinterpret_steal<object>(std::is_floating_point<T>::value
? PyNumber_Float(src.ptr())
: PyNumber_Long(src.ptr()));
Expand Down
34 changes: 27 additions & 7 deletions tests/test_builtin_casters.py
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,13 @@ class DeepThought(object):
def __int__(self):
return 42

class DoubleThought(object):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double meaning it has index and int?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I meant :-) As admitted yesterday:

yes, this naming joke got out of hand quite quickly

Should I still pick better names?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. IntIndexThought, at least. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done so. This should make things more clear? More boring as well, but ...

7fd2db5 has only changes to our own tests, so if these pass, look good/better to you, and you're happy with the way DoubleThought/IntAndIndex is handled, then we can merge this, @henryiii

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. IntIndexThought, at least. :)

I liked the mix of HHGG and 1984, but yes ;-)

def __int__(self):
return 42

def __index__(self):
return 0

class ShallowThought(object):
pass

Expand All @@ -267,6 +274,13 @@ class IndexedThought(object):
def __index__(self):
return 42

class TypeErrorThought(object):
def __index__(self):
raise TypeError

def __int__(self):
return 42

class RaisingThought(object):
def __index__(self):
raise ValueError
Expand All @@ -276,7 +290,7 @@ def __int__(self):

convert, noconvert = m.int_passthrough, m.int_passthrough_noconvert

def require_implicit(v):
def requires_conversion(v):
pytest.raises(TypeError, noconvert, v)

def cant_convert(v):
Expand All @@ -286,14 +300,20 @@ def cant_convert(v):
assert noconvert(7) == 7
cant_convert(3.14159)
assert convert(DeepThought()) == 42
require_implicit(DeepThought())
requires_conversion(DeepThought())
assert convert(DoubleThought()) == 0 # Fishy; `int(DoubleThought)` == 42
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not great, but kind of a consequence of us saying "everything with __index__ is already an int, so don't try converting".

assert noconvert(DoubleThought()) == 0
cant_convert(ShallowThought())
cant_convert(FuzzyThought())
if env.PY >= (3, 8):
# Before Python 3.8, `int(obj)` does not pick up on `obj.__index__`
assert convert(IndexedThought()) == 42
assert noconvert(IndexedThought()) == 42
cant_convert(RaisingThought()) # no fall-back to `__int__`if `__index__` raises

# Before Python 3.8, `PyLong_AsLong` does not pick up on `obj.__index__`,
# but pybind11 "backports" this behavior.
assert convert(IndexedThought()) == 42
assert noconvert(IndexedThought()) == 42
assert convert(TypeErrorThought()) == 42
requires_conversion(TypeErrorThought())
assert convert(RaisingThought()) == 42
requires_conversion(RaisingThought())


def test_numpy_int_convert():
Expand Down