`dyn FnOnce` should always be `Sync`

language design Unsafe Code Guidelines
#1

It appears right now that there are no special rules for FnOnce related to Sync. This makes things like Arc<Box<dyn FnOnce() + Send>> impossible, which is annoying.

I believe dync FnOnce is always Sync because there are no (unsafe) operations that could be done on &FnOnce (from multiple threads). The only operation is consuming self, so it's not available behind a reference.

#2

There is no need for special rules, can be just a supertrait (in case this is indeed sound - haven't checked it). Edit: Oh it will make Fn and FnMut require Sync too.

#3

Why would we want this at all? What does this enable? You cannot do anything with it!

#4

This is also incompatible with multiple dyn Traits, because this will allow dyn FnOnce() + Clone for example.

#5

Arc is useful because you can share it. Eventually you can extract it via Arc::into_raw and execute. See also - Lift Sync requirement by kvark · Pull Request #3 · kvark/choir · GitHub

#6

And how do you call it after? AFAIK we have no way to call dyn FnOnce on a raw pointer.

#7

Sorry for confusion! I meant Arc::try_unwrap, which gives you the value back. See relevant code for example.

#10

Oh I got it, I think. But it already requires Send to be useful, so why not write the Sync explicitly too?

You essentially want to work around a limitation of the type system. You don't really share the value, only send it, but the type system is unable to see it. This is essentially the same question that I remember appeared in this forum "how to send a Rc to another thread when I know it has no other references". Nothing special to FnOnce except that we, the humans, can prove it is always sound. So why not do what was recommended in the Rc case and use an unsafe wrapper that implements Sync to move it the thread? Especially when the details around the soundness of this are so subtle.

#11

Why should dyn FnOnce specifically have this magic? For another example dyn Future also has no &self operations available.

Instead of using any magic you can do this using user code trivially, see SyncWrapper which can generically allow passing non-Sync data around as Sync by blocking all &self access.

6 Likes
#12

It is different from that since Rc is not Send.

Basically what @kvark is looking for is a "channel" to "send" a piece of data from one thread to another -- Send should be enough. However, they are using Arc for this purpose, which requires T: Sync for Arc<T>: Send (and for good reasons). I think what you'd really want here is a different data structure, something more like mpsc::sync_channel(1).

#13

I don't want my API to constrain more than necessary. It's a task system. Send is assumed, since tasks are executed on other threads. But Sync makes no sense there.

So why not do what was recommended in the Rc case and use an unsafe wrapper that implements Sync to move it the thread?

This is what I'm doing currently - Lift Sync requirement by kvark · Pull Request #3 · kvark/choir · GitHub

And it's all fine, technically, since unsafety is totally private to the implementation. However, there are some forces/people in the community that really frown upon unsafe code. I.e. having this single unsafe means I can no longer cater to the group of people who prefer "no unsafe". See TinyVec for example.

Why should dyn FnOnce specifically have this magic? For another example dyn Future also has no &self operations available.

The question is a bit different. It's whether or not Rust compiler (or the standard library) allow for these specific rules to be written. If it does, then I think it would make sense to encode those extra rules, making dyn Future and dyn FnOnce more useful in safe code. If the rules are non-writable, then we can just go home and put unsafe in our code.

@RalfJung I'm using Arc because I really need reference counting there, in addition to sending it across. A task is only extracted via Arc::try_unwrap if it has no live dependencies, which is when it can be scheduled for execution. I don't believe this usecase is begging for anything different than Arc.

#14

Arc has deref, and that's why we need T: Sync for Arc<T>: Send. It seems like you don't need deref for your type, you only need try_unwrap. So to me that sounds like you don't want Arc, you want something more targeted. (Maybe Arc<SyncWrapper<T>> is good enough though.)

1 Like
#15

I guess that's another way to look at this. I could work with Arc without Deref. Technically, what Arc could do is having the Deref require T: Sync.

That would mean I could use Arc normally on a non-sync data as long as I don't deref it. And it looks like the compiler would be happy with such an implementation (based on my playground sample). Is there any reason to not do it in std?

#16

That might work. (There might be more operations that also need that bound. Basically everything that hands out a &T.)

I wonder what that would do to our Arc soundness proof in RustBelt...

#17

That would be a breaking change, because you can already Arc::new with !Sync types and use most of the methods and trait implementations, including Deref. The only place we currently require T: Sync + Send at all is for Arc<T>'s own Sync and Send.

2 Likes
#18

Ok, all in all, our discussion concluded in 3 different solutions:

  1. use unsafe code (or other library with unsafe code), since the compiler doesn't see far enough
  2. investigate it the compiler can make dyn FnOnce: Sync
  3. investigate if Arc can soften up its bounds

I've already done (1) on my end. What would be the next steps about (2) and/or (3)? Filing an RFC? Making a PR into Rust?

#19

Hmm, maybe we can just do this in core, without compiler help:

unsafe impl<'a, Args, Output> Sync for dyn FnOnce<Args, Output = Output> + 'a {}
unsafe impl<'a, Args, Output> Sync for dyn FnOnce<Args, Output = Output> + Send + 'a {}

unsafe impl<'a, Args, Output> Sync for dyn FnMut<Args, Output = Output> + 'a {}
unsafe impl<'a, Args, Output> Sync for dyn FnMut<Args, Output = Output> + Send + 'a {}

The compiler does accept that, but I don't know if there are any caveats.

In general, I think something like SyncWrapper is fine though.

1 Like
#20

In current Rust, is there any rule that would forbid someone from writing unsafe code to downcast a &dyn Trait into a &ConcreteType, if the programmer can prove that the trait object is of that concrete type? It seems like that is something that a person might expect to be allowed to do using unsafe code, and adding this Sync impl would deny that.

#21

No, and this is exactly what Any is automating. What this is taking advantage of to be sound, though, is that the Fn* traits can't be implemented manually for a known/nonopaque type.

Adding those impls would either tie the Fn* traits to either be unsafe (you must maintain this property manually) or magicked by the compiler to only ever be implemented for types where this impl is sound.

1 Like
#22

I feel like I'm missing something obvious, but how do you get a dyn Any from a dyn FnOnce? I thought you needed the trait to explicitly support something like as_any? Or even with bespoke unsafe code, wouldn't you need a way to dig out a TypeId or similar?

Hmm, or maybe you mean a more mundane pointer comparison -- "Pointer 0xF00123 is in my collection of known Foo pointers, so cast it!"