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

Add destructors for objects #196

Merged
merged 2 commits into from
Aug 5, 2020
Merged

Add destructors for objects #196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96c588c
Add destructors for objects
jhugman Aug 3, 2020
e90e748
Fixup to work with new errors in PR #31
jhugman Aug 5, 2020
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
8 changes: 8 additions & 0 deletions examples/sprites/tests/bindings/test_sprites.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,11 @@ assert( s.getPosition() == Point(1.0, 2.0) )

s.moveBy(Vector(-4.0, 2.0))
assert( s.getPosition() == Point(-3.0, 4.0) )

s.destroy()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering where we'd put documentation for this, so consumers know they need to call the destroy function to do the deallocation. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's a really interesting question.

For now, I've documented in the generated section, however, we do need a better solution for this.

try {
s.moveBy(Vector(0.0, 0.0))
assert(false) { "Should not be able to call anything after `destroy`" }
} catch(e: IllegalStateException) {
assert(true)
}
2 changes: 1 addition & 1 deletion uniffi/src/bindings/kotlin/templates/ErrorTemplate.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ internal open class RustError : Structure() {
@Synchronized
fun ensureConsumed() {
if (this.message != null) {
_UniFFILib.INSTANCE.{{ci.namespace()}}_string_free(this.message!!)
_UniFFILib.INSTANCE.{{ ci.ffi_string_free().name() }}(this.message!!)
this.message = null
}
}
Expand Down
30 changes: 30 additions & 0 deletions uniffi/src/bindings/kotlin/templates/Helpers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// A handful of classes and functions to support the generated data structures.
// This would be a good candidate for isolating in its own ffi-support lib.

abstract class FFIObject(
private val handle: AtomicLong
) {
open fun destroy() {
this.handle.set(0L)
}

internal inline fun <R> callWithHandle(block: (handle: Long) -> R) =
this.handle.get().let { handle ->
if (handle != 0L) {
block(handle)
} else {
throw IllegalStateException("${this.javaClass.simpleName} object has already been destroyed")
}
}
}

inline fun <T : FFIObject, R> T.use(block: (T) -> R) =
try {
block(this)
} finally {
try {
this.destroy()
} catch (e: Throwable) {
// swallow
}
}
Comment on lines +21 to +30
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We all liked use for testing, so I implemented it here.

38 changes: 30 additions & 8 deletions uniffi/src/bindings/kotlin/templates/ObjectTemplate.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,47 @@
class {{ obj.name()|class_name_kt }}(
handle: Long
) : FFIObject(AtomicLong(handle)) {

class {{ obj.name()|class_name_kt }}(handle: Long) {
private var handle: AtomicLong = AtomicLong(handle)
{%- for cons in obj.constructors() %}
constructor({% call kt::arg_list_decl(cons) -%}) :
this({% call kt::to_rs_call(cons) %})
{%- endfor %}

// XXX TODO: destructors or equivalent.
/**
* Disconnect the object from the underlying Rust object.
*
* It can be called more than once, but once called, interacting with the object
* causes an `IllegalStateException`.
*
* Clients **must** call this method once done with the object, or cause a memory leak.
*/
override fun destroy() {
try {
callWithHandle {
super.destroy() // poison the handle so no-one else can use it before we tell rust.
_UniFFILib.INSTANCE.{{ obj.ffi_object_free().name() }}(it)
}
} catch (e: IllegalStateException) {
// The user called this more than once. Better than less than once.
}
}

{% for meth in obj.methods() -%}
{%- match meth.return_type() -%}

{%- when Some with (return_type) -%}
fun {{ meth.name()|fn_name_kt }}({% call kt::arg_list_decl(meth) %}): {{ return_type|type_kt }} {
val _retval = {% call kt::to_rs_call_with_prefix("this.handle.get()", meth) %}
return {{ "_retval"|lift_kt(return_type) }}
}
fun {{ meth.name()|fn_name_kt }}({% call kt::arg_list_decl(meth) %}): {{ return_type|type_kt }} =
callWithHandle {
{% call kt::to_rs_call_with_prefix("it", meth) %}
}.let {
{{ "it"|lift_kt(return_type) }}
}

{%- when None -%}
fun {{ meth.name()|fn_name_kt }}({% call kt::arg_list_decl(meth) %}) =
{% call kt::to_rs_call_with_prefix("this.handle.get()", meth) %}
callWithHandle {
{% call kt::to_rs_call_with_prefix("it", meth) %}
}
{% endmatch %}
{% endfor %}
}
39 changes: 19 additions & 20 deletions uniffi/src/bindings/kotlin/templates/macros.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,28 @@
#}

{%- macro to_rs_call(func) -%}
{% match func.throws() %}
{% when Some with (e) %}
rustCall({{e}}.ByReference()) { e ->
_UniFFILib.INSTANCE.{{ func.ffi_func().name() }}({% call _arg_list_rs_call(func) -%}{% if func.arguments().len() > 0 %},{% endif %}e)
}
{% else %}
_UniFFILib.INSTANCE.{{ func.ffi_func().name() }}({% call _arg_list_rs_call(func) -%})

{% endmatch %}
{%- match func.throws() %}
{%- when Some with (e) -%}
rustCall({{e}}.ByReference()) { e ->
_UniFFILib.INSTANCE.{{ func.ffi_func().name() }}({% call _arg_list_rs_call(func) -%}{% if func.arguments().len() > 0 %},{% endif %}e)
}
{%- else -%}
_UniFFILib.INSTANCE.{{ func.ffi_func().name() }}({% call _arg_list_rs_call(func) -%})
{%- endmatch %}
{%- endmacro -%}

{%- macro to_rs_call_with_prefix(prefix, func) -%}
{% match func.throws() %}
{% when Some with (e) %}
rustCall({{e}}.ByReference()) { e ->
{%- macro to_rs_call_with_prefix(prefix, func) %}
{%- match func.throws() %}
{%- when Some with (e) -%}
rustCall({{e}}.ByReference()) { e ->
_UniFFILib.INSTANCE.{{ func.ffi_func().name() }}(
{{- prefix }}, {% call _arg_list_rs_call(func) %}{% if func.arguments().len() > 0 %}, {% endif %}e)
}
{%- else -%}
_UniFFILib.INSTANCE.{{ func.ffi_func().name() }}(
{{- prefix }}, {% call _arg_list_rs_call(func) %}{% if func.arguments().len() > 0 %},{% endif %}e)
}
{% else %}
_UniFFILib.INSTANCE.{{ func.ffi_func().name() }}(
{{- prefix }}{% if func.arguments().len() > 0 %},{% endif %}{% call _arg_list_rs_call(func) %})
{% endmatch %}
{%- endmacro -%}
{{- prefix }}{% if func.arguments().len() > 0 %}, {% endif %}{% call _arg_list_rs_call(func) %})
{%- endmatch %}
{%- endmacro %}


{%- macro _arg_list_rs_call(func) %}
Expand Down
2 changes: 2 additions & 0 deletions uniffi/src/bindings/kotlin/templates/wrapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ import java.util.concurrent.atomic.AtomicLong

{% include "NamespaceLibraryTemplate.kt" %}

{% include "Helpers.kt" %}

// Public interface members begin here.
// Public facing enums
{% for e in ci.iter_enum_definitions() %}
Expand Down
3 changes: 2 additions & 1 deletion uniffi/src/bindings/python/templates/ObjectTemplate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ def __init__(self, {% call py::arg_list_decl(cons.arguments()) -%}):
self._handle = {% call py::to_rs_call(cons) %}
{%- endfor %}

# XXX TODO: destructors or equivalent.
def __del__(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this similarly poison the object on the python side, like we do in Kotlin? IIRC it is possible that someone might subclass one of these and extend __del__ in a way that resurrects the the python-side object after the rust-side destructor has been called. (They really shouldn't do that, but it's possible).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ug. I think I'm going to file a python-only issue to investigate stopping resurrecting python side objects.

_UniFFILib.{{ obj.ffi_object_free().name() }}(self._handle)

{% for meth in obj.methods() -%}
{%- match meth.return_type() -%}
Expand Down
2 changes: 1 addition & 1 deletion uniffi/src/bindings/python/templates/macros.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#}

{%- macro to_rs_call(func) -%}
_UniFFILib.{{ func.ffi_func().name() }}({% call _arg_list_rs_call(func.arguments()) -%})
_UniFFILib.{{ func.ffi_func().name() }}({% call _arg_list_rs_call(func.ffi_func().arguments()) -%})
{%- endmacro -%}

{%- macro to_rs_call_with_prefix(prefix, func) -%}
Expand Down
7 changes: 4 additions & 3 deletions uniffi/src/bindings/swift/templates/ObjectTemplate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ public class {{ obj.name() }} {
}
{%- endfor %}

// XXX TODO: destructors or equivalent.

deinit {
{{ obj.ffi_object_free().name() }}(handle)
}

// TODO: Maybe merge the two templates (i.e the one with a return type and the one without)
{% for meth in obj.methods() -%}
Expand All @@ -17,7 +18,7 @@ public class {{ obj.name() }} {
{%- when Some with (return_type) -%}
public func {{ meth.name()|fn_name_swift }}({% call swift::arg_list_decl(meth) %}) {% call swift::throws(meth) %} -> {{ return_type|decl_swift }} {
let _retval = {% call swift::to_rs_call_with_prefix("self.handle", meth) %}
return {%- call swift::try(meth) %} {{ "_retval"|lift_swift(return_type) }}
return {% call swift::try(meth) %} {{ "_retval"|lift_swift(return_type) }}
}

{%- when None -%}
Expand Down
2 changes: 1 addition & 1 deletion uniffi/src/bindings/swift/templates/TopLevelFunctionTemplate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

public func {{ func.name()|fn_name_swift }}({%- call swift::arg_list_decl(func) -%}) {% call swift::throws(func) %} -> {{ return_type|decl_swift }} {
let _retval = {% call swift::to_rs_call(func) %}
return {%- call swift::try(func) %} {{ "_retval"|lift_swift(return_type) }}
return {% call swift::try(func) %} {{ "_retval"|lift_swift(return_type) }}
}

{% when None -%}
Expand Down
42 changes: 21 additions & 21 deletions uniffi/src/bindings/swift/templates/macros.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,30 @@
#}

{%- macro to_rs_call(func) -%}
{% match func.throws() %}
{% when Some with (e) %}
try rustCall({{e}}.NoError) { err in
{{ func.ffi_func().name() }}({% call _arg_list_rs_call(func) -%}{% if func.arguments().len() > 0 %},{% endif %}err)
}
{% else %}
{{ func.ffi_func().name() }}({% call _arg_list_rs_call(func) -%})
{% endmatch %}
{%- match func.throws() %}
{%- when Some with (e) %}
try rustCall({{e}}.NoError) { err in
{{ func.ffi_func().name() }}({% call _arg_list_rs_call(func) -%}{% if func.arguments().len() > 0 %},{% endif %}err)
}
{%- else -%}
{{ func.ffi_func().name() }}({% call _arg_list_rs_call(func) -%})
{%- endmatch %}
{%- endmacro -%}

{%- macro to_rs_call_with_prefix(prefix, func) -%}
{% match func.throws() %}
{% when Some with (e) %}
try rustCall({{e}}.NoError) { err in
{%- match func.throws() %}
{%- when Some with (e) %}
try rustCall({{e}}.NoError) { err in
{{ func.ffi_func().name() }}(
{{- prefix }}, {% call _arg_list_rs_call(func) -%}{% if func.arguments().len() > 0 %},{% endif %}err
)
}
{%- else -%}
{{ func.ffi_func().name() }}(
{{- prefix }}, {% call _arg_list_rs_call(func) -%}{% if func.arguments().len() > 0 %},{% endif %}err
)
}
{% else %}
{{ func.ffi_func().name() }}(
{{- prefix }}{% if func.arguments().len() > 0 %},{% endif %}{% call _arg_list_rs_call(func) -%}
)
{% endmatch %}
{%- endmacro -%}
)
{%- endmatch %}
{%- endmacro %}

{%- macro _arg_list_rs_call(func) %}
{%- for arg in func.arguments() %}
Expand Down Expand Up @@ -63,9 +63,9 @@ try rustCall({{e}}.NoError) { err in
{%- endmacro -%}

{%- macro throws(func) %}
{% match func.throws() %}{% when Some with (e) %}throws{% else %}{% endmatch %}
{%- match func.throws() %}{% when Some with (e) %}throws{% else %}{% endmatch %}
{%- endmacro -%}

{%- macro try(func) %}
{% match func.throws() %}{% when Some with (e) %}try{% else %}try!{% endmatch %}
{%- match func.throws() %}{% when Some with (e) %}try{% else %}try!{% endmatch %}
{%- endmacro -%}
44 changes: 32 additions & 12 deletions uniffi/src/interface/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ impl<'ci> ComponentInterface {

pub fn ffi_bytebuffer_alloc(&self) -> FFIFunction {
FFIFunction {
name: format!("{}_bytebuffer_alloc", self.namespace()),
name: format!("ffi_{}_bytebuffer_alloc", self.namespace()),
arguments: vec![Argument {
name: "size".to_string(),
type_: TypeReference::U32,
Expand All @@ -158,7 +158,7 @@ impl<'ci> ComponentInterface {

pub fn ffi_bytebuffer_free(&self) -> FFIFunction {
FFIFunction {
name: format!("{}_bytebuffer_free", self.namespace()),
name: format!("ffi_{}_bytebuffer_free", self.namespace()),
arguments: vec![Argument {
name: "buf".to_string(),
type_: TypeReference::Bytes,
Expand All @@ -173,7 +173,7 @@ impl<'ci> ComponentInterface {

pub fn ffi_string_free(&self) -> FFIFunction {
FFIFunction {
name: format!("{}_string_free", self.namespace()),
name: format!("ffi_{}_string_free", self.namespace()),
arguments: vec![Argument {
name: "str".to_string(),
type_: TypeReference::RawStringPointer,
Expand All @@ -190,9 +190,9 @@ impl<'ci> ComponentInterface {
self.objects
.iter()
.map(|obj| {
obj.constructors
.iter()
.map(|f| f.ffi_func.clone())
vec![obj.ffi_object_free()]
.into_iter()
.chain(obj.constructors.iter().map(|f| f.ffi_func.clone()))
.chain(obj.methods.iter().map(|f| f.ffi_func.clone()))
})
.flatten()
Expand Down Expand Up @@ -446,7 +446,6 @@ impl FFIFunction {
pub fn return_type(&self) -> Option<&TypeReference> {
self.return_type.as_ref()
}

pub fn has_out_err(&self) -> bool {
self.has_out_err
}
Expand Down Expand Up @@ -579,11 +578,21 @@ impl APIConverter<Enum> for weedle::EnumDefinition<'_> {
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct Object {
name: String,
namespace: String,
constructors: Vec<Constructor>,
methods: Vec<Method>,
}

impl Object {
fn new(name: String, namespace: String) -> Object {
Object {
name,
namespace,
constructors: Default::default(),
methods: Default::default(),
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🦀 question: Should this be new? By convention, does new have arguments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be new, and it's fine for new to have arguments if they're obviously needed for making an instance.

However, I don't think you'll need this at all if you can avoid creating an API-level Method for this, and use only an FFIFunction in the style of the bytebuffer helpers.

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 added new.

However, avoiding adding an reserved Method meant that I was passing in the namespace into the Object now.


pub fn name(&self) -> &str {
&self.name
}
Expand All @@ -596,6 +605,21 @@ impl Object {
self.methods.iter().collect()
}

pub fn ffi_object_free(&self) -> FFIFunction {
FFIFunction {
name: format!("ffi_{}_{}_object_free", self.namespace, self.name),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yes, I see the trick here with needing more of the global context for the namespace. This seems OK I think, thanks for digging in.

arguments: vec![Argument {
name: "handle".to_string(),
type_: TypeReference::Object(self.name.clone()),
by_ref: false,
optional: false,
default: None,
}],
return_type: None,
has_out_err: false,
}
}

fn derive_ffi_funcs(&mut self, ci_prefix: &str) -> Result<()> {
for cons in self.constructors.iter_mut() {
cons.derive_ffi_func(ci_prefix, &self.name)?
Expand Down Expand Up @@ -742,11 +766,7 @@ impl APIConverter<Object> for weedle::InterfaceDefinition<'_> {
if self.inheritance.is_some() {
bail!("interface inheritence is not supported");
}
let mut object = Object {
name: self.identifier.0.to_string(),
constructors: Default::default(),
methods: Default::default(),
};
let mut object = Object::new(self.identifier.0.to_string(), ci.namespace().to_string());
for member in &self.members.body {
match member {
weedle::interface::InterfaceMember::Constructor(t) => {
Expand Down
Loading