Fail on passing py::object with wrong Python type to py::object subclass using PYBIND11_OBJECT macro

[YannickJadoul]

Throwing this out there and seeing how it reacts to existing code, and opening this up for discussion.

As discussed in #2340, this would throw an error on e.g., py::bytes(o) with o any py::object that isn't a bytes or subclass instance.

Notes:

• I'm not happy about the error message myself. I think it throwing a type_error is OK, but I'm hoping someone has a better formulations of the error message.

[YannickJadoul]

Python 2 tests fail, but #2360 needs to be merged (or we need to disable the Python 2 tests for py::ellipsis, if #2360 doesn't get merged)

[rwgk]

I'm not sure this works correctly for nested classes. Questions:

• Is there a higher-level function to get the fully-qualified class name?
• Could we exercise with a native nested class, and a nested pybind11 extension class?

[YannickJadoul]

I know it looks horrible, but Py_TYPE(o.ptr())->tp_name is used in other places as well.
About nested types: good point. I should try. Probably something with __qualname__.

[rwgk]

I know it looks horrible, but Py_TYPE(o.ptr())->tp_name is used in other places as well.
About nested types: good point. I should try. Probably something with __qualname__.

This thought randomly crossed my mind:
The "proper" way of doing this is:

• Implement a helper function that returns the fully-qualified type name (module + qualname + tp_name) as a string.
• Unit test that separately.
• Hook it in here, the existing simple test is fine as-is.

[YannickJadoul]

Yeah, I just need to find time to check how the full name is normally retrieved :-)

[YannickJadoul]

Actually, a quick lookup confirms that tp_name is the way to go?

Pointer to a NUL-terminated string containing the name of the type. For types that are accessible as module globals, the string should be the full module name, followed by a dot, followed by the type name; for built-in types, it should be just the type name. If the module is a submodule of a package, the full package name is part of the full module name. For example, a type named T defined in module M in subpackage Q in package P should have the tp_name initializer "P.Q.M.T".

https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_name

Let me confirm with a few tests ;-)

[YannickJadoul]

Fix needed for PyPy, though, it seems. It looks like PyPy has tp_name the same as __name__. Need to check and confirm if I got that correct with half-sleepy mind.

[henryiii]

I've merged the tp_name PR, should this be rebased?

[YannickJadoul]

Submitted an issue to PyPy: https://foss.heptapod.net/pypy/pypy/-/issues/3318

I guess I'll just try to see how we can best sweep this thing under the rug?

[YannickJadoul]

This (i.e., f2424c5) should do it, I think. Is this too heavy?

The other option (while waiting for PyPy for a response and/or solution) would be to change the tests and have them accept not just ellipsis but also __builtin__.ellipsis or builtins.ellipsis.

[rwgk]

Is this too heavy?

Don't think so. Looks good to me.

[wjakob]

This change looks fine to me. Regarding:

This (i.e., f2424c5) should do it, I think. Is this too heavy?

Your change is very minor, and we have some very heavy workarounds for a few PyPy-specific problems.

[YannickJadoul]

Green! Thanks, all, for the thorough reviews!

[nyibbang]

This is a breaking change. It breaks some of my code, such as:

  const py::bytes uid = py::getattr(obj, uidAttrName, py::none());
  if (!uid.is_none())
    return deserialize(static_cast<std::string>(uid));

If the attribute was not found before, it would work fine. Now it raises an exception saying that Object of type 'NoneType' is not an instance of 'bytes'.

I know that the fix is easy to implement, but I was hoping that upgrading from pybind 2.5.0 to 2.9.0 would not break my code, according to SemVer.
I believe this patch should have been merged in a major upgrade of the library version.

[henryiii]

SemVer is not a solution. You can't get a consensus as to what a breaking change is. Every change could break someone's code, and so the only perfect SemVer is to make every single release a major version, which makes it useless. Anything else is a practical approximation.

In this case, you were relying on undefined behavior - we didn't support converting to an object subclass if the types were invalid before either, it just failed to throw an error - it just happened to work for you since the only thing you used was is_none(). But that was not tested or defined behavior. The "fix" will also work just fine in 2.5, so I consider this a strict improvement.

Now, one could argue that "None" is a special case, especially due to the existence of is_none on all types. However, this would likely complicate code, because it would force you to check for None more often, unless there is some valid way to get None for py::bytes without a conversion like this. If there is some other way to get a None currently valid, then this is possibly worth a special case.

I believe this is the fix and is valid on both 2.5 and 2.9:

  const py::object uid = py::getattr(obj, uidAttrName, py::none());
  if (!uid.is_none())
    return deserialize(static_cast<std::string>(uid));

[nyibbang]

SemVer is not a solution. You can't get a consensus as to what a breaking change is. Every change could break someone's code, and so the only perfect SemVer is to make every single release a major version, which makes it useless. Anything else is a practical approximation.

This is true, so then the decision to release a major version is decided by the risk that a change might break client code.
As stated on the SemVer website:

Use your best judgment. If you have a huge audience that will be drastically impacted by changing the behavior back to what the public API intended, then it may be best to perform a major version release, even though the fix could strictly be considered a patch release.

I could argue that this change had the potential to affect a lot of client code, especially since it concerns implicit conversions. Although I guess I'm wrong since it seems I'm the only one that got bothered by it.

In this case, you were relying on undefined behavior - we didn't support converting to an object subclass if the types were invalid before either, it just failed to throw an error - it just happened to work for you since the only thing you used was is_none(). But that was not tested or defined behavior. The "fix" will also work just fine in 2.5, so I consider this a strict improvement.

The documentation was scarce in 2.5.0 about py::object inherited types constructors, and I had to read the code to understand what they meant. I especially got confused about the conversion from py::none to py::str. It has since been improved.

I didn't realize that my code was undefined behavior when I wrote it.

I believe this is the fix and is valid on both 2.5 and 2.9:

  const py::object uid = py::getattr(obj, uidAttrName, py::none());
  if (!uid.is_none())
    return deserialize(static_cast<std::string>(uid));

That's pretty much what I've done. It's actually more sensible to write it that way compared to before.

[henryiii]

SemVer is best considered "an abbreviated changelog". A patch release has only fixes - but could break your code. A minor release has features, but mostly should be compatible with past releases - but could break your code. A major release should have large new things a user should review, and is more likely, but not guarantied, to break some older code. For a stable library, in fact, unless you are a heavy user, major versions are not likely to break you - any more likely than patch/minor releases, actually.

And it differs based on the language - in JavaScript, you have nested dependencies, so having frequent major releases is encouraged - one library can use version 2 and another can use version 3 and both work together (though there turn out to be a lot of caveats, and I'm not sure it's really "better"). You can read more about this (and my opinions on capping) here: https://iscinumpy.dev/post/bound-version-constraints/

In Python, major releases are highly discouraged, and instead deprecation periods are encouraged. If possible, warnings should be produced for 1-3 releases, and then finally removed. You should upgrade frequently in order to benefit from this. Python itself follows this (Guido recently promised at least version 3.33) - things will slowly be removed and changed, but no more major versions, to avoid a transition like 2 to 3. This is a social construct of Python packaging.

Anyway, we've done quite a bit in pybind11 to improve the checking for undefined behavior. This had reduced bugs in client code. We do try to avoid breaking changes if possible - we test quite a bit of downstream code, and changes are required that are strictly fixes, we allow that, but if other types of changes are required, we try to avoid that. Major versions will likely be based on incrementing the ABI, not on the public API, though we will likely remove a few deprecated things on major version bumps. (PS: This is probably true for Python as well - ABI3 cannot be changed without a bump to Python 4, so this will likely be a/the driving factor).