Skip to content

additional assert demonstrating that pybind::str can also by bytes in… #2343

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

Closed
wants to merge 1 commit into from

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Jul 30, 2020

…deed (proving isinstance right)

@YannickJadoul
Copy link
Collaborator

Alright, this makes sense from the code's perspective, since both isinstance as well as the converting constructor from const object&/object&& use the static check_ method of the py::object subclasses:

template <typename T, detail::enable_if_t<std::is_base_of<object, T>::value, int> = 0>
bool isinstance(handle obj) { return T::check_(obj); }

and

#define PYBIND11_OBJECT_CVT(Name, Parent, CheckFun, ConvertFun) \
    PYBIND11_OBJECT_COMMON(Name, Parent, CheckFun) \
    /* This is deliberately not 'explicit' to allow implicit conversion from object: */ \
    Name(const object &o) \
    : Parent(check_(o) ? o.inc_ref().ptr() : ConvertFun(o.ptr()), stolen_t{}) \
    { if (!m_ptr) throw error_already_set(); } \
    Name(object &&o) \
    : Parent(check_(o) ? o.release().ptr() : ConvertFun(o.ptr()), stolen_t{}) \
    { if (!m_ptr) throw error_already_set(); } \
    template <typename Policy_> \
    Name(const ::pybind11::detail::accessor<Policy_> &a) : Name(object(a)) { }

This seems wrong to me, though. Especially because there is a str(const bytes&) constructor as well, that actually does the conversion:

inline str::str(const bytes& b) {
    char *buffer;
    ssize_t length;
    if (PYBIND11_BYTES_AS_STRING_AND_SIZE(b.ptr(), &buffer, &length))
        pybind11_fail("Unable to extract bytes contents!");
    auto obj = reinterpret_steal<object>(PyUnicode_FromStringAndSize(buffer, (ssize_t) length));
    if (!obj)
        pybind11_fail("Could not allocate string object!");
    m_ptr = obj.release().ptr();
}

but d["str"] (with py::dict d), doesn't return a str, but just some proxy that's implicitly convertible to py::object, and only the "dynamic Python type" is actually bytes, so the C++ overload resolution never works here.

@YannickJadoul
Copy link
Collaborator

(I'm assuming this is a demonstrating PR, and not something you actually want to get merged, right?)

@rwgk
Copy link
Collaborator Author

rwgk commented Jul 30, 2020

(I'm assuming this is a demonstrating PR, and not something you actually want to get merged, right?)

Yes, this PR is just a quick separate demonstration of the bug. I'm thinking of merging this change with 4a4684b under #2256, and get that merged as a baseline for fixing.

@rwgk
Copy link
Collaborator Author

rwgk commented Aug 11, 2020

PR #2366 just merged includes another way to expose that pybind11::str can hold bytes. Dropping this PR (it was meant to be a demonstration only from the beginning).

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.

2 participants