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

async: Fix returning Result<(), OpaqueType> #221

Merged
merged 3 commits into from
May 4, 2023

Conversation

jfaust
Copy link
Contributor

@jfaust jfaust commented Apr 25, 2023

Fixes #220

This supports bridges of the form:

#[swift_bridge::bridge]
mod ffi {
    extern "Rust" {
        type ErrorType;
        async fn some_function() -> Result<(), ErrorType>;
    }
}

async fn some_function() -> Result<(), ErrorType> {
  Ok(())
}

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.

Thanks a lot for whipping this up!!

Looks great. Left very minor feedback then this can land.

@chinedufn
Copy link
Owner

chinedufn commented Apr 26, 2023

Also, please update your PR body with an example bridge module illustrating what this PR has fixed.

You can find examples of this here https://github.com/chinedufn/swift-bridge/pulls?q=is%3Apr+is%3Aclosed

@jfaust
Copy link
Contributor Author

jfaust commented Apr 26, 2023

@chinedufn think I've resolved all your requests.

One thing I'd like to note as a relatively new contributor - getting the right Rust code for the codegen tests for a change like this was the most difficult part - this small change has outside effects on the code being output that I really didn't have much insight into. If I hadn't discovered that rust-analyzer has an option for macro expansion I'm not sure how I would have done it.

Swift & C are of course easy, because they're written to files directly.

@chinedufn
Copy link
Owner

One thing I'd like to note as a relatively new contributor

Thanks a lot for leaving feedback.

I can't tell what you mean by getting the right Rust code for the codegen test.

Do you mean knowing what to put inside of the expected Rust tokens, or being able to track down the difference between the expected Rust tokens and the actual generated ones?

@chinedufn chinedufn merged commit 7c14ea0 into chinedufn:master May 4, 2023
5 checks passed
@jfaust
Copy link
Contributor Author

jfaust commented May 4, 2023

@chinedufn both of those - the output from the test when it fails is stripped of whitespace, which makes it hard to parse what's different, even when doing a side by side diff.

But initially yes, knowing what to put for the Rust tokens. In my first PR a little while ago it was fairly easy, because all the changes to the output were ones I had explicitly made. With this PR, the actual change was tiny and caused large differences in output due to code paths I have no familiarity with. So I had to make the assumption that the code being generated was correct (it compiled and ran), and use the rust-analyzer: Expand macro recursively option in VSCode to find what was necessary to put into the test.

@chinedufn
Copy link
Owner

Gotcha, thanks for explaining. Any suggestions for what could have made your contribution process easier?

@jfaust
Copy link
Contributor Author

jfaust commented May 4, 2023

@chinedufn For the first part, if it's possible to output the input tokens instead of the whitespace-stripped ones that'd be extremely helpful.

For the second, maybe something in the contributing section of the book about how to figure out what the code should look like? e.g. explain where to look for the Swift & C code, and how to get the output from the macro? I don't have a lot of time, but can look at writing something up.

@chinedufn
Copy link
Owner

Hmm yeah we could probably use something like prettyplease for that, good idea.

That'd be great if you have the time.

Thanks for the ideas!

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.

async: Error returning Result<(), OpaqueType>
2 participants