pybind11 icon indicating copy to clipboard operation
pybind11 copied to clipboard

Expand float and complex strict mode to allow ints and ints/float (for PEP 484 compatibility).

Open InvincibleRMC opened this issue 2 months ago β€’ 26 comments

Description

Breaking change to allow Python ints into float when under strict typing (noconvert), to better match PEP 484 numeric tower rules.

This change was also done to complex to allow floats and ints. Now that Python ints can be passed into floats it changes behavior for overload resolution. A method that takes float that is registered before a method that takes an int will now get executed when a Python int is passed in. This overload resolution also affects methods with std::complex.

Resolves #5878 Provides a work around for #5767

NOTE: This PR was reviewed extensively and is approved. However, because of the breaking change, it will be merged only after the next patch release.

Suggested changelog entry:

Breaking change for PEP 484 compatibility: Expand float and complex strict mode to allow ints and ints/float.


πŸ“š Documentation preview πŸ“š: https://pybind11--5879.org.readthedocs.build/

InvincibleRMC avatar Oct 23 '25 16:10 InvincibleRMC

I have started working on this too. Your code change is more complex than required. I will finish up the documentation changes and commit what I have and you can take it from there.

gentlegiantJGC avatar Oct 23 '25 16:10 gentlegiantJGC

Here is my implementation and documentation changes. I have modified the behaviour of complex as well. https://github.com/gentlegiantJGC/pybind11/tree/fix-pep-484 https://github.com/pybind/pybind11/compare/master...gentlegiantJGC:pybind11:fix-pep-484

It needs tests but I am going to stop for today. You are welcome to use my commits.

gentlegiantJGC avatar Oct 23 '25 16:10 gentlegiantJGC

You will need to remove the constexpr check. I didn't realise it was a newer C++ feature

gentlegiantJGC avatar Oct 23 '25 17:10 gentlegiantJGC

TODO: figure out why it doesn't work in C++11.

InvincibleRMC avatar Oct 23 '25 19:10 InvincibleRMC

I have just found that a python float passed to an int argument will raise a TypeError. It will happily except anything that implements __int__ (eg a numpy float) but explicity not a python float.

This is probably something we should resolve here because it effects these tests.

m.def("func", [](int){});
>>> func.__doc__
  func(arg: typing.SupportsInt) -> None
>>> func(5.5)
  TypeError
>>> func(numpy.float32(5.5))  # This works fine

gentlegiantJGC avatar Oct 24 '25 07:10 gentlegiantJGC

Here is the commit that implemented that. https://github.com/pybind/pybind11/commit/3f200fab2237ea478a71997a9864b935506bb96e

@rwgk Can I get your input on this? The int type caster supports anything that implements __int__ with the explicit exception of the python float.

This conflicts with the typing.SupportsInt type hint.

gentlegiantJGC avatar Oct 24 '25 07:10 gentlegiantJGC

Here is my proposed change. You will need to update the tests. https://github.com/gentlegiantJGC/pybind11/commit/d42c8e8af752d20c569e6aba34a0e236353155c1 https://github.com/gentlegiantJGC/pybind11/tree/pr-5879-jgc

Here are my unittests test_cast.py cast.cpp

gentlegiantJGC avatar Oct 24 '25 08:10 gentlegiantJGC

I noticed something else to have address. According to the documentation all of int,float,complex support falling back on __index__ when converting so updated the type hint from typing.SupportsInt -> typing.SupportsInt | typing.SupportsIndex. complex also supports a __float__ fall back so its type is now typing.SupportsComplex | typing.SupportsFloat | typing.SupportsIndex

InvincibleRMC avatar Oct 24 '25 19:10 InvincibleRMC

Here is the commit that implemented that. 3f200fa

@rwgk Can I get your input on this? The int type caster supports anything that implements __int__ with the explicit exception of the python float.

This conflicts with the typing.SupportsInt type hint.

I need to find a block of time to look at this carefully. Might be a couple days.

Thanks for working on this!

rwgk avatar Oct 26 '25 06:10 rwgk

Hi @InvincibleRMC @gentlegiantJGC, I will review this PR now / this morning. QQ just in case you're around: Are there still any open questions from your side?

rwgk avatar Nov 08 '25 18:11 rwgk

My main question is the casting behaviour to C++ and how noconvert and converting casting should be implemented. It seems to be a bit inconsistently implemented and in some cases noconvert is not implemented (eg #5889)

My understanding is that in the default (convert) mode any python type that can be converted to that type should be. In the noconvert mode it should only accept the types that match the type hint. What is the convention on what the noconvert type hint should be?

The converting integral type caster accepts any object that implements __int__ with the explicit exception of float which stems from this commit from 2016. https://github.com/pybind/pybind11/commit/3f200fab2237ea478a71997a9864b935506bb96e It will accept numpy floats but not python floats. Is this behaviour intended. If so why?

gentlegiantJGC avatar Nov 08 '25 19:11 gentlegiantJGC

FYI β€” I'm playing with cursor-agent while reviewing this PR. The commit that I just pushed was generated by cursor.

I'm pushing it JIC you want to follow along. There are some other things I still want to work on. I might not get all the way through today. I'll let you know here when I've arrived at a stable point, but please let me know any comments anytime.

rwgk avatar Nov 08 '25 20:11 rwgk

Follow up to my last message: Should the noconverting caster accept only python int or any object that can be implicitly converted to int (eg a numpy int)? In the latter case the type hint would be typing.SupportsIndex

I don't think there is a way to tell if an object is inherintly a float so I don't know how that would work for floating types.

gentlegiantJGC avatar Nov 08 '25 20:11 gentlegiantJGC

Here is how far I got today (I'll have another block of time tomorrow afternoon):

To the best of my current understanding, please nudge/correct as needed (this was partially generated by cursor; hope that's OK in terms of tone):

Focus of this PR: I think it'll be best to keep this PR focused on allowing int β†’ float and int/float β†’ complex in strict mode (noconvert), aligning with PEP 484's numeric tower.

Regarding noconvert and __index__:

For float and complex casters, this PR LGTM as-is: noconvert does not convert via __index__.

  • Good IMO: Float caster (cast.h line 248): Strict mode only accepts PyFloat_Check OR PYBIND11_LONG_CHECK (Python int). It does not check PYBIND11_INDEX_CHECK, so custom __index__ objects are rejected in strict mode.

  • Good IMO: Complex caster (complex.h lines 54-58): Same pattern β€” strict mode only accepts PyComplex_Check, PyFloat_Check, or PYBIND11_LONG_CHECK. No PYBIND11_INDEX_CHECK.

The tests confirm this: requires_conversion(Index()) for both float and complex casters β€” custom __index__ objects are correctly rejected in strict mode.

The inconsistency you mentioned: The int caster is the outlier β€” it currently accepts __index__ objects in strict mode (line 253 checks PYBIND11_INDEX_CHECK). However, that's a separate issue from this PR's scope. I'd suggest handling that in a follow-up PR/issue to keep this PR focused.

Regarding type hints: The type hints for noconvert currently say int/float/complex, which matches the actual behavior for float/complex (they accept Python int via PYBIND11_LONG_CHECK, but not custom __index__ objects). The int caster's type hint vs. behavior mismatch is part of that separate issue.

Regarding float exclusion from int caster: That's also a separate design question (related to #5889 you mentioned) that should be addressed independently.


I'll see your comments, but probably won't be able to focus on this PR until tomorrow afternoon.

rwgk avatar Nov 08 '25 20:11 rwgk

I looked more at this PR, but still need more time.

There are many changes to the tests. I'm wondering about this part:

Add typing.SupportsIndex to the int/float input types to better match fall backs. This corrects a mistake that these where supported but, the type hint was not updated.

Could we break that out as a separate PR? Would that lead to a significant reduction in the number of test changes in the main PR?

rwgk avatar Nov 10 '25 07:11 rwgk

I'll look into how easy it is to split them apart after work.

InvincibleRMC avatar Nov 10 '25 15:11 InvincibleRMC

I'll look into how easy it is to split them apart after work.

Give me a moment, I'm trying to do that with cursor right now. I'm hoping that way we don't have to do the legwork ourselves.

rwgk avatar Nov 10 '25 20:11 rwgk

I just created PR #5891 to isolate out the typing.SupportsIndex fixes.

@InvincibleRMC @gentlegiantJGC When you see that the tests are passing, could you please review? Please approve formally if the PR looks good to you.

rwgk avatar Nov 10 '25 21:11 rwgk

That's much easier to review now!

The production code changes are actually very easy to review now.

I'll comb through the test changes carefully tomorrow, for a final confirmation that everything is OK.

I want to hold back merging though. The plan I have in mind:

  • Review ~and merge~ #5887
  • Make a pybind11 v3.0.2 release.
  • Merge this PR, bumping the version to v3.1.0a0
  • Let this sit on master for a few weeks to see what feedback we get.

@henryiii @swolchok for visibility

rwgk avatar Nov 11 '25 05:11 rwgk

xref regarding merge & release planning: https://github.com/pybind/pybind11/pull/5887#issuecomment-3518889816

rwgk avatar Nov 11 '25 22:11 rwgk

I've been working through the PR to understand the behavior changes and ensure everything is correct. Sorry it took me way longer than I'd want to admit to realize that this PR was enabling implicit, narrowing Python float to C++ int conversions. Here is where I realized:

  • c7e959e1b577ddf7c3bb6172141b62e4c3cb655b

After the penny dropped, I added this commit:

  • e4023cd39fff42d43fd87184a5c6697bef5000be - Reject float β†’ int conversion even in convert mode

Please see the comprehensive commit message for the rationale.

These are follow-on commits to make the tests pass again:

  • cc981a887b0d795b7a26a3258a07d4a5663427d8 β€” Revert test changes that sidestepped implicit floatβ†’int conversion
  • cc411c6e172d1cca63b7dd305a6b404a3b2a0d64 β€” Replace env.deprecated_call() with pytest.deprecated_call()
  • ac957e386dc2b02e7bf66e1423df5a7e6651b385 β€” Update test expectations for swapped NoisyAlloc overloads

Before that I added a couple minor commits:

  • 0a00147a13511647e3bfce70b81532ffaee527e3 - Undo inconsequential change to regex in test_enum.py
  • 41ff8766311601d55adb4c5f8ee1dcb63c1a7532 - Improve comment clarity for PyPy __index__ handling

There are still things I want to look into [EDIT: DONE], to make sure we're not creating weird oddities (beyond already existing oddities):

  • ~What is the conversion behavior for NumPy scalars (e.g., np.float32, np.float64)? Should we add corresponding tests?~
  • What is the PyNumber_Index behavior with floats?
  • Should we add a validation to ensure that __index__ actually returns int (when converting to C++ int)?

I probably won't have a block of time to focus on this PR again before next weekend, but I'll read all comments.

Note: I've been using Cursor a lot, in case you wonder about all the long commit messages.

rwgk avatar Nov 12 '25 01:11 rwgk

FYI β€” While I was at it, I isolated out this change, to be merged asap:

  • https://github.com/rwgk/pybind11/commit/89917e9264dc8839fab16afda0f0d7ae24ffda0d

I'll mark you a co-authors there. (I should have done that yesterday for the other PR, too, but sorry it missed the opportunity.)

The goal is to make this critical PR as clean and lean as possible. I expect that it will be looked back at a lot, b/o the behavior change.

I'll convert the branch to a PR when the CI here is done. (We only have the free GHA quota now, unfortunately.)

EDIT: That's PR #5893 now.

rwgk avatar Nov 12 '25 02:11 rwgk

I will open a new issue about the custom float -> int conversion

gentlegiantJGC avatar Nov 12 '25 07:11 gentlegiantJGC

The extra tests (commit f1d81588b9b3dfb78111a23cd1853841394a29f2) are:

  • To be certain about the exact behavior in pathological cases, so we're sure we don't have silent float to int conversions.
  • To find out if the PyPy code path in cast.h behaves identically.

This is a bit on the paranoid side, but I wanted to reassure myself that we're not putting the world through a behavior change without having looked at every dark corner.

rwgk avatar Nov 12 '25 09:11 rwgk

BTW, having looked at this so intensely ...

        bool py_err = py_value == (py_type) -1 && PyErr_Occurred();

this will be OK if Python is healthy, but if something goes wrong (e.g. UB, or bug in Python beta testing) and py_value is not -1 even though PyErr_Occurred() is true, the error will not be cleared and could get reported in the wrong context.

So that condition basically errs on the unsafe side, it prioritizes the return value check over error detection.

I think it'd be better to write:

        bool py_err = PyErr_Occurred();
        if (py_err) assert(py_value == (py_type) -1);

This prioritizes error detection over return value validation.

WDYT?

rwgk avatar Nov 12 '25 10:11 rwgk

I can't see any obvious issues

Same. I looked through everything several times now. Thanks!

rwgk avatar Nov 12 '25 16:11 rwgk