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

Fix py::cast from pytype rvalue to pytype #3949

Merged
merged 2 commits into from
May 16, 2022

Conversation

MaartenBaert
Copy link
Contributor

Previously, py::cast blindly assumed that the destination type was a C++ type rather than a python type when the source type was an rvalue.

Description

The following piece of code:

py::int_ value = py::float_(1.0).cast<py::int_>();

... results in the following bizarre runtime error:

RuntimeError: Unable to cast Python instance of type <class 'float'> to C++ type 'int_'

Whereas this works fine:

py::float_ temp = 1.0;
py::int_ value = temp.cast<py::int_>();

The problem seems to be that in the first case, the py::float_ is an rvalue, which causes pybind to use a different implementation of py::cast which blindly assumes that the destination type is always a C++ type and attempts to use the type_caster mechanism - unlike the normal py::cast implementation which checks the destination type to determine whether to use type_caster or just call the constructor of the destination type.

This commit does three things:

  • Add a static assertion to load_type such that if it gets invoked with a python type rather than a C++ type, it will produce a meaningful compile-time error rather than the bizarre runtime error shown above.
  • Modify py::cast(object &&) such that it actually checks whether the destination type is a python object, and if so, invokes the move constructor instead of using type_caster.
  • Add a test to make sure that the problem is fixed.

Suggested changelog entry:

Fix cast from pytype rvalue to another pytype.

MaartenBaert and others added 2 commits May 16, 2022 20:15
Previously, py::cast blindly assumed that the destination type was a C++
type rather than a python type when the source type was an rvalue.
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I was looking for @Skylion007's comment that I saw in the email, but cannot find it here (anymore?). FWIW: I'm fine either way, slightly in favor of the straightforward &&.

@Skylion007
Copy link
Collaborator

@rwgk Yeah, I looked more closely at the logic and realized all_of any_of would not have helped.

@rwgk rwgk merged commit 72eea20 into pybind:master May 16, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 16, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants