Open goffrie opened 1 year ago
madsmtm
commented
1 year ago Parts of the code for generating the encodings in a const context from a Encoding::ENCODING is done, but we're missing a few Rust features to make it a reality (see https://github.com/madsmtm/objc2/pull/70).
But we can definitely add a way to do it in an unsafe way for now.
Do you have an example of a few APIs that fail, so that I can add them to the test suite when implementing the feature?
goffrie
commented
1 year ago The ones I know of are NSFileProviderManager's reimportItemsBelowItemWithIdentifier:completionHandler: and waitForStabilizationWithCompletionHandler:, but I'm not sure how easy it would be to set up a test for those since it requires a FileProvider domain to be set up and everything.
madsmtm
commented
1 year ago Thanks for the examples, I haven't worked with FileProvider myself, so probably the reason why I haven't hit this particular snag myself yet.
I probably won't have the time to implement a function for creating blocks with encodings at the moment (I'm out of the country, and my focus lies elsewhere), but I will happily review and accept a PR that does it (and I could mentor an implementer as well).
PaulDance
commented
3 months ago I seem to have hit this limitation as well. The concerned framework is NetworkExtension, and the specific API is NEFilterDataProvider's applySettings:completionHandler.
The process crashes on the uncaught exception Block was not compiled using a compiler that inserts type information about arguments. The full error can be seen below.
```
This is a case where it is needed to give a new block to the framework and Obj-C runtime instead of simply receiving one from it and calling it, for which there is no problem. Interestingly however, not all such cases end up triggering an error: both NEFilterManager's loadFromPreferencesWithCompletionHandler and saveToPreferencesWithCompletionHandler require and call a block given by the caller, but succeed to do so without any issue whatsoever, even though the input and output type are exactly the same as applySettings' (*mut NSError -> ()). I don't really get why some functions would check for the block's signature and not others. The only difference I see between the two cases is the value I move into the closure given to RcBlock, although I'm not sure it has an effect on anything: in (loadFrom|saveTo)Preferences...'s case, it is a Arc<dispatch::Semaphore> and in applySettings', it is an RcBlock result of a Block::copy of the block received by startFilter....
It would therefore be nice to have an easy way to make this work out of the box. In the meantime, I'll try the workaround mentioned in https://github.com/madsmtm/objc2/issues/434#issuecomment-1704403793 or https://github.com/madsmtm/objc2/issues/615#issuecomment-2117703880 or something like that.
PaulDance
commented
3 months ago Actually, it really seems to be rather linked to the Block object itself and not directly the encoding of its argument and result types. Indeed, NSError already has an appropriate RefEncode implementation; comparing the block received from the Obj-C runtime and the one created using RcBlock::new, I can observe that only in the former case are flags.has_signature, flags.has_extended_layout and descriptor.encoding set to non-default values. I therefore don't think there is a way to currently work around this.
Thinking a bit out loud how to solve this: @madsmtm, do you think it would be possible to expose a possibly-unsafe function or method of at least one of the ...Blocks in order to enable one to construct or modify a block so that it would have the given encoding set for it and the appropriate flags as well? I think it would be preferable if it could be done using the already-existing objc2::Encoding, but if it's not possible, then maybe passing the raw encoding string could also be a possibility, although I would expect it to be much more unsafe. What do you think about this as a workaround candidate?
Regarding what is mentioned in https://github.com/madsmtm/objc2/pull/70#issue-1043551027, I find the macro version to be an acceptable backup solution while waiting for the desired Rust features. At the very least and if I understand things correctly, it would make it an optional requirement of (Ref)?Encode, which should mean easier integration with existing codebases. The macro might need to enclose the implementation however, as I don't think a macro can access information outside of its invocation brackets, although that remains to be checked for proc-macros. Also, the main solution could be tried again to see if the more recent versions of Rust don't support the desired operations as I couldn't see anything wildly exotic in them.
Another possibility would be to generate the C string at runtime and store it in the (Rc|Stack)Block or is its memory layout too restricted? The pointer to the heap-allocated bytes could therefore be set in the BlockDescriptorSignature without creating any self-referencing issues, supposedly.
Yet another possibility would be to use some kind of proc-macro, for example a derive one that would automatically implement an additional trait for the type that only carries the wanted constant and that would be a subtrait of (Ref)?Encode.
madsmtm
commented
2 months ago I don't really get why some functions would check for the block's signature and not others
Yeah, me neither. A guess is that the applySettings:completionHandler: method made a breaking change sometime in its history, and perhaps didn't originally pass the error to the completion handler, but now does if the block's signature allows for it (or something like that). But that's wild speculation. I'll also note that it's newer than the other methods you mentioned.
The only difference I see between the two cases is the value I move into the closure given to
RcBlock, although I'm not sure it has an effect on anything
No, that shouldn't have an effect, those contents are fully transparent to Objective-C.
Actually, it really seems to be rather linked to the
Blockobject itself and not directly the encoding of its argument and result types.
Indeed.
expose a possibly-
unsafefunction or method of at least one of the...Blocks in order to enable one to construct or modify a block so that it would have the given encoding set for it and the appropriate flags as well?
I'd be fine with that - sorry for not responding sooner, I guess I kinda wanted to respond with a PR doing exactly this, but just never got around to doing so.
I will review and accept a PR if you want to attempt doing it, though? I think what I'd prefer would be something like the following, on both StackBlock and RcBlock, let's ignore it on global blocks for now (since I don't know what I'd like the syntax to be):
pub unsafe fn with_encoding<Closure>(encoding: &'static CStr, closure: Closure) -> Self
where
// ... Same as the existing `new` methods
{
#[cfg(debug_assertions)]
{
// Somehow help ensure that the encoding is correct. Not important, but nice to have.
A::ENCODINGS;
R::ENCODING_RETURN;
}
// ... implementation
}
// Usage
let block = RcBlock::with_encoding(c"i12@?0i8", |arg: i32| {
arg + 2
});There's a bit more to this, especially given that the block encoding seems to include some numbers, perhaps similar to the encoding for methods? Should be documented in the method, so that users are empowered to write the correct encoding.
Also, the main solution could be tried again to see if the more recent versions of Rust don't support the desired operations as I couldn't see anything wildly exotic in them.
I'm pretty sure that we still wouldn't be able to do what I wanted in that PR (automatic const ENCODING_STR: &str; on Encode), but that doesn't mean that a solution doesn't exist for block2's use-case that I just haven't come up with yet!
If I remember correctly, the basic limitation in const that we're hitting is that there is no way to allocate, so we have to create an array to put the encoding string into, and convert that to a byte slice. To do so, we have to know the size of said array - and to do that we need to use the encoding from the generic. Something like this:
const fn compute_static_str_len(encoding: &Encoding) -> usize { todo!() }
fn new<R: EncodeReturn>() {
// Create a temporary array to store the static encoding in
let arr: [0u8; compute_static_str_len(&R::ENCODING_RETURN)] = [0u8; compute_static_str_len(&R::ENCODING_RETURN)];
// Use array
}Which doesn't work currently. But maybe there are ways around this that I don't know about?
Another possibility would be to generate the C string at runtime and store it in the
(Rc|Stack)Blockor is its memory layout too restricted? The pointer to the heap-allocated bytes could therefore be set in theBlockDescriptorSignaturewithout creating any self-referencing issues, supposedly.
Yeah, there might be ways to do this by storing the string and the descriptor in the block itself (though it'd probably have to store NonNull<CStr> and manually free it when the block drops, I don't think storing Box would be correct in the Rust abstract machine, but that's an implementation detail).
A bigger problem that I see with this would be regards to the correctness: It's been a while since I combed the spec, but I don't think the lifetime of the descriptor is explicitly noted anywhere in there? So it's a possibility that there is Objective-C code out there that relies on the descriptor or the encoding being 'static instead of the lifetime being tied to the block itself.
Yet another possibility would be to use some kind of proc-macro, for example a derive one that would automatically implement an additional trait for the type that only carries the wanted constant and that would be a subtrait of
(Ref)?Encode.
True, though overkill IMO.
PaulDance
commented
2 months ago Hey, thanks for the answer!
I will review and accept a PR if you want to attempt doing it, though?
I don't exactly need it anymore as it seems that the default settings fit my requirements just fine, but it did limit me in my ability to experiment and having this part of the API available to me would help me investigate and understand some more things in order to get more confidence about what I'm doing. I might therefore open such a PR this week, as it shouldn't be too complicated.
Which doesn't work currently.
Yes, I can indeed reproduce separately: the constant expression depends on a generic parameter error blocks any use of R where we want. I experimented a bit and didn't find much ways to circumvent this since using an array forces const at least somewhere. However, I was able to find one possibility using &'static [u8] instead and by requiring a dummy method on the concerned trait to be implemented identically by each type, as Self also counts as a generic parameter when used in the trait's definition:
trait EncodeReturn {
const ENCODING_RETURN: Encoding;
fn init_arr() -> &'static [u8];
}
impl Encoding {
// The aforementioned `compute_static_str_len`.
const fn str_len(&self) -> usize {
todo!()
}
}
struct X;
impl EncodeReturn for X {
const ENCODING_RETURN: Encoding = todo!();
// Always the same for everyone:
fn init_arr() -> &'static [u8] {
&[0; Self::ENCODING_RETURN.str_len()]
}
}
fn new<R: EncodeReturn>() {
let arr = R::init_arr();
// Use array
}in which new may never be const since init_arr can't either due to it simply being a trait function. I don't really known if it will exactly fit our case, but at least it worked when I used &static strs instead of &'static Encodings as both str::as_bytes and slice::len are const. If it does fit, macros should help to ensure the init_arr implementation does not have to be actually repeated every time and that it is correct every time. Using a supplementary trait may help avoid breaking compatibility.
madsmtm
commented
1 month ago I would really like to avoid having to change the Encode/RefEncode traits, as they're public API with a fairly clean interface, and because doing these things will be possible in a future Rust version, at which point we'll have to change the traits back :/.
I have posted an alternative in https://github.com/madsmtm/objc2/pull/636#issuecomment-2227388348, let's discuss it there.
madsmtm
commented
3 weeks ago I looked into how it's implemented for FileProvider (have not yet looked at NetworkExtension, the setup for testing it is kinda bothersome):
It seems like it's implemented by passing the block's encoding to -[NSMethodSignature signatureWithObjCTypes:] to construct an NSInvocation, which is then passed to the internal fp_zeroOutReplyBlockArgumentsWithError: before executing to trigger the block. Decompiling FileProvider (and cleaning it up) yields something like:
/* @class NSInvocation(FPExtensions) */
-(void)fp_zeroOutReplyBlockArgumentsWithError:(NSError*)error {
NSMethodSignature* signature = [self methodSignature];
NSUInteger numArgs = [signature numberOfArguments];
NSUInteger frameLen = [signature frameLength];
void* zerovalue = stack - (frameLen + 0xf & 0xfffffffffffffff0); // No idea what this is
bzero(zerovalue, frameLen);
if (numArgs >= 2) {
for (int i = 1; i < numArgs; i++) {
[self setArgument:zerovalue atIndex:i];
}
}
NSUInteger i = [signature fp_indexOfLastArgumentWithType:"@\"NSError\""];
if (i != 0x7fffffffffffffff) {
[self setArgument:error atIndex:i];
}
}Not entirely sure what's going on, but I think my suspicion is correct, that they do this for backwards compatibility. From this, I also see a few major points:
NSInvocation uses the signature to correctly invoke the block).
https://github.com/madsmtm/objc2/blob/dd6c804d1e189fd8c9b313d8c1901ee9e7286551/crates/block2/src/concrete_block.rs#L189
Some newer Apple APIs appear to fail in unexpected ways when called with blocks that don't have signatures. It would be useful to at least have some way of manually specifying a signature, though unsafe. Generating it automatically would be even better of course, though it's an interesting question how to actually do so.