[BUG]: SupportsInt and SupportsFloat changes cause type checking failures on python classes that override these functions

[daltairwalter]

Required prerequisites

• [x] Make sure you've read the documentation. Your issue may be addressed there.
• [x] Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
• [x] Consider asking first in the Gitter chat room or in a Discussion.

What version (or hash if on master) of pybind11 are you using?

3.0.0

Problem description

#5540 changed type hints from int and float to SupportsInt and SupportsFloat. While these may be nice for use cases where a C++ module is exposed to python, this is causing a lot of type problems for cases where a C++ function is trampolined and overridden in python. The base class is now using SupportsInt and SupportsFloat and this is more generic than the python derived classes that uses int, float so mypy issues errors like:

Argument 1 of "pdf" is incompatible with supertype "Distribution"; supertype defines the argument type as "SupportsFloat" [override] def pdf(self, x: float) -> float:

Am I missing something here, or is there a way to turn this feature off?

Reproducible example code

Is this a regression? Put the last known working version here if it is.

smartholder branch on 2024-08-09

[timohl]

Is changing the type hint for the derived class an option in order to match the new type hints? I suppose you would have to add a x = float(x) as first line if the code does require actual floats.

Otherwise, I think there is currently no way for turning this off.

For fixing your issue I could imagine two options:

1. Adding a global macro to set the type hints for int and float. The following code already changes type hints depending on the python version, so something similar could be used to globally change the type hint for int/float: https://github.com/pybind/pybind11/blob/7f5eea432e7a6861da251021736cbabeed8c6bda/include/pybind11/detail/common.h#L271-L299 While this fixes this issue, I do not like it, since it also turns it off at other more useful places.

2. Adding py::typing::int and py::typing::float to pybind11/typing.h that forces using int/float. Using these in the C++ trampline class could maybe fix this, if I understand your usecase correctly. I would prefer this since it is more localized.

Both options are just thoughts on how a fix for your usecase could be implemented, so I am happy to hear other thoughts.

[InvincibleRMC]

You can also do the following.

    m.def("float_passthrough_noconvert", [](float arg) { return arg; }, py::arg{}.noconvert());

Generates

def float_passthrough_noconvert(arg0: float) -> float:

[timohl]

You can also do the following.

m.def("float_passthrough_noconvert", [](float arg) { return arg; }, py::arg{}.noconvert());

Generates

def float_passthrough_noconvert(arg0: float) -> float:

Oh, how could I forget, probably too late in the evening.

But yes, that is the way to go! 👍

[daltairwalter]

I will look into the noconvert option. Is there any way to do this at the module level rather than at the function level?

[daltairwalter]

noconvert() does produce the type hints I want, but the documentation also indicates that this changes the functionality too. According to the noconvert documentation it will prevent an int from being used as a float. If I understand all this correctly, it seems like this implementation of noconvert directly conflicts with PEP 484 which states that when an argument is annotated as having type float, an argument of type int is acceptable.

[daltairwalter]

Looking back at @timohl's options for fixing this, it seems like the global macro would definitely work. If I understand correctly, this would effectively just replace typing.SupportsInt and typing.SupportsFloat with int and float everywhere.

I am not sure I understand the option for using py::typing::int and py::typing::float in the trampolines. My understanding is that the type hints are coming from the module definition and not the trampoline so it seems like this would also require changes in the module definition level and this would snowball into a huge amount of changes and boiler plate code.

I don't know how this would be done, but as a user I would most prefer to have a module level option for this where I see the option multiple_interpreters::per_interpreter_gil something like type_hinting::use_int_and_float

[gentlegiantJGC]

2. Adding py::typing::int and py::typing::float to pybind11/typing.h that forces using int/float.

I think this is redundant. The current casting behaviour of py::int_ only accepts python ints and py::float_ only accepts python floats. I believe type type hints for py::int_ should be int and py::float_ should be float. See #5839 I believe py::float_ should also accept python ints per PEP 484. ~~See #5840~~ See #5839

This would allow the SupportsInt/Float behaviour through the native int and float and python-like behaviour through py::int_ and py::float_

[daltairwalter]

This would allow the SupportsInt/Float behaviour through the native int and float and python-like behaviour through py::int_ and py::float_

@gentlegiantJGC this behavior does not fix the problem. It would require wrapping everything that should have normal int/float type hints in py::int_ and py::float_ and this egregious. Some more general way of suppressing the SupportsInt and SupportsFloat is needed.

[gentlegiantJGC]

Can I suggest creating a pull request. As timohl said here the simple solution is to add a configuration macro and a preprocessor directive to switch the behaviour.

Something like this

// common.h
#ifndef PYBIND11_ENABLE_NUMERIC_SUPPORTS_HINT
#define PYBIND11_ENABLE_NUMERIC_SUPPORTS_HINT 1
#endif

And this https://github.com/pybind/pybind11/blob/326b10637a9fe2d1c9f290b6e052097ffc2413c3/include/pybind11/cast.h#L348C1-L350C93 Would be changed to something like this

    PYBIND11_TYPE_CASTER(T,
                         io_name<std::is_integral<T>::value>(
#if PYBIND11_ENABLE_NUMERIC_SUPPORTS_HINT
                             "typing.SupportsInt", "int", "typing.SupportsFloat", "float"));
#else 
                             "int", "int", "float", "float"));
#endif

Maybe switching the whole section might be eaiser to read. I will leave the implementation to you.

[rwgk]

I'm not a big fan of the global macro. If we want to test it, we have to make many adjustment to the unit tests. I see that as an indication of how disruptive it will be in general if we make it possible that some extensions in the wild produce float and others SupportsFloat.

Coming back to this:

Argument 1 of "pdf" is incompatible with supertype "Distribution"; supertype defines the argument type as "SupportsFloat [override] def pdf(self, x: float) -> float:

What if you make this

    def pdf(self, x: typing.SupportsFloat) -> float:

or x: float | typing.SupportsFloat, or in the worst case disable the mypy check?

Not great maybe, but on balance, I think that's better than introducing global non-uniform behavior.

[rwgk]

Responding to https://github.com/pybind/pybind11/pull/5875#issuecomment-3423089099 here (to keep the discussions in one place):

I will try to make a single test that uses this macro instead of the other change - this looks straightforward, but I have never used the pybind11 testing system so we will see.

The changes to the tests by themselves are NOT my main worry (I think we can handle that pretty easily and nicely). — Please don't spend any time on the tests until we're sure that's what we want.

My main worry is (what I wrote before):

I see that as an indication of how disruptive it will be in general if we make it possible that some extensions in the wild produce float and others SupportsFloat.

... introducing global non-uniform behavior.

Could you please build a stronger case here, why we should do that? — See questions in my previous comment.

[daltairwalter]

What if you make this

    def pdf(self, x: typing.SupportsFloat) -> float:

or x: float | typing.SupportsFloat, or in the worst case disable the mypy check?

Not great maybe, but on balance, I think that's better than introducing global non-uniform behavior.

This defines the public interface of the library that is being created with pybind11. I could accept that I need to deal with this for my use, but I can't pass this complexity on to customers.

I understand the concern about this being a global define. Do you have other suggestions about how this can be done?

[timohl]

If you have control over the base class bindings, I think the cleanest solution would be to use .noconvert() to change the type hints for the base class arguments, as @InvincibleRMC mentioned earlier.

You mentioned this might prevent passing int. If this is the case, I could see the case for changing the behavior of the caster. I did not take a deeper look into the caster, but I think this is the spot: https://github.com/pybind/pybind11/blob/e6984c805ec09c0e5f826e3081a32f322a6bfe63/include/pybind11/cast.h#L240-L310

I think it would be best if the type caster is as close to the type hints as possible. So, without .noconvert() we get typing.SupportsFloat which allows any type implementing __float__() and with .noconvert() we get float which allows int and float according to PEP 484. I think this conversion rule would make it more transparent.

Is there any use case for accepting float but not int? If we lift the restriction, would this break anything?

[rwgk]

Is there any use case for accepting float but not int? If we lift the restriction, would this break anything?

Certain combinations of overloads might get resolved differently.

But if we can agree on a clear rationale, I think it’s worth considering a behavior change.

Thinking out loud (totally untested):

void foo(int);
void foo(float);
...
m.def("foo_if", overload_cast_<int>()(&foo));
m.def("foo_if", overload_cast_<float>()(&foo));
m.def("foo_fi", overload_cast_<float>()(&foo));
m.def("foo_fi", overload_cast_<int>()(&foo));

How does that play out with pybind11 as-is?

How would it behave if we accepted int anywhere we accept float?

What would the explanation (PR description, Upgrade guide) for that behavior change look like?

[gentlegiantJGC]

I am getting confused when you are talking about C int and float and Python int and float.

I believe that the type hint should match the type caster by default. If the type caster is less restrictive than the type hint expanding the type hint is less breaking than restricting the type caster.

The solutions I see are:

1. Accept that this is a breaking change in 3.0 and that dependends need to adopt the new behaviour
   • this is problematic if there are recursive dependents
2. Modify pybind11 to allow dependents to express the old behaviour
   • The simplest solution I can think of is a global macro.
3. Find a workaround that direct dependents can use to express the old behaviour
   • py::int or py::float can be used to achieve something similar to the old behaviour although the type caster is more restrictive
   • noconvert. This seems to also limit the type caster
   • A custom class and type caster to substitute for std::u?int_(8|16|32|64)_t
   • Inject custom type hints using py::options disable_function_signatures and py::doc

I see that as an indication of how disruptive it will be in general if we make it possible that some extensions in the wild produce float and others SupportsFloat.

I don't see this as an issue. The python user only sees the python type hint so the fact that the type caster is less restrictive does not matter. It would be down to each library to opt into the old behaviour if they want to.

Here is a rough implementation of a custom type caster. There is probably a better way to do this. I can't get the full cast support. Perhaps someone can fix it but I have spent enough time on this.

template <typename T>
struct NumericWrapper {
    T value;
    NumericWrapper() { }
    NumericWrapper(T value_)
        : value(value_)
    {
    }
    operator T() const { return value; }
};

namespace pybind11 {
namespace detail {

    template <typename T>
    struct type_caster<NumericWrapper<T>, enable_if_t<std::is_arithmetic_v<T>>> {
        PYBIND11_TYPE_CASTER(NumericWrapper<T>, io_name<std::is_integral_v<T>>("int", "int", "float", "float"));

        bool load(handle src, bool /*convert*/)
        {
            make_caster<T> caster;
            if (caster.load(src, true)) {
                value = cast_op<T>(caster);
                return true;
            }
            return false;
        }
    };

} // namespace detail
} // namespace pybind11

void foo(int arg) { std::cout << arg << std::endl; }
void bar(float arg) { std::cout << arg << std::endl; }

m.def("foo", [](NumericWrapper<int> arg) { foo(arg); });
m.def("bar", [](NumericWrapper<float> arg) { bar(arg); });

[daltairwalter]

Thank you for all the suggestions on this. After considering this, I think the global macro changes idea still seems the most viable for my use case because it doesn't require code changes on my side, produce functionality changes, or cause my project to use custom code that could become a maintenance issue. This may seem hypocritical and wrong, but there are advantages to having both the float type hint and convert functionality.

The global macro changes are the simplest solution. It seems like the worst thing that would happen here is that someone may get the wrong type hints if two modules are loaded and there are symbol conflicts. This concern probably effects all common.h options. I think these concerns are legitimate and I will understand if the community doesn't want to go forward with this.

My other alternative is to make stubs and correct them with a script using libcst, this is a little more complicated on my side, but I already do this for other stub generation issues so I will understand if this ends up being the solution and pybind11 is left unchanged.

[rwgk]

I was surprised to see @gentlegiantJGC 's https://github.com/pybind/pybind11/issues/5767#issuecomment-3425603229

Maybe I'm confused about what @timohl meant here?

I think it would be best if the type caster is as close to the type hints as possible. So, without .noconvert() we get typing.SupportsFloat which allows any type implementing __float__() and with .noconvert() we get float which allows int and float according to PEP 484. I think this conversion rule would make it more transparent.

Is there any use case for accepting float but not int? If we lift the restriction, would this break anything?

@timohl could you please help me understand? I assumed you wanted to change something in pybind11. What exactly?

[gentlegiantJGC]

I was surprised to see @gentlegiantJGC 's #5767 (comment)

Why is that? We changed something that had a minor breaking side effect and I was trying to suggest solutions to the issue.

I believe the new behaviour is correct but I understand that people have built code based on the old behaviour.

The change is purely to the type hint. It has no effect on the runtime behaviour.

I think it would be best if the type caster is as close to the type hints as possible.

I aggree with this statement but the other way around. The type hint should describe what python types the funciton can accept.

The int and float type casters could accept more than just the python int and float so we expanded the type hint to describe what the caster actually did.

with .noconvert() we get float which allows int and float according to PEP 484.

I have not used noconvert so I don't know how it behaves. Does this mean it only accepts a python float and not int?

I don't know how you would type hint that because float per PEP 484 really means float | int. I feel like that should be its own issue because it is beyond the scope of this issue.

[rwgk]

Does this mean it only accepts a python float and not int?

That was my guess, but I don't actually know.

I'm pretty strongly opposed to introducing the global macro, unless we have very clear and convincing documentation why that's needed. This should be understandable by a general audience, without requiring a lot of prior knowledge about type hints.

[InvincibleRMC]

Does this mean it only accepts a python float and not int?

That was my guess, but I don't actually know.

Correct if you use py::noconvert{} with a C++ float a python int cannot be passed in. As @gentlegiantJGC mentions this causes some problems with pep 484 around float meaning float | int. That discussion is beyond this scope since there is just no way to describe float but not int as a type. There is always lots of discussions about this quirk, for more info check out this discussion.

[rwgk]

Correct if you use py::noconvert{} with a C++ float a python int cannot be passed in.

I think we should change that, and that would close this issue.

[gentlegiantJGC]

I think we should change that

Wouldn't that break the whole purpose of noconvert? https://pybind11.readthedocs.io/en/stable/advanced/functions.html#non-converting-arguments

[gentlegiantJGC]

I have opened #5878 to track the noconvert float issue.

[timohl]

I think we should change that

Wouldn't that break the whole purpose of noconvert? https://pybind11.readthedocs.io/en/stable/advanced/functions.html#non-converting-arguments

This would certainly break the current purpose of noconvert() and repurpose it to match what the type hints float and typing.SupportsFloat allow us to annotate. Unfortunately, there is no type hint for "float without int" as far as I know, so it is impossible to generate accurate type hints for the current behavior.

For me, matching the Python type hints is more important than providing a validation feature, so I would welcome the change.

Looking at the caster code this should be easy to change and not have a negative performance impact.

[gentlegiantJGC]

there is no type hint for "float without int" as far as I know

This is correct.

I worry that changing the behaviour of noconvert would have breaking changes.

Here is an example I can think of that would break.

m.def("test", [](float arg) { std::cout << "float " << arg << std::endl; }, py::arg("arg").noconvert());
m.def("test", [](int arg) { std::cout << "int " << arg << std::endl; }, py::arg("arg").noconvert());

>>> from m import test
>>> test(5.5)
float 5.5
>>> test(5)
int 5

I don't know if anyone is using that functionality so it may not be an issue but worth thinking about

[gentlegiantJGC]

@daltairwalter another solution I have just thought of is to embed the type hint in the C++ code.

If you don't have many functions to modify this is probably the easiest solution.

This will give the full float type caster and the float type hint.

py::options options;
options.disable_function_signatures();
m.def(
    "func", 
    [](float arg){}, 
    py::doc("func(arg: float) -> None"));
options.enable_function_signatures();

[rwgk]

Wouldn't that break the whole purpose of noconvert?

To me, this looks like a corner case that we just got wrong — viewed from a practical, usefulness-oriented perspective.

noconvert makes sense for many other types, but here we’d only be changing behavior for Python int → C++ float/double. It really doesn’t make sense that this specific case isn’t converted.

If the src is not a Python int, the behavior would remain unchanged.

The only hiccups I’d expect are in the (probably uncommon) cases where people have arranged overloads around the current behavior. Having to adjust those seems like a very minor downside compared to the larger upside — PEP 484 conformance: “when an argument is annotated as having type float, an argument of type int is acceptable.”

I think we’ll learn a lot by actually making the change and running the tests: How many failures appear, and what does it take to fix them?

[InvincibleRMC]

Looking at a simple fix https://github.com/pybind/pybind11/pull/5879. This most exposes problems around resolution order now that int can go into float. Ideally to be the least breaking for the past would int would use int methods args before float args if possible.