implement PyTracebackMethods and PySliceMethods

[Icxolu]

I took a stab at porting PyTraceback and PySlice over to the new Bound API from #3684.

Mostly mechanical changes and fixes to make the compiler happy again.

[codspeed-hq]

CodSpeed Performance Report

Merging #3763 will degrade performances by 10.44%

_{Comparing Icxolu:slice-traceback (7fddd98) with main (f449fc0)}

Summary

❌ 1 regressions
✅ 77 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	Icxolu:slice-traceback	Change
❌	extract_int_downcast_fail	238.3 ns	266.1 ns	-10.44%

[davidhewitt]

Brilliant, thank you for helping us get this API over the finish line!

There's a few minor comments, other than those this looks great.

Are you interested in helping out with more of these? (And if so, have you got a next item that you plan to pick up?)

Also, I'd love to hear any thoughts you have about this new Bound API. The documentation on it is still quite thin but you've done a good job following the patterns we've established so far with it. If you had any thoughts about the name, or documentation which is missing, or anything else, now before release is a great time for us to make stuff better 😂

[davidhewitt]

Also, don't worry about the codspeed "regressions" here, there's a bit of noisy volatility on a couple of particular benchmarks that probably just needs tidying up a bit.

[Icxolu]

Thanks for the warm words!

Are you interested in helping out with more of these? (And if so, have you got a next item that you plan to pick up?)

I am, PyComplex seems to look like a simple one i might try next

Also, I'd love to hear any thoughts you have about this new Bound API. The documentation on it is still quite thin but you've done a good job following the patterns we've established so far with it. If you had any thoughts about the name, or documentation which is missing, or anything else, now before release is a great time for us to make stuff better

I do like this new API, I think it makes a lot of sense to decouple the GIL lifetime, from the Rust reference lifetime. For me the naming makes sense. At least Bound does right now, for Borrowed i can see the intent, but I haven quite got the difference and overlap of Borrowed<'a, 'py, _> vs &'a Bound<'py, _> and when one would use one over the other. This is something i would need to look into in more detail, and this will probably deserve a section in the guide. I guess this has to do with that it describes something that could not be expressed with the current GIL-refs.

[davidhewitt]

Overall looking great! Fortunately renaming the method to traceback_bound helped catch a case where the change in ownership means we need to make an adjustment to avoid a dangling pointer. I think after that's addressed, this will be good to merge.

I am, PyComplex seems to look like a simple one i might try next

Fantastic, thank you so much!

I do like this new API, I think it makes a lot of sense to decouple the GIL lifetime, from the Rust reference lifetime. For me the naming makes sense. At least Bound does right now, for Borrowed i can see the intent, but I haven quite got the difference and overlap of Borrowed<'a, 'py, _> vs &'a Bound<'py, _> and when one would use one over the other. This is something i would need to look into in more detail, and this will probably deserve a section in the guide. I guess this has to do with that it describes something that could not be expressed with the current GIL-refs.

Yes Borrowed is definitely the only remaining rough edge. It's mostly forced upon us because some CPython APIs return object references that do not carry ownership, e.g. PyTuple_GetItem. To make those sound, we either have to increase the reference count and make them into a full Bound, or in some cases where we know it's safe to pass around the borrowed reference, we can make Borrowed.

As much as possible I've been trying to keep Borrowed in the user-facing API isolated to higher-performance alternatives, e.g. PyTupleMethods::get_borrowed_item. Because I agree with you that most people should be thinking in Bound and &Bound, especially while these are new ideas.

I think I need to send a PR to types.md in the guide to try to explain these new types ASAP.

[davidhewitt]

Interestingly here I think we now end up introducing a dangling pointer, because the .map_or will throw away the original bound.

So here we need to do something like

            let traceback = self.traceback_bound(py);
            ffi::PyErr_Display(
                 self.get_type(py).as_ptr(),
                 self.value(py).as_ptr(),
                 traceback.as_ref().map_or(std::ptr::null_mut(), Bound::as_ptr)
           )

It'll be hard to observe a crash from the dangling pointer because traceback will continue to be owned on the error object, but in a free threaded Python future there is just the tiniest possibility that some other thread assigns to __traceback__ on this error at the same time as this is running, and that could then cause a use-after-free.

[Icxolu]

Oh wow, very subtle. But I agree, the bound traceback gets dropped after the argument got evaluated, rendering the pointer invalid. I changed it to your suggestion and added a small comment for future reference.

[davidhewitt]

This looks great, thanks!

The test_compile_error failure is an annoying one; that particular UI test has been unstable lately, though it seems that for a given set of sources it is at least deterministic. It's been trending towards the output which we're getting now in this PR, so hopefully once this is merged we'll have a new stable output...

If you run TRYBUILD=overwrite cargo test --test test_compile_error then you should find it updates the output locally for you. Otherwise, I can push a commit for it.

[davidhewitt]

Ah, maybe I should have said, the UI tests are sensitive to Rust version so you'll need to be running exactly current stable (1.75) - older or newer and the output will break CI 🙈

[davidhewitt]

Looks like GitHub actions flakiness...