Skip to content

pytypes: Add Gotchas section about default-constructed wrapper types and py::none() #2362

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

Conversation

EricCousineau-TRI
Copy link
Collaborator

Resolves #2361 (through docs)

@EricCousineau-TRI
Copy link
Collaborator Author

This is the "document the status quo" solution. I'm a bit fearful about changing the behavior just yet, but will speak more to that in the issue.

@EricCousineau-TRI
Copy link
Collaborator Author

Man... trying to explain this contract and compare against other languages was a bit of a fun exercise...

@YannickJadoul
Copy link
Collaborator

I also find this very confusing, actually. None is not a str or a dict or anything in Python (just check isinstance), so it's only because Python is dynamically types that it feels like you can assign None to a str variable.

What's happening:

  • In a pybind11-bound function, isinstance(None, str) fails so pybind11 says it doesn't match the signature (type_caster basically uses isinstance).
  • In a pybind11-bound function with default arguments, pybind11 just converts default arguments (py::arg(...) = ...) as py::object and adds these values to the arguments if the user doesn't pass enough arguments. Same happens as in the first bullet, None (created by py::none()) is not a str nor dict nor ... so the actual overload resolution fails. (Yes, this is a bit weird, that you can add default arguments that can never match the type of the overload! I think it's just hard to fix, since these are all different phases of pybind11.)
  • In a normal C++ function (or lambda), there is an implicit conversion available from py::object to py::str or py::dict or ... (see py::str x = py::none() is a bit confusing; it converts None to a string, rather than setting the value to None #2361 (comment)). The C++ constructor/implicit conversion of str and dict check if the object respectively is a str or dict, and if not, calls a conversion function. In case of str, str(None) works in Python, while dict(None) results in an error.

So actually, I don't see what's wrong/broken in pybind11, and what should need explaining. This addition to the docs seems disproportionally large to me, for what's basically consistent behavior (pybind11-bound functions are consistent with Python, C++ functions are consistent with C++)?

This PR seems to add to the confusion between the actual "C++ interface for Python" with the "overload resolution of bound functions to Python".

I also strongly object against mentioning Java and C# in our docs. This is about Python and C++, and they both have a different way of handling variable. Basically, Java and C# (non-primitive) variables are all pointers and those pointers can be a null pointer. Python variables work differently, since you bind a name to an object. And the singleton None object has a different type than str, so they are not compatible. This is also why mypy distinguishes between str and Optional[str] (see https://mypy.readthedocs.io/en/stable/kinds_of_types.html#strict-optional).

NB: std::optional<py::str> should work, no? Otherwise, just 2 overloads (one with py::str, one with py::none) should also do the job.

@EricCousineau-TRI
Copy link
Collaborator Author

So actually, I don't see what's wrong/broken in pybind11, and what should need explaining [...]

I'm down for reducing the volume of the text, but I do feel that it's an edge case worth mentioning. We've spent time hashing out this behavior here and in the issue, so I believe that is sufficient evidence that it does need explaining, but perhaps more briefly.

I also strongly object against mentioning Java and C# in our docs.

I'm good with that. I intended to capture this as a rather complex contract, but yes, relating to Java or C# may confuse the point further.

This is also why mypy distinguishes between str and Optional[str] (see https://mypy.readthedocs.io/en/stable/kinds_of_types.html#strict-optional).

Good point!

NB: std::optional<py::str> should work, no? Otherwise, just 2 overloads (one with py::str, one with py::none) should also do the job.

Yup! Hence only capturing this as documentation, rather than vying for a change in functionality.

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.

OK, my original comment/review was quite harsh, looking back at it, now. Sorry :-|

But I do like this better, actually. It feels like less of a single use-case story, and more to the point.
Thanks for actually creating this PR, @EricCousineau-TRI! :-)

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.

Thanks Eric!

@EricCousineau-TRI EricCousineau-TRI changed the title pytypes: Add doc section and tests about interaction with None pytypes: Add Gotchas section about default-constructed wrapper types and py::none() Sep 4, 2020
@EricCousineau-TRI EricCousineau-TRI merged commit 44fa79c into pybind:master Sep 4, 2020
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::str x = py::none() is a bit confusing; it converts None to a string, rather than setting the value to None
6 participants