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 returning Result<(), OpaqueRust> from Rust functions #180

Merged
merged 7 commits into from
Feb 26, 2023

Conversation

jfaust
Copy link
Contributor

@jfaust jfaust commented Feb 24, 2023

Adds partial support for #177 - supports returning Result<(), OpaqueRust> but not passing as an arg.

The change to examples/rust-binary-calls-swift-package/build.rs was to support xcode not existing in the default location (I use the xcodes tool).

This PR also supports empty types, based on @chinedufn's recommendation.

This enables the following:

#[swift_bridge::bridge]
mod ffi {
    struct UnitType;
    extern "Rust" {
        type OpaqueType;
        fn null_result() -> Result<(), OpaqueType>;
        fn unit_result() -> Result<UnitType, OpaqueType>;
    }
}

fn null_result() -> Result<(), OpaqueType> {
    Ok(())
}

fn unit_result() -> Result<UnitType, OpaqueType> {
    Ok(UnitType {})
}

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.

This is AWESOME work, thanks.

Left some minor comments then we can merge this.


If you don't mind, how did you figure out where/how to implement this?

I'm asking because the goal is to make it easier for folks to contribute, so I'm curious about how hard or easy it was to make your first contribution?

Cheers and thanks!

@chinedufn
Copy link
Owner

chinedufn commented Feb 25, 2023

@jfaust mind updating your PR body to include a bridge module that illustrates what this PR enables?

Here are some other PRs to use as inspiration:

#169
#158
#178


The example will get used in our release notes, and also makes old PRs that much more useful (in case we want to link someone to another PR that implemented something similar).

@jfaust
Copy link
Contributor Author

jfaust commented Feb 25, 2023

@chinedufn happy to make these changes, though it may take me a couple days to get to them.

Honestly making the changes was fairly straightforward - I found result.rs, read through it and got a bare-minimum understanding of what it was doing, and then grepped the code for ResultPtrAndPtr, which found most of the places I needed to make changes. From there it was mostly copy/paste with minor changes from the other Result code, which made it pretty easy.

I was testing it all using my codebase, so once it was working I found the rust & swift Result tests, and added test cases to them.

I'm guessing this was probably one of the easier changes to make as a first-time contributor.

swift-analyzer's "Expand macro recursively" command in vscode was also a huge help.

Thanks for building/maintaining this project!

@jfaust
Copy link
Contributor Author

jfaust commented Feb 26, 2023

@chinedufn I think I've resolved all your comments. Let me know if there's anything else to do!

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.

Looks great!! Once tests are passing we can merge!

@@ -58,10 +58,11 @@ pub(crate) trait BridgeableType: Debug {
!self.is_built_in_type()
}

/// Whether or not this type can be encoded to exactly one representation.
/// Whether or not this type can be encoded to exactly one representation,;
/// and therefore can be encoded with zero bytes.
Copy link
Owner

Choose a reason for hiding this comment

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

nice

crates/swift-integration-tests/src/result.rs Outdated Show resolved Hide resolved
@jfaust
Copy link
Contributor Author

jfaust commented Feb 26, 2023

@chinedufn I'm not sure why the Swift tests aren't compiling - they work fine on my machine with Xcode 14.2. Do you know what version of xcode is in use in the Action?

@chinedufn
Copy link
Owner

I see this in the CI output

/Applications/Xcode_13.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/swift-frontend

So I'm assuming we're on 13.2.1 in CI.

I'm seeing this as the error:

Unable to infer complex closure return type; add explicit type to disambiguate

Hmm...

@chinedufn
Copy link
Owner

chinedufn commented Feb 26, 2023

I'm on Xcode 13.3.1 and can reproduce the error locally.

image

public func rust_func_return_result_null_opaque_rust(_ succeed: Bool) throws -> () {
    try { let val = __swift_bridge__$rust_func_return_result_null_opaque_rust(succeed); if val.is_ok { return () } else { throw ResultTestOpaqueRustType(ptr: val.err!) } }()
}

@chinedufn
Copy link
Owner

Hmm.. so this fails to compile:

func foo() {
    { let foo = 5; return () }()
}

But this compiles successfully:

func foo() {
    { let foo = 5; return }()
}

@chinedufn
Copy link
Owner

chinedufn commented Feb 26, 2023

Ok, looks like it's possible to add an explicit signature at the beginning of the closure.

For example, this compiles:

struct MyStruct {}

func foo() {
    { () -> () in let foo = 5; return () }()
}

func bar() {
    { () -> MyStruct in let foo = 5; return MyStruct() }()
}

So our options (assuming we want to continue to support Xcode 13) are to either:

  1. For the null type use return instead of return (), along with a comment explaining that it's due to an Xcode 13 bug
  2. For the null type use an explicit () -> () in at the beginning of the closure, along with a comment explaining that it's due to an Xcode 13 bug
  3. OR, add the explicit () -> ReturnType in to the beginning of the closure regardless of the type

I would have assumed that having explicit types always leads to better compile times, but I did some quick googling (only looked at one result) and found that using explicit types was slower in the one link that I read (did not verify results myself, did not do thorough research, this post is two years old) https://forums.swift.org/t/regarding-swift-type-inference-compile-time-performance/49748/16

So... for now I like option 1 or 2.

Option 2 boils down to:

let mut maybe_closure_type = "";

// There is a Swift compiler bug in Xcode 13 where using an explicit `()` here somehow leads
// the Swift compiler to a compile time error:
// "Unable to infer complex closure return type; add explicit type to disambiguate"
//
// It's asking us to add a `{ () -> () in .. }` explicit type to the beginning of our closure.
//
// We can remove this if statement whenever we stop supporting Xcode 13;
if self.is_null() {
    maybe_closure_type = " () -> () in "
}

format!("try {{{maybe_closure_type} let val = {expression}; if val.is_ok {{ return {ok} }} else {{ throw {err} }} }}()")

On line 176 in bridgeable_result.rs

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.

None yet

2 participants