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

Make py::isinstance<py::str> less permissive #1463

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

jagerman
Copy link
Member

py::str's uses a permissive check function that allows both str (Py 3)/
unicode (Py 2), and bytes (Py 3)/str (Py 2) so that a py::str can
be created from either, but this makes the default isinstance
implementation overly permissive.

Fixes #1461

py::str's uses a permissive check function that allows both str (Py 3)/
unicode (Py 2), *and* bytes (Py 3)/str (Py 2) so that a `py::str` can
be created from either, but this makes the default `isinstance`
implementation overly permissive.
@bstaletic
Copy link
Collaborator

If I got this right, this won't break code that does the following:

std::string GetStringFromPythonObject( pybind11::object o ) {
        // If it is any kind of string like python object just cast it to std::string
        if ( pybind11::isinstance< bytes >( o ) || pybind11::isinstance< str >( o ) ) {
                return o.cast< std::string >();
        }
        // Else, go through pybind11::str()
        return pybind11::str( o ).cast< std::string >();
}

Is that correct?

@jagerman
Copy link
Member Author

It looks like this PR does break Python 2, where py::isinstance<str> is being used to test whether the value is convertible to a string. This is probably at least somewhat historic: we used to have a check() method which isinstance replaced, but the check was intentionally more permissive for strs.

This might be trickier to fix without breaking things.

@jagerman
Copy link
Member Author

jagerman commented Jul 19, 2018

If I got this right, this won't break code that does the following:

Your code is fine; the problem is that we have code expecting that isinstance<str>(o) is going to return true when o is a Py2 str. Your code is fine because isinstance<str>(o) || isinstance<bytes>(o) is the proper test.

@wjakob
Copy link
Member

wjakob commented Aug 28, 2018

How about only enabling the more restrictive check for Python >= 3.x?

wjakob and others added 8 commits June 11, 2019 22:07
fix some uninitialized members in `value_and_holder` for
some of the constructurs.

Found with coverity in a downstream project.
)

In some cases the user of pythonbuf needs to allocate the internal
buffer to a specific size e.g. for performance or to enable synchronous
writes to the buffer.
By changing `pythonbuf::d_buffer` to be dynamically allocated we can now
enable these use-cases while still providing the default behavior of
allocating a 1024 byte internal buffer (through a default parameter).
…1587) (pybind#1595)

* Fix async Python functors invoking from multiple C++ threads (pybind#1587)

Ensure GIL is held during functor destruction.

* Add async Python callbacks test that runs in separate Python thread
In def_readonly and def_readwrite, there is an assertion that the member comes
from the class or a base class:

    static_assert(std::is_base_of<C, type>::value, "...");

However, if C and type are the same type, is_base_of will still only be true
if they are the same _non-union_ type.  This means we can't define accessors
for the members of a union type because of this assertion.

Update the assertion to test

    std::is_same<C, type>::value || std::is_base_of<C, type>::value

which will allow union types, or members of base classes.

Also add a basic unit test for accessing unions.
@wjakob
Copy link
Member

wjakob commented Jun 11, 2019

This had some weird conflicts, I ended up applying the patch directly myself.

@wjakob
Copy link
Member

wjakob commented Jun 11, 2019

Whoops, I missed the fact that this does indeed break the test suite on 2.7. Reverting & reopening.

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.

py::isinstance<py::str>(py_bytes_object) should not return true
8 participants