Changing pybind11::str to only hold PyUnicodeObject (NOT also bytes).

[rwgk]

This PR obsoletes PR #2256. The older PR convoluted three behavior changes. This new PR changes exactly one behavior.

The corresponding behavior changes are captured by changes in the tests. A significant effort was made to keep the test diffs minimal but also comprehensive and easy to read.

The two other behavior changes discussed under PR #2256 are avoided here (1. disabling implicit decoding from bytes to unicode; 2. list_caster behavior change). Based on this PR, those can be easily implemented if and when desired.

[bstaletic]

I do like this one. Should it be merged into str_bytes_cleanup instead of master?

[rwgk]

I do like this one. Should it be merged into str_bytes_cleanup instead of master?

Thanks Boris!
I'll start a thread about the branch on slack.

[YannickJadoul]

Found a shorter solution (and more elegant (?); I like it better, at least).

The only other suggestion I have is to leave out PYBIND11_DISABLE_IMPLICIT_STR_FROM_BYTES from this PR, and give it its own discussion. But maybe that's too nitpicky?

[YannickJadoul]

It took me a while to figure out what's going on here. While technically correct, it's confusing that "template header" on line 1633 belongs to the template function on line 1651 rather than 1636.

It feels like there has to be a better way? Either by writing to separate #ifdefs or by reversing the order and first putting the #ifndef (or #if !defined(...)) branch?

[YannickJadoul]

Next, this is also duplicated code with raw_str. Couldn't this be simplified to this?

    template <typename T = type, enable_if_t<std::is_same<T, str>::value, int> = 0>
    bool load(handle src, bool /* convert */) {
        if (isinstance<bytes>(src)) {
            value = str(reinterpret_borrow<bytes>(src));
            return true;
        }
        if (!isinstance<type>(src))
            return false;
        value = reinterpret_borrow<type>(src);
        return true;
    }

[YannickJadoul]

Or even smaller, combining my two remarks:

    template <typename T = type, enable_if_t<std::is_base_of<object, T>::value, int> = 0>
    bool load(handle src, bool /* convert */) {
#ifndef PYBIND11_DISABLE_IMPLICIT_STR_FROM_BYTES
        if (std::is_same<T, str>::value && isinstance<bytes>(src)) {
            value = str(reinterpret_borrow<bytes>(src));
            return true;
        }
#endif
        if (!isinstance<type>(src))
            return false;
        value = reinterpret_borrow<type>(src);
        return true;
    }

[YannickJadoul]

OK, wow, apparently py::str(handle) doesn't do the same as py::str(bytes)!??

Anyway, like THIS, then:

    template <typename T = type, enable_if_t<std::is_base_of<object, T>::value, int> = 0>
    bool load(handle src, bool /* convert */) {
#ifndef PYBIND11_DISABLE_IMPLICIT_STR_FROM_BYTES
        if (std::is_same<T, str>::value && isinstance<bytes>(src)) {
            value = str(src);
            return true;
        }
#endif
        if (!isinstance<type>(src))
            return false;
        value = reinterpret_borrow<type>(src);
        return true;
    }

[rwgk]

Oh, thanks!! The std::is_same inside the load function is much better! (I was trapped in not considering combining the compile-time condition with the runtime condition.)

value = str(src) doesn't work with Python 2 or Python 3, but putting my original code there does the trick.

For completeness, with value = str(src) the errors are:

Python2:

>           m.pass_to_pybind11_str(malformed_utf8)
E           RuntimeError: Unable to compute length of object

Python 3:

>       assert m.pass_to_pybind11_str(b"Bytes") == 5  # implicit decode
E       AssertionError: assert 8 == 5
E        +  where 8 = <built-in method pass_to_pybind11_str of PyCapsule object at 0x7f925c32d090>(b'Bytes')
E        +    where <built-in method pass_to_pybind11_str of PyCapsule object at 0x7f925c32d090> = m.pass_to_pybind11_str

[YannickJadoul]

OK, I already get where the Python 3 error originates! In Python 3, str(b"Bytes") results in 'b"Bytes"'!
Trying to understand the Python 2 error, but then there's maybe no way to avoid the C API call code duplication :-(

[YannickJadoul]

Aha. That's because the unexpected Python 2 error is a bug showing itself. I'll create a PR ;-)

[YannickJadoul]

There we go: #2392

[YannickJadoul]

(Can't believe I'm still fixing Python 2 bugs ... :-| )

[YannickJadoul]

This is quite a horribly long name.

But actually, it's not related to this PR, right? Introducing it seems like it could be another discussion/PR, since maybe we ought to deprecate this str-from-bytes behavior so it can be removed in a pybind11 3.0.0?

[rwgk]

It's not meant to be a public or permanent macro. I could also just put a couple comments marking the begin and end of the part that implements the implicit decoding. The macro has the advantage (IMO) that it stands out more and that we can slightly more easily experiment in the run-up for version 3.0.0 (you can just put a #define right before). I don't feel very strongly about macro versus comment; only that we should have some demarkation.