Skip to content

Fixing pybind11::bytes() ambiguous conversion issue. #2442

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

Merged
merged 1 commit into from
Aug 28, 2020

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Aug 28, 2020

Adding missing bytes type to test_constructors(), to exercise the code change.

The changes in the PR were cherry-picked from PR #2409 (with a very minor
modification in test_pytypes.py related to flake8). Via PR #2409, these
changes were extensively tested in the Google environment, as summarized here:
https://docs.google.com/document/d/1TPL-J__mph_yHa1quDvsO12E_F5OZnvBaZlW9IIrz8M/
The changes in this PR did not cause an issues at all.

Note that test_constructors() before this PR passes for Python 2 only
because pybind11::str can hold PyUnicodeObject or PyBytesObject. As a
side-effect of this PR, test_constructors() no longer relies on this
permissive pybind11::str behavior. However, the permissive behavior is still
exercised/exposed via the existing test_pybind11_str_raw_str().

The test code change is designed to enable easy removal later, when Python 2
support is dropped.

For completeness: confusingly, the non-test code changes travelled through PR
#2340, #2348, and #2409; this is best ignored.

Example ambiguous conversion error fixed by this PR:

pybind11/tests/test_pytypes.cpp:214:23: error: ambiguous conversion for functional-style cast from 'pybind11::detail::item_accessor' (aka 'accessor<accessor_policies::generic_item>') to 'py::bytes'
            "bytes"_a=py::bytes(d["bytes"]),
                      ^~~~~~~~~~~~~~~~~~~~
pybind11/include/pybind11/detail/../pytypes.h:957:21: note: candidate constructor
    PYBIND11_OBJECT(bytes, object, PYBIND11_BYTES_CHECK)
                    ^
pybind11/include/pybind11/detail/../pytypes.h:957:21: note: candidate constructor
pybind11/include/pybind11/detail/../pytypes.h:987:15: note: candidate constructor
inline bytes::bytes(const pybind11::str &s) {
              ^
1 error generated.

Adding missing `bytes` type to `test_constructors()`, to exercise the code change.

The changes in the PR were cherry-picked from PR pybind#2409 (with a very minor
modification in test_pytypes.py related to flake8). Via PR pybind#2409, these
changes were extensively tested in the Google environment, as summarized here:
https://docs.google.com/document/d/1TPL-J__mph_yHa1quDvsO12E_F5OZnvBaZlW9IIrz8M/
The changes in this PR did not cause an issues at all.

Note that `test_constructors()` before this PR passes for Python 2 only
because `pybind11::str` can hold `PyUnicodeObject` or `PyBytesObject`. As a
side-effect of this PR, `test_constructors()` no longer relies on this
permissive `pybind11::str` behavior. However, the permissive behavior is still
exercised/exposed via the existing `test_pybind11_str_raw_str()`.

The test code change is designed to enable easy removal later, when Python 2
support is dropped.

For completeness: confusingly, the non-test code changes travelled through PR

Example `ambiguous conversion` error fixed by this PR:
```
pybind11/tests/test_pytypes.cpp:214:23: error: ambiguous conversion for functional-style cast from 'pybind11::detail::item_accessor' (aka 'accessor<accessor_policies::generic_item>') to 'py::bytes'
            "bytes"_a=py::bytes(d["bytes"]),
                      ^~~~~~~~~~~~~~~~~~~~
pybind11/include/pybind11/detail/../pytypes.h:957:21: note: candidate constructor
    PYBIND11_OBJECT(bytes, object, PYBIND11_BYTES_CHECK)
                    ^
pybind11/include/pybind11/detail/../pytypes.h:957:21: note: candidate constructor
pybind11/include/pybind11/detail/../pytypes.h:987:15: note: candidate constructor
inline bytes::bytes(const pybind11::str &s) {
              ^
1 error generated.
```
@YannickJadoul
Copy link
Collaborator

These changes were already once approved and merged (i.e., #2340), but not into master, since more refactoring is/was happening on str/bytes. (Later on, #2390 drastically improved the first proposed changes.)

For completeness: confusingly, the non-test code changes travelled through PR
#2340, #2348, and #2409; this is best ignored.

For historic and traceability purposes, #2340 is important and should not be ignored. The missing overload was first discovered in #2340 (comment), and I discovered and proposed the current solution later in this thread: #2340 (comment). Further down are links and the explanation on where this issue originated in pybind11's history (and why the current situation is most likely unintended).

Also, #2348 shows these changes in relation to the other proposed changes to the py::str and py::bytes types.

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

That being said, I do think we figured out by now that these changes are independent enough of the other pending changes to str and bytes, so this can probably be merged independently.

@rwgk
Copy link
Collaborator Author

rwgk commented Aug 28, 2020

Thank you @YannickJadoul and @henryiii !

@rwgk rwgk merged commit 3c061f2 into pybind:master Aug 28, 2020
@rwgk rwgk deleted the bytes_constructor_fix branch August 28, 2020 18:53
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.

3 participants