Use `FfiConverter` for metadata generation/storage #1469

bendk · 2023-02-03T21:16:08Z

This PR makes it so proc-macros aren't responsible for parsing type names to figure out information about the underlying type. Parsing type names is complicated and impossible to get 100% correct since users can use type aliases.

The main motivation for this is to try out the macro code in application-services. Currently, we can't use them because we type alias our Result<> types.

The proc-macros needed to parse type names was to generate the interface metadata and store it in the binary. Now the proc-macros generate const functions/expressions to create the static variables. A key part here is that FfiConverter now stores a const META_TYPE_ID value, which identifies the type.

The other reason the proc-macros parsed the type names was to handle function returns. We had two different code paths for this, depending on if the return type was a Result<> or not. This changes things to only have 1 code path, which means the proc-macros can ignore this issue.

This is a WIP because there's still a couple of todo items:

Update the checksum code. One side-effect of the changes is that we no longer can use the checksum in the FFI function name. If we decide to go down this route then we need a different system for checksums. I think what we can do is to store the checksum in a separate symbol, then have the bindings check that value against their expected checksum when loading the library.
Make sure that the uitests error messages aren't going to change each release

jplatte · 2023-02-03T21:47:20Z

Based on the PR description, this sounds really great! Going to review next week.

uniffi_core/src/lib.rs

mhammond

Far out, this is a big patch and I didn't get through it - but there's alot to like here. I'll get back to it soon, but @jplatte's review is probably the one you should care about :)

docs/manual/src/internals/object_references.md

fixtures/keywords/rust/src/lib.rs

fixtures/metadata/src/tests.rs

fixtures/uitests/tests/ui/non_hashable_record_key.stderr

uniffi_core/src/lib.rs

jplatte · 2023-02-10T12:48:13Z

Still on my todo, but I didn't manage to get to reviewing this week.

bendk · 2023-03-08T22:17:15Z

Rebased this based on the async changes as well as the macro changes. @jplatte do you think you can review this soon? I'm really hoping to get it merged then test out macros with the application-services code.

jplatte · 2023-03-10T19:18:50Z

I'll try.. 🥲
Will start on mobile now 😅

jplatte

Looks really really good! Left some nitpicks.

fixtures/metadata/Cargo.toml

uniffi_bindgen/src/interface/error.rs

uniffi_bindgen/src/scaffolding/templates/namespace_metadata.rs

uniffi_core/src/lib.rs

uniffi_meta/src/lib.rs

uniffi_meta/src/reader.rs

jplatte · 2023-03-14T16:42:30Z

With metadata being properly attributed to a namespaced type, is there any reason we should keep the one-crate restriction when generating bindings by the way?

bendk · 2023-03-16T23:32:05Z

With metadata being properly attributed to a namespaced type, is there any reason we should keep the one-crate restriction when generating bindings by the way?

I hope not, in fact that's one of the first things I want to test when this gets merged.

bendk · 2023-03-17T00:05:27Z

Thanks for the review! I updated the code based on your suggestions and also made one last refactoring for the rustfuture code.

I think the last step for this is figuring out the checksum code, I hope to have that out soon.

badboy

Didn't get through the important bits yet (but as Mark said: Jonas' review is the important one).
Minor nitpicks in wording, I'll get back to this tonight or tomorrow.

uniffi_core/src/lib.rs

uniffi_core/src/metadata.rs

jplatte

🚢 it

uniffi_bindgen/src/interface/error.rs

jplatte · 2023-03-20T14:12:31Z

I think the last step for this is figuring out the checksum code, I hope to have that out soon.

So something is still off then? CI is green 😄
Let me know if I can help :)

uniffi_bindgen/src/scaffolding/templates/namespace_metadata.rs

bendk · 2023-03-29T18:29:24Z

I just pushed what's hopefully the last bit of functionality here and removed the [WIP] label, I think this one is maybe ready to go now.

The last commit implemented a new checksum system, which the new metadata system required. I'd love feedback from on this one. It also updated the proc-macro code slightly, @jplatte do the changes look okay to you?

There's LOC count is very high on this one, but there are some reasons to be okay with that:

A big portion is the proc-macro system which is somewhat experimental and @jplatte has approved those
The other main change is the checksum code, but it's accompanied by scripts that can be used to verify what happens when the checksum is off. (Also those test scripts make up a large number of lines).

FWIW, here's some sample output of the checksum scripts. This shows an example of what a checksum error will look like with these changes:

+ python3 python_test.py
Traceback (most recent call last):
  File "/home/bdk/uniffi-rs/ffi-converter-metadata/target/version-mismatch-workdir/python_test.py", line 1, in <module>
    from fixture_version_mismatch import *
  File "/home/bdk/uniffi-rs/ffi-converter-metadata/target/version-mismatch-workdir/fixture_version_mismatch.py", line 381, in <module>
    _UniFFILib = loadIndirect()
  File "/home/bdk/uniffi-rs/ffi-converter-metadata/target/version-mismatch-workdir/fixture_version_mismatch.py", line 361, in loadIndirect
    uniffi_check_api_checksums(lib)
  File "/home/bdk/uniffi-rs/ffi-converter-metadata/target/version-mismatch-workdir/fixture_version_mismatch.py", line 376, in uniffi_check_api_checksums
    raise InternalError("UniFFI API checksum mismatch: try cleaning and rebuilding your project")
fixture_version_mismatch.InternalError: UniFFI API checksum mismatch: try cleaning and rebuilding your project

mhammond

Wow, yeah, this is a big patch :) It's a bit of a shame the checksum code has grown a fair bit of complexity, but I won't pretend to understand everything well enough to try and suggest it's not necessary. So I'm still +1 on getting this landed sooner rather than later. Note there remain a number of unresolved comments which should probably get addressed before landing too.

docs/manual/src/internals/object_references.md

bendk · 2023-03-30T15:00:44Z

I think I resolved all the comments.

I think the reason for the extra checksum code complexity is we now have to support 2 systems of checksums. I think there's a way for the UDL-based code to use the same metadata buffer checksum, but I didn't want to put in the extra time now to implement that. I think this could be a future follow-up though -- or maybe we just have to wait until we can finally replace all UDL use with proc-macros.

jplatte · 2023-03-30T15:03:41Z

I don't fully understand why module_path!() didn't work out, it's a bit sad that we're back to the other solution. But it's probably fine.

There is just thing I'm curious about, if you declare two identical types (that derive uniffi::Record or uniffi::Enum) with the same name in different modules of one crate, what will happen? They should get exactly identical metadata now, I think (but that might not be a problem).

bendk · 2023-03-30T15:20:17Z

There is just thing I'm curious about, if you declare two identical types (that derive uniffi::Record or uniffi::Enum) with the same name in different modules of one crate, what will happen? They should get exactly identical metadata now, I think (but that might not be a problem).

The crate won't compile, because the metadata symbols will collide. I think we need some way of getting the module path in the proc-macro in order to support this.

uniffi-rs/uniffi_macros/src/util.rs

Lines 155 to 169 in 4c47478

    
           let const_ident = 
        
               format_ident!("UNIFFI_META_CONST_{crate_name_upper}_{kind_upper}_{name_upper}"); 
        
           let static_ident = format_ident!("UNIFFI_META_{crate_name_upper}_{kind_upper}_{name_upper}"); 
        
           let checksum_fn = checksum_fn_name.map(|name| { 
        
               let ident = Ident::new(&name, Span::call_site()); 
        
               quote! { 
        
                   #[doc(hidden)] 
        
                   #[no_mangle] 
        
                   pub extern "C" fn #ident() -> u16 { 
        
                       #const_ident.checksum() 
        
                   } 
        
               } 
        
           });

bendk · 2023-03-30T15:21:16Z

I think it would work on nightly though, since the mod_path() function there actually gives you the full path. However, I haven't tested.

- Added the `uniffi_core::metadata` module, which provides a system for building metadata buffers using const code. - Added the `FfiConverter::TYPE_ID_META` const, which holds metadata to identify the type. - Made the proc-macros generate use the new system to generate metadata consts, then export them using static variables. - Removed the current code to generate/store metadata based on the syn parsing. - Removed the type assertions and the requirement for a `uniffi_types` module. We don't need them now that we`re getting the type ids from the type itself. - Made the `FnMetadata.throws` field a Type rather than a String. - Calculate module paths with the `module_path!()` macro. This means we get accurate module paths without nightly. Changed mod_path to be a String, rather than a Vec since this is what `module_path!()` outputs. - Added code to strip the `r#` part out of raw idents before serializing it into the metadata. - Replaced the `collect_params()` function with the `ScaffoldingBits` struct. There was too much complexity for a single function -- for example unzip() only works with pairs, not 3-tuples. In general, the new metadata system is more reliable doing everything in the proc-macros. Proc-macros can only see the type identifiers, but they don't anything about the underlying type, since users can rename types with type aliases. It's also simpler since you don't have to walk the AST so much. One TODO is getting checksum working again. One limitation of the const code is that we can't use it to generate function identifiers.

Added the `FfiConverter::lower_return` method. This is like `lower()` but specialized for scaffolding returns. This allows us to always use a single function to handle scaffolding calls, rather than `call_with_output` or `call_with_result` depending on if the return type is a `Result<>` or not. Having a single code-path for return handling simplifies the code generation, especially the macros. We no longer need to try to parse `Result<>` type names. This is especially useful for crates that type-alias their `Result<>` types. Updated the async code to work with the new system. Now `RustFuture` only needs 1 generic argument. Updated `object_references.md` and removed example code that's no longer valid. Replaced it with higher-level descriptions of what's going on, hopefully this will stay not get out of date as quickly.

Define a lower-level version of `rust_call()` that `uniffi_rustfuture_poll` can use without going through so many hoops.

Updated the checksum handling to work with the new proc-macro metadata system. Now that the metadata is calcualted using const functions rather than inside the macro code, we don't have a way to include the checksum in the exported symbol name. Instead, calculate the metadata and store it as its own symbol. Then the bindings code checks the checksums on startup as well as the contract ABI version. One nice aspect of this is that we can output a nicer error message than just "linker error". - Added scripts to test this. - Added flag to allow FFIFunctions to not always have the `&mut RustCallStatus` arg. - Changed the contract version to be a u32 rather than a &str. This simplifies the bindings code. - Added the `uniffi_meta::ffi_names` module. This contains the canonical code to generate ffi symbol names. - Use the module path that we calculate in the proc-macro when generating the metadata rather than calling `module_path!()`. `module_path!()` returns the full module path, but it's more important that our metadata agrees with the module path we used whene generating the scaffolding. - Reworked the `metadata::reader` code - Make `uniffi_meta` depend on `uniffi_core`. I was trying to avoid this, but uniffi_core is where the metadata buffer code lives and I don't want to duplicate it. Maybe we could make this a separate crate, but it doesn't seem worth it yet since `uniffi_core` is fairly small.

bendk · 2023-03-31T15:27:39Z

Let's merge this one, I'm excited to try it out with app-services.

PR mozilla#1469 introduced an incompatibility with a local `type Result<T> = std::result::Result<T, MyError>` type alias. As can be seen with the included test without the fix applied: ``` error[E0107]: this type alias takes 1 generic argument but 2 generic arguments were supplied --> /mounted_workdir/target/debug/build/unary-result-alias-f408c03f1955dadd/out/unary-result-alias.uniffi.rs:79:1 | 79 | / #[::uniffi::ffi_converter_error( 80 | | tag = crate::UniFfiTag, 81 | | flat_error, 82 | | 83 | | )] | | ^ | | | | |__expected 1 generic argument | help: remove this generic argument | note: type alias defined here, with 1 generic parameter: `T` --> fixtures/regressions/unary-result-alias/src/lib.rs:7:10 | 7 | pub type Result<T> = std::result::Result<T, MyError>; | ^^^^^^ - = note: this error originates in the macro `::uniffi::ffi_converter_default_return` which comes from the expansion of the attribute macro `::uniffi::ffi_converter_error` (in Nightly builds, run with -Z macro-backtrace for more info) ``` Fixing the macros in `uniffi_core` to use `::std::result::Result` fixes that. This exposes a further case in the generated code: ``` error[E0107]: this type alias takes 1 generic argument but 2 generic arguments were supplied --> /mounted_workdir/target/debug/build/unary-result-alias-f408c03f1955dadd/out/unary-result-alias.uniffi.rs:108:40 | 108 | uniffi::rust_call(call_status, || <Result<(), r#MyError> as ::uniffi::FfiConverter<crate::UniFfiTag>>::lower_return( | ^^^^^^ --------- help: remove this generic argument | | | expected 1 generic argument | note: type alias defined here, with 1 generic parameter: `T` --> fixtures/regressions/unary-result-alias/src/lib.rs:7:10 | 7 | pub type Result<T> = std::result::Result<T, MyError>; | ^^^^^^ - ``` So the same change is required to the output produced by bindgen.

Post-#1469 cleanup

bendk requested a review from a team as a code owner February 3, 2023 21:16

bendk requested review from badboy and removed request for a team February 3, 2023 21:16

bendk force-pushed the ffi-converter-metadata branch from 9c7b013 to 057a2f5 Compare February 3, 2023 21:42

bendk requested review from mhammond and jplatte February 3, 2023 21:44

bendk commented Feb 3, 2023

View reviewed changes

uniffi_core/src/lib.rs Show resolved Hide resolved

mhammond reviewed Feb 7, 2023

View reviewed changes

bendk force-pushed the ffi-converter-metadata branch from 057a2f5 to 36b5126 Compare February 7, 2023 22:28

bendk mentioned this pull request Feb 9, 2023

feat: Add support for async/Future #1409

Merged

41 tasks

bendk force-pushed the ffi-converter-metadata branch 3 times, most recently from a1096fb to b48013b Compare March 8, 2023 22:17

jplatte approved these changes Mar 11, 2023

View reviewed changes

bendk force-pushed the ffi-converter-metadata branch from b48013b to e32d181 Compare March 17, 2023 00:04

badboy reviewed Mar 20, 2023

View reviewed changes

jplatte mentioned this pull request Mar 20, 2023

Remove uniffi_api modules matrix-org/matrix-rust-sdk#1682

Merged

jplatte approved these changes Mar 20, 2023

View reviewed changes

uniffi_bindgen/src/interface/error.rs Outdated Show resolved Hide resolved

jplatte reviewed Mar 24, 2023

View reviewed changes

uniffi_bindgen/src/scaffolding/templates/namespace_metadata.rs Outdated Show resolved Hide resolved

bendk force-pushed the ffi-converter-metadata branch from e32d181 to b90f253 Compare March 29, 2023 18:19

bendk changed the title ~~[WIP] Use FfiConverter for metadata generation/storage~~ Use FfiConverter for metadata generation/storage Mar 29, 2023

bendk force-pushed the ffi-converter-metadata branch from b90f253 to d8a85b0 Compare March 29, 2023 18:28

bendk mentioned this pull request Mar 29, 2023

[Propsal] Updating FFI naming and checksums #1271

Closed

mhammond approved these changes Mar 30, 2023

View reviewed changes

docs/manual/src/internals/object_references.md Outdated Show resolved Hide resolved

docs/manual/src/internals/object_references.md Outdated Show resolved Hide resolved

docs/manual/src/internals/object_references.md Outdated Show resolved Hide resolved

bendk added 6 commits March 31, 2023 11:13

Fixes from review.

e1fcb39

Refactoring the rustcall/rustfuture code

6bd31ad

Define a lower-level version of `rust_call()` that `uniffi_rustfuture_poll` can use without going through so many hoops.

Fixes from review

e8e5ff6

bendk force-pushed the ffi-converter-metadata branch from 4c47478 to e8e5ff6 Compare March 31, 2023 15:13

bendk merged commit e8e5ff6 into mozilla:main Mar 31, 2023

bendk mentioned this pull request Mar 31, 2023

Add "return_type()" to Constructor #1490

Closed

ijc mentioned this pull request Apr 6, 2023

Fully qualify uses of Result in scaffolding/macro code #1510

Merged

ijc mentioned this pull request Apr 6, 2023

Ensure bridging header function prototypes are valid #1512

Merged

bendk added a commit that referenced this pull request Apr 6, 2023

Merge pull request #1509 from matrix-org/jplatte/rhash

58f4f5a

Post-#1469 cleanup

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `FfiConverter` for metadata generation/storage #1469

Use `FfiConverter` for metadata generation/storage #1469

bendk commented Feb 3, 2023 •

edited

Loading

jplatte commented Feb 3, 2023

mhammond left a comment

jplatte commented Feb 10, 2023

bendk commented Mar 8, 2023

jplatte commented Mar 10, 2023

jplatte left a comment •

edited

Loading

jplatte commented Mar 14, 2023

bendk commented Mar 16, 2023

bendk commented Mar 17, 2023

badboy left a comment

jplatte left a comment

jplatte commented Mar 20, 2023

bendk commented Mar 29, 2023

mhammond left a comment

bendk commented Mar 30, 2023

jplatte commented Mar 30, 2023 •

edited

Loading

bendk commented Mar 30, 2023

bendk commented Mar 30, 2023

bendk commented Mar 31, 2023

Use FfiConverter for metadata generation/storage #1469

Use FfiConverter for metadata generation/storage #1469

Conversation

bendk commented Feb 3, 2023 • edited Loading

jplatte commented Feb 3, 2023

mhammond left a comment

Choose a reason for hiding this comment

jplatte commented Feb 10, 2023

bendk commented Mar 8, 2023

jplatte commented Mar 10, 2023

jplatte left a comment • edited Loading

Choose a reason for hiding this comment

jplatte commented Mar 14, 2023

bendk commented Mar 16, 2023

bendk commented Mar 17, 2023

badboy left a comment

Choose a reason for hiding this comment

jplatte left a comment

Choose a reason for hiding this comment

jplatte commented Mar 20, 2023

bendk commented Mar 29, 2023

mhammond left a comment

Choose a reason for hiding this comment

bendk commented Mar 30, 2023

jplatte commented Mar 30, 2023 • edited Loading

bendk commented Mar 30, 2023

bendk commented Mar 30, 2023

bendk commented Mar 31, 2023

Use `FfiConverter` for metadata generation/storage #1469

Use `FfiConverter` for metadata generation/storage #1469

bendk commented Feb 3, 2023 •

edited

Loading

jplatte left a comment •

edited

Loading

jplatte commented Mar 30, 2023 •

edited

Loading