Skip to content

Commit

Permalink
Handle NULL errors
Browse files Browse the repository at this point in the history
Fixes #653.
  • Loading branch information
madsmtm committed Jan 16, 2025
1 parent 45f153b commit f5f28d9
Show file tree
Hide file tree
Showing 20 changed files with 729 additions and 978 deletions.
2 changes: 2 additions & 0 deletions crates/objc2/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
automatically implementing the auto traits `Send` and `Sync`.
* **BREAKING**: Fixed the signature of `NSObjectProtocol::isEqual` to take a
nullable argument.
* Fixed handling of methods that return NULL errors. This affected for example
`-[MTLBinaryArchive serializeToURL:error:]`.


## 0.5.2 - 2024-05-21
Expand Down
1 change: 1 addition & 0 deletions crates/objc2/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ objc2-foundation = { path = "../../framework-crates/objc2-foundation", default-f
"NSDate",
"NSDictionary",
"NSEnumerator",
"NSError",
"NSKeyValueObserving",
"NSNotification",
"NSObject",
Expand Down
1 change: 1 addition & 0 deletions crates/objc2/src/__macro_helpers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ mod method_family;
mod module_info;
mod msg_send;
mod msg_send_retained;
mod null_error;
mod os_version;
mod sync_unsafe_cell;
mod writeback;
Expand Down
19 changes: 6 additions & 13 deletions crates/objc2/src/__macro_helpers/msg_send.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use crate::rc::Retained;
use crate::runtime::{AnyClass, AnyObject, MessageReceiver, Sel};
use crate::{ClassType, Encode, Message};

use super::null_error::encountered_error;
use super::{ConvertArguments, ConvertReturn, TupleExtender};

pub trait MsgSend: Sized {
Expand Down Expand Up @@ -83,7 +84,7 @@ pub trait MsgSend: Sized {
*mut *mut E: Encode,
A: TupleExtender<*mut *mut E>,
<A as TupleExtender<*mut *mut E>>::PlusOneArgument: ConvertArguments,
E: Message,
E: ClassType,
{
let mut err: *mut E = ptr::null_mut();
let args = args.add_argument(&mut err);
Expand All @@ -107,7 +108,7 @@ pub trait MsgSend: Sized {
*mut *mut E: Encode,
A: TupleExtender<*mut *mut E>,
<A as TupleExtender<*mut *mut E>>::PlusOneArgument: ConvertArguments,
E: Message,
E: ClassType,
{
let mut err: *mut E = ptr::null_mut();
let args = args.add_argument(&mut err);
Expand All @@ -132,7 +133,7 @@ pub trait MsgSend: Sized {
*mut *mut E: Encode,
A: TupleExtender<*mut *mut E>,
<A as TupleExtender<*mut *mut E>>::PlusOneArgument: ConvertArguments,
E: Message,
E: ClassType,
{
let mut err: *mut E = ptr::null_mut();
let args = args.add_argument(&mut err);
Expand All @@ -145,14 +146,6 @@ pub trait MsgSend: Sized {
}
}

#[cold]
#[track_caller]
unsafe fn encountered_error<E: Message>(err: *mut E) -> Retained<E> {
// SAFETY: Ensured by caller
unsafe { Retained::retain(err) }
.expect("error parameter should be set if the method returns NO")
}

impl<T: MessageReceiver> MsgSend for T {
type Inner = T::__Inner;

Expand Down Expand Up @@ -200,7 +193,7 @@ mod tests {
macro_rules! test_error_bool {
($expected:expr, $($obj:tt)*) => {
// Succeeds
let res: Result<(), Retained<RcTestObject>> = unsafe {
let res: Result<(), Retained<NSObject>> = unsafe {
msg_send![$($obj)*, boolAndShouldError: false, error: _]
};
assert_eq!(res, Ok(()));
Expand All @@ -209,7 +202,7 @@ mod tests {
// Errors
let res = autoreleasepool(|_pool| {
// `Ok` type is inferred to be `()`
let res: Retained<RcTestObject> = unsafe {
let res: Retained<NSObject> = unsafe {
msg_send![$($obj)*, boolAndShouldError: true, error: _]
}.expect_err("not err");
$expected.alloc += 1;
Expand Down
21 changes: 6 additions & 15 deletions crates/objc2/src/__macro_helpers/msg_send_retained.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use crate::runtime::{AnyClass, AnyObject, Sel};
use crate::{sel, ClassType, DefinedClass, Message};

use super::defined_ivars::set_finalized;
use super::null_error::encountered_error;
use super::{Alloc, ConvertArguments, Copy, Init, MsgSend, MutableCopy, New, Other, TupleExtender};

pub trait MsgSendRetained<T, U> {
Expand All @@ -29,7 +30,7 @@ pub trait MsgSendRetained<T, U> {
*mut *mut E: Encode,
A: TupleExtender<*mut *mut E>,
<A as TupleExtender<*mut *mut E>>::PlusOneArgument: ConvertArguments,
E: Message,
E: ClassType,
Option<R>: MaybeUnwrap<Input = U>,
{
let mut err: *mut E = ptr::null_mut();
Expand Down Expand Up @@ -111,7 +112,7 @@ pub trait MsgSendSuperRetained<T, U> {
*mut *mut E: Encode,
A: TupleExtender<*mut *mut E>,
<A as TupleExtender<*mut *mut E>>::PlusOneArgument: ConvertArguments,
E: Message,
E: ClassType,
Option<R>: MaybeUnwrap<Input = U>,
{
let mut err: *mut E = ptr::null_mut();
Expand Down Expand Up @@ -140,7 +141,7 @@ pub trait MsgSendSuperRetained<T, U> {
*mut *mut E: Encode,
A: TupleExtender<*mut *mut E>,
<A as TupleExtender<*mut *mut E>>::PlusOneArgument: ConvertArguments,
E: Message,
E: ClassType,
Option<R>: MaybeUnwrap<Input = U>,
{
let mut err: *mut E = ptr::null_mut();
Expand All @@ -156,16 +157,6 @@ pub trait MsgSendSuperRetained<T, U> {
}
}

// Marked `cold` to tell the optimizer that errors are comparatively rare.
// And intentionally not inlined, for much the same reason.
#[cold]
#[track_caller]
unsafe fn encountered_error<E: Message>(err: *mut E) -> Retained<E> {
// SAFETY: Ensured by caller
unsafe { Retained::retain(err) }
.expect("error parameter should be set if the method returns NULL")
}

impl<T: MsgSend, U: ?Sized + Message> MsgSendRetained<T, Option<Retained<U>>> for New {
#[inline]
unsafe fn send_message_retained<
Expand Down Expand Up @@ -1032,7 +1023,7 @@ mod tests {
($expected:expr, $if_autorelease_not_skipped:expr, $sel:ident, $($obj:tt)*) => {
// Succeeds
let res = autoreleasepool(|_pool| {
let res: Result<Retained<RcTestObject>, Retained<RcTestObject>> = unsafe {
let res: Result<Retained<RcTestObject>, Retained<NSObject>> = unsafe {
msg_send_id![$($obj)*, $sel: false, error: _]
};
let res = res.expect("not ok");
Expand All @@ -1053,7 +1044,7 @@ mod tests {

// Errors
let res = autoreleasepool(|_pool| {
let res: Result<Retained<RcTestObject>, Retained<RcTestObject>> = unsafe {
let res: Result<Retained<RcTestObject>, Retained<NSObject>> = unsafe {
msg_send_id![$($obj)*, $sel: true, error: _]
};
$expected.alloc += 1;
Expand Down
137 changes: 137 additions & 0 deletions crates/objc2/src/__macro_helpers/null_error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
use core::ffi::CStr;
use std::sync::OnceLock;

use crate::ffi::NSInteger;
use crate::rc::{autoreleasepool, Retained};
use crate::runtime::{AnyClass, NSObject};
use crate::{msg_send_id, ClassType};

// Marked `#[cold]` to tell the optimizer that errors are comparatively rare.
//
// And intentionally not `#[inline]`, we'll let the optimizer figure out if it
// wants to do that or not.
#[cold]
pub(crate) unsafe fn encountered_error<E: ClassType>(err: *mut E) -> Retained<E> {
// SAFETY: Caller ensures that the pointer is valid.
unsafe { Retained::retain(err) }.unwrap_or_else(|| {
let err = null_error();
assert!(E::IS_NSERROR_COMPATIBLE);
// SAFETY: Just checked (via `const` assertion) that the `E` type is
// either `NSError` or `NSObject`, and hence it is valid to cast the
// `NSObject` that we have here to that.
unsafe { Retained::cast_unchecked(err) }
})
}

/// Poor mans string equality in `const`. Implements `a == b`.
const fn is_eq(a: &str, b: &str) -> bool {
let a = a.as_bytes();
let b = b.as_bytes();

if a.len() != b.len() {
return false;
}

let mut i = 0;
while i < a.len() {
if a[i] != b[i] {
return false;
}
i += 1;
}

true
}

// TODO: Use inline `const` once in MSRV (or add proper trait bounds).
trait IsNSError {
const IS_NSERROR_COMPATIBLE: bool;
}

impl<T: ClassType> IsNSError for T {
const IS_NSERROR_COMPATIBLE: bool = {
if is_eq(T::NAME, "NSError") || is_eq(T::NAME, "NSObject") {
true
} else {
// The post monomorphization error here is not nice, but it's
// better than UB because the user used a type that cannot be
// converted to NSError.
//
// TODO: Add a trait bound or similar instead.
panic!("error parameter must be either `NSError` or `NSObject`")
}
};
}

#[cold] // Mark the NULL error branch as cold
fn null_error() -> Retained<NSObject> {
static CACHED_NULL_ERROR: OnceLock<NSErrorWrapper> = OnceLock::new();

// We use a OnceLock here, since performance doesn't really matter, and
// using an AtomicPtr would leak under (very) high initialization
// contention.
CACHED_NULL_ERROR.get_or_init(create_null_error).0.clone()
}

struct NSErrorWrapper(Retained<NSObject>);

// SAFETY: NSError is immutable and thread safe.
unsafe impl Send for NSErrorWrapper {}
unsafe impl Sync for NSErrorWrapper {}

#[cold] // Mark the error creation branch as cold
fn create_null_error() -> NSErrorWrapper {
// Wrap creation in an autoreleasepool, since we don't know anything about
// the outside world, and we don't want to appear to leak.
autoreleasepool(|_| {
// TODO: Replace with c string literals once in MSRV.

// SAFETY: The string is NUL terminated.
let cls = unsafe { CStr::from_bytes_with_nul_unchecked(b"NSString\0") };
// Intentional dynamic lookup, we don't know if Foundation is linked.
let cls = AnyClass::get(cls).unwrap_or_else(foundation_not_linked);

// SAFETY: The string is NUL terminated.
let domain = unsafe { CStr::from_bytes_with_nul_unchecked(b"__objc2.missingError\0") };
// SAFETY: The signate is correct, and the string is UTF-8 encoded and
// NUL terminated.
let domain: Retained<NSObject> =
unsafe { msg_send_id![cls, stringWithUTF8String: domain.as_ptr()] };

// SAFETY: The string is valid.
let cls = unsafe { CStr::from_bytes_with_nul_unchecked(b"NSError\0") };
// Intentional dynamic lookup, we don't know if Foundation is linked.
let cls = AnyClass::get(cls).unwrap_or_else(foundation_not_linked);

let domain: &NSObject = &domain;
let code: NSInteger = 0;
let user_info: Option<&NSObject> = None;
// SAFETY: The signate is correct.
let err: Retained<NSObject> =
unsafe { msg_send_id![cls, errorWithDomain: domain, code: code, userInfo: user_info] };
NSErrorWrapper(err)
})
}

fn foundation_not_linked() -> &'static AnyClass {
panic!("Foundation must be linked to get a proper error message on NULL errors")
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_is_eq() {
assert!(is_eq("NSError", "NSError"));
assert!(!is_eq("nserror", "NSError"));
assert!(!is_eq("CFError", "NSError"));
assert!(!is_eq("NSErr", "NSError"));
assert!(!is_eq("NSErrorrrr", "NSError"));
}

#[test]
fn test_create() {
let _ = create_null_error().0;
}
}
4 changes: 3 additions & 1 deletion crates/objc2/src/macros/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -820,7 +820,8 @@ macro_rules! __class_inner {
///
/// In particular, if you make the last argument the special marker `_`, then
/// the macro will return a `Result<(), Retained<E>>` (where you must specify
/// `E` yourself, usually you'd use `objc2_foundation::NSError`).
/// `E` yourself, and where `E` must be either [`NSObject`] or
/// `objc2_foundation::NSError`).
///
/// At runtime, we create the temporary error variable for you on the stack
/// and send it as the out-parameter to the method. If the method then returns
Expand All @@ -832,6 +833,7 @@ macro_rules! __class_inner {
///
/// [cocoa-error]: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ErrorHandlingCocoa/ErrorHandling/ErrorHandling.html
/// [swift-error]: https://developer.apple.com/documentation/swift/about-imported-cocoa-error-parameters
/// [`NSObject`]: crate::runtime::NSObject
///
///
/// # Panics
Expand Down
Loading

0 comments on commit f5f28d9

Please sign in to comment.