Avoid using NonNull directly in return types

This is required because nullability attributes in Clang are a hint, and
not an ABI stable promise, so we must unwrap internally.

More work is still needed to make things fully sound here, but this is
at least a step towards that.

Author: madsmtm

crates/header-translator/src/lib.rs

@@ -1,4 +1,5 @@
 #![recursion_limit = "256"]
+#![allow(clippy::collapsible_else_if)]
 
 #[macro_use]
 extern crate tracing;

crates/header-translator/src/rust_type.rs

@@ -1766,7 +1766,7 @@ impl Ty {
                             write!(f, "...")?;
                         }
                         write!(f, ")")?;
-                        write!(f, "{}", result_type.fn_return())?;
+                        write!(f, "{}", result_type.fn_type_return())?;
                         if *nullability != Nullability::NonNull {
                             write!(f, ">")?;
                         }
@@ -1880,7 +1880,7 @@ impl Ty {
                         write!(f, "{}, ", arg.plain())?;
                     }
                     write!(f, ")")?;
-                    write!(f, "{}", result_type.fn_return())?;
+                    write!(f, "{}", result_type.fn_type_return())?;
                     if *no_escape {
                         write!(f, " + '_")?;
                     } else {
@@ -1900,6 +1900,20 @@ impl Ty {
 
     pub(crate) fn method_return(&self) -> impl fmt::Display + '_ {
         FormatterFn(move |f| match self {
+            // Don't output anything here.
+            Self::Primitive(Primitive::Void) => Ok(()),
+            Self::Pointer {
+                nullability,
+                pointee,
+                ..
+            } if pointee.is_static_object() => {
+                if *nullability == Nullability::NonNull {
+                    // TODO: Add runtime nullability check here.
+                    write!(f, " -> &'static {}", pointee.behind_pointer())
+                } else {
+                    write!(f, " -> Option<&'static {}>", pointee.behind_pointer())
+                }
+            }
             Self::Pointer {
                 nullability,
                 lifetime: _, // TODO: Use this somehow?
@@ -1921,7 +1935,7 @@ impl Ty {
                 write!(f, " -> bool")
             }
             Self::Primitive(Primitive::ObjcBool) => write!(f, " -> bool"),
-            _ => write!(f, "{}", self.fn_return()),
+            _ => write!(f, " -> {}", self.plain()),
         })
     }
 
@@ -1983,88 +1997,117 @@ impl Ty {
         })
     }
 
-    pub(crate) fn fn_return(&self) -> impl fmt::Display + '_ {
-        FormatterFn(move |f| {
-            if let Self::Primitive(Primitive::Void) = self {
-                // Don't output anything
-                return Ok(());
-            }
-
-            match self {
-                Self::Pointer {
-                    nullability,
-                    pointee,
-                    ..
-                } if pointee.is_static_object() => {
-                    if *nullability == Nullability::NonNull {
-                        write!(f, " -> &'static {}", pointee.behind_pointer())
-                    } else {
-                        write!(f, " -> Option<&'static {}>", pointee.behind_pointer())
-                    }
+    fn fn_type_return(&self) -> impl fmt::Display + '_ {
+        FormatterFn(move |f| match self {
+            // Don't output anything here.
+            Self::Primitive(Primitive::Void) => Ok(()),
+            Self::Pointer {
+                nullability,
+                pointee,
+                ..
+            } if pointee.is_static_object() => {
+                if *nullability == Nullability::NonNull {
+                    // TODO: Add runtime nullability check here (can we even
+                    // do that?).
+                    write!(f, " -> &'static {}", pointee.behind_pointer())
+                } else {
+                    write!(f, " -> Option<&'static {}>", pointee.behind_pointer())
                 }
-                _ => write!(f, " -> {}", self.plain()),
             }
+            _ => write!(f, " -> {}", self.plain()),
         })
     }
 
     pub(crate) fn fn_return_required_items(&self) -> impl Iterator<Item = ItemTree> {
         let mut items: Vec<_> = self.required_items().collect();
         match self {
-            Self::Pointer {
-                lifetime: Lifetime::Unspecified,
-                pointee,
-                ..
-            } if pointee.is_cf_type() => {
+            Self::Pointer { pointee, .. } if pointee.is_cf_type() => {
                 items.push(ItemTree::cf("CFRetained"));
                 items.push(ItemTree::core_ptr_nonnull());
             }
-            Self::Pointer {
-                lifetime: Lifetime::Unspecified,
-                pointee,
-                ..
-            } if pointee.is_object_like() && !pointee.is_static_object() => {
+            Self::Pointer { pointee, .. }
+                if pointee.is_object_like() && !pointee.is_static_object() =>
+            {
                 items.push(ItemTree::objc("Retained"));
             }
             _ => {}
         }
         items.into_iter()
     }
 
-    pub(crate) fn fn_return_converter(
+    pub(crate) fn fn_return(
         &self,
         returns_retained: bool,
-    ) -> Option<(
-        impl fmt::Display + '_,
-        impl fmt::Display + '_,
+    ) -> (
         impl fmt::Display + '_,
-    )> {
+        Option<(
+            impl fmt::Display + '_,
+            impl fmt::Display + '_,
+            impl fmt::Display + '_,
+        )>,
+    ) {
         let start = "let ret = ";
         // SAFETY: The function is marked with the correct retain semantics,
         // otherwise it'd be invalid to use from Obj-C with ARC and Swift too.
+        let end_cf = |nullability| {
+            match (nullability, returns_retained) {
+                // TODO: Avoid NULL check, and let CFRetain do that instead?
+                (Nullability::NonNull, true) => ";\nlet ret = ret.expect(\"function was marked as returning non-null, but actually returned NULL\");\nunsafe { CFRetained::from_raw(ret) }",
+                (Nullability::NonNull, false) => ";\nlet ret = ret.expect(\"function was marked as returning non-null, but actually returned NULL\");\nunsafe { CFRetained::retain(ret) }",
+                // CFRetain aborts on NULL pointers, so there's not really a more
+                // efficient way to do this (except if we were to use e.g.
+                // `CGColorRetain`/`CVOpenGLBufferRetain`/..., but that's a huge
+                // hassle).
+                (_, true) => ";\nret.map(|ret| unsafe { CFRetained::from_raw(ret) })",
+                (_, false) => ";\nret.map(|ret| unsafe { CFRetained::retain(ret) })",
+            }
+        };
         let end_objc = |nullability| {
             match (nullability, returns_retained) {
                 (Nullability::NonNull, true) => {
-                    ";\nunsafe { Retained::from_raw(ret.as_ptr()) }.expect(\"function was marked as returning non-null, but actually returned NULL\")"
+                    ";\nunsafe { Retained::from_raw(ret) }.expect(\"function was marked as returning non-null, but actually returned NULL\")"
                 }
                 (Nullability::NonNull, false) => {
-                    ";\nunsafe { Retained::retain_autoreleased(ret.as_ptr()) }.expect(\"function was marked as returning non-null, but actually returned NULL\")"
+                    ";\nunsafe { Retained::retain_autoreleased(ret) }.expect(\"function was marked as returning non-null, but actually returned NULL\")"
                 }
                 (_, true) => ";\nunsafe { Retained::from_raw(ret) }",
                 (_, false) => ";\nunsafe { Retained::retain_autoreleased(ret) }",
             }
         };
-        let end_cf = |nullability| match (nullability, returns_retained) {
-            (Nullability::NonNull, true) => ";\nunsafe { CFRetained::from_raw(ret) }",
-            (Nullability::NonNull, false) => ";\nunsafe { CFRetained::retain(ret) }",
-            // CFRetain aborts on NULL pointers, so there's not really a more
-            // efficient way to do this (except if we were to use e.g.
-            // `CGColorRetain`/`CVOpenGLBufferRetain`/..., but that's a huge
-            // hassle).
-            (_, true) => ";\nNonNull::new(ret).map(|ret| unsafe { CFRetained::from_raw(ret) })",
-            (_, false) => ";\nNonNull::new(ret).map(|ret| unsafe { CFRetained::retain(ret) })",
-        };
 
-        match self {
+        let ret = FormatterFn(move |f| match self {
+            // Don't output anything here.
+            Self::Primitive(Primitive::Void) => Ok(()),
+            Self::Pointer {
+                nullability,
+                is_const,
+                pointee,
+                ..
+            } => {
+                // Ignore nullability, always emit a nullable pointer. We will
+                // unwrap it later in `fn_return_converter`.
+                //
+                // This is required because nullability attributes in Clang
+                // are a hint, and not an ABI stable promise.
+                if pointee.is_static_object() {
+                    write!(f, "-> Option<&'static {}>", pointee.behind_pointer())
+                } else if pointee.is_cf_type() {
+                    write!(f, "-> Option<NonNull<{}>>", pointee.behind_pointer())
+                } else if pointee.is_object_like() {
+                    write!(f, "-> *mut {}", pointee.behind_pointer())
+                } else {
+                    if *nullability == Nullability::NonNull {
+                        write!(f, "-> Option<NonNull<{}>>", pointee.behind_pointer())
+                    } else if *is_const {
+                        write!(f, " -> *const {}", pointee.behind_pointer())
+                    } else {
+                        write!(f, " -> *mut {}", pointee.behind_pointer())
+                    }
+                }
+            }
+            _ => write!(f, " -> {}", self.plain()),
+        });
+        let converter = match self {
             _ if self.is_objc_bool() => Some((" -> bool".to_string(), "", ".as_bool()")),
             Self::TypeDef { id, .. } if matches!(&*id.name, "Boolean" | "boolean_t") => {
                 Some((" -> bool".to_string(), start, ";\nret != 0"))
@@ -2082,7 +2125,15 @@ impl Ty {
                     _ => error!(?lifetime, returns_retained, "invalid lifetime"),
                 }
 
-                if pointee.is_cf_type() {
+                if pointee.is_static_object() {
+                    if *nullability == Nullability::NonNull {
+                        let res = format!(" -> &'static {}", pointee.behind_pointer());
+                        Some((res, start, ";\nret.expect(\"function was marked as returning non-null, but actually returned NULL\")"))
+                    } else {
+                        // No conversion necessary
+                        None
+                    }
+                } else if pointee.is_cf_type() {
                     let res = if *nullability == Nullability::NonNull {
                         format!(" -> CFRetained<{}>", pointee.behind_pointer())
                     } else {
@@ -2097,11 +2148,17 @@ impl Ty {
                     };
                     Some((res, start, end_objc(*nullability)))
                 } else {
-                    None
+                    if *nullability == Nullability::NonNull {
+                        let res = format!(" -> NonNull<{}>", pointee.behind_pointer());
+                        Some((res, start, ";\nret.expect(\"function was marked as returning non-null, but actually returned NULL\")"))
+                    } else {
+                        None
+                    }
                 }
             }
             _ => None,
-        }
+        };
+        (ret, converter)
     }
 
     pub(crate) fn var(&self) -> impl fmt::Display + '_ {

crates/header-translator/src/stmt.rs

@@ -2685,6 +2685,7 @@ impl Stmt {
                     arguments,
                     result_type,
                     body: Some(_),
+                    returns_retained,
                     ..
                 } => {
                     write!(f, "// TODO: ")?;
@@ -2693,7 +2694,8 @@ impl Stmt {
                         let param = handle_reserved(&crate::to_snake_case(param));
                         write!(f, "{param}: {},", arg_ty.fn_argument())?;
                     }
-                    writeln!(f, "){};", result_type.fn_return())?;
+                    let (ret, _) = result_type.fn_return(*returns_retained);
+                    writeln!(f, "){ret};")?;
                 }
                 Self::FnDecl {
                     id,
@@ -2710,7 +2712,7 @@ impl Stmt {
                 } => {
                     let abi = if *can_unwind { "C-unwind" } else { "C" };
 
-                    let return_converter = result_type.fn_return_converter(*returns_retained);
+                    let (ret, return_converter) = result_type.fn_return(*returns_retained);
 
                     let needs_wrapper = *safe
                         || return_converter.is_some()
@@ -2731,7 +2733,7 @@ impl Stmt {
                             let param = handle_reserved(&crate::to_snake_case(param));
                             write!(f, "{param}: {},", arg_ty.fn_argument())?;
                         }
-                        writeln!(f, "){};", result_type.fn_return())?;
+                        writeln!(f, "){ret};")?;
 
                         Ok(())
                     };
@@ -2760,7 +2762,7 @@ impl Stmt {
                         if let Some((ty, _, _)) = &return_converter {
                             write!(f, "{ty}")?;
                         } else {
-                            write!(f, "{}", result_type.fn_return())?;
+                            write!(f, "{ret}")?;
                         }
                         writeln!(f, " {{")?;
 
@@ -2818,7 +2820,9 @@ impl Stmt {
                 } => {
                     let abi = if *can_unwind { "C-unwind" } else { "C" };
 
-                    // Only emit for base types, not for mutable subclasses,
+                    let (ret, _) = result_type.fn_return(false);
+
+                    // Only emit for base types, not foretr mutable subclasses,
                     // as it's unclear whether it's safe to downcast to
                     // mutable subclasses.
                     write!(f, "{}", self.cfg_gate_ln(config))?;
@@ -2827,15 +2831,10 @@ impl Stmt {
                     write!(f, "{}", documentation.fmt(None))?;
                     writeln!(f, "    #[doc(alias = {:?})]", id.name)?;
                     writeln!(f, "    #[inline]")?;
-                    writeln!(f, "    fn type_id(){} {{", result_type.fn_return())?;
+                    writeln!(f, "    fn type_id(){ret} {{")?;
 
                     writeln!(f, "        extern {abi:?} {{")?;
-                    writeln!(
-                        f,
-                        "            fn {}(){};",
-                        id.name,
-                        result_type.fn_return()
-                    )?;
+                    writeln!(f, "            fn {}(){ret};", id.name,)?;
                     writeln!(f, "        }}")?;
 
                     writeln!(f, "        unsafe {{ {}() }}", id.name)?;