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.

Sign up for GitHub

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

Jump to bottom

WIP give ComponentInterface members a pointer to their parent. #1003

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

WIP give ComponentInterface members a pointer to their parent. #1003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 3 additions & 3 deletions uniffi_bindgen/src/bindings/kotlin/templates/EnumTemplate.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ enum class {{ e.name()|class_name_kt }} {
{% else %}

{% call kt::unsigned_types_annotation(e) %}
sealed class {{ e.name()|class_name_kt }}{% if e.contains_object_references(ci) %}: Disposable {% endif %} {
sealed class {{ e.name()|class_name_kt }}{% if e.contains_object_references() %}: Disposable {% endif %} {
{% for variant in e.variants() -%}
{% if !variant.has_fields() -%}
object {{ variant.name()|class_name_kt }} : {{ e.name()|class_name_kt }}()
Expand Down Expand Up @@ -85,14 +85,14 @@ sealed class {{ e.name()|class_name_kt }}{% if e.contains_object_references(ci)
}.let { /* this makes the `when` an expression, which ensures it is exhaustive */ }
}

{% if e.contains_object_references(ci) %}
{% if e.contains_object_references() %}
@Suppress("UNNECESSARY_SAFE_CALL") // codegen is much simpler if we unconditionally emit safe calls here
override fun destroy() {
when(this) {
{%- for variant in e.variants() %}
is {{ e.name()|class_name_kt }}.{{ variant.name()|class_name_kt }} -> {
{% for field in variant.fields() -%}
{%- if ci.type_contains_object_references(field.type_()) -%}
{%- if field.contains_object_references() -%}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems like a nice improvement in usability for the template logic.

this.{{ field.name() }}?.destroy()
{% endif -%}
{%- endfor %}
Expand Down
6 changes: 3 additions & 3 deletions uniffi_bindgen/src/bindings/kotlin/templates/ErrorTemplate.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ interface CallStatusErrorHandler<E> {

// Error {{ e.name() }}
{%- let toplevel_name=e.name()|exception_name_kt %}
sealed class {{ toplevel_name }}: Exception(){% if e.contains_object_references(ci) %}, Disposable {% endif %} {
sealed class {{ toplevel_name }}: Exception(){% if e.contains_object_references() %}, Disposable {% endif %} {
// Each variant is a nested class
{% for variant in e.variants() -%}
{% if !variant.has_fields() -%}
Expand Down Expand Up @@ -60,14 +60,14 @@ sealed class {{ toplevel_name }}: Exception(){% if e.contains_object_references(
}
}

{% if e.contains_object_references(ci) %}
{% if e.contains_object_references() %}
@Suppress("UNNECESSARY_SAFE_CALL") // codegen is much simpler if we unconditionally emit safe calls here
override fun destroy() {
when(this) {
{%- for variant in e.variants() %}
is {{ e.name()|class_name_kt }}.{{ variant.name()|class_name_kt }} -> {
{% for field in variant.fields() -%}
{%- if ci.type_contains_object_references(field.type_()) -%}
{%- if field.contains_object_references() -%}
this.{{ field.name() }}?.destroy()
{% endif -%}
{%- endfor %}
Expand Down
6 changes: 3 additions & 3 deletions uniffi_bindgen/src/bindings/kotlin/templates/RecordTemplate.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ data class {{ rec.name()|class_name_kt }} (
{%- endmatch -%}
{% if !loop.last %}, {% endif %}
{%- endfor %}
) {% if rec.contains_object_references(ci) %}: Disposable {% endif %}{
) {% if rec.contains_object_references() %}: Disposable {% endif %}{
companion object {
internal fun lift(rbuf: RustBuffer.ByValue): {{ rec.name()|class_name_kt }} {
return liftFromRustBuffer(rbuf) { buf -> {{ rec.name()|class_name_kt }}.read(buf) }
Expand All @@ -33,11 +33,11 @@ data class {{ rec.name()|class_name_kt }} (
{% endfor %}
}

{% if rec.contains_object_references(ci) %}
{% if rec.contains_object_references() %}
@Suppress("UNNECESSARY_SAFE_CALL") // codegen is much simpler if we unconditionally emit safe calls here
override fun destroy() {
{% for field in rec.fields() %}
{%- if ci.type_contains_object_references(field.type_()) -%}
{%- if field.contains_object_references() -%}
this.{{ field.name() }}?.destroy()
{% endif -%}
{%- endfor %}
Expand Down
2 changes: 1 addition & 1 deletion uniffi_bindgen/src/bindings/swift/templates/EnumTemplate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,6 @@ extension {{ e.name()|class_name_swift }}: ViaFfiUsingByteBuffer, ViaFfi {
}
}

{% if ! e.contains_object_references(ci) %}
{% if ! e.contains_object_references() %}
extension {{ e.name()|class_name_swift }}: Equatable, Hashable {}
{% endif %}
2 changes: 1 addition & 1 deletion uniffi_bindgen/src/bindings/swift/templates/ErrorTemplate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ extension {{ e.name()|class_name_swift }}: ViaFfiUsingByteBuffer, ViaFfi {
}
}

{% if !e.contains_object_references(ci) %}
{% if !e.contains_object_references() %}
extension {{ e.name()|class_name_swift }}: Equatable, Hashable {}
{% endif %}
extension {{ e.name()|class_name_swift }}: Error { }
Expand Down
2 changes: 1 addition & 1 deletion uniffi_bindgen/src/bindings/swift/templates/RecordTemplate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ public struct {{ rec.name()|class_name_swift }} {
}
}

{% if ! rec.contains_object_references(ci) %}
{% if ! rec.contains_object_references() %}
extension {{ rec.name()|class_name_swift }}: Equatable, Hashable {
public static func ==(lhs: {{ rec.name()|class_name_swift }}, rhs: {{ rec.name()|class_name_swift }}) -> Bool {
{%- for field in rec.fields() %}
Expand Down
116 changes: 75 additions & 41 deletions uniffi_bindgen/src/interface/enum_.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,65 +78,84 @@

use anyhow::{bail, Result};

use super::record::Field;
use super::record::{Field, FieldDescr};
use super::types::Type;
use super::{APIConverter, ComponentInterface};
use super::{APIConverter, ComponentInterface, CINode};

/// Represents an enum with named variants, each of which may have named
/// and typed fields.
///
/// Enums are passed across the FFI by serializing to a bytebuffer, with a
/// i32 indicating the variant followed by the serialization of each field.
#[derive(Debug)]
pub struct Enum<'a> {
pub(super) parent: &'a ComponentInterface,
pub(super) descr: &'a EnumDescr,
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the Enum struct is now a kind of "fat pointer" thing that contains both a pointer to the data and parent ref to the containing object.


#[derive(Debug, Clone, Hash)]
pub struct Enum {
pub struct EnumDescr {
pub(super) name: String,
pub(super) variants: Vec<Variant>,
pub(super) variants: Vec<VariantDescr>,
// "Flat" enums do not have, and will never have, variants with associated data.
pub(super) flat: bool,
}

impl Enum {
impl<'a> Enum<'a> {
pub fn name(&self) -> &str {
&self.name
&self.descr.name
}

pub fn variants(&self) -> Vec<&Variant> {
self.variants.iter().collect()
pub fn variants(&self) -> Vec<Variant<'_, Self>> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here, when consuming code lists the variants of the enum, so see that we create a bunch of Variant instances on demand with the appropriate pointers.

self.descr
.variants
.iter()
.map(|v| Variant {
parent: self,
descr: v,
})
.collect()
}

pub fn is_flat(&self) -> bool {
self.flat
self.descr.flat
}

pub fn contains_object_references(&self, ci: &ComponentInterface) -> bool {
// *sigh* at the clone here, the relationship between a ComponentInterace
// and its contained types could use a bit of a cleanup.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey look, no more clone!

ci.type_contains_object_references(&Type::Enum(self.name.clone()))
pub fn contains_object_references(&self) -> bool {
self.variants().iter().any(|v| {
v.contains_object_references()
})
}

pub fn contains_unsigned_types(&self, ci: &ComponentInterface) -> bool {
self.variants().iter().any(|v| {
v.fields()
pub fn contains_unsigned_types(&self, _ci: &ComponentInterface) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to keep _ci here? Is it necessary or just work-in-progress for now?

self.descr.variants.iter().any(|v| {
v.fields
.iter()
.any(|f| ci.type_contains_unsigned_types(&f.type_))
.any(|f| self.parent.type_contains_unsigned_types(&f.type_))
})
}
}

impl<'a> CINode for Enum<'a> {
fn ci(&self) -> &ComponentInterface {
self.parent
}
}

// Note that we have two `APIConverter` impls here - one for the `enum` case
// and one for the `[Enum] interface` case.

impl APIConverter<Enum> for weedle::EnumDefinition<'_> {
fn convert(&self, _ci: &mut ComponentInterface) -> Result<Enum> {
Ok(Enum {
impl APIConverter<EnumDescr> for weedle::EnumDefinition<'_> {
fn convert(&self, _ci: &mut ComponentInterface) -> Result<EnumDescr> {
Ok(EnumDescr {
name: self.identifier.0.to_string(),
variants: self
.values
.body
.list
.iter()
.map::<Result<_>, _>(|v| {
Ok(Variant {
Ok(VariantDescr {
name: v.0.to_string(),
..Default::default()
})
Expand All @@ -147,20 +166,20 @@ impl APIConverter<Enum> for weedle::EnumDefinition<'_> {
}
}

impl APIConverter<Enum> for weedle::InterfaceDefinition<'_> {
fn convert(&self, ci: &mut ComponentInterface) -> Result<Enum> {
impl APIConverter<EnumDescr> for weedle::InterfaceDefinition<'_> {
fn convert(&self, ci: &mut ComponentInterface) -> Result<EnumDescr> {
if self.inheritance.is_some() {
bail!("interface inheritence is not supported for enum interfaces");
}
// We don't need to check `self.attributes` here; if calling code has dispatched
// to this impl then we already know there was an `[Enum]` attribute.
Ok(Enum {
Ok(EnumDescr {
name: self.identifier.0.to_string(),
variants: self
.members
.body
.iter()
.map::<Result<Variant>, _>(|member| match member {
.map::<Result<VariantDescr>, _>(|member| match member {
weedle::interface::InterfaceMember::Operation(t) => Ok(t.convert(ci)?),
_ => bail!(
"interface member type {:?} not supported in enum interface",
Expand All @@ -176,27 +195,42 @@ impl APIConverter<Enum> for weedle::InterfaceDefinition<'_> {
/// Represents an individual variant in an Enum.
///
/// Each variant has a name and zero or more fields.

#[derive(Debug)]
pub struct Variant<'a, Parent: CINode> {
pub(super) parent: &'a Parent,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this starts to get a bit complicated...the Variant struct may be used as part of an Enum or as part of an Error, so its parent pointer type needs to be generic. The main property that we want from the parent pointer is the ability to get all the way up to the containing ComponentInterface, so I made a trait for that.

An alternative could be to have it point directly to the containing ComponentInterface rather than its containing object, which would make this more of an "owner" pointer than a "parent" pointer. But, I can imagine reasons why a variant might need to be able to inspect the properties of its containing Enum/Error.

pub(super) descr: &'a VariantDescr,
}

#[derive(Debug, Clone, Default, Hash)]
pub struct Variant {
pub struct VariantDescr {
pub(super) name: String,
pub(super) fields: Vec<Field>,
pub(super) fields: Vec<FieldDescr>,
}

impl Variant {
impl<'a, Parent: CINode> Variant<'a, Parent> {
pub fn name(&self) -> &str {
&self.name
&self.descr.name
}
pub fn fields(&self) -> Vec<&Field> {
self.fields.iter().collect()
pub fn fields(&self) -> Vec<Field<'_, Self>> {
self.descr.fields.iter().map(|f| Field { parent: self, descr: f }).collect()
}

pub fn has_fields(&self) -> bool {
!self.fields.is_empty()
!self.descr.fields.is_empty()
}
pub fn contains_object_references(&self) -> bool {
self.fields().iter().any(|f| f.contains_object_references())
}
}

impl<'a, Parent: CINode + 'a> CINode for Variant<'a, Parent> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could imagine generating these impls automatically from some sort of macro.

fn ci(&self) -> &ComponentInterface {
self.parent.ci()
}
}

impl APIConverter<Variant> for weedle::interface::OperationInterfaceMember<'_> {
fn convert(&self, ci: &mut ComponentInterface) -> Result<Variant> {
impl APIConverter<VariantDescr> for weedle::interface::OperationInterfaceMember<'_> {
fn convert(&self, ci: &mut ComponentInterface) -> Result<VariantDescr> {
if self.special.is_some() {
bail!("special operations not supported");
}
Expand All @@ -219,7 +253,7 @@ impl APIConverter<Variant> for weedle::interface::OperationInterfaceMember<'_> {
_ => bail!("enum interface members must have plain identifers as names"),
}
};
Ok(Variant {
Ok(VariantDescr {
name,
fields: self
.args
Expand All @@ -232,17 +266,17 @@ impl APIConverter<Variant> for weedle::interface::OperationInterfaceMember<'_> {
}
}

impl APIConverter<Field> for weedle::argument::Argument<'_> {
fn convert(&self, ci: &mut ComponentInterface) -> Result<Field> {
impl APIConverter<FieldDescr> for weedle::argument::Argument<'_> {
fn convert(&self, ci: &mut ComponentInterface) -> Result<FieldDescr> {
match self {
weedle::argument::Argument::Single(t) => t.convert(ci),
weedle::argument::Argument::Variadic(_) => bail!("variadic arguments not supported"),
}
}
}

impl APIConverter<Field> for weedle::argument::SingleArgument<'_> {
fn convert(&self, ci: &mut ComponentInterface) -> Result<Field> {
impl APIConverter<FieldDescr> for weedle::argument::SingleArgument<'_> {
fn convert(&self, ci: &mut ComponentInterface) -> Result<FieldDescr> {
let type_ = ci.resolve_type_expression(&self.type_)?;
if let Type::Object(_) = type_ {
bail!("Objects cannot currently be used in enum variant data");
Expand All @@ -255,7 +289,7 @@ impl APIConverter<Field> for weedle::argument::SingleArgument<'_> {
}
// TODO: maybe we should use our own `Field` type here with just name and type,
// rather than appropriating record::Field..?
Ok(Field {
Ok(FieldDescr {
name: self.identifier.0.to_string(),
type_,
required: false,
Expand Down
Loading