deprecate PyRef(Mut) (Part 1)

[Icxolu]

First step of deprecating PyRef and PyRefMut. This PR deprecates the types itself (targeting 0.30.0 as per #6083 (comment), so leaving as draft for now)

We will need a migration guide entry, but I would like to defer that to the second part when I deprecate the borrow methods.

See also #6083

[Icxolu]

I just expected these for now. PyClassGuard(Mut) does give the same error, PyClassGuard could be allowed I guess, since it is not mutable and does not give access to a Python token. Not sure if that's worth the effort though

[codspeed-hq]

Merging this PR will degrade performance by 8.32%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 2 regressed benchmarks
✅ 138 untouched benchmarks
⏩ 6 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
❌	into_u128_zero	816 ns	981.3 ns	-16.84%
❌	into_u128_small	876.1 ns	1,041.4 ns	-15.87%
⚡	test_empty_class_init	31.6 µs	28.7 µs	+10.14%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing Icxolu:deprecate/pyref (6922a94) with main (ee629fb)}

Footnotes

1. 6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[davidhewitt]

Sorry to be so slow to review this; it's on my mind, I've just been wading a bit through a ~LIFO PR stack.

Interestingly I noted when working on #6407 today that PyClassGuard::deref is potentially quite side-effectful. It goes through PyClassObjectLayout::get_ptr which performs a lot of the type-lookup machinery discussed in that PR (if using abi3 / abi3t variable object layout).

I somewhat wonder whether this suggests that we should calculate the contents ptr of PyClassGuard up-front and store it in the guard, similar to what I do with the PyClassTraverseGuard in #6407.

I'm not sure if it's too late and counts as breaking? Hopefully not, given PyClassGuard is still only semi-public.

[Icxolu]

No worries 🙂

I think we could cache it. We would loose the #[repr(transparent)] but since our field is private and we have not explicitly documented that, it is not part of our public API according to the Nomicon. Otherwise I think it would purely be an internal change (from a quick look).

I just wonder whether we should and if this would be the best place, Are you concerned because this is Deref specifically? Basically everything calling the any of the contents methods has the same side effects (which also includes the Deref for PyRef, #[pyo3(get)], ...). My main concern would be that it creates a tighter coupling with the quite complex internals of the class layouts, that could easily get missed if we ever refactor those again. But if you see enough benefit to caching I'd be ok with it. In any case that should be a separate precursor or followup PR.

[davidhewitt]

I just wonder whether we should and if this would be the best place, Are you concerned because this is Deref specifically?

Yes, precisely because it's on Deref which I think most people will imagine to be cheap. I worry about code such as

let guard = obj.try_borrow_guard();
let x = guard.x;
let y = guard.y;
let z = guard.z;

... which seems like three cheap field accesses but really is re-resolving the Rust data each time. I concede that #[pyo3(get)] and similar access from Python would do that, however I think everyone expects Python attribute lookup to do all sorts of possible crazy 😬

One alternative to unblock this could be to remove the Deref implementation and add a .lookup_data() method which is explicit enough that it might do work? That would allow us to decide in the future whether to add caching into the guard. (Though it's not impossible that might break some users given PyClassGuard is reachable - possibly more breaking than adding the cache.)

I think I'd like to figure out what we do about this (or decide it's a non-issue) before deprecating PyRef, just so that users go through the API churn in one go.