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

Implement record type #243

Merged
merged 1 commit into from
Aug 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions examples/rondpoint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use std::collections::HashMap;

#[derive(Debug, Clone)]
pub struct Dictionnaire {
un: Enumeration,
Expand Down Expand Up @@ -36,6 +38,10 @@ fn copie_enumerations(e: Vec<Enumeration>) -> Vec<Enumeration> {
e
}

fn copie_carte(e: HashMap<String, Enumeration>) -> HashMap<String, Enumeration> {
e
}

fn copie_dictionnaire(d: Dictionnaire) -> Dictionnaire {
d
}
Expand Down
1 change: 1 addition & 0 deletions examples/rondpoint/src/rondpoint.idl
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ namespace rondpoint {
Dictionnaire copie_dictionnaire(Dictionnaire d);
Enumeration copie_enumeration(Enumeration e);
sequence<Enumeration> copie_enumerations(sequence<Enumeration> e);
record<DOMString, Enumeration> copie_carte(record<DOMString, Enumeration> c);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • IDL record type is limited: the key has to be a string type, however the value can be anything.
  • weedle will not parse string, so you have to declare a DOMString instead (it still maps properly to a rust String, we actually ignore the Key type).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is an okay start, and we can figure out other key types as we go. Kinda surprised weedle wouldn't parse string, but oh well.

boolean switcheroo(boolean b);
};

Expand Down
1 change: 1 addition & 0 deletions examples/rondpoint/tests/bindings/test_rondpoint.kts
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ assert(dico == copyDico)

assert(copieEnumeration(Enumeration.DEUX) == Enumeration.DEUX)
assert(copieEnumerations(listOf(Enumeration.UN, Enumeration.DEUX)) == listOf(Enumeration.UN, Enumeration.DEUX))
assert(copieCarte(mapOf("1" to Enumeration.UN, "2" to Enumeration.DEUX)) == mapOf("1" to Enumeration.UN, "2" to Enumeration.DEUX))

assert(switcheroo(false))
1 change: 1 addition & 0 deletions examples/rondpoint/tests/bindings/test_rondpoint.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ assert(dico == copyDico)

assert(copieEnumeration(e: .deux) == .deux)
assert(copieEnumerations(e: [.un, .deux]) == [.un, .deux])
assert(copieCarte(c: ["1": .un, "2": .deux]) == ["1": .un, "2": .deux])

assert(switcheroo(b: false))
90 changes: 65 additions & 25 deletions uniffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ use anyhow::{bail, Result};
use bytes::buf::{Buf, BufMut};
use ffi_support::ByteBuffer;
use paste::paste;
use std::convert::TryFrom;
use std::ffi::CString;
use std::{collections::HashMap, convert::TryFrom, ffi::CString};

// It would be nice if this module was behind a cfg(test) guard, but it
// doesn't work between crates so let's hope LLVM tree-shaking works well.
Expand Down Expand Up @@ -75,27 +74,27 @@ pub unsafe trait ViaFfi: Sized {
/// serializing the data for transfer. In theory it could be possible to build a matching
/// `#[repr(C)]` struct for a complex data type and pass that instead, but explicit
/// serialization is simpler and safer as a starting point.
type Value;
type FfiType;

/// Lower a rust value of the target type, into an FFI value of type Self::Value.
/// Lower a rust value of the target type, into an FFI value of type Self::FfiType.
///
/// This trait method is used for sending data from rust to the foreign language code,
/// by (hopefully cheaply!) converting it into someting that can be passed over the FFI
/// and reconstructed on the other side.
///
/// Note that this method takes an owned `self`; this allows it to transfer ownership
/// in turn to the foreign language code, e.g. by boxing the value and passing a pointer.
fn lower(self) -> Self::Value;
fn lower(self) -> Self::FfiType;

/// Lift a rust value of the target type, from an FFI value of type Self::Value.
/// Lift a rust value of the target type, from an FFI value of type Self::FfiType.
///
/// This trait method is used for receiving data from the foreign language code in rust,
/// by (hopefully cheaply!) converting it from a low-level FFI value of type Self::Value
/// by (hopefully cheaply!) converting it from a low-level FFI value of type Self::FfiType
/// into a high-level rust value of the target type.
///
/// Since we cannot statically guarantee that the foreign-language code will send valid
/// values of type Self::Value, this method is fallible.
fn try_lift(v: Self::Value) -> Result<Self>;
/// values of type Self::FfiType, this method is fallible.
fn try_lift(v: Self::FfiType) -> Result<Self>;

/// Write a rust value into a bytebuffer, to send over the FFI in serialized form.
///
Expand Down Expand Up @@ -163,13 +162,13 @@ macro_rules! impl_via_ffi_for_num_primitive {
$(
paste! {
unsafe impl ViaFfi for $T {
type Value = Self;
type FfiType = Self;

fn lower(self) -> Self::Value {
fn lower(self) -> Self::FfiType {
self
}

fn try_lift(v: Self::Value) -> Result<Self> {
fn try_lift(v: Self::FfiType) -> Result<Self> {
Ok(v)
}

Expand All @@ -196,17 +195,17 @@ impl_via_ffi_for_num_primitive! {
/// Booleans are passed as a `u8` in order to avoid problems with handling
/// C-compatible boolean values on JVM-based languages.
unsafe impl ViaFfi for bool {
type Value = u8;
type FfiType = u8;

fn lower(self) -> Self::Value {
fn lower(self) -> Self::FfiType {
if self {
1
} else {
0
}
}

fn try_lift(v: Self::Value) -> Result<Self> {
fn try_lift(v: Self::FfiType) -> Result<Self> {
Ok(match v {
0 => false,
1 => true,
Expand Down Expand Up @@ -234,13 +233,13 @@ unsafe impl ViaFfi for bool {
/// `None` option is represented as a null pointer and the `Some` as a valid pointer,
/// but that seems more fiddly and less safe in the short term, so it can wait.
unsafe impl<T: ViaFfi> ViaFfi for Option<T> {
type Value = ffi_support::ByteBuffer;
type FfiType = ffi_support::ByteBuffer;

fn lower(self) -> Self::Value {
fn lower(self) -> Self::FfiType {
lower_into_bytebuffer(self)
}

fn try_lift(v: Self::Value) -> Result<Self> {
fn try_lift(v: Self::FfiType) -> Result<Self> {
try_lift_from_bytebuffer(v)
}

Expand Down Expand Up @@ -273,20 +272,20 @@ unsafe impl<T: ViaFfi> ViaFfi for Option<T> {
/// pair but that seems tremendously fiddly and unsafe in the short term.
/// Maybe one day...
unsafe impl<T: ViaFfi> ViaFfi for Vec<T> {
type Value = ffi_support::ByteBuffer;
type FfiType = ffi_support::ByteBuffer;

fn lower(self) -> Self::Value {
fn lower(self) -> Self::FfiType {
lower_into_bytebuffer(self)
}

fn try_lift(v: Self::Value) -> Result<Self> {
fn try_lift(v: Self::FfiType) -> Result<Self> {
try_lift_from_bytebuffer(v)
}

fn write<B: BufMut>(&self, buf: &mut B) {
// TODO: would be nice not to panic here :-/
let len = u32::try_from(self.len()).unwrap();
buf.put_u32(len); // We limit arrays to u32::MAX bytes
buf.put_u32(len); // We limit arrays to u32::MAX items
for item in self.iter() {
ViaFfi::write(item, buf);
}
Expand All @@ -303,6 +302,47 @@ unsafe impl<T: ViaFfi> ViaFfi for Vec<T> {
}
}

/// Support for associative arrays via the FFI.
/// Note that because of webidl limitations,
/// the key must always be of the String type.
///
/// HashMaps are currently always passed by serializing to a bytebuffer.
/// We write a `u32` entries count
/// followed by each entry (string key followed by the value) in turn.
unsafe impl<V: ViaFfi> ViaFfi for HashMap<String, V> {
type FfiType = ffi_support::ByteBuffer;

fn lower(self) -> Self::FfiType {
lower_into_bytebuffer(self)
}

fn try_lift(v: Self::FfiType) -> Result<Self> {
try_lift_from_bytebuffer(v)
}

fn write<B: BufMut>(&self, buf: &mut B) {
// TODO: would be nice not to panic here :-/
let len = u32::try_from(self.len()).unwrap();
buf.put_u32(len); // We limit HashMaps to u32::MAX entries
for (key, value) in self.iter() {
ViaFfi::write(key, buf);
ViaFfi::write(value, buf);
}
}

fn try_read<B: Buf>(buf: &mut B) -> Result<Self> {
check_remaining(buf, 4)?;
let len = buf.get_u32();
let mut map = HashMap::with_capacity(len as usize);
for _ in 0..len {
let key = String::try_read(buf)?;
let value = <V as ViaFfi>::try_read(buf)?;
map.insert(key, value);
}
Ok(map)
}
}

/// Support for passing Strings via the FFI.
///
/// Unlike many other implementations of `ViaFfi`, this passes a pointer rather
Expand All @@ -317,19 +357,19 @@ unsafe impl<T: ViaFfi> ViaFfi for Vec<T> {
/// (In practice, we currently do end up copying the data, the copying just happens
/// on the foreign language side rather than here in the rust code.)
unsafe impl ViaFfi for String {
type Value = *mut std::os::raw::c_char;
type FfiType = *mut std::os::raw::c_char;

// This returns a raw pointer to the underlying bytes, so it's very important
// that it consume ownership of the String, which is relinquished to the foreign
// language code (and can be restored by it passing the pointer back).
fn lower(self) -> Self::Value {
fn lower(self) -> Self::FfiType {
ffi_support::rust_string_to_c(self)
}

// The argument here *must* be a uniquely-owned pointer previously obtained
// from `info_ffi_value` above. It will try to panic if you give it an invalid
// pointer, but there's no guarantee that it will.
fn try_lift(v: Self::Value) -> Result<Self> {
fn try_lift(v: Self::FfiType) -> Result<Self> {
if v.is_null() {
bail!("null pointer passed as String")
}
Expand Down
34 changes: 34 additions & 0 deletions uniffi_bindgen/src/bindings/kotlin/gen_kotlin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ mod filters {
}
Type::Optional(t) => format!("{}?", type_kt(t)?),
Type::Sequence(t) => format!("List<{}>", type_kt(t)?),
Type::Map(t) => format!("Map<String, {}>", type_kt(t)?),
})
}

Expand Down Expand Up @@ -124,6 +125,14 @@ mod filters {
calculate_write_size(&"v", t)?,
write_kt(&"v", &"buf", t)?
),
Type::Map(t) => format!(
"lowerMap({}, {{ k, v -> {} + {} }}, {{ k, v, buf -> {}; {} }})",
nm,
calculate_write_size(&"k", &Type::String)?,
calculate_write_size(&"v", t)?,
write_kt(&"k", &"buf", &Type::String)?,
write_kt(&"v", &"buf", t)?
),
Comment on lines +128 to +135
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤮

Copy link
Collaborator

Choose a reason for hiding this comment

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

lol, but also, I wonder if something similar to what I tried for python over in #214 could be slightly less vomit-inducing here; thoughts? (for a future PR of course, not this one)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I'm concerned about is that we would generate a lot of extra code for each new record/sequence/etc?

Copy link
Collaborator

@rfk rfk Aug 17, 2020

Choose a reason for hiding this comment

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

Yeah, I guess it's similar to the use of monomorphization in rust, where you end up generating lots of almost-duplicate code. (I don't think it would balloon too badly in practice, because you'd only get one method per type-you-actually-use-in-a-sequence, not one for every single type. But, worth watching out for).

_ => format!("{}.lower()", nm),
})
}
Expand Down Expand Up @@ -151,6 +160,13 @@ mod filters {
target,
write_kt(&"v", &"buf", t)?
),
Type::Map(t) => format!(
"writeMap({}, {}, {{ k, v, buf -> {}; {} }})",
nm,
target,
write_kt(&"k", &"buf", &Type::String)?,
write_kt(&"v", &"buf", t)?
),
_ => format!("{}.write({})", nm, target),
})
}
Expand All @@ -175,6 +191,12 @@ mod filters {
nm,
calculate_write_size(&"v", t)?
),
Type::Map(t) => format!(
"calculateWriteSizeMap({}, {{ k, v -> {} + {} }})",
nm,
calculate_write_size(&"k", &Type::String)?,
calculate_write_size(&"v", t)?
),
_ => format!("{}.calculateWriteSize()", nm),
})
}
Expand All @@ -192,6 +214,12 @@ mod filters {
Type::Sequence(t) => {
format!("liftSequence({}, {{ buf -> {} }})", nm, read_kt(&"buf", t)?)
}
Type::Map(t) => format!(
"liftMap({}, {{ buf -> Pair({}, {}) }})",
nm,
read_kt(&"buf", &Type::String)?,
read_kt(&"buf", t)?
),
_ => format!("{}.lift({})", type_kt(type_)?, nm),
})
}
Expand All @@ -209,6 +237,12 @@ mod filters {
Type::Sequence(t) => {
format!("readSequence({}, {{ buf -> {} }})", nm, read_kt(&"buf", t)?)
}
Type::Map(t) => format!(
"readMap({}, {{ buf -> Pair({}, {}) }})",
nm,
read_kt(&"buf", &Type::String)?,
read_kt(&"buf", t)?
),
_ => format!("{}.read({})", type_kt(type_)?, nm),
})
}
Expand Down
1 change: 1 addition & 0 deletions uniffi_bindgen/src/bindings/kotlin/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ pub fn compile_bindings(ci: &ComponentInterface, out_dir: &Path) -> Result<()> {
let mut jar_file = PathBuf::from(out_dir);
jar_file.push(format!("{}.jar", ci.namespace()));
let status = Command::new("kotlinc")
.arg("-Xopt-in=kotlin.RequiresOptIn")
.arg("-classpath")
.arg(env::var("CLASSPATH").unwrap_or_else(|_| "".to_string()))
.arg(&kt_file)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,21 @@ fun<T> readSequence(buf: ByteBuffer, readItem: (ByteBuffer) -> T): List<T> {
}
}

fun<V> liftMap(rbuf: RustBuffer.ByValue, readItem: (ByteBuffer) -> Pair<String, V>): Map<String, V> {
return liftFromRustBuffer(rbuf) { buf -> readMap(buf, readItem) }
}

fun<V> readMap(buf: ByteBuffer, readItem: (ByteBuffer) -> Pair<String, V>): Map<String, V> {
val len = Int.read(buf)
@OptIn(ExperimentalStdlibApi::class)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What effect will this opting-in to experiment stuff have on our consumers, if any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should result in a warning during compilation, if we find this unacceptable we can always switch to a more stable construct.

return buildMap<String, V>(len) {
repeat(len) {
val (k, v) = readItem(buf)
put(k, v)
}
}
}

// Helpers for lowering primitive data types into a bytebuffer.
// Since we need to allocate buffers from rust, the lowering process needs to be
// able to calculate ahead-of-time what the required size of the buffer will be.
Expand Down Expand Up @@ -244,6 +259,21 @@ fun<T> writeSequence(v: List<T>, buf: ByteBuffer, writeItem: (T, ByteBuffer) ->
v.forEach { writeItem(it, buf) }
}

fun<V> lowerMap(m: Map<String, V>, calculateWriteSize: (String, V) -> Int, writeEntry: (String, V, ByteBuffer) -> Unit): RustBuffer.ByValue {
return lowerIntoRustBuffer(m, { m -> calculateWriteSizeMap(m, calculateWriteSize) }, { m, buf -> writeMap(m, buf, writeEntry) })
}

fun<V> calculateWriteSizeMap(v: Map<String, V>, calculateWriteSize: (String, V) -> Int): Int {
var len = v.size.calculateWriteSize()
v.forEach { k, v -> len += calculateWriteSize(k, v) }
return len
}

fun<V> writeMap(v: Map<String, V>, buf: ByteBuffer, writeEntry: (String, V, ByteBuffer) -> Unit) {
v.size.write(buf)
v.forEach { k, v -> writeEntry(k, v, buf) }
}

fun<T> lowerOptional(v: T?, calculateWriteSize: (T) -> Int, writeItem: (T, ByteBuffer) -> Unit): RustBuffer.ByValue {
return lowerIntoRustBuffer(v, { v -> calculateWriteSizeOptional(v, calculateWriteSize) }, { v, buf -> writeOptional(v, buf, writeItem) })
}
Expand Down
8 changes: 7 additions & 1 deletion uniffi_bindgen/src/bindings/python/gen_python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,13 @@ mod filters {
Type::Enum(type_name) => format!("{} = {}({})", nm, type_name, nm),
Type::Record(type_name) => format!("{} = {}._coerce({})", nm, type_name, nm),
Type::Optional(t) => format!("(None if {} is None else {})", nm, coerce_py(nm, t)?),
Type::Sequence(t) => format!("({} for x in {})", coerce_py(&"x", t)?, nm), // TODO: name hygiene
Type::Sequence(t) => format!("({} for x in {})", coerce_py(&"x", t)?, nm), // TODO: name hygiene,
Type::Map(t) => format!(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't finish the python implementation, I think the other PR needs to land first.

"({}:{} for (k, v) in {}.items())",
coerce_py(&"k", t)?,
coerce_py(&"v", t)?,
nm
),
})
}

Expand Down
1 change: 1 addition & 0 deletions uniffi_bindgen/src/bindings/swift/gen_swift.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ mod filters {
}
Type::Optional(type_) => format!("{}?", type_swift(type_)?),
Type::Sequence(type_) => format!("[{}]", type_swift(type_)?),
Type::Map(type_) => format!("[String:{}]", type_swift(type_)?),
})
}

Expand Down
Loading