Skip to content

[BUG] Confusing weakref constructor copy the object instead of creating a weakref #2536

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

Closed
moralex1 opened this issue Sep 28, 2020 · 2 comments · Fixed by #2832
Closed

[BUG] Confusing weakref constructor copy the object instead of creating a weakref #2536

moralex1 opened this issue Sep 28, 2020 · 2 comments · Fixed by #2832

Comments

@moralex1
Copy link

moralex1 commented Sep 28, 2020

My colleague and I stumbled on a very confusing part of the API. When calling pybind11::weakref with a single argument that is of type py::object as such:


py::object WRONGcreateWeakRef(py::object object)
{
        return py::weakref(object);
}

Here is a correct way to call it:

py::object GOODcreateWeakRef(py::object object)
{
        return py::weakref(static_cast<py::handle>(object));
}

This is caused by the fact that the weakref constructor is explicit and the copy constructor is called instead. The result is that no weakref is created. This is very confusing. The copy constructor should allow only to copy weakref. May be by using a private copy constructor that takes an object and an public copy constructor that takes a weakref.

After some discussion on gitter, it seems that #2349 will solve this issue by raising a TypeError. However, the API would be still awkward.

@YannickJadoul
Copy link
Collaborator

I agree, here (also having taken part in the discussion on Gitter). Normally, handle and object are kind of interchangeable, but not here? We have a similar problem for py::str, btw, where the extra constructor applies str to the passed handle.

@YannickJadoul
Copy link
Collaborator

Fixed in #2832, @moralex1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants