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

Remove Error and Sendable implementations for RustStringRef #309

Open
chinedufn opened this issue Jan 27, 2025 · 10 comments · May be fixed by #310
Open

Remove Error and Sendable implementations for RustStringRef #309

chinedufn opened this issue Jan 27, 2025 · 10 comments · May be fixed by #310
Labels
good first issue Good for newcomers

Comments

@chinedufn
Copy link
Owner

Originally reported: #296 (comment)


The Error protocol was implemented for RustStringRef in #296

The RustStringRef class looks like:

// Swift

class RustStringRef {
    ptr: UnsafeMutableRawPointer

    // ...
}

Up until Swift 5.6, UnsafeMutableRawPointer implemented the Sendable protocol, meaning it was possible to "safely" implement the Error protocol on a class that contains an UnsafeMutableRawPointer (Error protocol requires Sendable).

As of Swift 5.6 UnsafeMutableRawPointer no longer implements Sendable. https://github.com/swiftlang/swift-evolution/blob/main/proposals/0331-remove-sendable-from-unsafepointer.md


We should remove the Error implementation from RustStringRef.

This is a thread safety issue, so we can release this breaking change in a 0.1.x release.
We won't promise stability until swift-bridge hits version 1.0.0.


Here's where Error is implemented:

/// exercised in SwiftRustIntegrationTestRunner/SwiftRustIntegrationTestRunnerTests/ResultTests.swift:
/// - see `func testSwiftCallRustReturnsResultString`
extension RustStringRef: Error {}

@chinedufn
Copy link
Owner Author

This issue should be closed since, as long as Rust's aliasing rules are followed, a RustString, RustStringRef and RustStringRefMut can all safely implement Sendable.


  • RustString can implement Swift Sendable because its owned by Swift. So, once Swift ownership support is complete Enforce ownership in generated Swift code #155 , there will only ever be one owner of an owned Rust String at any time
  • RustStringRef is immutable, and a Rust String is Rust Send+Sync, so RustStringRef can be Swift Sendable
  • If Rust's aliasing rules are followed, then there can only be one RustStringRefMut at any time. Meaning it is safe to send across threads. Once we support Swift ~Copyable, Swift code won't be able to implicitly copy a RustStringRefMut

So, after Swift ownership (i.e. ~Copyable) is supported RustString, RustStringRef and RustStringRefMut will all be safe to implement Swift Sendable.
For RustStringRef and RustStringRefMut the user's Swift code will need to ensure that the lifetimes are upheld. If they are, then they can be safely used in multi-threaded code.

Here's an example of Rust code that passes a Rust - &mut String across threads. It's safe because Rust's aliasing rules require that there is only one mutable reference to a type.
So, as long as the Swift code respects Rust's aliasing rules, we can safely implement Swift Sendable for RustStringRefMut.

For example, the following Rust code compiles, which demonstrates that a Rust &mut String
can be safely passed across threads.

playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a0f25b32652be6df51993f01b8d51728

let mut string = "hello".to_string();

let string_ref_mut = &mut string;
std::thread::scope(move |scope| {
    let string_ref_mut = string_ref_mut;

    scope.spawn(move || {
        string_ref_mut.push_str(" world");
    });
});

assert_eq!(string, "hello world");

@colinmarc
Copy link

This issue should be closed since, as long as Rust's aliasing rules are followed, a RustString, RustStringRef and RustStringRefMut can all safely implement Sendable.

Agreed, as long as...

Swift ownership support is complete.

So I think RustStr and RustStrRefMut can't be Sendable right now, but they can be made Sendable in the future once aliasing is (mostly) prevented by the work in #155. I don't think this issue should be closed yet.

RustStrRef could be considered immutable today, but I don't see a way to make it Sendable since it's not final (and the other classes inherit from it).

@chinedufn
Copy link
Owner Author

swift-bridge's current approach to safety is to list a set of rules, and if those rules are followed then your interactions with the code that swift-bridge generates is safe.

Supporting Swift's ownership will allow us to reduce that list of rules, but unless Swift releases more support for lifetimes and borrowing we won't get that list down to zero.

Here is an example of a rule that can be deleted once Swift ownership is implemented, since it will automatically enforced: https://github.com/chinedufn/swift-bridge/blob/master/book/src/safety/README.md?plain=1#L122-L148


RustString, RustStringRef and RustStringRefMut are thread safe to use if the user upholds swift-bridge's memory safety rules

## Memory Safety
You can ensure the memory safety of your Rust and Swift projects by following these rules:
- In Swift, never use a reference to a Rust type after its lifetime.
- In Swift, pass a mutable reference to Rust that will live alongside another active reference to that type.
- In Swift, never use a type after passing ownership to Rust.

Our current approach is to trust that the user will follow these rules, thereby enabling us to release functionality that relies on these rules.

If you are not following those memory safety rules then your program will be riddled with undefined behavior regardless of whether or not RustString* is Sendable.
Sending objects across threads does increase the likelihood of you making a mistake, but this is currently left to the user to manage until we're better able to support them.

In short, we're putting max power/performance into your fingertips first, while simultaneously introducing new ways to reduce the number of mistakes that you'll make.
More complex / error-prone applications can use synchronization primitives to help avoid mistakes (i.e. wrap everything in an Arc<Mutex<T>>.

Therefore, we should keep RustString* as Sendable.


In short, if you aren't following those memory safety rules your program is already exhibiting UB, and if you are following them then RustString* are thread safe.
So, our focus should be on introducing more memory safety guardrails ( such as #155 ).


Let me know if you take issue with this approach. If not, I'll close out #310

@colinmarc
Copy link

I understand your argument, and of course it's your call in the end. I think I'd still draw a distinction between "we give you these unsafe tools, here are the rules for using them" and "we're marking a type as Sendable even though it's extremely not". The latter, to me, amounts to incorrect documentation. The user has tools to circumvent swift's restrictions if they really want to; you can wholesale disable swift's concurrency checks at compile-time if you want, or they could mark their error type as @unchecked Sendable themselves.

Finally, I'll point out that without #310, swift-bridge's generated code simply does not compile on recent swift versions.

Like I said, totally your call! Please feel free to close #310 if that's your decision, I don't mind. I would like to work more on #307, and I can't get it to compile on master right now - that was the motivation.

@chinedufn
Copy link
Owner Author

chinedufn commented Feb 4, 2025

The main wrench is that to implement Error requires implementing Sendable.
So if we were to remove Sendable support then users writing single threaded code couldn't return RustStrings as errors despite it being perfectly safe for them to do so.
I suppose they could add @unchecked Sendable themselves like you said.


I started working on adding ownership support to RustString.

Along the way I ran into a problem, if your type is ~Copyable then you currently can't implement the Error protocol.

Meaning, as it stands now, if we implemented the Swift ownership proposal from #155 (comment) then extern Rust types could no longer be used as errors when returning a -> Result<T, E> from Rust to Swift.

I opened an issue in swiftlang/swift swiftlang/swift#79141

If the maintainers don't reply in the next few days then we can revert Error/Sendable support for RustString so that #307 can land.

Or, better yet, how about this:

  • can you make your revert PR comment out the RustString Error stuff that isn't compiling
  • in each place, leave a TODO that describes when we can uncomment it, and says "search this string '...' to find the other TODOs"
  • this way, when we eventually have ownership for RustString we can easily go uncomment all the commented out tests

And if someone asks why the behavior regressed we can point them towards that comment (or link to this issue if you want put the checklist of things to uncomment into this issue).

If that works for you we can revert the RustString's Error conformance.


Here's how the new RustString looks (WIP):

// Rust type

#[repr(C)]
pub struct RustString {
    ptr: *mut u8,
    len: usize,
    capacity: usize,
}

impl RustString {
    fn into_std_string(self) -> String {
        unsafe { String::from_raw_parts(self.ptr, self.len, self.capacity) }
    }

    // Used by the bridge macro to convert a `std::string::String` into a `RustString`.
    #[doc(hidden)]
    pub fn from_std_string(mut string: String) -> Self {
        let rust_string = Self {
            ptr: string.as_mut_ptr(),
            len: string.len(),
            capacity: string.capacity(),
        };

        std::mem::forget(string);

        rust_string
    }
}
// C header
typedef struct __swift_bridge__RustString { uint8_t* const ptr; uintptr_t len; uintptr_t capacity } __swift_bridge__RustString;
// Swift

/// A string that was created in Rust.
public struct RustString: ~Copyable {
    private var string: __swift_bridge__RustString

    public init() {
        self.string = __swift_bridge__$RustString$new()
    }

    public init<GenericToRustStr: ToRustStr>(_ str: GenericToRustStr) {
        str.toRustStr({ strAsRustStr in
            self.string = __swift_bridge__$RustString$new_with_str(strAsRustStr)
        })
    }

    /// Get a pointer to the string's UTF-8 buffer.
    func asPtr() -> UnsafePointer<UInt8> {
        let ptr = self.string.ptr!
        return UnsafePointer(ptr)
    }

    /// Get the string's length.
    func len() -> UInt {
        self.string.len
    }

    /// Return a reference to the string's UTF-8 byte buffer.
    func asStr() -> RustStr {
        RustStr(start: self.string.ptr, len: self.string.len)
    }

    func trim() -> RustStr {
        self.asStr().trim()
    }

    /// Convert the `RustString` into a Swift `String`.
    func toString() -> String {
        let rust_str = self.asStr();
        let swift_string = rust_str.toString()
        return swift_string
    }

    deinit {
        __swift_bridge__$RustString$_free(self.string)
    }
}

A nice advantage is that before this new implementation we passed Rust's std::string::String to Swift by boxing it Box::into_raw(Box::new("hello world".to_string()));

In the new implementation we pass the Rust std::string::String from Rust -> Swift without allocating.


here's the implementation that is currently in the master branch (that the above sketch should replace once swiftlang/swift#79141 is addressed)

#[swift_bridge_macro::bridge(swift_bridge_path = crate)]
mod ffi {
extern "Rust" {
type RustString;
#[swift_bridge(init)]
fn new() -> RustString;
#[swift_bridge(init)]
fn new_with_str(str: &str) -> RustString;
fn len(&self) -> usize;
fn as_str(&self) -> &str;
fn trim(&self) -> &str;
}
}

@colinmarc
Copy link

colinmarc commented Feb 5, 2025

Along the way I ran into a problem, if your type is ~Copyable then you currently can't implement the Error protocol.

It seems like Error types are really meant to be immutable enums and structs with copy semantics. That's the direction the language is pushing you.

I raised this at some point earlier, but what about just copying the error into an immutable string on the swift side? I think in the general case, it makes sense to have String match Rust semantics, and be a pointer to a heap allocation. For Error messages, it's probably not worth the hassle.

That would mean that:

  • RustString can be Sendable (using your code above)
  • RustString can't be Error
  • We can still support Result<_, String> ~> throws by special-casing it
  • We can add a swift_bridge(error) annotation, but only for transparent structs/enums, and support Result<_, T> for those types

@chinedufn
Copy link
Owner Author

We can add a swift_bridge(error) annotation, but only for transparent structs/enums

Ideally transparent structs/enums would be non-copyable so that ownership could be enforced.

What would this attribute do exactly?

but what about just copying the error into an immutable string on the swift side? I think in the general case, it makes sense to have String match Rust semantics, and be a pointer to a heap allocation.
For Error messages, it's probably not worth the hassle.

The only reason RustString exists is to avoid implicitly copying the string's memory. #5 (comment)

When I checked in Dec 2021 there was no way to create a Swift String from a byte buffer without copying.

To avoid scenarios where a user experiences surprisingly bad performance when dealing with large strings, we want to avoid implicitly copying a potentiall a Rust std::string::String's byte buffer,
but explicitly copying it could be okay.


perhaps something like:

#[swift_bridge::bridge]
mod ffi {
	extern "Rust" {
        #[swift_bridge(some_attribute_that_indciates_the_string_will_be_cloned)]
        fn return_result() -> Result<(), String>;
    }
}

Then on the Swift side the user would get a Swift String as the error. The user would never interact with a RustString in the above example FFI.


An alternative design would be using a different type in the bridge module that is explicitly documented to always lead to copying the string's bytes.

#[swift_bridge::bridge]
mod ffi {
	extern "Rust" {
        fn return_result() -> Result<(), SwiftString>;
    }
}

fn return_result() -> Result<(), std::string::String> {
    Err("".to_string())
}

When we see this SwiftString type (I'm just making up a placeholder name ...) in an extern "Rust" function's return value we'd automatically convert into a Swift String in the generated Swift code.
Our documentation would explain that the String's bytes would be copied.

It seems like Error types are really meant to be immutable enums and structs with copy semantics. That's the direction the language is pushing you.

from the Swift Error docs:

Any type that declares conformance to the Error protocol can be used to represent an error in Swift’s error handling system.
Because the Error protocol has no requirements of its own, you can declare conformance on any custom type you create.

via: https://developer.apple.com/documentation/swift/error

This gives me the impression that Error protocol is meant to be flexible enough for any use case.

@chinedufn
Copy link
Owner Author

^ both -> Result<(), String> designs above seem unpleasant. I just wrote them out to explore the design space ..

Ideal designs would be:

  • Noncopyable types can implement the Error protocol, meaning a new ~Copyable RustString implementation could implement Error
  • OR zero-copy conversion between Rust std::string::String -> Swift stdlib String meaning RustString could be deleted entirely

Neither of those ideal designs are possible today (unless something has changed in the last few years that makes it possible to create a Swift String with zero-copying. I haven't looked back into that.)

@chinedufn
Copy link
Owner Author

It turns out that noncopyable Swift types are more limited than I realized. I wrote a bit about this here #155 (comment)

In short, they can't implement Swift protocols like Error, Hashable, Equatable or nearly anything else.

So, switching RustString over to be ~Noncopyable has trade-offs. Enough trade-offs that we can't do it blindly without some design work.


So, we'll keep RustString, RustStringRef and RustStringRefMut as class based for now since more work needs to be done to figure out how to best integrate Noncopyable structs into swift-bridge.


Here are my propose next steps:

  • fix the RustString* issues on Swift 6 by implementing @unchecked Sendable for RustStringRef
  • document the safety considerations when using RustString*
  • this does mean that a user that breaks Rust's aliasing rules will be capable of accidentally introducing thread safety bugs into their code, but it is worth noting that they are already entering undefined-behavior if they're breaking the aliasing rules.

Reasons to go with it:

  • better experience for using writing single threaded code that just want to be able to return errors
  • non-breaking change (we already implement @Sendable for RustStringRef
  • it's only not-thread-safe when you're already doing things that are unsafe (breaking Rust's aliasing rules)
    • ultimately we have to choose between these sorts of trade-offs since Swift does not currently enable us to make FFI code fully safe. Users must follow a set of rules, and making RustString @Sendable does not increase this ruleset (can still add more documentation about thread safety of strings

I'll proceed with implementing this. I'll wait at least a couple days to think it over. If you have any new points/challenges feel free to share.

@colinmarc
Copy link

Sorry for pushing back so much - I just disagree and I'm argumentative by nature :) Feel free to ignore/override any of the below, I won't take it personally!

Immutable strings not necessarily slower

I agree with your contention that copying around heap strings is not an inherently fast operation. However, immutable, copy-on-write strings are not an anti-pattern! Golang and Swift are both fast languages that insist on immutable strings, and are comfortable with the performance properties of them.

People using Result<_, String> don't care about performance

The following code is very far from optimal:

fn toggle_foobar() -> Result<(), String> {
    return Err("help I can't toggle the foobar".to_owned())
}

Let's assume a pathological case where we're calling this a million times a second, and we've implemented my suggestion of copying the string just for Result<_, String>. I would be much more worried about the allocation than the copy.

Note that in real life, errors usually are an exception, not the rule, so they're usually not a hot path.

Result<_, String> is an antipattern anyway

Neither language encourages you to do the above. Here is what a good error looks like in Rust (from the rust docs)

#[derive(Debug, Clone)]
struct DoubleError;

impl fmt::Display for DoubleError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "invalid first item to double")
    }
}

And here is an error implementation from the swift docs:

enum IntParsingError: Error {
    case overflow
    case invalidInput(Character)
}

<snip>

do {
    let price = try Int(validating: "$100")
} catch IntParsingError.invalidInput(let invalid) {
    print("Invalid character: '\(invalid)'")
} catch IntParsingError.overflow {
    print("Overflow error")
} catch {
    print("Other error")
}

Note that in either case, there are no mutable, heap-allocated strings! Both use static compile-time strings for printing error messages. And both languages push you towards using values or records as errors, not objects.

A ~Copyable heap pointer is still a great idea

Like you say, always copying heap strings around could be a performance trap - I'm only advocating for doing that for the Result<_, String> case (or just not supporting that at all). I still very much like your idea of having RustString: ~Copyable for most strings.

For RustString or some other opaque type, it's actually a feature, not a bug, that they are not Equatable, Hashable, etc. That's not how Eq or Hash work in Rust either. Let me explain with a quick example:

impl PartialEq for Point {
    fn eq(&self, other: &Self) -> bool {
        self.x == other.x && self.y == other.y
    }
}

The critical piece here is that eq takes a borrow, not a mutable pointer or an owned value. In your proposed framework, this would mean that RustStrRef could be Hashable, Equatable, etc, because RustStrRef is safe to copy around. This exactly matches rust semantics.

You would have to explicitly borrow the RustString in swift, I guess:

s1.asRef() == s2.asRef()

But that's just performing an operation that rust does implicitly, when you call <foo as String>::eq(other_string).

Pass-by-value structs and enums should be usable across the bridge

Responding to this:

Ideally transparent structs/enums would be non-copyable so that ownership could be enforced.

Maybe I'm misunderstanding, but I definitely don't think you should force everyone to use pointers across the bridge for every non-primitive type. Most enums and small structs are just fine as values - it makes code simpler and in some cases even faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants