More fixes from review.

Most of these are doc fixes.  The one behavior change is that the
callback return values were updated.  If we're breaking the ABI we might
as well make these be a bit more logical.

CHANGELOG.md

@@ -15,7 +15,9 @@
 [All changes in [[UnreleasedVersion]]](https://github.com/mozilla/uniffi-rs/compare/v0.23.0...HEAD).
 
 ### ⚠️ Breaking Changes ⚠️
-- ABI: Implemented a new callback-interface ABI that significantly improves performance on Python and Kotlin.  UniFFI users will automatically get the benefits of this without any code changes.  This is a breaking change because the new generated code is not compatible with code generated with UniFFI 0.23.x.
+- ABI: Implemented a new callback-interface ABI that significantly improves performance on Python and Kotlin.
+  - UniFFI users will automatically get the benefits of this without any code changes.
+  - External bindings authors will need to update their bindings code.  See PR #1494 for details.
 
 ### What's changed
 

fixtures/benchmarks/README.md

@@ -1,5 +1,10 @@
 This fixture runs a set of benchmark tests, using criterion to test the performance.
 
+- `cargo bench` to run all benchmarks.
+- `cargo bench -- -p` to run all python benchmarks (or -s for swift, -k for kotlin)
+- `cargo bench -- [glob]` to run a subset of the benchmarks
+- `cargo bench -- --help` for more details on the CLI
+
 Benchmarking UniFFI is tricky and involves a bit of ping-pong between Rust and
 the foreign language:
 

fixtures/benchmarks/benches/bindings/run_benchmarks.kts

@@ -6,14 +6,14 @@ import uniffi.benchmarks.*
 import kotlin.system.measureNanoTime
 
 class TestCallbackObj : TestCallbackInterface {
-    override fun testMethod(a: Int, b: Int, data: TestData): String {
+    override fun method(a: Int, b: Int, data: TestData): String {
         return data.bar;
     }
 
-    override fun testVoidReturn(a: Int, b: Int, data: TestData) {
+    override fun methodWithVoidReturn(a: Int, b: Int, data: TestData) {
     }
 
-    override fun testNoArgsVoidReturn() {
+    override fun methodWithNoArgsAndVoidReturn() {
     }
 
     override fun runTest(testCase: TestCase, count: ULong): ULong {

fixtures/benchmarks/benches/bindings/run_benchmarks.py

@@ -6,13 +6,13 @@
 import time
 
 class TestCallbackObj:
-    def test_method(self, a, b, data):
+    def method(self, a, b, data):
         return data.bar
 
-    def test_void_return(self, a, b, data):
+    def method_with_void_return(self, a, b, data):
         pass
 
-    def test_no_args_void_return(self):
+    def method_with_no_args_and_void_return(self):
         pass
 
     def run_test(self, test_case, count):

fixtures/benchmarks/benches/bindings/run_benchmarks.swift

@@ -13,14 +13,14 @@
 #endif
 
 class TestCallbackObj: TestCallbackInterface {
-    func testMethod(a: Int32, b: Int32, data: TestData) -> String {
+    func method(a: Int32, b: Int32, data: TestData) -> String {
         return data.bar
     }
 
-    func testVoidReturn(a: Int32, b: Int32, data: TestData) {
+    func methodWithVoidReturn(a: Int32, b: Int32, data: TestData) {
     }
 
-    func testNoArgsVoidReturn() {
+    func methodWithNoArgsAndVoidReturn() {
     }
 
     func runTest(testCase: TestCase, count: UInt64) -> UInt64 {

fixtures/benchmarks/src/benchmarks.udl

@@ -31,9 +31,9 @@ callback interface TestCallbackInterface {
   // including: popping arguments from the stack, unpacking RustBuffers,
   // pushing return values back to the stack, etc.
 
-  string test_method(i32 a, i32 b, TestData data);  // Should return data.bar
-  void test_void_return(i32 a, i32 b, TestData data);
-  void test_no_args_void_return();
+  string method(i32 a, i32 b, TestData data);  // Should return data.bar
+  void method_with_void_return(i32 a, i32 b, TestData data);
+  void method_with_no_args_and_void_return();
 
   // Run a performance test N times and return the elapsed time in nanoseconds
   u64 run_test(TestCase test_case, u64 count);

fixtures/benchmarks/src/lib.rs

@@ -20,9 +20,9 @@ pub enum TestCase {
 }
 
 pub trait TestCallbackInterface {
-    fn test_method(&self, a: i32, b: i32, data: TestData) -> String;
-    fn test_void_return(&self, a: i32, b: i32, data: TestData);
-    fn test_no_args_void_return(&self);
+    fn method(&self, a: i32, b: i32, data: TestData) -> String;
+    fn method_with_void_return(&self, a: i32, b: i32, data: TestData);
+    fn method_with_no_args_and_void_return(&self);
     fn run_test(&self, test_case: TestCase, count: u64) -> u64;
 }
 
@@ -61,7 +61,7 @@ pub fn run_benchmarks(language: String, cb: Box<dyn TestCallbackInterface>) {
         .noise_threshold(0.05)
         .bench_function(format!("{language}-callbacks-basic"), |b| {
             b.iter(|| {
-                cb.test_method(
+                cb.method(
                     10,
                     100,
                     TestData {
@@ -73,7 +73,7 @@ pub fn run_benchmarks(language: String, cb: Box<dyn TestCallbackInterface>) {
         })
         .bench_function(format!("{language}-callbacks-void-return"), |b| {
             b.iter(|| {
-                cb.test_void_return(
+                cb.method_with_void_return(
                     10,
                     100,
                     TestData {
@@ -84,7 +84,7 @@ pub fn run_benchmarks(language: String, cb: Box<dyn TestCallbackInterface>) {
             })
         })
         .bench_function(format!("{language}-callbacks-no-args-void-return"), |b| {
-            b.iter(|| cb.test_no_args_void_return())
+            b.iter(|| cb.method_with_no_args_and_void_return())
         });
 
     c.final_summary();

uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceRuntime.kt

@@ -41,6 +41,10 @@ interface ForeignCallback : com.sun.jna.Callback {
 // Magic number for the Rust proxy to call using the same mechanism as every other method,
 // to free the callback once it's dropped by Rust.
 internal const val IDX_CALLBACK_FREE = 0
+// Callback return codes
+internal const val UNIFFI_CALLBACK_SUCCESS = 0
+internal const val UNIFFI_CALLBACK_ERROR = 1
+internal const val UNIFFI_CALLBACK_UNEXPECTED_ERROR = 2
 
 public abstract class FfiConverterCallbackInterface<CallbackInterface>(
     protected val foreignCallback: ForeignCallback

uniffi_bindgen/src/bindings/kotlin/templates/CallbackInterfaceTemplate.kt

@@ -27,15 +27,15 @@ internal class {{ foreign_callback }} : ForeignCallback {
         return when (method) {
             IDX_CALLBACK_FREE -> {
                 {{ ffi_converter_name }}.drop(handle)
-                // No return value.
-                // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs`
-                0
+                // Successful return
+                // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs`
+                UNIFFI_CALLBACK_SUCCESS
             }
             {% for meth in cbi.methods() -%}
             {% let method_name = format!("invoke_{}", meth.name())|fn_name -%}
             {{ loop.index }} -> {
                 // Call the method, write to outBuf and return a status code
-                // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` for info
+                // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs` for info
                 try {
                     this.{{ method_name }}(cb, argsData, argsLen, outBuf)
                 } catch (e: Throwable) {
@@ -46,20 +46,20 @@ internal class {{ foreign_callback }} : ForeignCallback {
                     } catch (e: Throwable) {
                         // If that fails, then it's time to give up and just return
                     }
-                    -1
+                    UNIFFI_CALLBACK_UNEXPECTED_ERROR
                 }
             }
             {% endfor %}
             else -> {
                 // An unexpected error happened.
-                // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs`
+                // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs`
                 try {
                     // Try to serialize the error into a string
                     outBuf.setValue({{ Type::String.borrow()|ffi_converter_name }}.lower("Invalid Callback index"))
                 } catch (e: Throwable) {
                     // If that fails, then it's time to give up and just return
                 }
-                -1
+                UNIFFI_CALLBACK_UNEXPECTED_ERROR
             }
         }
     }
@@ -84,7 +84,7 @@ internal class {{ foreign_callback }} : ForeignCallback {
                 {%- endfor %}
             )
             outBuf.setValue({{ return_type|ffi_converter_name }}.lowerIntoRustBuffer(returnValue))
-            return 1
+            return UNIFFI_CALLBACK_SUCCESS
         }
         {%- when None %}
         fun makeCall() : Int {
@@ -94,7 +94,7 @@ internal class {{ foreign_callback }} : ForeignCallback {
                 {%- if !loop.last %}, {% endif %}
                 {%- endfor %}
             )
-            return 1
+            return UNIFFI_CALLBACK_SUCCESS
         }
         {%- endmatch %}
 
@@ -107,7 +107,7 @@ internal class {{ foreign_callback }} : ForeignCallback {
         } catch (e: {{ error_type|type_name }}) {
             // Expected error, serialize it into outBuf
             outBuf.setValue({{ error_type|ffi_converter_name }}.lowerIntoRustBuffer(e))
-            -2
+            UNIFFI_CALLBACK_ERROR
         }
         {%- endmatch %}
 

uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceRuntime.py

@@ -40,6 +40,10 @@ def remove(self, handle):
 # Magic number for the Rust proxy to call using the same mechanism as every other method,
 # to free the callback once it's dropped by Rust.
 IDX_CALLBACK_FREE = 0
+# Return codes for callback calls
+UNIFFI_CALLBACK_SUCCESS = 0
+UNIFFI_CALLBACK_ERROR = 1
+UNIFFI_CALLBACK_UNEXPECTED_ERROR = 2
 
 class FfiConverterCallbackInterface:
     _handle_map = ConcurrentHandleMap()

uniffi_bindgen/src/bindings/python/templates/CallbackInterfaceTemplate.py

@@ -40,7 +40,7 @@ def makeCallAndHandleReturn():
             {%- when None %}
             makeCall()
             {%- endmatch %}
-            return 1
+            return UNIFFI_CALLBACK_SUCCESS
 
         {%- match meth.throws_type() %}
         {%- when None %}
@@ -53,7 +53,7 @@ def makeCallAndHandleReturn():
             with RustBuffer.allocWithBuilder() as builder:
                 {{ err|write_fn }}(e, builder)
                 buf_ptr[0] = builder.finalize()
-            return -2
+            return UNIFFI_CALLBACK_ERROR
         {%- endmatch %}
 
     {% endfor %}
@@ -64,15 +64,15 @@ def makeCallAndHandleReturn():
 
     if method == IDX_CALLBACK_FREE:
         {{ ffi_converter_name }}.drop(handle)
-        # No return value.
-        # See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs`
-        return 0
+        # Successfull return
+        # See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs`
+        return UNIFFI_CALLBACK_SUCCESS
 
     {% for meth in cbi.methods() -%}
     {% let method_name = format!("invoke_{}", meth.name())|fn_name -%}
     if method == {{ loop.index }}:
         # Call the method and handle any errors
-        # See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs` for details
+        # See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs` for details
         try:
             return {{ method_name }}(cb, RustBufferStream(args_data, args_len), buf_ptr)
         except BaseException as e:
@@ -83,16 +83,16 @@ def makeCallAndHandleReturn():
             except:
                 # If that fails, just give up
                 pass
-            return -1
+            return UNIFFI_CALLBACK_UNEXPECTED_ERROR
     {% endfor %}
 
     # This should never happen, because an out of bounds method index won't
     # ever be used. Once we can catch errors, we should return an InternalException.
     # https://github.com/mozilla/uniffi-rs/issues/351
 
     # An unexpected error happened.
-    # See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs`
-    return -1
+    # See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs`
+    return UNIFFI_CALLBACK_UNEXPECTED_ERROR
 
 # We need to keep this function reference alive:
 # if they get GC'd while in use then UniFFI internals could attempt to call a function

uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceRuntime.swift

@@ -58,3 +58,7 @@ fileprivate class UniFFICallbackHandleMap<T> {
 // Magic number for the Rust proxy to call using the same mechanism as every other method,
 // to free the callback once it's dropped by Rust.
 private let IDX_CALLBACK_FREE: Int32 = 0
+// Callback return codes
+private let UNIFFI_CALLBACK_SUCCESS: Int32 = 0
+private let UNIFFI_CALLBACK_ERROR: Int32 = 1
+private let UNIFFI_CALLBACK_UNEXPECTED_ERROR: Int32 = 2

uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift

@@ -37,7 +37,7 @@ fileprivate let {{ foreign_callback }} : ForeignCallback =
             var writer = [UInt8]()
             {{ return_type|write_fn }}(result, into: &writer)
             out_buf.pointee = RustBuffer(bytes: writer)
-            return 1
+            return UNIFFI_CALLBACK_SUCCESS
         }
         {%- when None %}
         func makeCall() throws -> Int32 {
@@ -47,7 +47,7 @@ fileprivate let {{ foreign_callback }} : ForeignCallback =
                     {%- if !loop.last %}, {% endif %}
                     {% endfor -%}
                 )
-            return 1
+            return UNIFFI_CALLBACK_SUCCESS
         }
         {%- endmatch %}
 
@@ -59,7 +59,7 @@ fileprivate let {{ foreign_callback }} : ForeignCallback =
             return try makeCall()
         } catch let error as {{ error_type|type_name }} {
             out_buf.pointee = {{ error_type|lower_fn }}(error)
-            return -2
+            return UNIFFI_CALLBACK_ERROR
         }
         {%- endmatch %}
     }
@@ -69,9 +69,9 @@ fileprivate let {{ foreign_callback }} : ForeignCallback =
     switch method {
         case IDX_CALLBACK_FREE:
             {{ ffi_converter_name }}.drop(handle: handle)
-            // No return value.
-            // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs`
-            return 0
+            // Sucessful return
+            // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs`
+            return UNIFFI_CALLBACK_SUCCESS
         {% for meth in cbi.methods() -%}
         {% let method_name = format!("invoke_{}", meth.name())|fn_name -%}
         case {{ loop.index }}:
@@ -80,22 +80,22 @@ fileprivate let {{ foreign_callback }} : ForeignCallback =
                 cb = try {{ ffi_converter_name }}.lift(handle)
             } catch {
                 out_buf.pointee = {{ Type::String.borrow()|lower_fn }}("{{ cbi.name() }}: Invalid handle")
-                return -1
+                return UNIFFI_CALLBACK_UNEXPECTED_ERROR
             }
             do {
                 return try {{ method_name }}(cb, argsData, argsLen, out_buf)
             } catch let error {
                 out_buf.pointee = {{ Type::String.borrow()|lower_fn }}(String(describing: error))
-                return -1
+                return UNIFFI_CALLBACK_UNEXPECTED_ERROR
             }
         {% endfor %}
         // This should never happen, because an out of bounds method index won't
         // ever be used. Once we can catch errors, we should return an InternalError.
         // https://github.com/mozilla/uniffi-rs/issues/351
         default:
             // An unexpected error happened.
-            // See docs of ForeignCallback in `uniffi/src/ffi/foreigncallbacks.rs`
-            return -1
+            // See docs of ForeignCallback in `uniffi_core/src/ffi/foreigncallbacks.rs`
+            return UNIFFI_CALLBACK_UNEXPECTED_ERROR
     }
 }