Capsule: simplify non zero size assertion

[Tpt]

It is now possible to do assertions in const functions, let's use that

[Icxolu]

Do we need the trait anymore than? We could use a const fn now or potentially even inline it in a const { ... } block. It might be worth to check how the error message looked like before and after to make sure we don't make this too confusing. (Do we need #[track_caller] for a const panic?)

[Tpt]

We could use a const fn now or potentially even inline it in a const { ... } block.

Great call! I tried calling directly the const fn but it was not working because the evaluation was done at runtime. Wrapping it in a const { works. I have not inlined it to make code I think a bit cleaner. Done.

It might be worth to check how the error message looked like before and after to make sure we don't make this too confusing

Yes! The new one is quite nice:

error[E0080]: evaluation panicked: PyCapsule value type T must not be zero-sized!
   --> src/types/capsule.rs:109:17
    |
109 |         const { assert_not_zero_size::<T>() }
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ evaluation of `types::capsule::PyCapsule::new_with_destructor::<(), {closure@src/types/capsule.rs:93:52: 93:58}>::{constant#0}` failed here

Do we need #[track_caller] for a const panic?

Yes, this way we avoid the not useful note:

note: inside `assert_not_zero_size::<()>`
   --> src/types/capsule.rs:566:5
    |
566 | /     assert!(
567 | |         size_of::<T>() != 0,
568 | |         "PyCapsule value type T must not be zero-sized!"
569 | |     )
    | |_____^ the failure occurred here

[Icxolu]

Would it make sense to add a #[track_caller] here (and above) as well, so that the error does not point into pyo3 code?

[davidhewitt]

I wonder, do we even need this assertion? I confess I forget why the original restriction was necessary, the best source I can find is #1980 (comment), where the restriction supposedly fixed a segfault (which might have just been due to an incorrect cast of the value coming out of the box).

I can see why it's not helpful to ever want a ZST in a capsule (serves no purpose), so maybe we can keep this restriction and just clarify better for users why we don't accept it?

[Tpt]

I wonder, do we even need this assertion?

Great point! Box::into_raw never returns a null pointer so it seems safe to me.

I can see why it's not helpful to ever want a ZST in a capsule (serves no purpose), so maybe we can keep this restriction and just clarify better for users why we don't accept it?

Maybe this restriction can be painful for some generic code? If they work, it seems more the realm of a warning than of an error. But I do not have a strong opinion on that, glad to tweak the MR to whatever the consensus is.

[Tpt]

I have opened #5889 that remove the zero-sized type limit and added a test that seems to pass