require #[pyclass] to be Sync

[davidhewitt]

For #4265

A brief attempt to add a compile error to require #[pyclass] values to be Sync. I allow this constraint to be opted-out with the existing #[pyclass(unsendable)] option.

Mostly here I've focused on implementing the compile error with a reasonable diagnostic, I think there's a lot of documentation to add too.

[ngoldbaum]

I rebased this and am looking at resolving the test failures.

The first thing I hit was test_getter_setter::cell_getter_setter, which is mostly testing that IntoPyObject for Cell<i32> works correctly. I think in order to get to a similar level of ergonomics for atomics and mutexes, we'll need to add IntoPyObject impls for all the atomic types and maybe for Mutex and RWLock? Otherwise you run into errors like

error[E0599]: no method named `into_pyobject` found for struct `AtomicI32` in the current scope
   --> tests/test_getter_setter.rs:232:42
    |
232 |         let cell = AtomicI32::new(20i32).into_pyobject(py).unwrap();
    |                                          ^^^^^^^^^^^^^ method not found in `AtomicI32`

Let me know if anyone thinks alternate approaches would be better.

[ngoldbaum]

Although that said, this might also be a separate issue:

error[E0277]: the trait bound `AtomicI32: PyFunctionArgument<'_, '_>` is not satisfied
   --> tests/test_getter_setter.rs:219:1
    |
219 | #[pyclass]
    | ^^^^^^^^^^ the trait `Clone` is not implemented for `AtomicI32`, which is required by `AtomicI32: PyFunctionArgument<'_, '_>`

[ngoldbaum]

The latest push switches members that use Cell for interior mutability or have Box members to atomics and mutexes. Now the only remaining issue with the cargo tests is the problems raised by test_getter_setter described above.

[codspeed-hq]

CodSpeed Performance Report

Merging #4566 will not alter performance

_{Comparing davidhewitt:pyclass-sync (ba09a08) with main (9f955e4)}

Summary

✅ 83 untouched benchmarks

[ngoldbaum]

If I add an IntoPyObject impl for AtomicI32 I still get the error about a missing Clone impl. However based on this answer to a similar question, I'm now thinking that the real issue is the generated get and set methods should be taking references to the inner value. I'm currently trying to understand how the get and set descriptors actually get set up inside the pyo3 proc macro, but it's pretty complicated.

[ngoldbaum]

This ultimately comes down to the implementation of the PyClassGetterGenerator class. It delegates in this case to a function called pyo3_get_value_into_pyobject which has a Clone bound on the FieldT parameter. Since atomics aren't Clone, this leads to the compilation error I'm seeing.

I think a way forward is to expand this machinery to recognize fields that are atomics (maybe via a macro to generate all the necessary code for each atomic type?).

I'm going to stop here because this is getting pretty far afield and someone with more rust expertise might be able to give an alternate path forward.

[Icxolu]

Can you try implementing IntoPyObject for &AtomicI32? I think this should fix the error. #4449 has the list of priorities for pyo3(get).

[davidhewitt]

I think the problem will be what kind of atomic ordering should we use? I guess Relaxed might be enough here.

[ngoldbaum]

Can you try implementing IntoPyObject for &AtomicI32

I tried adding impls for both and am still getting the same error, see last push. I get the same error if I do the impl for either of or both &AtomicI32 and Atomic32.

[ngoldbaum]

I get the same error if I do the impl for either of or both &AtomicI32 and Atomic32.

Actually this is not the same error. We fixed the issue in the GetterGenerator but now there's a different issue in the argument extraction.

[davidhewitt]

Actually, I had a better idea; let's not worry about atomics in this PR and instead just add unsendable designator to this class for now.

[davidhewitt]

Opened #4668 for the atomics question. For now @ngoldbaum I force-pushed here.

I think there is copy that needs to be added to the migration guide and also maybe class.md where we talk about Send, we should probably now also talk about Sync. I can try to have a push at getting the docs done tonight.

[ngoldbaum]

I think there is copy that needs to be added to the migration guide and also maybe class.md where we talk about Send, we should probably now also talk about Sync. I can try to have a push at getting the docs done tonight.

Thanks for taking this on! I added a WIP changelog entry that should link to this. Should the changelog entry give a brief explanation for why we're doing this?

[ngoldbaum]

Woohoo, tests are green! Just needs docs I think. I may try to write something if David doesn't get to it.

[mejrs]

I don't like the stderr output; it's a lot of text. Is there a way to reduce that?

[mejrs]

Is this the same logic that Exclusive uses? If so, I would prefer that we use that (or our own version of it) rather than this impl.

[ngoldbaum]

I marked this ready for review, but I think this hasn't been looked at yet. I'll leave that to David since I'm not sure if using Exclusive makes sense.

[mejrs]

I'd prefer "replace non-Sync things by Sync things" - not sure on the wording for that.

[mejrs]

I'd like the tone to be more rustc-y. Also, I feel like "needs to implement Sync" could be understood as "I need to unsafe impl Sync for MyClass" - we should avoid that.

Suggested change

	label = "needs to implement `Sync` to be `#[pyclass]`",
	note = "required by `#[pyclass]`",

[ngoldbaum]

Is there a way to reduce that?

I think the most straightforward way to get a shorter error message is to mark the pyclass itself with on_unimplemented for Sync. But according to the RFC for diagnostic::on_unimplemented adding this on types is a "future possibility", and I think it's not currently possible.

Perhaps it's possible to refactor this so that there's a little less indirection and maybe get rid of one of the frames in the error message? I don't immediately see how.

[ngoldbaum]

I tried refactoring this to avoid the long error and couldn't figure it out. I hoped that if I made PyClassSync have a blanket impl for Sync that I might be able to generate the diagnostic but it looks like the compiler ignores PyClassSync in that case, so the default generics trickery used here or something like it does seem to be necessary to get on_unimplemented to trigger.

[ngoldbaum]

I think the big to-do here is docs. @davidhewitt would you like me to take a look at that sometime this week or do you still want to take a crack at it yourself?

[davidhewitt]

I hope to make various progress on release prep & final pass over docs tomorrow, but feel free to push here if you have time.

Otherwise maybe I'll merge this PR as is and we can have one or more docs follow ups to round off the release 😁

[mejrs]

Can you please update (or maybe delete) this line and the discussion it references? I'm fine with docs being added in followup PRs but it's bad form to leave docs in a wrong (rather than just incomplete or missing) state.

[ngoldbaum]

Done, thanks for pointing out that I missed that.

[davidhewitt]

Super, thanks. Let's merge this and then we can parallelise finishing work on docs for 0.23 👍