Add support for enums with associated data (aka "tagged unions", aka "sum types", aka ...) #381

rfk · 2021-02-08T03:52:57Z

Our fxa-client component makes use of a Rust enum with associated data to represent the different kinds of event that can happen to your account. The Rust code looks like this:

pub enum AccountEvent {
    IncomingDeviceCommand(Box<IncomingDeviceCommand>),
    ProfileUpdated,
    DeviceConnected {
        device_name: String,
    },
    [...other cases omitted...]
}

In the hand-written bindings, we reflect this to Kotlin as a sealed class and reflect it to Swift as an enum with associated data.

UniFFI doesn't yet support this kind of data structure, which makes it awkward to use for the fxa-client component. So let's see if we can add it!

WebIDL doesn't seem to have a notion of a datatype like this or offer any ready syntax for it (It has union types, but they're like C-style unions and do not have an explicit discriminant). I messed around a bit and came up with a syntax that doesn't seem too bad, and is similar to how we're implemented errors. This PR contains just the docs and test changes that would happen if we chose to implement tagged unions in this manner. I'm looking for early feedback on whether this seems like a good idea.

/cc @mhammond @lougeniaC64 @jhugman @dmose

rfk · 2021-02-08T03:58:23Z

docs/manual/src/udl/enumerations.md

+  V4(u8 q1, u8 q2, u8 q3, u8 q4);
+  V6(string addr);
+};
+```


This is the proposed UDL syntax. I think it looks nice, but it's a pretty serious abuse of WebIDL syntax. It parses, but e.g. the V4 line actually parses as an anonymous special method returning a datatype named V4 rather than as a field or method named V4. I personally have no problem with us re-interpreting that as a tagged union, but it seems gross so YMMV. I haven't been able to come up with anything better though :-/

I had hoped to be able to do something clever with the existing WebIDL enum declaration, along the lines of:

[TaggedUnion] enum IpAddr { [Fields(u8)] "V4", [Fields(u8, string)] "V6", }

But I couldn't come up with something that would parse. In particular, attributes don't seem to be supported on the variants of an enum.

I love the syntax you've arrived at.

This and #379 reminds me we never quite resolved the question of writing our own parser. Soon is coming the time to reconsider ADR-0001!

Soon is coming the time to reconsider ADR-0001!

Yes! I have many half-baked thoughts on this. What do you think is a good way to structure that kind of discussion, should we e.g. open an open-ended "discuss" issue where we can kick some ideas around?

+1 to what James said, although I'm slightly more skeptical we will find time to rewrite the parser until that's our final option :)

I agree with Mark here: I expect we'll only seriously consider doing the work once we've exhausted.

That'd be the wrong time for a discussion, so yes, let's start a doc/issue and iterate.

That'd be the wrong time for a discussion, so yes, let's start a doc/issue and iterate.

Sounds good 👍

(FWIW, I was pretty close to declaring "WebIDL has no good syntax for this" before I discovered the anonymous-special-method-returning-a-named-datatype hackery you see here, so I won't be surprised if we run out of runway with WebIDL sooner than we expect)

I filed a "discuss" issue here: #385

rfk · 2021-02-08T03:59:01Z

examples/rondpoint/tests/bindings/test_rondpoint.kts

+    "0" to EnumerationAvecDonnees.ZERO(),
+    "1" to EnumerationAvecDonnees.UN(1),
+    "2" to EnumerationAvecDonnees.DEUX(2, "deux")
+))


(I haven't actually run any of these examples, just trying my best to sketch out what the resulting code would actually look like to use).

In Kotlin, since they're inner classes, they'd be spelled with CamelCase.

They'd be defined as:

sealed class EnumerationAvecDonnees { object Zero: EnumerationAvecDonnees() // object for noargs data class Un(val value: Int): EnumerationAvecDonnees() data class Deux(val value: Int, val nombre: String): EnumerationAvecDonnees() } assert(EnumerationAvecDonnees.Zero != EnumerationAvecDonnees.Deux(2, "two"))

(in retrospect, this comment feels like "Well Actually". Sorry)

(in retrospect, this comment feels like "Well Actually". Sorry)

Heh, no worries at all, exactly the sort of early feedback I'm after 👍

rfk · 2021-02-08T04:01:49Z

I haven't even begun to think about what this would look like for the Gecko-JS bindings.

rfk · 2021-02-08T04:04:29Z

docs/manual/src/udl/enumerations.md

+Can be exposed in the UDL file with:
+
+```idl
+[TaggedUnion]


Please feel welcome to propose your favourite colour for this bikeshed. I chose "tagged union" because that's the canonical name of the Wikipedia page about this concept.

heh - see above - I mildly prefer "tagged enum" given we are in rust.

mhammond

This is awesome. I skipped over the parser code, but will try and get back to it over the next few days, but thought I'd share my hearty +1 in the meantime.

mhammond · 2021-02-09T07:41:24Z

README.md

@@ -88,7 +88,7 @@ Things that are implemented so far:

 * Primitive numeric types, equivalents to those offered by Rust (`u32`, `f64`, etc).
 * Strings (which are always UTF-8, like Rust's `String`).
-* C-style enums (just the discriminant, no associated data).
+* Enums, including enums associated data (aka "Tagged Unions").


nit: add "with"

mhammond · 2021-02-09T07:44:13Z

docs/manual/src/internals/lifting_and_lowering.md

@@ -85,6 +86,7 @@ The details of this format are internal only and may change between versions of
 | `sequence<T>` | Serialized `i32` item count followed by serialized items; each item is a serialized `T` |
 | `record<string, T>` | Serialized `i32` item count followed by serialized items; each item is a serialized `string` followed by a serialized `T` |
 | `enum` | Serialized `u32` indicating variant, numbered in declaration order starting from 1 |
+| `[TaggedUnion] interface` | Serialized `u32` indicating variant as per `enum`, followed by the serialized value of each field, in declaration order |


I don't think "union" and "enum" are simple synonyms in rust - is TaggedEnum a better name?

Yeah, that's a good point re: the likelihood the name here will be interpreted somewhere close to Rust's semantics.

I spent a good deal of time thinking about this yesterday, and still couldn't remember TaggedUnion. Having said that: what a lovely bikeshed!

I'm going to try just [Enum] and see how I like it...

mhammond · 2021-02-09T07:45:18Z

docs/manual/src/udl/enumerations.md

+Can be exposed in the UDL file with:
+
+```idl
+[TaggedUnion]


heh - see above - I mildly prefer "tagged enum" given we are in rust.

mhammond · 2021-02-09T07:46:58Z

docs/manual/src/udl/enumerations.md

+  V4(u8 q1, u8 q2, u8 q3, u8 q4);
+  V6(string addr);
+};
+```


+1 to what James said, although I'm slightly more skeptical we will find time to rewrite the parser until that's our final option :)

mhammond · 2021-02-09T07:47:29Z

docs/manual/src/udl/enumerations.md

+};
+```
+
+Only enums with named fields are supported by this syntax.


(to reinforce the naming thing - the docs keep calling them "enums" :)

mhammond · 2021-02-09T07:47:53Z

examples/rondpoint/src/rondpoint.udl

@@ -2,7 +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);
+  record<DOMString, EnumerationAvecDonnees> copie_carte(record<DOMString, EnumerationAvecDonnees> c);


A big +1 for the french :)

(On a completely less serious note, I wonder if the French in rondpoint is a bigger risk than the parser in the shorter term ;)

On the other hand, trying to switch this away from French would be like going through the Sync codebase and removing all the puns...I don't think I'm ready for that just yet 😢 😅

How about writing newly added code in English, and changing bits of codes that we touch incrementally, as it makes sense?

+1 for @dmose 's new code in English. I'd add the innovation of breaking up rondpoint into multiple examples: e.g. primitives, collections, enums, callbacks, etc.

rfk · 2021-02-09T09:58:11Z

A quick update here - there are plenty of rough bits remaining, but the modified Kotlin test seems to be running and working as intended!

rfk · 2021-02-09T10:06:12Z

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

+// Kotlin's `enum class` constuct doesn't support variants with associated data,
+// but is a little nicer for consumers than its `sealed class` enum pattern.
+// So, we switch here, using `enum class` for enums with no associated data
+// and `sealed class` for the general case.


@jhugman here's a design question I'd appreciate your insight on. Kotlin's enum class declaration doesn't support associated data in the way we want to use it, so in the general case we have to use the sealed class approach. However, when there isn't any associated data, Kotlin's enum seems to be a little nicer to work with. Here I've opted to switch between them - enums without data will be an enum class just as they were under the previous implementation, while newly-supported enums with data will be a sealed class.

This has the nice property of being backwards-compatible for existing consumers of existing UDL files. But I'm a little concerned about potential developer confusion in future.

For example, suppose someone currently has a plain-old-enum with no data in their UDL file, and their Kotlin consumers are consuming it through an enum class. The component API evolves and adds a new case to the enum, this time with some associated data. For Kotlin consumers this addition will change the entire type of the thing they're dealing with, including changing the capitalization of existing items as it switches from being an enum class to a sealed class.

Maybe this is OK, since consumers will need to update their code to handle the new variant anyway. Or maybe it's leaking implementation details in a way that we shouldn't need to do.

Thoughts?

It's a great question.

From our (nimbus + uniffi) point of view, I don't think it matters: we have a tiny amount of code using UDL enums, so changing them will have some coordination cost when changing them to a new format.

For android devs: adding another enum variant is going to be a breaking change.

There is some JVM cost to using sealed classes over Java style (around class initialization, maximum method counts etc). I don't think we're at the stage we need to worry about that.

While thinking about this, it did strike me that Kotlin has had to make a similar choice: Java enums existed after Java5. Kotlin came up with a better way, yet still supported old school Java enums. I should imagine their backwards compatibility problem was more persuasive/pressing than ours.

Having thought about it, I have a stong-preference-weakly-held for keeping the implementing UDL's enum as enum class in Kotlin.

Other things this question made me think about:

Typescript can do Tagged Unions, and whatever Javascript can support natively.

Javascript has barely has the simple enums.

Until we have a idiomatically consumable version of tagged unions in JS, we're going to be discouraging use of this feature.

Until we have a idiomatically consumable version of tagged unions in JS, we're going to be discouraging use of this feature.

Yeah. Python has a similar problem, which is one of the reasons I like having the python backend here, to think about how things will look in a language with basically the opposite of a Rust-style strong algebraic type system.

dmose · 2021-02-10T17:47:14Z

As you mentioned above, the GeckoJS stuff is going to be the higher risk. The thing that jumps out at me is wondering whether the stuff you've done for UDL is even going to be expressable in a way that the Firefox WebIDL parser will understand...

rfk · 2021-02-15T05:33:03Z

I've got this working in a way that I'm pretty happy with for the Kotlin, Swift and Python backends. I haven't tackled the gecko_js backend yet, and similar to my comments here, I think we should have a little sync-up about how we'll approach that backend strategically in the short-term. I'm happy to do the work! I just don't want to get stuck down a rabbit-hole if it's not strategically important right now.

(Unlike #384, I don't really have a good idea for how to expose these through C++ and into special Gecko JS. We could try to do something similar to what the python backend is currently doing here, basically creating a separate class for each variant and packaging them up into a convenient API...but I can easily imagine that not playing nicely with all the C++ codegen and what-not).

rfk · 2021-02-16T03:18:01Z

A note to self from trying this out in the fxa-client component: we'll need to be careful with name conflicts at different scopes.

The fxa-client declared a set of enums like this:

[Enum]
interface AccountEvent {
  IncomingDeviceCommand(IncomingDeviceCommand command );
  ProfileUpdated();
  AccountAuthStateChanged();
  AccountDestroyed();
  DeviceConnected(string device_name );
  DeviceDisconnected(string device_id, boolean is_local_device );
};

[Enum]
interface IncomingDeviceCommand {
  TabReceived(Device? sender, SendTabPayload payload );
};

Note that the name IncomingDeviceCommand is both a top-level enum, and one of the variants on the AccountEvent enum. When I generated the Kotlin code for this, we wound up with:

sealed class IncomingDeviceCommand {
    class TabReceived(
        val sender: Device?,
        val payload: SendTabPayload
    ) : IncomingDeviceCommand() { }

    companion object {
        internal fun read(buf: ByteBuffer): IncomingDeviceCommand {
            // details omitted
        }
    }
}

sealed class AccountEvent {
    class IncomingDeviceCommand(
        val command: IncomingDeviceCommand
    ) : AccountEvent() { ... }

    // Other cases omitted

    companion object {
        internal fun read(buf: ByteBuffer): AccountEvent {
            return when (buf.getInt()) {
                1 -> AccountEvent.IncomingDeviceCommand(
                    //
                    //  +---- this line gives a compile error
                    //  |
                    //  v
                    IncomingDeviceCommand.read(buf)
                )
                // other cases omitted
                else -> throw RuntimeException("invalid enum value, something is very wrong!!")
            }
        }
    }
}

This doesn't compile, because Kotlin thinks that IncomingDeviceCommand.read(buf) is referring to the inner IncomingDeviceCommand class defined on the AccountEvent enum, rather than the top-level IncomingDeviceCommand class.

I've worked around this for now by avoiding the name conflict, but we should figure out a better approach before landing this PR. Maybe there's a way to do the codegen so that the names are not ambiguous, or maybe we need to just check for duplicate names and error in that case.

At the very least, we need a testcase for this.

rfk · 2021-02-18T01:41:12Z

Alrighty, I've rebased this atop latest main, and added an early bail out if we see an enum variant name that conflicts with a top-level type name.

rfk · 2021-02-18T02:24:10Z

uniffi_bindgen/src/bindings/gecko_js/templates/SharedHeaderTemplate.h

-struct ViaFfi<{{ e.name()|class_name_cpp(context) }}, uint32_t> {
-  [[nodiscard]] static bool Lift(const uint32_t& aLowered, {{ e.name()|class_name_cpp(context) }}& aLifted) {
-    switch (aLowered) {
+struct Serializable<{{ e.name()|class_name_cpp(context) }}> {


This PR changes the way enums are passed over the FFI, even enums without any associated data. I've done my best to update this and will see if I can try it out in mozilla-central following the instructions in this doc but I don't have high confidence in my skills in this area.

I was able to integrate this into m-c and have it compile, then fail on a linker error. So I guess that's a good sign.

mhammond

This looks great with the usual attention to detail we've come to expect from your PRs :) r+ from me, but I'll let James give formal approval once he has cast his eye again over the generated swift/kotlin

mhammond · 2021-02-18T05:11:03Z

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

+
+# Now, a little trick - we make each nested variant class by a subclass of the main
+# enum subclass, so that method calls and instance checks etc will work intuitively.
+# We might be able to do this a little more nearly with a metaclass, but this'll do.


nit: neatly?

mhammond · 2021-02-18T05:12:26Z

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

@@ -97,7 +97,7 @@ def writeF64(self, v):
    {% when Type::Boolean -%}

    def writeBool(self, v):
-        self._pack_into(1, ">b", 0 if v else 1)
+        self._pack_into(1, ">b", 1 if v else 0)


this is an unrelated bugfix, right?

Oh, yes, yes it is - but one that I only happened to discover in the process of getting all this working.

jhugman

I'm pretty happy with how this is coming along; I have a few questions, but this is looking really close.

jhugman · 2021-02-18T13:45:45Z

examples/rondpoint/src/rondpoint.udl

@@ -2,7 +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);
+  record<DOMString, EnumerationAvecDonnees> copie_carte(record<DOMString, EnumerationAvecDonnees> c);


+1 for @dmose 's new code in English. I'd add the innovation of breaking up rondpoint into multiple examples: e.g. primitives, collections, enums, callbacks, etc.

jhugman · 2021-02-18T13:47:20Z

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

+
+{% else %}
+
+enum class {{ e.name()|class_name_kt }} {


I'm glad you went for this in the end.

jhugman · 2021-02-18T13:50:31Z

uniffi_bindgen/src/bindings/swift/templates/EnumTemplate.swift

+        {% for field in variant.fields() %}
+        {{ field.name()|var_name_swift }}: {{ field.type_()|type_swift }}{%- if loop.last -%}{%- else -%},{%- endif -%}
+        {% endfor %}


This looks very like an already existing macro: swift:arg_list_decl, IIRC.

It's copy-pasted from the logic for declaring record fields. I'm going to extract them into a swift::field_list_decl macro for convenience.

jhugman · 2021-02-18T13:55:59Z

uniffi_bindgen/src/bindings/swift/templates/EnumTemplate.swift

@@ -1,30 +1,40 @@
-public enum {{ e.name()|class_name_swift }}: ViaFfi {
+public enum {{ e.name()|class_name_swift }}: ViaFfiUsingByteBuffer, ViaFfi, Equatable {


I don't think we should address it yet, but at some point (hopefully) someone is going to file a bug about a missing indirect keyword here so we can support recursive enums. What do you think: should we file an issue about it, and put the link in as a comment here?

Sounds good, I filed #396 for future discussions.

Ah, OK.

My thought was that a developer gets an error swiftc pointing to the generated code, goes and looks at the generate code, finds the comment with the link to the issue. WDYT?

This review cycle has been long, so I wouldn't be against leaving it to another issue.

That makes sense. I think in practice they will probably get an error before they even try to generate swift code, because the Rust code will need to use some Box indirection and the generated scaffolding won't support it. But I can add a link to the issue in a few key places in the hope that any such users will find it.

jhugman · 2021-02-18T13:59:53Z

uniffi_bindgen/src/interface/attributes.rs

+        // Can't be both `[Threadsafe]` and an `[Enum]`.
+        if attrs.len() > 1 {
+            bail!("conflicting attributes on interface definition");
+        }


jhugman · 2021-02-18T14:08:09Z

uniffi_bindgen/src/interface/enum_.rs

+    }
+
+    pub fn has_associated_data(&self) -> bool {
+        self.variants.iter().any(Variant::has_fields)


IIUC: I think this should be more about how it was defined in the UDL, rather than the content of the variants, since it may produce some surprising changes in the bindings:

[Enum] interface Foo { Bar }

changing to

[Enum] interface Foo { Bar(string baz) }

will change from Foo.BAR to Foo.Bar(baz: String)

Yeah, I think you're right. I'm not super happy about it because it feels like an implementation detail of the (what we hope to be temporary) IDL leaking all the way through to the generated bindings, but that's probably less bad than having a confusing user experience.

I'm updating this to mark enums that come from enum foo as "flat" and enums that come from [Enum] interface as "not flat", and will use that in codegen to switch between the two implementations.

jhugman · 2021-02-18T14:13:25Z

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

+
+sealed class {{ e.name()|class_name_kt }} {
+    {% for variant in e.variants() -%}
+    class {{ variant.name()|class_name_kt }}({% if variant.has_fields() %}


Nit: variants without fields should be objects.

Suggested change

class {{ variant.name()|class_name_kt }}({% if variant.has_fields() %}

{% if !variant.has_fields() -%}

object {{ variant.name()|class_name_kt }} : {{ e.name()|class_name_kt }}

{% else -%}

class {{ variant.name()|class_name_kt }}(

FWIW, I mildly dislike the asymmetry this produces between EnumerationAvecDonnees.Zero without parens and EnumerationAvecDonnees.Un(1u) with parens, but am happy to defer to your judgement here.

jhugman · 2021-02-18T14:14:27Z

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

+        {% for field in variant.fields() -%}
+        val {{ field.name()|var_name_kt }}: {{ field.type_()|type_kt}}{% if loop.last %}{% else %}, {% endif %}
+        {% endfor -%}
+    {%- endif %}) : {{ e.name()|class_name_kt }}() {


In the same vein:

Suggested change

{%- endif %}) : {{ e.name()|class_name_kt }}() {

: {{ e.name()|class_name_kt }}() { {%- endif %})

jhugman · 2021-02-18T14:56:17Z

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

+            if (other is {{ e.name()|class_name_kt }}.{{ variant.name()|class_name_kt }}) {
+                {% if variant.has_fields() -%}
+                {% for field in variant.fields() -%}
+                {{ field.name()|var_name_kt }} == other.{{ field.name()|var_name_kt }}{% if loop.last %}{% else %} && {% endif -%}
+                {% endfor -%}
+                {% else -%}
+                true
+                {%- endif %}
+            } else {
+                false
+            }


I see what's happening here, there are two ways I can suggest to improve it:

Suggested change

if (other is {{ e.name()|class_name_kt }}.{{ variant.name()|class_name_kt }}) {

{% if variant.has_fields() -%}

{% for field in variant.fields() -%}

{{ field.name()|var_name_kt }} == other.{{ field.name()|var_name_kt }}{% if loop.last %}{% else %} && {% endif -%}

{% endfor -%}

{% else -%}

true

{%- endif %}

} else {

false

}

(other is {{ e.name()|class_name_kt }}.{{ variant.name()|class_name_kt }})

{% for field in variant.fields() -%}

&& {{ field.name()|var_name_kt }} == other.{{ field.name()|var_name_kt }}

{% endfor -%}

The second is the more idiomatic way of doing things in Kotlin: by doing it in a single equals method and the outer Enum level.

sealed class Enum { object Zero : Enum() class One(val v: Int) : Enum() class Two(val a: Int, val b: Int) : Enum() fun equals(other: Any) = when { this is Zero && other is Zero -> true this is One && other is One -> this.v == other.v this is Two && other is Two -> this.a == other.a && this.b == other.b else -> false } }

I was trying to do something with destructuring of Pair, so we could when (this to other) but gave up.

The galaxy brain way is: to use data classes, which generates the equals, hashCode, toString() and copy for us.

sealed class Enum { object Zero : Enum() data class One(val v: Int) : Enum() data class Two(val a: Int, val b: Int) : Enum() }

🤯

The galaxy-brain approach seems to work a treat, and I'll add a couple more testcases around equality to ensure that. Very nice!

jhugman · 2021-02-18T15:06:23Z

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

+    }
+
+    internal open fun write(buf: RustBufferBuilder) {
+        throw RuntimeException("enum variant should have overridden `write` method, something is very wrong!!")


On reflection, I think I'd rather put the write method and friends in the top level, and use pattern matching to work out what to do.

(FWIW I don't think there's any perf difference, in fact: given it's thee idiomatic way of doing tagged-unions, I expect there's some particular optimizations we could end up losing out on by adding methods to individual variant classes.)

rfk · 2021-02-19T05:03:21Z

Updated in response to @jhugman latest feedback.

rfk · 2021-02-19T05:05:52Z

examples/rondpoint/tests/bindings/test_rondpoint.kts

+
+val var1: EnumerationAvecDonnees = EnumerationAvecDonnees.Zero
+val var2: EnumerationAvecDonnees = EnumerationAvecDonnees.Un(1u)
+val var3: EnumerationAvecDonnees = EnumerationAvecDonnees.Un(2u)


The typed variables here are to outsmart the Kotlin compiler, which will give a compile-time error if you try to directly compare something of type EnumerationAvecDonnees.Zero and something of type EnumerationAvecDonnees.Un.

rfk · 2021-02-19T05:28:30Z

Hrm, the rondpoint crate is failing to build in CI here, with what appear to be out-of-memory errors:

Caused by:
  process didn't exit successfully: `rustc --crate-name uniffi_rondpoint --edition=2018 examples/rondpoint/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type cdylib --crate-type lib --emit=dep-info,link -C debuginfo=2 -C metadata=053719643b56441c --out-dir /home/circleci/project/target/debug/deps -C incremental=/home/circleci/project/target/debug/incremental -L dependency=/home/circleci/project/target/debug/deps --extern uniffi=/home/circleci/project/target/debug/deps/libuniffi-622b493c8a6725e6.rlib --extern uniffi_macros=/home/circleci/project/target/debug/deps/libuniffi_macros-b7aff2897c4d9de6.so -D warnings` (signal: 9, SIGKILL: kill)
warning: build failed, waiting for other jobs to finish...
error: linking with `cc` failed: exit code: 1
  |
  = note: "cc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-m64" "-L" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "/home/circleci/project/target/debug/deps/arithmetical.128vv0nl8ohl5duk.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.17qde80626cmwono.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.190shw0nb0jwrvnq.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.1efkqkyw2vfvq0cq.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.1h549mnvlhu6kx62.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.1trgsh1jgtu2dzg4.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.201cn5r1kdf42vwp.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.23xtc1yr9w6voutq.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.249yaojpywzbmc58.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.26tnl9p6edsmo1vn.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.2b8zs3xp8yccjv9n.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.2dfznzhzko3hfucn.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.2o3y1jvfz588xr4j.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.2whez0kgj4a6yh6v.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.31ziggzrgykvtvvh.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.3fo6cwgekbm8eipp.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.3smn1wl22kf53084.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.42sg7z6pu41rx0oj.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.4blho4emn5w3yt1w.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.4ssytj083kstompf.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.544wkcpgqb1z1i1a.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.56t3ybnmrjh0xjzu.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.5fgy3snlmii7kl8g.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.dsblm0i1ak2u4v8.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.nqushftut1gb8rg.rcgu.o" "/home/circleci/project/target/debug/deps/arithmetical.nwud4vssfr9v9r5.rcgu.o" "-o" "/home/circleci/project/target/debug/deps/libarithmetical.so" "-Wl,--version-script=/tmp/rustcImMpM0/list" "/home/circleci/project/target/debug/deps/arithmetical.hkx95sgdce0u0cv.rcgu.o" "-Wl,--gc-sections" "-Wl,-zrelro" "-Wl,-znow" "-nodefaultlibs" "-L" "/home/circleci/project/target/debug/deps" "-L" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-Wl,-Bstatic" "/home/circleci/project/target/debug/deps/libuniffi-622b493c8a6725e6.rlib" "/home/circleci/project/target/debug/deps/libuniffi_bindgen-16e505356ba326f5.rlib" "/home/circleci/project/target/debug/deps/libweedle-aef167c1aa3dbb32.rlib" "/home/circleci/project/target/debug/deps/libnom-0fee0736e68b1069.rlib" "/home/circleci/project/target/debug/deps/libheck-e9f9f2619703ceaa.rlib" "/home/circleci/project/target/debug/deps/libunicode_segmentation-c8f494fc70b1117f.rlib" "/home/circleci/project/target/debug/deps/libaskama-537ffd130493464b.rlib" "/home/circleci/project/target/debug/deps/libaskama_shared-9451c3a778333d62.rlib" "/home/circleci/project/target/debug/deps/libtoml-31756e16259888ea.rlib" "/home/circleci/project/target/debug/deps/libsyn-1cb4f73bae3a226d.rlib" "/home/circleci/project/target/debug/deps/libnom-9b9d4b929f8fedf4.rlib" "/home/circleci/project/target/debug/deps/libmemchr-b63c81c95b3ea372.rlib" "/home/circleci/project/target/debug/deps/liblexical_core-5908b33ac15872fb.rlib" "/home/circleci/project/target/debug/deps/libarrayvec-fc27a13ca7389030.rlib" "/home/circleci/project/target/debug/deps/libbitflags-599c3a9be4aadd44.rlib" "/home/circleci/project/target/debug/deps/libbitvec-c3fcb913cff8ff19.rlib" "/home/circleci/project/target/debug/deps/libfunty-19a720bfa18dce89.rlib" "/home/circleci/project/target/debug/deps/libwyz-4577ea1179b041c6.rlib" "/home/circleci/project/target/debug/deps/libtap-fd238052b670c4b5.rlib" "/home/circleci/project/target/debug/deps/libradium-0a3a3c08ef28929c.rlib" "/home/circleci/project/target/debug/deps/libquote-7e1852a0cea0cdb2.rlib" "/home/circleci/project/target/debug/deps/libproc_macro2-815d44389fbca503.rlib" "/home/circleci/project/target/debug/deps/libunicode_xid-5bbc8b1bac57b67c.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libproc_macro-9fd29953164d86f0.rlib" "/home/circleci/project/target/debug/deps/libaskama_escape-ed1e0cc3b58d2466.rlib" "/home/circleci/project/target/debug/deps/libstatic_assertions-7b24323df9107fb0.rlib" "/home/circleci/project/target/debug/deps/libcargo_metadata-e1494bb5b6f651dd.rlib" "/home/circleci/project/target/debug/deps/libserde_json-e2226d212e2ed73f.rlib" "/home/circleci/project/target/debug/deps/libryu-65130c6d4b3381b5.rlib" "/home/circleci/project/target/debug/deps/libitoa-eca5c176e5c68ac7.rlib" "/home/circleci/project/target/debug/deps/libsemver-7b8295dac8c0b8df.rlib" "/home/circleci/project/target/debug/deps/libserde-81a5e1ceab613554.rlib" "/home/circleci/project/target/debug/deps/libsemver_parser-906f2c60081c06fb.rlib" "/home/circleci/project/target/debug/deps/libffi_support-0e7f42b523b4e0fd.rlib" "/home/circleci/project/target/debug/deps/liblazy_static-5390404b369a64d3.rlib" "/home/circleci/project/target/debug/deps/liblog-f8ad7d725fc2e075.rlib" "/home/circleci/project/target/debug/deps/libcfg_if-bba59873400967df.rlib" "/home/circleci/project/target/debug/deps/libbytes-040a8c7b9588319c.rlib" "/home/circleci/project/target/debug/deps/libanyhow-34a3b06fd061f9d1.rlib" "/home/circleci/project/target/debug/deps/libthiserror-bae65f7bcdf0be60.rlib" "-Wl,--start-group" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-7c5e456310a1373c.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libpanic_unwind-b981d9b2a408308f.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libhashbrown-43d0ea1b5ae34d0d.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_std_workspace_alloc-09e7f22e773899cd.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libbacktrace-aa74f166651adf6e.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libbacktrace_sys-22c386707b639611.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_demangle-db04c9c5cd3bcf45.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libunwind-bb27492f721492e8.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcfg_if-6b95245dbf686e20.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liblibc-287409d75db2ecd3.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc-a93f70ee2006b6e3.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_std_workspace_core-566cdfbcc94b4360.rlib" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore-7bb8dddc7ce34e92.rlib" "-Wl,--end-group" "/usr/local/rustup/toolchains/1.43.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcompiler_builtins-b117658e17259aa6.rlib" "-Wl,-Bdynamic" "-ldl" "-lrt" "-lpthread" "-lgcc_s" "-lc" "-lm" "-lrt" "-lpthread" "-lutil" "-lutil" "-shared"
  = note: /usr/bin/ld: final link failed: Cannot allocate memory
          collect2: error: ld returned 1 exit status

I'm kind of hoping it's a transient infrastructure issue, because I haven't made any changes to the Rust code there since the previous iteration of this PR, which compiled successfully.

jhugman

This is great.

I'm not qualified to comment about Python, or gecko-js, but Swift and Kotlin get my thumbs up.

jhugman · 2021-02-19T13:01:52Z

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

 }
+
+{% endif %}


This whole file: lovely.

jhugman · 2021-02-19T13:07:47Z

uniffi_bindgen/src/bindings/swift/templates/EnumTemplate.swift

@@ -1,30 +1,40 @@
-public enum {{ e.name()|class_name_swift }}: ViaFfi {
+public enum {{ e.name()|class_name_swift }}: ViaFfiUsingByteBuffer, ViaFfi, Equatable {


Ah, OK.

My thought was that a developer gets an error swiftc pointing to the generated code, goes and looks at the generate code, finds the comment with the link to the issue. WDYT?

This review cycle has been long, so I wouldn't be against leaving it to another issue.

jhugman · 2021-02-19T13:36:02Z

uniffi_bindgen/src/interface/enum_.rs

-    pub(super) variants: Vec<String>,
+    pub(super) variants: Vec<Variant>,
+    // "Flat" enums do not have, and will never have, variants with associated data.
+    pub(super) flat: bool,


jhugman · 2021-02-19T19:20:50Z

uniffi_bindgen/src/interface/mod.rs

+        for e in self.enums.iter() {
+            for variant in e.variants.iter() {
+                if self.types.get_type_definition(variant.name()).is_some() {
+                    bail!(
+                        "Enum variant names must not shadow type names: \"{}\"",
+                        variant.name()
+                    )
+                }
+            }
+        }


This commit adds support for enums variants having named data fields, in a style similar to records. It also attempts to expose both "plain" enums and the new data-bearing enums to target foreign languages in an idiomatic way. Unfortunately for us, WebIDL doesn't have native syntax for this kind of data. Fortunately for us, we can fake it by using anonymous special interface methods via a syntax like: ``` [Enum] interface EnumWithData { VariantName(type1 name1, type2 name2, ...); } ```

rfk commented Feb 8, 2021

View reviewed changes

rfk force-pushed the tagged-unions branch from 918cba6 to ac6395e Compare February 8, 2021 04:00

rfk commented Feb 8, 2021

View reviewed changes

rfk force-pushed the component-interface-deconstruction branch 2 times, most recently from 9fd863c to b2fb6f8 Compare February 9, 2021 01:38

Base automatically changed from component-interface-deconstruction to main February 9, 2021 01:42

mhammond reviewed Feb 9, 2021

View reviewed changes

rfk force-pushed the tagged-unions branch 2 times, most recently from 45554da to 36e07d7 Compare February 9, 2021 09:57

rfk force-pushed the tagged-unions branch from 36e07d7 to f447b31 Compare February 9, 2021 09:59

rfk commented Feb 9, 2021

View reviewed changes

data-sync-user mentioned this pull request Feb 11, 2021

Add enums-with-associated-data to uniffi mozilla/application-services#3864

Closed

rfk mentioned this pull request Feb 15, 2021

[discuss] next steps for interface definition language? #385

Closed

rfk force-pushed the tagged-unions branch 2 times, most recently from ab5ed10 to 73cfb08 Compare February 15, 2021 02:51

rfk changed the title ~~Add support for tagged unions (aka "enums with associated data", aka "sum types", aka ...)~~ Add support for enums with associated data (aka "tagged unions", aka "sum types", aka ...) Feb 15, 2021

rfk force-pushed the tagged-unions branch from 73cfb08 to 2058650 Compare February 15, 2021 05:19

rfk force-pushed the tagged-unions branch from 2058650 to 351e098 Compare February 15, 2021 05:33

rfk force-pushed the tagged-unions branch 2 times, most recently from 5de2a4d to fe06656 Compare February 18, 2021 01:40

rfk requested review from jhugman and mhammond February 18, 2021 01:41

rfk force-pushed the tagged-unions branch from fe06656 to 3a63c2b Compare February 18, 2021 02:12

rfk force-pushed the tagged-unions branch from 3a63c2b to 61ccf63 Compare February 18, 2021 02:13

rfk commented Feb 18, 2021

View reviewed changes

rfk force-pushed the tagged-unions branch 2 times, most recently from 5ec3b08 to f46d83c Compare February 18, 2021 04:41

mhammond reviewed Feb 18, 2021

View reviewed changes

rfk force-pushed the tagged-unions branch from f46d83c to d8ada0e Compare February 18, 2021 05:26

jhugman reviewed Feb 18, 2021

View reviewed changes

rfk mentioned this pull request Feb 19, 2021

Add support for recursive enums #396

Open

rfk force-pushed the tagged-unions branch from d8ada0e to 755940c Compare February 19, 2021 05:02

rfk requested a review from jhugman February 19, 2021 05:03

rfk force-pushed the tagged-unions branch from 755940c to 3985a58 Compare February 19, 2021 05:04

rfk commented Feb 19, 2021

View reviewed changes

rfk force-pushed the tagged-unions branch from 3985a58 to 3b8de43 Compare February 19, 2021 05:11

jhugman approved these changes Feb 19, 2021

View reviewed changes

rfk force-pushed the tagged-unions branch from 3b8de43 to 6a7745c Compare February 22, 2021 04:09

rfk merged commit f7c8ccd into main Feb 22, 2021

rfk deleted the tagged-unions branch February 22, 2021 04:14

rfk mentioned this pull request May 25, 2021

Support rich structured Enums for reporting errors #460

Closed

This was referenced Jun 23, 2021

Support rich structured Enums for reporting errors mozilla/application-services#4237

Closed

Add support for recursive enums mozilla/application-services#4248

Closed

[discuss] next steps for interface definition language? mozilla/application-services#4293

Closed

		@@ -1,30 +1,40 @@
		public enum {{ e.name()\|class_name_swift }}: ViaFfi {
		public enum {{ e.name()\|class_name_swift }}: ViaFfiUsingByteBuffer, ViaFfi, Equatable {

	{%- endif %}) : {{ e.name()\|class_name_kt }}() {
	: {{ e.name()\|class_name_kt }}() { {%- endif %})

Add support for enums with associated data (aka "tagged unions", aka "sum types", aka ...) #381

Add support for enums with associated data (aka "tagged unions", aka "sum types", aka ...) #381

Conversation

rfk commented Feb 8, 2021

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

Choose a reason for hiding this comment

jhugman Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Feb 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhammond 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Feb 9, 2021

Choose a reason for hiding this comment

jhugman Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmose commented Feb 10, 2021

rfk commented Feb 15, 2021

rfk commented Feb 16, 2021

rfk commented Feb 18, 2021

rfk Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhammond 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

jhugman 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

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

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 Feb 19, 2021

Choose a reason for hiding this comment

rfk commented Feb 19, 2021

jhugman 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

jhugman Feb 8, 2021 •

edited

Loading

rfk Feb 9, 2021 •

edited

Loading

jhugman Feb 9, 2021 •

edited

Loading

rfk Feb 18, 2021 •

edited

Loading