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 throwing initializers #287

Merged
merged 6 commits into from
Aug 15, 2024

Conversation

NiwakaDev
Copy link
Collaborator

@NiwakaDev NiwakaDev commented Aug 12, 2024

This PR implements throwing initializers. See: https://docs.swift.org/swift-book/documentation/the-swift-programming-language/errorhandling/.

Here's an example of using this feature:

// Rust
#[swift_bridge::bridge]
mod ffi {
    enum ResultTransparentEnum {
        NamedField { data: i32 },
        UnnamedFields(u8, String),
        NoFields,
    }
    extern "Rust" {
        type ThrowingInitializer;
        #[swift_bridge(init)]
        fn new(succeed: bool) -> Result<ThrowingInitializer, ResultTransparentEnum>;
        fn val(&self) -> i32;
    }
}
// Swift
do {
    let throwingInitializer = try ThrowingInitializer(false)
} catch let error as ResultTransparentEnum {
    //...
} catch {
    //...
}

@NiwakaDev NiwakaDev force-pushed the feat/throwing_init branch 2 times, most recently from 91f7898 to 646ab64 Compare August 12, 2024 12:03
TypePosition::ThrowingInit(lang) => {
match lang {
HostLang::Rust => format!(
"let val = {expression}; switch val.tag {{ case {c_ok_name}: self.init(ptr: val.payload.ok) case {c_err_name}: throw {err_swift_type} default: fatalError() }}",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess that it's not suitable because the convert_ffi_value_to_swift_value method returns a expression.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure what you're communicating?

Copy link
Owner

Choose a reason for hiding this comment

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

I think you're asking whether it's okay to return two statements here instead of a single expression?

This should be fine since the ThrowingInit code here is only ever used inside of an initializer and isn't ever embedded inside of another expression.

Copy link
Collaborator Author

@NiwakaDev NiwakaDev Aug 15, 2024

Choose a reason for hiding this comment

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

I'm not sure what you're communicating?

The other return values of convert_ffi_value_to_swift_value return closures, but this case doesn't return a closure. This means a simple statement, not expression.

For examples:

return format!("try {{ let val = {expression}; if val != nil {{ throw {err} }} else {{ return{ok} }} }}()", expression = expression, err = err, ok = ok);

"try {{ let val = {expression}; switch val.tag {{ case {c_ok_name}: return{ok_swift_type} case {c_err_name}: throw {err_swift_type} default: fatalError() }} }}()",

The above examples return closures.

@NiwakaDev NiwakaDev changed the title feat: support throwing initializers Support throwing initializers Aug 15, 2024
Copy link
Owner

@chinedufn chinedufn left a comment

Choose a reason for hiding this comment

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

Let's document #[swift_bridge(init)] in the Function Attributes section in the book

Can add a #[swift_bridge(init)] section.

Can include examples of:

  • regular init
  • throwing init
  • option init

TypePosition::ThrowingInit(lang) => {
match lang {
HostLang::Rust => format!(
"let val = {expression}; switch val.tag {{ case {c_ok_name}: self.init(ptr: val.payload.ok) case {c_err_name}: throw {err_swift_type} default: fatalError() }}",
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure what you're communicating?

@@ -527,3 +527,82 @@ void* __swift_bridge__$Foo$new(void);
.test();
}
}

/// Verify that we can use a Swift class with a throwing init.
Copy link
Owner

Choose a reason for hiding this comment

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

use a Swift class "create a Swift class"

}
extension Foo {
public convenience init() throws {
let val = __swift_bridge__$Foo$new(); switch val.tag { case __swift_bridge__$ResultFooAndSomeErrEnum$ResultOk: self.init(ptr: val.payload.ok) case __swift_bridge__$ResultFooAndSomeErrEnum$ResultErr: throw val.payload.err.intoSwiftRepr() default: fatalError() }
Copy link
Owner

Choose a reason for hiding this comment

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

Let's use an if statement instead of a switch so that we don't need this default: fatalError()

The default: fatalError makes it seem like this can fail, even though it can't.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. However, I don't address the others yet.

For example:

"try {{ let val = {expression}; switch val.tag {{ case {c_ok_name}: return{ok_swift_type} case {c_err_name}: throw {err_swift_type} default: fatalError() }} }}()",

I'll address this in the next PR, or we can open it as good first issue.

Comment on lines +299 to +302
matches!(
func.swift_failable_initializer,
Some(FailableInitializerType::Option)
);
Copy link
Owner

Choose a reason for hiding this comment

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

Need a test for FailableInitializerType::Throwing

@chinedufn
Copy link
Owner

Great work on this. Looks good. Left some minor feedback.

@NiwakaDev
Copy link
Collaborator Author

@chinedufn
Done.

@chinedufn
Copy link
Owner

Looks great thanks.

@chinedufn chinedufn merged commit 1b547a5 into chinedufn:master Aug 15, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants