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

Support failable initializers #276

Merged
merged 4 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -220,4 +220,11 @@ class OptionTests: XCTestCase {
XCTAssertEqual(reflectedSome!.field, 123)
XCTAssertNil(reflectedNone)
}

/// Verify that we can use failable initializers defined on the Rust side.
func testFailableInitializer() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From now on all tests should say what we're verifying.

/// Verify that ...

This helps:

  • new maintainers since it's easier to read English than code.
  • forces the author to be clear about what behavior they're trying to test. Needing to explain ourselves makes us think critically about what we are doing and if we're covering enough cases.
  • the reviewer noticing missing tests. For example, if the reviewer sees that what the test says that it is testing doesn't seem to match the code inside, they can comment on that. This tends to increase the quality of a test suite.

For simple stuff like this a sentence is fine.

XCTAssertEqual(FailableInitType(false), nil)
let failableInitType = FailableInitType(true)
XCTAssertEqual(failableInitType!.count(), 132)
}
}
9 changes: 9 additions & 0 deletions crates/swift-bridge-ir/src/bridged_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ pub(crate) trait BridgeableType: Debug {

fn as_result(&self) -> Option<&BuiltInResult>;

fn as_option(&self) -> Option<&BridgedOption>;

/// True if the type's FFI representation is a pointer
fn is_passed_via_pointer(&self) -> bool;

Expand Down Expand Up @@ -494,6 +496,13 @@ impl BridgeableType for BridgedType {
}
}

fn as_option(&self) -> Option<&BridgedOption> {
match self {
BridgedType::StdLib(StdLibType::Option(ty)) => Some(ty),
_ => None,
}
}

fn is_passed_via_pointer(&self) -> bool {
match self {
BridgedType::StdLib(StdLibType::Vec(_)) => true,
Expand Down
4 changes: 4 additions & 0 deletions crates/swift-bridge-ir/src/bridged_type/bridgeable_pointer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ impl BridgeableType for BuiltInPointer {
todo!()
}

fn as_option(&self) -> Option<&super::bridged_option::BridgedOption> {
todo!();
}

fn is_passed_via_pointer(&self) -> bool {
todo!()
}
Expand Down
4 changes: 4 additions & 0 deletions crates/swift-bridge-ir/src/bridged_type/bridgeable_string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ impl BridgeableType for BridgedString {
None
}

fn as_option(&self) -> Option<&super::bridged_option::BridgedOption> {
todo!()
}

fn is_passed_via_pointer(&self) -> bool {
true
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ impl BridgeableType for OpaqueForeignType {
None
}

fn as_option(&self) -> Option<&super::bridged_option::BridgedOption> {
None
}

fn is_passed_via_pointer(&self) -> bool {
true
}
Expand Down
4 changes: 4 additions & 0 deletions crates/swift-bridge-ir/src/bridged_type/built_in_tuple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ impl BridgeableType for BuiltInTuple {
todo!();
}

fn as_option(&self) -> Option<&super::bridged_option::BridgedOption> {
todo!()
}

fn is_passed_via_pointer(&self) -> bool {
todo!();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -456,3 +456,74 @@ typedef struct MyType MyType;
.test();
}
}

/// Verify that we generated a Swift class with a failable init method.
mod extern_rust_class_with_failable_init {
Copy link
Collaborator Author

@NiwakaDev NiwakaDev Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chinedufn
I guess it's suitable to add the test here.

Related: #276 (comment)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

use super::*;

fn bridge_module_tokens() -> TokenStream {
quote! {
mod foo {
extern "Rust" {
type Foo;

#[swift_bridge(init)]
fn new() -> Option<Foo>;
}
}
}
}

fn expected_rust_tokens() -> ExpectedRustTokens {
ExpectedRustTokens::Contains(quote! {
# [export_name = "__swift_bridge__$Foo$new"]
pub extern "C" fn __swift_bridge__Foo_new () -> * mut super :: Foo {
if let Some (val) = super :: Foo :: new () {
Box :: into_raw (Box :: new (val))
} else {
std :: ptr :: null_mut ()
}
}
})
}

const EXPECTED_SWIFT: ExpectedSwiftCode = ExpectedSwiftCode::ContainsAfterTrim(
r#"
public class Foo: FooRefMut {
var isOwned: Bool = true

public override init(ptr: UnsafeMutableRawPointer) {
super.init(ptr: ptr)
}

deinit {
if isOwned {
__swift_bridge__$Foo$_free(ptr)
}
}
}
extension Foo {
public convenience init?() {
guard let val = __swift_bridge__$Foo$new() else { return nil }; self.init(ptr: val)
}
}
"#,
);

const EXPECTED_C_HEADER: ExpectedCHeader = ExpectedCHeader::ContainsAfterTrim(
r#"
void* __swift_bridge__$Foo$new(void);
"#,
);

#[test]
fn extern_rust_class_with_failable_init() {
CodegenTest {
bridge_module: bridge_module_tokens().into(),
expected_rust_tokens: expected_rust_tokens(),
expected_swift_code: EXPECTED_SWIFT,
expected_c_header: EXPECTED_C_HEADER,
}
.test();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@ pub(super) fn gen_func_swift_calls_rust(
if function.is_copy_method_on_opaque_type() {
"public init".to_string()
} else {
"public convenience init".to_string()
if function.is_swift_failable_initializer {
"public convenience init?".to_string()
} else {
"public convenience init".to_string()
}
}
} else {
if let Some(swift_name) = &function.swift_name_override {
Expand Down Expand Up @@ -179,7 +183,14 @@ pub(super) fn gen_func_swift_calls_rust(
if function.is_copy_method_on_opaque_type() {
call_rust = format!("self.bytes = {}", call_rust)
} else {
call_rust = format!("self.init(ptr: {})", call_rust)
if function.is_swift_failable_initializer {
call_rust = format!(
"guard let val = {} else {{ return nil }}; self.init(ptr: val)",
call_rust
)
} else {
call_rust = format!("self.init(ptr: {})", call_rust)
}
}
}

Expand Down Expand Up @@ -316,5 +327,6 @@ return{maybe_try}await {with_checked_continuation_function_name}({{ (continuatio
call_rust = call_rust,
)
};

func_definition
}
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ extension {ty_name}Ref: Hashable{{
"".to_string()
}
};

let class = format!(
r#"
{class_decl}{initializers}{owned_instance_methods}{class_ref_decl}{ref_mut_instance_methods}{class_ref_mut_decl}{ref_instance_methods}{generic_freer}{equatable_method}{hashable_method}"#,
Expand Down
33 changes: 26 additions & 7 deletions crates/swift-bridge-ir/src/parse/parse_extern_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,21 +111,27 @@ impl<'a> ForeignModParser<'a> {
}

let return_type = &func.sig.output;
let mut is_swift_failable_initializer = false;
if let ReturnType::Type(_, return_ty) = return_type {
if BridgedType::new_with_type(return_ty.deref(), &self.type_declarations)
.is_none()
{
let bridged_return_type =
BridgedType::new_with_type(return_ty.deref(), &self.type_declarations);
if let Some(ty) = &bridged_return_type {
if ty.as_option().is_some() && attributes.is_swift_initializer {
is_swift_failable_initializer = true;
}
}
if bridged_return_type.is_none() {
self.unresolved_types.push(return_ty.deref().clone());
}
}

let first_input = func.sig.inputs.iter().next();

let associated_type = self.get_associated_type(
first_input,
func.clone(),
&attributes,
&mut local_type_declarations,
is_swift_failable_initializer,
)?;

if attributes.is_swift_identifiable {
Expand Down Expand Up @@ -225,10 +231,12 @@ impl<'a> ForeignModParser<'a> {
}
}
}

let func = ParsedExternFn {
func,
associated_type,
is_swift_initializer: attributes.is_swift_initializer,
is_swift_failable_initializer: is_swift_failable_initializer,
is_swift_identifiable: attributes.is_swift_identifiable,
host_lang,
rust_name_override: attributes.rust_name,
Expand Down Expand Up @@ -294,6 +302,7 @@ impl<'a> ForeignModParser<'a> {
func: ForeignItemFn,
attributes: &FunctionAttributes,
local_type_declarations: &mut HashMap<String, OpaqueForeignTypeDeclaration>,
is_swift_failable_initializer: bool,
) -> syn::Result<Option<TypeDeclaration>> {
let associated_type = match first {
Some(FnArg::Receiver(recv)) => {
Expand Down Expand Up @@ -337,6 +346,7 @@ impl<'a> ForeignModParser<'a> {
func.clone(),
attributes,
local_type_declarations,
is_swift_failable_initializer,
)?;
associated_type
}
Expand Down Expand Up @@ -373,10 +383,19 @@ Otherwise we use a more general error that says that your argument is invalid.
ty_string
}
};
if is_swift_failable_initializer {
// Safety: since we've already checked ty_string is formatted as "Option<~>" before calling this function.
let last_bracket = ty_string.rfind(">").unwrap();

let inner = &ty_string[0..last_bracket];
let inner = inner.trim_start_matches("Option < ").trim_end_matches(" ");
let ty = self.type_declarations.get(inner);
ty.map(|ty| ty.clone())
} else {
let ty = self.type_declarations.get(&ty_string);

let ty = self.type_declarations.get(&ty_string);

ty.map(|ty| ty.clone())
ty.map(|ty| ty.clone())
}
} else {
None
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,27 @@ mod tests {
assert!(func.is_swift_initializer);
}

/// Verify that we can parse an failable init function.
#[test]
fn failable_initializer() {
let tokens = quote! {
mod foo {
extern "Rust" {
type Foo;

#[swift_bridge(init)]
fn bar () -> Option<Foo>;
}
}
};

let module = parse_ok(tokens);

let func = &module.functions[0];
assert!(func.is_swift_initializer);
assert!(func.is_swift_failable_initializer);
}

/// Verify that we can parse an init function that takes inputs.
#[test]
fn initializer_with_inputs() {
Expand Down
4 changes: 4 additions & 0 deletions crates/swift-bridge-ir/src/parsed_extern_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ pub(crate) struct ParsedExternFn {
pub host_lang: HostLang,
/// Whether or not this function is a Swift initializer.
pub is_swift_initializer: bool,
/// Whether or not this function is a Swift failable initializer.
/// For more details, see:
/// [Swift Documentation - Failable Initializers](https://docs.swift.org/swift-book/documentation/the-swift-programming-language/initialization/#Failable-Initializers)
pub is_swift_failable_initializer: bool,
/// Whether or not this function should be used for the associated type's Swift
/// `Identifiable` protocol implementation.
pub is_swift_identifiable: bool,
Expand Down
26 changes: 26 additions & 0 deletions crates/swift-integration-tests/src/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,15 @@ mod ffi {
fn swift_arg_option_str(arg: Option<&str>) -> bool;
// fn swift_reflect_option_str(arg: Option<&str>) -> Option<&str>;
}

extern "Rust" {
#[swift_bridge(Equatable)]
type FailableInitType;

#[swift_bridge(init)]
fn new(success: bool) -> Option<FailableInitType>;
fn count(&self) -> i32;
}
}

fn test_rust_calls_swift_option_primitive() {
Expand Down Expand Up @@ -340,3 +349,20 @@ fn rust_reflect_option_struct_with_no_data(
) -> Option<ffi::OptionStruct> {
arg
}

#[derive(PartialEq)]
struct FailableInitType;

impl FailableInitType {
fn new(success: bool) -> Option<FailableInitType> {
if success {
Some(FailableInitType)
} else {
None
}
}

fn count(&self) -> i32 {
132
}
}
Loading