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

Provide more external type information to FfiType::RustBuffer #2195

Merged
merged 9 commits into from
Aug 1, 2024
Merged

Provide more external type information to FfiType::RustBuffer #2195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a524879
Thread `ExternalType` metadata into RustBuffer
skeet70 Jul 18, 2024
fecb169
Switch to `test --no-run` from `build` for cdylib
skeet70 Jul 19, 2024
f75fc47
`cargo fmt`, make `no_deps` optional in lib mode
skeet70 Jul 25, 2024
310aa55
Add changelog entry per contributing guidelines
skeet70 Jul 25, 2024
294bee7
Rename FfiMetadata struct
skeet70 Jul 31, 2024
b792ff2
Merge branch 'main' into thread-external-type-metadata-rustbuffer
skeet70 Jul 31, 2024
1af5330
Merge branch 'main' into thread-external-type-metadata-rustbuffer
skeet70 Jul 31, 2024
1561db3
Touch up missed spots
skeet70 Jul 31, 2024
87948d9
Remove outdated changelog
skeet70 Jul 31, 2024
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
7 changes: 4 additions & 3 deletions uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,9 +431,10 @@ impl KotlinCodeOracle {
FfiType::Float64 => "Double".to_string(),
FfiType::Handle => "Long".to_string(),
FfiType::RustArcPtr(_) => "Pointer".to_string(),
FfiType::RustBuffer(maybe_suffix) => {
format!("RustBuffer{}", maybe_suffix.as_deref().unwrap_or_default())
}
FfiType::RustBuffer(maybe_external) => match maybe_external {
Some(external_meta) => format!("RustBuffer{}", external_meta.name),
None => "RustBuffer".to_string(),
},
FfiType::RustCallStatus => "UniffiRustCallStatus.ByValue".to_string(),
FfiType::ForeignBytes => "ForeignBytes.ByValue".to_string(),
FfiType::Callback(name) => self.ffi_callback_name(name),
Expand Down
10 changes: 6 additions & 4 deletions uniffi_bindgen/src/bindings/python/gen_python/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,8 +377,8 @@ impl PythonCodeOracle {
FfiType::Float64 => "ctypes.c_double".to_string(),
FfiType::Handle => "ctypes.c_uint64".to_string(),
FfiType::RustArcPtr(_) => "ctypes.c_void_p".to_string(),
FfiType::RustBuffer(maybe_suffix) => match maybe_suffix {
Some(suffix) => format!("_UniffiRustBuffer{suffix}"),
FfiType::RustBuffer(maybe_external) => match maybe_external {
Some(external_meta) => format!("_UniffiRustBuffer{}", external_meta.name),
None => "_UniffiRustBuffer".to_string(),
},
FfiType::RustCallStatus => "_UniffiRustCallStatus".to_string(),
Expand Down Expand Up @@ -407,8 +407,10 @@ impl PythonCodeOracle {
| FfiType::Int64 => "0".to_owned(),
FfiType::Float32 | FfiType::Float64 => "0.0".to_owned(),
FfiType::RustArcPtr(_) => "ctypes.c_void_p()".to_owned(),
FfiType::RustBuffer(maybe_suffix) => match maybe_suffix {
Some(suffix) => format!("_UniffiRustBuffer{suffix}.default()"),
FfiType::RustBuffer(maybe_external) => match maybe_external {
Some(external_meta) => {
format!("_UniffiRustBuffer{}.default()", external_meta.name)
}
None => "_UniffiRustBuffer.default()".to_owned(),
},
_ => unimplemented!("FFI return type: {t:?}"),
Expand Down
17 changes: 15 additions & 2 deletions uniffi_bindgen/src/interface/ffi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub enum FfiType {
/// or pass it to someone that will.
/// If the inner option is Some, it is the name of the external type. The bindings may need
/// to use this name to import the correct RustBuffer for that type.
RustBuffer(Option<String>),
RustBuffer(Option<ExternalFfiMetadata>),
/// A borrowed reference to some raw bytes owned by foreign language code.
/// The provider of this reference must keep it alive for the duration of the receiving call.
ForeignBytes,
Expand Down Expand Up @@ -144,13 +144,26 @@ impl From<&Type> for FfiType {
Type::External {
name,
kind: ExternalKind::DataClass,
module_path,
namespace,
..
} => FfiType::RustBuffer(Some(name.clone())),
} => FfiType::RustBuffer(Some(ExternalFfiMetadata {
name: name.clone(),
module_path: module_path.clone(),
namespace: namespace.clone(),
})),
Type::Custom { builtin, .. } => FfiType::from(builtin.as_ref()),
}
}
}

#[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
pub struct ExternalFfiMetadata {
pub name: String,
pub module_path: String,
pub namespace: String,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes a lot of sense to me, but I know that @mhammond has some plans for external types. Would this change conflict with them?

Hopefully at some point we can consolidate module_path and namespace, but it makes sense to me to have 2 fields for now since Type::External also have 2 fields.

Copy link
Member

Choose a reason for hiding this comment

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

I do want to kill Type::External and replace it with the actual type but that's my problem :)

I don't get why we need both namespace and module_path though? If the name is to the Rust ffi function namespace might be problematic - that's not known by the udl/scaffolding, so gets fixed for the binding generation - meaning rust code generated from udl might have a different value than the bindings get.

ExternalFfiMetadata or similar is probably a better name so it's not confused as a uniffi_meta struct.

Not that you should add it now, but I wonder if you will ever strike a RustArcPtr ever needing a fq-name too?

Copy link
Contributor Author

@skeet70 skeet70 Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The namespace and module_path are needed to generate the qualified type path for RustBuffer in Java. The module_path is used to get the crate to look up in config.external_packages, and the namespace is needed for the default if nothing is found.

I haven't yet needed a fully qualified RustArcPtr and looking at how it's used I don't think I will, but it's certainly possible.

Copy link
Member

Choose a reason for hiding this comment

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

Right, same as us :) But yeah, that namespace probably isn't going to work in many cases - in the case it does (eg, "single crate with udl") namespace is probably module path anyway. But yeah, I'm totally fine with carrying that all around, I just long for a time when it's less muddy.


// Needed for rust scaffolding askama template
impl From<Type> for FfiType {
fn from(ty: Type) -> Self {
Expand Down
3 changes: 2 additions & 1 deletion uniffi_testing/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,8 @@ fn get_cargo_metadata() -> Metadata {

fn get_cargo_build_messages() -> Vec<Message> {
let mut child = Command::new(env!("CARGO"))
.arg("build")
.arg("test")
.arg("--no-run")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be build --profile=dev? That seems to capture the intent better to me.

Copy link
Contributor Author

@skeet70 skeet70 Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It was already build (without --release), which should've defaulted to --profile=dev right? cdylibs of uniffi-bindgen-java dev deps weren't found on creation of the TestHelper as it was. cargo build --profile=test might work, but that feels a little less straightforward than test --no-run to me personally.

.arg("--message-format=json")
.stdout(Stdio::piped())
.spawn()
Expand Down