Inherit weakref from handle

[cyyever]

The design and uses of weakref in CPython API and pybind11 show that weakref should be better treated as handle and avoid the intentional release()

[rwgk]

I can only look more tomorrow (US west coast time).

Two quick simple things:

• What was your motivation for starting this PR? (Was something broken?)

• Please do not force push. It makes it harder to follow the work. Best to only git merge master and add commits. When we're done here the commits will be squashed, so the master branch will not have the PR dev history. Having it here can be useful in the future.

[cyyever]

@rwgk Thank you. The motivation was that static analyzer reported some results of object::released calls were discarded. After digging into the code base, I found the weakref usage pattern. According to the CPython doc, the returned handle should be released in the callback. But this indicated that weakref shouldn't be considered as pybind::object.

[rwgk]

Collecting some background info:

class weakptr was introduced in pybind11 with this commit: b2c2c79

It inherited from object from the start.

It still inherits from object in nanobind (current nanobind master head):

https://github.com/wjakob/nanobind/blob/61ca0f94867fd6461f072a93d9b222bd5e3198f9/include/nanobind/nb_types.h#L629-L638

Looking in the Google codebase (which includes many 3rd-party packages), I only see a handful of weakref use cases. But this PR would break them. They'd have to remove their .release()s or make other adjustments.

My judgement call: I agree, it's better if weakref does not inherited from object, but it would be too disruptive to change that now. The gain is very small, the confusion created by the change would be much bigger.

[rwgk]

Hi @cyyever, if you could add a source code comment to explain that .release() needs to be called, that would be great. Maybe also add "See also PR #4925."

[cyyever]

Hi @cyyever, if you could add a source code comment to explain that .release() needs to be called, that would be great. Maybe also add "See also PR #4925."

After some investigation, I decided to revert the change. If the callback is passed in PyWeakref_NewRef, then the caller may want to dec_ref the weakref handle in the callback, then release should be used, otherwise, if the callback is null, then the weakref should be managed as a pybind11::object. In addition, the former pattern is used in pybind11 mainly as a caching mechanism relying on the GC strategy. Will it be more efficient to use a proper GC API?

[rwgk]

if the callback is null, then the weakref should be managed as a pybind11::object.

Oh, wow, I was wondering about that. I think it would definitely be good to explain that in a source code comment. The weakref documentation isn't clear about that (at least not to me).

will it be more efficient to use a proper GC API?

I'd ask first: what problem are we trying to solve? If it's just to squeeze out a little bit more efficiency, is that worth the human brain cycles?

IIUC you started here because of static analyzer warnings? Could we fix those with (void) casts?

[cyyever]

@rwgk I have another confusion, suppose that a callback is provided and we do normal ref-counting on the weakref object, will the callback receive a null weakref or un-triggered? The doc is unclear about this..

[rwgk]

@rwgk I have another confusion, suppose that a callback is provided and we do normal ref-counting on the weakref object, will the callback receive a null weakref or un-triggered? The doc is unclear about this..

I don't know, but similar questions crossed my mind when I was looking at the weakref documentation yesterday.

I was thinking it's necessary to inspect the Python sources, and usually I validate my conclusions by writing tests/experiments, but I totally don't have the free bandwidth for that.