deprecate get_refcnt (#3357)

[luxedo]

Closes #3357 and PR #4065 which is now obsolete

From Python docs https://docs.python.org/3/glossary.html#term-reference-count

In CPython, reference counts are not considered to be stable or well-defined values; the number of references to an object, and how that number is affected by Python code, may be different between versions.

Things yet to be determined in this PR:

• What to do with tests that uses get_refcnt? Use #[allow(deprecated)]; Have an internal private method; Remove these assertions

We'll have a private method for internal use

• What new method to use in the documentation? I used is_none

Ok, but don't change migration.md

Other ideas:

Instead of deprecating, return an enum https://docs.python.org/3/c-api/object.html#c.PyUnstable_IsImmortal

pub enum RefCnt {
  Immortal(isize),
  Count(isize),
}

[ngoldbaum]

I'm ok with this.

While we're at it, we should add wrappers for PyUnstable_Object_IsUniquelyReferenced. Checking if the refcnt is 1 is a common idiom to find uniquely referenced object and PyUnstable_Object_IsUniquelyReferenced is supposed to replace that check.

On Pythons where it isn't available, we can do what the backport in the capicompat header does: https://github.com/python/pythoncapi-compat/blob/f6121eb60f9fe9ff227fc78f9b39d3bd2a81f434/pythoncapi_compat.h#L2224-L2238

[luxedo]

I'd be glad to give it a try. I guess in #5386 right?

[davidhewitt]

Thanks for moving this forward. I think my feeling is that we need to test refcounting internally, and we have fairly tightly controlled examples in those cases, so I'd prefer to keep the tests as they currently are until anything should eventually change which motivates changing the tests.

So I think I'd like there to be a pub(crate) replacement which we can migrate internal code to.

As for the proposal of returning an enum, I would prefer not, I think "immortal objects" are an unstable concept for now and IMO a little different from the object's reference count.

[luxedo]

So I created the internal _get_refcnt method and adapted the tests. Except for some in tests/ that will complain about the private method being called outside their crates

https://github.com/PyO3/pyo3/pull/5797/changes#diff-123d47537fa2fcfd0b823bb9b6ad3debf53b32bdc07317969421bd2aa19fa1ff

[codspeed-hq]

Merging this PR will not alter performance

✅ 105 untouched benchmarks
⏩ 1 skipped benchmark¹

---

_{Comparing luxedo:3357/deprecate-get_refcnt (92373f4) with main (3ffad5a)}

Footnotes

1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩

[davidhewitt]

Thanks, I have some suggestions how we can migrate the tests outside of the main lib.

[davidhewitt]

I think we can just remove the println! here, I have a feeling I left those in during debugging at some point 🙈

[luxedo]

Removed ✔️

[davidhewitt]

See bound_from_borrowed_ptr_constructors test in instance.rs which uses closure destructor to check for dropping. This test could migrate to use the same strategy to check the dict dropped instead of using the refcount of a sentinel object(). To avoid unsafe probably better to use Arc<AtomicBool> than the mutable bool on the stack which those tests use.

[luxedo]

This is a bit out of my comfort zone but I think at least it should look similar to this