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

Add "return_type()" to Constructor #1490

Closed
Closed
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
36 changes: 36 additions & 0 deletions uniffi_bindgen/src/interface/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,42 @@ impl APIConverter<Argument> for weedle::argument::SingleArgument<'_> {
}
}

/// Implemented by function-like types (Function, Method, Constructor)
pub trait Callable {
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, why is the arguments API return a vec of references where the other two return owned Type?

Is it because it was hard to have then all return references without overcomplicating the code?

Just curious!

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, would we like to add a name function too?

Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, why is the arguments API return a vec of references where the other two return owned Type?

I think that was copied from the #1469 implementation. I didn't put a lot of thought into it, but that seemed like the path of least resistance to me. In general, it seems like we tend to clone Type rather than try to manage a reference to it. I'm not sure if that's the best policy going forward or not.

Additionally, would we like to add a name function too?

Maybe? I'm not totally sure though, there could be callables that don't have a name. For example a destructor. Also, the primary constructor currently has a name field, but it's just hard coded to "new". That said, I'm not really against it I just didn't put the time into defining it.

fn arguments(&self) -> Vec<&Argument>;
fn return_type(&self) -> Option<Type>;
fn throws_type(&self) -> Option<Type>;
}

impl Callable for Function {
fn arguments(&self) -> Vec<&Argument> {
self.arguments()
}

fn return_type(&self) -> Option<Type> {
self.return_type().cloned()
}

fn throws_type(&self) -> Option<Type> {
self.throws_type()
}
}

// Needed because Askama likes to add extra refs to variables
impl<T: Callable> Callable for &T {
fn arguments(&self) -> Vec<&Argument> {
(*self).arguments()
}

fn return_type(&self) -> Option<Type> {
(*self).return_type()
}

fn throws_type(&self) -> Option<Type> {
(*self).throws_type()
}
}

#[cfg(test)]
mod test {
use super::*;
Expand Down
2 changes: 1 addition & 1 deletion uniffi_bindgen/src/interface/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub use enum_::Enum;
mod error;
pub use error::Error;
mod function;
pub use function::{Argument, Function};
pub use function::{Argument, Callable, Function};
mod literal;
pub use literal::{Literal, Radix};
mod namespace;
Expand Down
40 changes: 39 additions & 1 deletion uniffi_bindgen/src/interface/object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ use uniffi_meta::Checksum;

use super::attributes::{Attribute, ConstructorAttributes, InterfaceAttributes, MethodAttributes};
use super::ffi::{FfiArgument, FfiFunction, FfiType};
use super::function::Argument;
use super::function::{Argument, Callable};
use super::types::{Type, TypeIterator};
use super::{convert_type, APIConverter, ComponentInterface};

Expand Down Expand Up @@ -248,6 +248,7 @@ pub struct Constructor {
// avoids a weird circular dependency in the calculation.
#[checksum_ignore]
pub(super) ffi_func: FfiFunction,
pub(super) return_type: Option<Type>,
pub(super) attributes: ConstructorAttributes,
}

Expand All @@ -268,6 +269,10 @@ impl Constructor {
&self.ffi_func
}

pub fn return_type(&self) -> Option<&Type> {
self.return_type.as_ref()
}

pub fn throws(&self) -> bool {
self.attributes.get_throws_err().is_some()
}
Expand All @@ -290,6 +295,9 @@ impl Constructor {
self.ffi_func.name = format!("{ci_prefix}_{obj_name}_{}", self.name);
self.ffi_func.arguments = self.arguments.iter().map(Into::into).collect();
self.ffi_func.return_type = Some(FfiType::RustArcPtr(obj_name.to_string()));

// this is a bit of a dirty place to put this, but there isn't another "general" pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we expand why this is not an ideal place? Inline comment is great, but if we have a more actionable way of making it more ideal then a bug filed with more context would be phenomenal!

Copy link
Contributor

Choose a reason for hiding this comment

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

#1469 adds an object_name field to Constructor, which would avoid this. Does that make sense for the long-term plan?

self.return_type = Some(Type::Object(obj_name.to_string()));
}

pub fn iter_types(&self) -> TypeIterator<'_> {
Expand All @@ -302,6 +310,7 @@ impl Default for Constructor {
Constructor {
name: String::from("new"),
arguments: Vec::new(),
return_type: None,
ffi_func: Default::default(),
attributes: Default::default(),
}
Expand All @@ -317,12 +326,27 @@ impl APIConverter<Constructor> for weedle::interface::ConstructorInterfaceMember
Ok(Constructor {
name: String::from(attributes.get_name().unwrap_or("new")),
arguments: self.args.body.list.convert(ci)?,
return_type: None,
ffi_func: Default::default(),
attributes,
})
}
}

impl Callable for Constructor {
fn arguments(&self) -> Vec<&Argument> {
self.arguments()
}

fn return_type(&self) -> Option<Type> {
self.return_type().cloned()
}

fn throws_type(&self) -> Option<Type> {
self.throws_type()
}
}

// Represents an instance method for an object type.
//
// The FFI will represent this as a function whose first/self argument is a
Expand Down Expand Up @@ -479,6 +503,20 @@ impl APIConverter<Method> for weedle::interface::OperationInterfaceMember<'_> {
}
}

impl Callable for Method {
fn arguments(&self) -> Vec<&Argument> {
self.arguments()
}

fn return_type(&self) -> Option<Type> {
self.return_type().cloned()
}

fn throws_type(&self) -> Option<Type> {
self.throws_type()
}
}

#[cfg(test)]
mod test {
use super::*;
Expand Down