Skip to content

Commit

Permalink
Fix handling of overloaded __new__ that takes no arguments
Browse files Browse the repository at this point in the history
  • Loading branch information
oremanj committed Nov 14, 2024
1 parent 4d55fbb commit dcb77ac
Show file tree
Hide file tree
Showing 7 changed files with 92 additions and 7 deletions.
5 changes: 5 additions & 0 deletions docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ Version TBD (unreleased)
``std::variant<U, T>`` might cast a Python object wrapping a ``T`` to a ``U`` if
there is an implicit conversion available from ``T`` to ``U``.

- Restored support for constructing types with an overloaded ``__new__`` that
takes no arguments, which regressed with the constructor vectorcall
acceleration that was added in nanobind 2.2.0.
(issue `#786 <https://github.com/wjakob/nanobind/issues/786>`__)

- Added a function annotation :cpp:class:`nb::call_policy\<Policy\>()
<call_policy>` which supports custom function wrapping logic,
calling ``Policy::precall()`` before the bound function and
Expand Down
16 changes: 13 additions & 3 deletions include/nanobind/nb_class.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,13 @@ enum class type_flags : uint32_t {
is_generic = (1 << 15),

/// Does the type implement a custom __new__ operator?
has_new = (1 << 16)
has_new = (1 << 16),

// Two more bits bits available without needing a larger reorganization
/// Does the type implement a custom __new__ operator that can take no args
/// (except the type object)?
has_nullary_new = (1 << 17)

// One more bit available without needing a larger reorganization
};

/// Flags about a type that are only relevant when it is being created.
Expand Down Expand Up @@ -430,7 +434,13 @@ struct new_<Func, Return(Args...)> {
// replacing it; this is important for pickle support.
// We can't do this if the user-provided __new__ takes no
// arguments, because it would make an ambiguous overload set.
detail::wrap_base_new(cl, sizeof...(Args) != 0);
constexpr size_t num_defaults =
((std::is_same_v<Extra, arg_v> ||
std::is_same_v<Extra, arg_locked_v>) + ... + 0);
constexpr size_t num_varargs =
((std::is_same_v<detail::intrinsic_t<Args>, args> ||
std::is_same_v<detail::intrinsic_t<Args>, kwargs>) + ... + 0);
detail::wrap_base_new(cl, sizeof...(Args) > num_defaults + num_varargs);

auto wrapper = [func = (detail::forward_t<Func>) func](handle, Args... args) {
return func((detail::forward_t<Args>) args...);
Expand Down
27 changes: 25 additions & 2 deletions src/nb_func.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -432,8 +432,31 @@ PyObject *nb_func_new(const void *in_) noexcept {
type_data *td = nb_type_data((PyTypeObject *) f->scope);
bool has_new = td->flags & (uint32_t) type_flags::has_new;

if (is_init && !has_new) {
td->init = func;
if (is_init) {
if (!has_new) {
td->init = func;
} else {
// Keep track of whether we have a __init__ overload that
// accepts no arguments (except self). If not, then we
// shouldn't allow calling the type object with no arguments,
// even though (for unpickling support) we probably do have
// a __new__ overload that accepts no arguments (except cls).
// This check is necessary because our type vectorcall shortcut
// skips Python's usual logic where __init__ is always called
// if __new__ returns an instance of the type.
bool noargs_ok = true;
for (size_t i = 1; i < fc->nargs - has_var_kwargs; ++i) {
if (has_var_args && i == fc->nargs_pos)
continue; // skip `nb::args` since it can be empty
if (has_args && fc->args[i].value != nullptr)
continue; // arg with default is OK
noargs_ok = false;
break;
}
if (noargs_ok) {
td->flags |= (uint32_t) type_flags::has_nullary_new;
}
}
} else if (is_new) {
td->init = func;
td->flags |= (uint32_t) type_flags::has_new;
Expand Down
7 changes: 5 additions & 2 deletions src/nb_type.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -968,12 +968,15 @@ static PyObject *nb_type_vectorcall(PyObject *self, PyObject *const *args_in,
self = inst_new_int(tp, nullptr, nullptr);
if (!self)
return nullptr;
} else if (nargs == 0 && !kwargs_in && nb_func_data(func)->nargs != 0) {
} else if (nargs == 0 && !kwargs_in &&
!(td->flags & (uint32_t) type_flags::has_nullary_new)) {
// When the bindings define a custom __new__ operator, nanobind always
// provides a no-argument dummy __new__ constructor to handle unpickling
// via __setstate__. This is an implementation detail that should not be
// exposed. Therefore, only allow argument-less calls if there is an
// actual __new__ overload with a compatible signature.
// actual __new__ overload with a compatible signature. This is
// detected in nb_func.cpp based on whether any __init__ overload can
// accept no arguments.

return func->vectorcall((PyObject *) func, nullptr, 0, nullptr);
}
Expand Down
18 changes: 18 additions & 0 deletions tests/test_classes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <nanobind/stl/pair.h>
#include <nanobind/stl/shared_ptr.h>
#include <nanobind/stl/tuple.h>
#include <nanobind/stl/unique_ptr.h>
#include <map>
#include <memory>
#include <cstring>
Expand Down Expand Up @@ -658,6 +659,23 @@ NB_MODULE(test_classes_ext, m) {
.def("value", &UniqueInt::value)
.def("lookups", &UniqueInt::lookups);

// issue #786
struct NewNone {};
struct NewDflt { int value; };
struct NewStar { size_t value; };
nb::class_<NewNone>(m, "NewNone")
.def(nb::new_([]() { return NewNone(); }));
nb::class_<NewDflt>(m, "NewDflt")
.def(nb::new_([](int value) { return NewDflt{value}; }),
"value"_a = 42)
.def_ro("value", &NewDflt::value);
nb::class_<NewStar>(m, "NewStar")
.def(nb::new_([](nb::args a, int value, nb::kwargs k) {
return NewStar{nb::len(a) + value + 10 * nb::len(k)};
}),
"args"_a, "value"_a = 42, "kwargs"_a)
.def_ro("value", &NewStar::value);

// issue #750
PyCFunctionWithKeywords dummy_init = [](PyObject *, PyObject *,
PyObject *) -> PyObject * {
Expand Down
11 changes: 11 additions & 0 deletions tests/test_classes.py
Original file line number Diff line number Diff line change
Expand Up @@ -888,6 +888,17 @@ def test46_custom_new():
with pytest.raises(RuntimeError):
t.UniqueInt.__new__(int)

# Make sure we do allow no-args construction for types that declare
# such a __new__
t.NewNone()
assert t.NewDflt().value == 42
assert t.NewDflt(10).value == 10
assert t.NewStar().value == 42
assert t.NewStar("hi").value == 43
assert t.NewStar(value=10).value == 10
assert t.NewStar("hi", "lo", value=10).value == 12
assert t.NewStar(value=10, other="blah").value == 20

def test47_inconstructible():
with pytest.raises(TypeError, match="no constructor defined"):
t.Foo()
Expand Down
15 changes: 15 additions & 0 deletions tests/test_classes_ext.pyi.ref
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,21 @@ class MyClass:

def f(self) -> None: ...

class NewDflt:
def __init__(self, value: int = 42) -> None: ...

@property
def value(self) -> int: ...

class NewNone:
def __init__(self) -> None: ...

class NewStar:
def __init__(self, *args, value: int = 42, **kwargs) -> None: ...

@property
def value(self) -> int: ...

class NonCopyableVec:
pass

Expand Down

0 comments on commit dcb77ac

Please sign in to comment.