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

Revert "Support Rust returning -> Result<_, String> (#296)" #310

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

colinmarc
Copy link

@colinmarc colinmarc commented Jan 27, 2025

This reverts commit c45a38c, which added a type assertion to mark RustStr as Error in the generated swift code. The motivation for that change was to allow rust functions that return Result<_, String> to throw an error when called from swift. The critical change was the following:

extension RustStrRef: Error // implies RustStrRef: Sendable

However, that change was unsound, because swift's Error inherits from Sendable, so the above code implies RustStrRef: Sendable. Furthermore, because RustStr and RustStrRefMut inherit from RustStrRef, that change marked all three as Sendable.

Swift requires Sendable types to either be immutable or mutable in a thread-safe way. RustStr does not fulfill those requirements, because it's a pointer to a mutable heap allocation. Swift 6 added a lint for this, which means that the above code refuses to compile. But even if we make it compile, adding @unchecked Sendable to work around the lint, the types still don't fulfill the required semantics, so it's a thread safety issue. Therefore, the best thing is just to back out the change.

Fixes #309. For a better approach to thread-safety and Sendable, see #150.

@chinedufn
Copy link
Owner

chinedufn commented Jan 30, 2025

Please modify your PR body such that a reader can get a decent sense of the problem that this PR is solving, without needing to click out to other links.

It's okay to link to other stuff, but we want to make sure that our commit messages (I'll use the PR body as the commit message when I rebase the PR onto the master branch) can be understood without an internet connection.

You can run git log to see examples of past commit messags.

Once that's set I can merge this.


Main thing to add to this PR body is the reason that we're reverting this.
Reader needs to get a basic understanding of the purpose of this change without needing to click through to the supplemental material that you've linked.

@colinmarc
Copy link
Author

Sorry, just for clarification, you want the full context in the PR body, the commit message, or both?

@chinedufn
Copy link
Owner

I'm going to take your PR body and use it as the final rebased commit message.

So, just the PR body is fine.

@colinmarc
Copy link
Author

I updated the PR body. :)

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.

Remove Error and Sendable implementations for RustStringRef
2 participants