`dyn FnOnce` should always be `Sync`

language design Unsafe Code Guidelines
23

Indeed, I'm not aware of any way to perform this downcast current, safe rust. But if you're writing unsafe code, there are a variety of ways to know that it has the correct type – most notably, [whoops, this wasn't correct - see geeklint's reply] the Any trait provides type_id as a trait method, and any 'static type implements Any, allowing you to do this (playground):

EDIT: Anyway, even though the above was wrong, there are other ways - as you suggest, you can pass around trait objects alongside your own records of what concrete type they are. And you can probably even check the vtables and see if they match, although there might be some gotchas with that.

24

That playground is getting the typeid of dyn FnOnce, not whatever the erased type is.

25

This isn't a sufficient guarantee, or at least not the only unstable feature that would be disrupted by this impl. It also touches type_alias_impl_trait, as follows (playground):

#![feature(type_alias_impl_trait)]
use std::cell::RefCell;

pub unsafe fn downcast_fnonce<'a, ConcreteType: 'static>(object: &'a (dyn FnOnce() + 'static)) -> &'a ConcreteType {
    unsafe {&*(object as *const dyn FnOnce()).cast::<ConcreteType>()}
}

type NonsyncClosure = impl Fn();

fn nonsync_closure(c: &'static RefCell<i32>) -> NonsyncClosure {
    || {
        *c.borrow_mut() += 1;
    }
}

fn main() {
    let c = Box::leak(Box::new(RefCell::new(1i32)));
    let nonsync_closure_reinterpreted: &'static dyn FnOnce() = Box::leak(Box::new(nonsync_closure(c)));
    
    std::thread::spawn(|| {
        let evil = unsafe { downcast_fnonce::<NonsyncClosure>(nonsync_closure_reinterpreted) };
        evil();
    });
}
26

It seems #2 is sketchy in the face of downcasting shenanigans, and #3 is a breaking change because loosening those bounds will require tightening T: Sync elsewhere. So that leaves #1 as the best option, when you can (unsafely) guarantee that you're not doing anything else that would break it.

1 Like
27

Not sure I follow (2). The downcast requires unsafe, at which point its the caster responsibility to ensure that the target type is or isn't Sync. That doesn't mean dyn FnOnce shouldn't be Sync.

28

The code that's doing the downcasting might be in a different crate than the one that's passing things between threads. They could both have safe interfaces built around their assumptions. So we need a common ground of what unsafe code is allowed to do, and that has to be upheld program-wide.

1 Like
29

This reads like a very general argument. Is this a hypothetical scenario that somebody (multiple parties) can write the safe interfaces in such a way that they accidentally misuse unsafety? If you have a more concrete example, that would help. I.e. what are the interfaces of libraries, and why they'd be assuming safety in their unsafe implementations.

30

It is hypothetical, but the point is that we have to identify what is actually "misuse" here. If it's ok for a &dyn Trait to be pointer-casted to the underlying &Foo (unsafely asserting that it really is that type), then I don't think we can use the particulars of Trait to ignore the real Sync to smuggle that reference around.

But with a wrapper like yours, you can justify it because you're making sure that & is never exposed, so Sync access never occurs.

1 Like