Implement record type #243

eoger · 2020-08-13T19:40:18Z

Fixes #226.

There's some awkwardness in here, but it's not a shower-stopper imo.

eoger · 2020-08-13T19:42:16Z

examples/rondpoint/src/rondpoint.idl

@@ -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);


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).

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.

eoger · 2020-08-13T19:42:46Z

uniffi_bindgen/src/bindings/kotlin/gen_kotlin.rs

+            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)?
+            ),


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)

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

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).

eoger · 2020-08-13T19:43:22Z

uniffi_bindgen/src/bindings/python/gen_python.rs

@@ -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!(


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

eoger · 2020-08-13T19:44:18Z

uniffi_bindgen/src/bindings/swift/templates/RustBufferHelper.swift

-    associatedtype Value
-    static func lift(_ v: Value) throws -> Self
-    func lower() -> Value
+    associatedtype FfiType


It's unfortunate, but in the extension Dictionary below the Swift compiler couldn't disambiguate between ViaFfi.Value and Dictionary.Value in the where declaration

FWIW I would support renaming this to FfiType or LoweredType or similar on the rust side as well, I think it more clearly communicates the intent.

tarikeshaq

Looks good to me, I do think we'll probably have to eventually add different key types (at least number types), but we can figure that out later if this helps us get our demos unblocked (👀 on the fxa_client)

tarikeshaq · 2020-08-13T20:08:08Z

examples/rondpoint/src/rondpoint.idl

@@ -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);


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.

tarikeshaq · 2020-08-13T20:19:38Z

uniffi/src/lib.rs

+    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 bytes


Suggested change

buf.put_u32(len); // We limit HashMaps to u32::MAX bytes

buf.put_u32(len); // We limit HashMaps to u32::MAX items

rfk · 2020-08-13T22:40:33Z

uniffi/src/lib.rs

+/// 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 in turn.


nit: please document that an "entry" is the string key followed by the value.

rfk · 2020-08-13T22:44:27Z

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

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


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

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

rfk · 2020-08-13T22:45:11Z

uniffi_bindgen/src/bindings/swift/templates/RustBufferHelper.swift

-    associatedtype Value
-    static func lift(_ v: Value) throws -> Self
-    func lower() -> Value
+    associatedtype FfiType


FWIW I would support renaming this to FfiType or LoweredType or similar on the rust side as well, I think it more clearly communicates the intent.

rfk · 2020-08-14T01:16:45Z

Base automatically changed from clean-up-trait-method-names to main now

Oh, that is very slick, thankyou github!

eoger commented Aug 13, 2020

View reviewed changes

eoger requested a review from a team August 13, 2020 19:45

tarikeshaq approved these changes Aug 13, 2020

View reviewed changes

rfk approved these changes Aug 14, 2020

View reviewed changes

rfk force-pushed the clean-up-trait-method-names branch from 7a45fb2 to c21480d Compare August 14, 2020 00:14

rfk mentioned this pull request Aug 14, 2020

Clarify terminology around passing values across the FFI. #241

Merged

rfk force-pushed the clean-up-trait-method-names branch from 525b8a8 to 759ce67 Compare August 14, 2020 01:11

Base automatically changed from clean-up-trait-method-names to main August 14, 2020 01:16

eoger force-pushed the record-type branch from e8f0f75 to a5fc09c Compare August 14, 2020 17:21

Implement record type

a9594da

eoger force-pushed the record-type branch from a5fc09c to a9594da Compare August 14, 2020 17:25

eoger merged commit e1cf4a7 into main Aug 14, 2020

eoger deleted the record-type branch August 14, 2020 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement record type #243

Implement record type #243

eoger commented Aug 13, 2020

eoger Aug 13, 2020

tarikeshaq Aug 13, 2020

eoger Aug 13, 2020

rfk Aug 14, 2020

eoger Aug 14, 2020

rfk Aug 17, 2020 •

edited

Loading

eoger Aug 13, 2020

eoger Aug 13, 2020

rfk Aug 13, 2020

tarikeshaq left a comment

tarikeshaq Aug 13, 2020

tarikeshaq Aug 13, 2020

rfk Aug 13, 2020

rfk Aug 13, 2020

eoger Aug 14, 2020

rfk Aug 13, 2020

rfk commented Aug 14, 2020

	buf.put_u32(len); // We limit HashMaps to u32::MAX bytes
	buf.put_u32(len); // We limit HashMaps to u32::MAX items

Implement record type #243

Implement record type #243

Conversation

eoger commented Aug 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarikeshaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Aug 14, 2020

rfk Aug 17, 2020 •

edited

Loading