search

madsmtm / objc2

Bindings to Apple's frameworks in Rust
https://docs.rs/objc2/
MIT License
290 stars 35 forks source link

Improve `block2` #168

Closed madsmtm closed 4 months ago

madsmtm commented 1 year ago

I haven't given this crate nearly as much love as it deserves!

In particular, it would be nice to allow mutating blocks, e.g. FnOnce and FnMut should become BlockOnce and BlockMut.

Also, I'm not certain the way ConcreteBlock is put on the stack is sound? And what about lifetime of the stuff used in the closure?

Issues in block

Work on this in other projects

madsmtm commented 1 year ago

BlockOnce, or whatever it will end up as, should try to (when debug assertions are enabled) ensure that it is actually only called once.

madsmtm commented 6 months ago

I'm starting to suspect that the naive approach of adding BlockMut/BlockOnce is actually a bad idea, and that we instead should be looking at Swift block semantics instead.

The following attributes affect blocks in particular:

Which seem to suggest that maybe we should instead have these four: Block, EscapingBlock, SendableBlock, EscapingSendableBlock?

madsmtm commented 6 months ago

@escaping is basically exactly equivalent to a 'static bound on the closure.

Although, escaping closures can’t capture mutable references, while non-escaping closures can, so maybe we can allow FnMut for non-escaping closures? Except Swift's capturing rules are a bit different, seems like escaping closures sometimes actually can capture mutable references?

madsmtm commented 6 months ago

I think we do want some way to have FnOnce-like blocks, it is a very common pattern.

E.g. basically all completionHandler:'s are FnOnce + 'static, they're just only attributed as @escaping. So we'll have to enrich framework data again sigh

madsmtm commented 5 months ago

Send + Sync vs. @Sendable:

A impl Fn() + Sync can easily be turned into impl Fn() + Send or impl Fn() + Send + Sync, while the opposite is not true. So impl Fn() + Sync is more restrictive on the user, but might not be necessary? The few Objective-C APIs that use NS_SWIFT_SENDABLE blocks seem like they only use the block from a single thread at a time; though to be safe, we'll probably have to be overly strict here.

madsmtm commented 5 months ago

Design idea:

struct Inner<'a, A, R, F> { ... }

pub type Block<'a, A, R> = Inner<A, R, dyn FnMut(A) -> R + 'a>;
pub type BlockMut<'a, A, R> = Inner<A, R, dyn FnMut(A) -> R + 'a>;
pub type BlockOnce<'a, A, R> = Inner<A, R, dyn FnOnce(A) -> R + 'a>;

pub type BlockSend<'a, A, R> = Block<A, R, dyn Fn(A) -> R + Send + 'a>;
// And so one for `BlockSendMut`, `BlockSendOnce`, `BlockSync`...

fn takes_sendable_block(block: &BlockSendSync<'static, (u32,), u8>) {}
fn takes_noescape_block(block: &Block<'_, (u32, u32), u32>) {}
fn takes_mutating_block(block: &mut BlockMut<'static, (), ()>) {}

This is kinda nice since we avoid a proliferation of traits that way.

Though reconsidering, maybe we can actually make it as simple as:

struct Block<F> { ... magic ... }

fn takes_sendable_block(block: &Block<dyn Fn(u32) -> u8 + Send + Sync>) {}
fn takes_noescape_block(block: &Block<dyn Fn(u32, u32) -> u32 + '_>) {}
fn takes_mutating_block(block: &mut Block<dyn FnMut()>) {}

That way, we should be able to support all possible configurations, and then we can let icrate/header-translator figure out which one is actually safe for a given function.

madsmtm commented 5 months ago

EDIT: Moved to https://github.com/madsmtm/objc2/issues/573.

madsmtm commented 4 months ago

Ideally, we'd be able to just write something like:

#[method(myMethodTakingBlock:)]
fn my_method(&self, block: impl IntoBlock<dyn Fn(i32) -> i32 + Send + '_>);

// impl<F: Fn(i32) -> i32 + Send> IntoBlock<dyn Fn(i32) -> i32 + Send + '_> for F { ... }

obj.my_method(|n| {
    n + 4
});

Though I think that may require us to merge the objc2 and block2 crates, since we:

Which means there's no longer a clear dependency relation between these two crates :/

We might also just be able to hack it in the #[method(...)] macro implementation.

Alternatively, allowing &dyn Fn() directly by implementing ConvertArgument might be possible? Though is it desirable? It will probably always require an allocation, which would usually have been possible for the user to avoid by taking &Block<dyn Fn()>.

madsmtm commented 4 months ago

On further reflection, I think FnOnce is fundamentally incompatible with the way the blocks runtime works.

In Rust, you are allowed to do two things with an FnOnce; Drop it, or call it, and hence move ownership.

Any C code that takes a block fundamentally cannot uphold these guarantees without us writing extra glue code. This is due to how destructors in blocks are implemented; you (or the compiler) calls Block_release separately from the invocation of the block (You could write code that assumed that invoking a block means you don't have to call Block_release, but that's not part of the spec, and that's not how Clang implements it).

I guess we could implement it such that when calling the block, we read the memory of it, and in the destructor we check whether the block has been called or not. For that we'd need to store a piece of state on whether or not the destructor has been run. Which... Is trivially doable in safe Rust with an Option!

pub fn fnonce_to_fnmut(f: impl FnOnce()) -> impl FnMut() {
    // Or with `ManuallyDrop`, if you wanted to micro-optimize
    // with the assumption that the block is always called.
    let mut container = Some(f);
    move || {
        // Or with `unwrap_unchecked`, if you wanted to micro-optimize
        // with the assumption that it is only called once
        if let Some(f) = container.take() {
            f()
        }
    }
}

Given that the implementation above is trivial and safe, and that it also varies what kind of micro-optimizations you are willing to make (depends on the guarantees of the API you're calling), I don't think we should attempt to provide a block wrapper around FnOnce.

madsmtm commented 4 months ago

Similarly to https://github.com/madsmtm/objc2/issues/563, I'm unsure if mutability is a good idea safety-wise? Though then again, in most usages of blocks they probably are mutable, it's just that blocks are still reference-counted, and it's very easy to then create e.g. a block calling itself (re-entrancy) (which would be UB if it was a FnMut).

madsmtm commented 4 months ago

Regarding blocks as objects:

Can we convert RcBlock to Id<Block>? And should Block then implement Message? And ClassType? When should the block be IsRetainable? Does this even work if CoreFoundation is not linked?

Also, pretty sure blocks cannot be used in WeakId, though!

So is there really any value in blocks being usable as objects, other than just to implement everything the spec says? Maybe there are instances of attributes stored in NSDictionary, where the value is a block?

madsmtm commented 4 months ago

I've opened https://github.com/madsmtm/objc2/issues/571, https://github.com/madsmtm/objc2/issues/572 and https://github.com/madsmtm/objc2/issues/573 to track the remaining parts of this issue.

yury commented 3 months ago

Hello, just want to share another model for blocks. Which I kinda like how it works.

#[repr(transparent)]
pub struct Block<Sig, Attr = NoEsc>(ns::Id, PhantomData<(Sig, Attr)>);

and use fn as Sig. (didn't find good way to restrict generic argument to fns). fns actually for <'a> fn (...) so they can be used as static external block refs: https://github.com/yury/cidre/blob/1fee0fd315d07549f196b9fc3fcff084a4dfe0cc/cidre/src/nw/parameters.rs#L116-L118

Open parameter Attr useful, for instance in dispatch::Block definition: https://github.com/yury/cidre/blob/1fee0fd315d07549f196b9fc3fcff084a4dfe0cc/cidre/src/dispatch.rs#L55

Next it can be NoEsc in dispatch sync calls and Send (Actually it should be Send + Sync, but I started easy there) can be used in async dispatch.

StackBlocks only supported for NoEsc blocks. (Issue, that it could be dereferenced to ns::Id and retained, that is why they unsafe for now) StaticBlocks (StackBlock with fn instead of FnMut) can only be constructed from fn and are safe to copy, so they are also StackBlocks.

block.call() doesn't require () (unit) inside.

But for constructors you have to specify N - number of arguments. new0 or new1.

madsmtm commented 3 months ago

Hello, just want to share another model for blocks.

Thanks, always open for feedback!

(didn't find good way to restrict generic argument to fns) for <'a> fn (...)

Yeah, there's basically two issues regarding lifetimes with my current implementation:

I've tried to document that somewhat, and there's UI tests for each of these, but these issues are kinda unsolvable in current Rust as far as I know.

Next it can be NoEsc in dispatch sync calls and Send (Actually it should be Send + Sync, but I started easy there) can be used in async dispatch.

I'm not sure I understand the value of NoEsc? It's just for requiring the closure to be 'static, right?

StackBlocks only supported for NoEsc blocks. (Issue, that it could be dereferenced to ns::Id and retained, that is why they unsafe for now) StaticBlocks (StackBlock with fn instead of FnMut) can only be constructed from fn and are safe to copy, so they are also StackBlocks.

I've called your StaticBlock for GlobalBlock, and the design is that both StackBlock and GlobalBlock derefs to Block. In general, I want to push users away from being concerned with whether the block was created from a static or on the stack, instead they should be passing Block around.

block.call() doesn't require () (unit) inside.

I initially didn't think this was possible, but checking again, I see that it is, have opened https://github.com/madsmtm/objc2/issues/575 to track that.

But for constructors you have to specify N - number of arguments. new0 or new1.

Yeah, I think it's better to have a trait so you can just use new, and let the compiler figure it out.

yury commented 3 months ago

I'm not sure I understand the value of NoEsc? It's just for requiring the closure to be 'static, right?

Nope, it is opposite.

It allows to model closure consumptions. So vars borrowed in them can be used again.

This is definition:
https://github.com/yury/cidre/blob/1fee0fd315d07549f196b9fc3fcff084a4dfe0cc/cidre/src/dispatch/data.rs#L169

This is call with closure consumption: https://github.com/yury/cidre/blob/1fee0fd315d07549f196b9fc3fcff084a4dfe0cc/cidre/src/dispatch/data.rs#L49-L57

And this actual example of usage:

https://github.com/yury/cidre/blob/1fee0fd315d07549f196b9fc3fcff084a4dfe0cc/cidre/src/dispatch/data.rs#L284-L290

madsmtm commented 3 months ago

Nope, it is opposite.

Right, I always mix those up.

I implemented this too in https://github.com/madsmtm/objc2/pull/569, by just allowing the user to specify a lifetime as dyn Fn() + 'static (escaping) or dyn Fn() + '_ (non-escaping).

I think the Send + Sync stuff is possible this way too, see https://github.com/madsmtm/objc2/issues/572.

yury commented 3 months ago

I implemented this too in https://github.com/madsmtm/objc2/pull/569, by just allowing the user to specify a lifetime as dyn Fn() + 'static (escaping) or dyn Fn() + '_ (non-escaping).

Yep, I had this before. But failed to model external blocks provided by frameworks. (I had to transmute them, which I did like).

madsmtm commented 3 months ago

Hmm, can you give an example of an external block that didn't work / required a transmute with the lifetime scheme?

yury commented 3 months ago

NW_PARAMETERS_DISABLE_PROTOCOL

madsmtm commented 3 months ago

Hmm, that's escaping, right? I think that's possible to model as a 'static block, something like the following:

extern "C" {
    #[link_name = "_nw_parameters_configure_protocol_disable"]
    static NW_PARAMETERS_DISABLE_PROTOCOL: &Block<dyn Fn(nw_protocol_options_t) + Send + Sync + 'static>;
}
yury commented 3 months ago

hmm... and if nw_protocol_options_t is a ref? Sad I didn't pushed my prev code where I have to use it pointer....

madsmtm commented 3 months ago

If nw_protocol_options_t is a reference, then you'd use:

extern "C" {
    #[link_name = "_nw_parameters_configure_protocol_disable"]
    static NW_PARAMETERS_DISABLE_PROTOCOL: &Block<dyn for<'a> Fn(&'a MyReference) + Send + Sync + 'static>;
    // Desugared from `dyn Fn(&MyReference) + Send + Sync`
}

Though as I noted above regarding lifetimes, you might have trouble calling this block from Rust, as the lifetime there is too generic for the (current) trait implementations. An implementation that solves this specific case is possible, it just isn't in the most generic fashion yet.

// This is possible, and would allow calling the above
impl<A> Block<dyn for<'a> Fn(&'a A) + Send + Sync + 'static> {
    fn call(&self) { ... }
}

// but would conflict with more generic impl
impl<A> Block<dyn Fn(A) + Send + Sync + 'static> {
    fn call(&self) { ... }
}

// (this generic impl effectively creates
impl<'a, A> Block<dyn Fn(&'a A) + Send + Sync + 'static> {
    fn call(&self) { ... }
}
// which is not generic enough).

// Ideally we need
impl Block<dyn for<A> Fn(A) + Send + Sync + 'static> {
    fn call(&self) { ... }
}
yury commented 3 months ago

Yes, that is exactly why I have to use newN constructors, (I can't implement them in general for A and &A arguments)

madsmtm commented 3 months ago

Hmm, I see what you're doing, here's my playground for trying to retrofit it into block2's current system, though I think you may be able to use that to make your new methods work generically.

A problem with your current design is indeed that the signature is just that, a signature, and the user can still put anything that implements Fn there, which can be confusing (e.g. they're allowed to pass all of fn(), dyn Fn(), &'static dyn Fn(), &'static dyn Fn() + Send, and so on).

yury commented 3 months ago

Close, but no luck :( https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=afcb2f027f1e851c2362aa3b47e24a47