Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve block2 #168

Closed
11 tasks done
madsmtm opened this issue Jun 13, 2022 · 25 comments
Closed
11 tasks done

Improve block2 #168

madsmtm opened this issue Jun 13, 2022 · 25 comments
Labels
A-block2 Affects the `block2` crate documentation Improvements or additions to documentation enhancement New feature or request

Comments

@madsmtm
Copy link
Owner

madsmtm commented Jun 13, 2022

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 madsmtm added documentation Improvements or additions to documentation enhancement New feature or request A-block2 Affects the `block2` crate labels Jun 13, 2022
@madsmtm
Copy link
Owner Author

madsmtm commented May 26, 2023

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
Copy link
Owner Author

madsmtm commented Dec 3, 2023

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
Copy link
Owner Author

madsmtm commented Dec 3, 2023

@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
Copy link
Owner Author

madsmtm commented Dec 3, 2023

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
Copy link
Owner Author

madsmtm commented Dec 29, 2023

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
Copy link
Owner Author

madsmtm commented Dec 29, 2023

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
Copy link
Owner Author

madsmtm commented Dec 29, 2023

EDIT: Moved to #573.

@madsmtm
Copy link
Owner Author

madsmtm commented Jan 7, 2024

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:

  • Need to implement ConvertArgument for closures while using block2 types.
  • But still needs block2 types to be able to use EncodeArgument/EncodeReturn (and in the future, ConvertArgument).

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()>.

This was referenced Jan 7, 2024
@madsmtm
Copy link
Owner Author

madsmtm commented Jan 12, 2024

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
Copy link
Owner Author

madsmtm commented Jan 24, 2024

Similarly to #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
Copy link
Owner Author

madsmtm commented Jan 24, 2024

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
Copy link
Owner Author

madsmtm commented Feb 2, 2024

I've opened #571, #572 and #573 to track the remaining parts of this issue.

@yury
Copy link

yury commented Feb 4, 2024

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
Copy link
Owner Author

madsmtm commented Feb 4, 2024

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:

  • Lifetimes 'a and 'r in dyn Fn(&'a i32) -> &'r i32 are invariant, instead of being covariant and contravariant.
  • I do restrict the generic parameter to BlockFn, but the implementation does something like impl<A, R> BlockFn for dyn Fn(A) -> R, while it should be doing something closer to impl BlockFn for dyn for<A, R> Fn(A) -> R, i.e. it should be higher-ranked over the generic parameters A and R, but that's not currently expressible in Rust.

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 #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
Copy link

yury commented Feb 4, 2024

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
Copy link
Owner Author

madsmtm commented Feb 4, 2024

Nope, it is opposite.

Right, I always mix those up.

I implemented this too in #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 #572.

@yury
Copy link

yury commented Feb 4, 2024

I implemented this too in #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
Copy link
Owner Author

madsmtm commented Feb 4, 2024

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

@yury
Copy link

yury commented Feb 4, 2024

NW_PARAMETERS_DISABLE_PROTOCOL

@madsmtm
Copy link
Owner Author

madsmtm commented Feb 4, 2024

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
Copy link

yury commented Feb 4, 2024

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
Copy link
Owner Author

madsmtm commented Feb 4, 2024

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
Copy link

yury commented Feb 4, 2024

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

@madsmtm
Copy link
Owner Author

madsmtm commented Feb 4, 2024

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
Copy link

yury commented Feb 4, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-block2 Affects the `block2` crate documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants