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

Add RustVec<T>.as_ptr method #216

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

aiongg
Copy link
Contributor

@aiongg aiongg commented Apr 24, 2023

This PR adds a method to the Vectorizable protocol:

static func vecOfSelfAsPtr(vecPtr: UnsafeMutableRawPointer) -> UnsafePointer<SelfRef>

My reason for adding this method was to be able to easily pass a serialized Protobuf object back and forth between Rust and Swift. Motivating example:

// rust

// Receives a serialized protobuf message and returns a serialized protobuf message
fn send_serialized_proto(bytes: &[u8]) -> Option<Vec<u8>> {
    // ...
}

Without .as_ptr(), it was necessary to return the raw pointer from Rust, which meant also returning the length separately, so using the Rust method was a bit less ergonomic on both sides. Here's how it looks now:

// swift

let message = My_Proto_Message()
// ...
guard let input = message.serializedData() else { ... }

guard let output: RustVec<UInt8>? = data.withUnsafeBytes {
    (ptr: UnsafeRawBufferPointer) -> RustVec<UInt8>? in
    let u8ptr = ptr.bindMemory(to: UInt8.self)
    return send_serialized_proto(u8ptr)
} else { ... }

do {
    let response = try My_Proto_Response.init(
        serializedData: Data(
            // The `.as_ptr()` method keeps this very straightforward
            bytes: output.as_ptr(),
            count: output.len()
        )
    )
} catch { ... }

// ...

Comment on lines +39 to +44
func testRustVecU8AsPtr() throws {
let vec = RustVec<UInt8>()
vec.push(value: 10)
let ptr = vec.as_ptr()
XCTAssertEqual(ptr.pointee, 10)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basic test as described in the issue.

Comment on lines 146 to 147
public typealias Elem = {swift_ty}

Copy link
Contributor Author

@aiongg aiongg Apr 24, 2023

Choose a reason for hiding this comment

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

Add the new typealias up top, and add the method implementation below (I put it after the get methods) - it's the same procedure for all of these generated code segments.

Comment on lines 187 to 190
public static func vecOfSelfAsPtr(vecPtr: UnsafeMutableRawPointer) -> UnsafePointer<Elem> {{
UnsafePointer<Elem>(OpaquePointer(__swift_bridge__$Vec_{rust_ty}$as_ptr(vecPtr)))
}}

Copy link
Contributor Author

@aiongg aiongg Apr 24, 2023

Choose a reason for hiding this comment

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

We just leave the type parameter as Elem in line with the Vectorizable protocol, and all we have to do is change the method name to call the appropriate $Vec_{rust_ty}$as_ptr function.

Comment on lines 90 to 93
public static func vecOfSelfAsPtr(vecPtr: UnsafeMutableRawPointer) -> UnsafePointer<UInt8> {
UnsafePointer<UInt8>(OpaquePointer(__swift_bridge__$Vec_RustString$as_ptr(vecPtr)))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Special handling for RustString, which takes a UInt8 type parameter instead of the usual Self.

Comment on lines 53 to 56
fn as_ptr(&self) -> *const u8 {
self.0.as_ptr()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementation for RustString. This is the *const u8 that maps to UnsafePointer<UInt8> in swift.

Comment on lines 94 to 96
associatedtype Elem
associatedtype SelfRef
associatedtype SelfRefMut
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new associatedtype Elem for the Vectorizable protocol

@chinedufn
Copy link
Owner

Thanks for submitting! Will review tonight or tomorrow

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.

.as_ptr() for a RustVec<RustString> should not return an UnsafePointer<UInt8>, but rather an UnsafePointer<RustStringRef>.

Consider the following Rust code:

let strings: Vec<String> = vec![];
let ptr: *const String = strings.as_ptr();

Vec<String>.as_ptr() returns a *const String.

Maybe you were accidentally looking at String.as_ptr(), which is unrelated to this PR.


So, I think we can remove the Elem associated type and instead just have the existing SelfRef associated type

, which from RustVec<RustString> is a RustStringRef
public static func vecOfSelfGet(vecPtr: UnsafeMutableRawPointer, index: UInt) -> Optional<RustStringRef> {

@aiongg
Copy link
Contributor Author

aiongg commented Apr 26, 2023

Ok great I'll try it. I think I tried that the first time and encountered some issue, but if you think it should work then I must have done something wrong so I'll try again and update the PR if it works.

@chinedufn
Copy link
Owner

chinedufn commented Apr 26, 2023

Mmm I'm not certain that it'll work exactly as I described it but it seems directionally correct. Let me know if you run into an issue.

@aiongg
Copy link
Contributor Author

aiongg commented Apr 26, 2023

@chinedufn you were right, I was confusing RustString with RustVec<RustString>. Works fine with SelfRef as you described. Makes more sense now, too. I guess that's what I get for trying to do it at 2am last time 😅

@chinedufn
Copy link
Owner

Thanks looks good.

All that's missing is to update your PR body with quick example code showing of what this PR now enables.

Here's an example from another PR that you can use as inspiration #189

These examples get used in our release notes, and they help folks that are looking at old PRs understand exactly what the PR does.

Once you add an example (and perhaps clean up the info in your PR body that is no longer true) we can merge this!

@aiongg
Copy link
Contributor Author

aiongg commented Apr 26, 2023

@chinedufn done, please review.

@chinedufn
Copy link
Owner

chinedufn commented Apr 26, 2023

Thanks!

FYI for PRs you can just show bridge module and an example of using that module like this:

// Rust

#[swift_bridge::bridge]
mod ffi {
    extern "Rust" {
        fn send_serialized_proto(bytes: &[u8]) -> Option<Vec<u8>>;
    }
}
// Swift

// ... then you could've put your example of using it here ...

@chinedufn chinedufn changed the title Add vecOfSelfAsPtr method Add RustVec<T>.as_ptr method Apr 26, 2023
@chinedufn chinedufn changed the title Add RustVec<T>.as_ptr method Add RustVec<T>.as_ptr method Apr 26, 2023
@chinedufn chinedufn merged commit 7e140ca into chinedufn:master Apr 26, 2023
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