Add initial proc-macro frontend bits #1259

jplatte · 2022-05-30T16:33:57Z

Based on #1258.
First part of #1257.

jplatte · 2022-05-31T14:56:10Z

There's also the issue that the proc-macros having their own output files as tracked files currently seems to result in cargo always considering them to require recompilation.

I have an idea for a fix, and will try to remember pushing it as a separate commit to make review easier.

jplatte · 2022-06-02T11:52:16Z

Now there's one clear person responsible for reviewing or tagging the next person. Still, I feel like I should cc one more person (@badboy) just so everyone involved has a chance of checking out what my idea discussed over the last week looks like in code.

badboy · 2022-06-02T13:29:16Z

Will give it a look.

bendk

This is really exciting. The general direction seems good to me, I left a few comments about specifics, but I like the way it's going overall.

bendk · 2022-06-02T16:19:37Z

uniffi_bindgen/src/interface/function.rs

+
+impl From<uniffi_meta::FnMetadata> for Function {
+    fn from(meta: uniffi_meta::FnMetadata) -> Self {
+        let checksum = uniffi_meta::checksum(&meta);


This makes checksums specific to a scaffolding function, rather than based on entire interface. This seems like it will work fine and I think it matches the decision from the planning meeting. Can you confirm @tarikeshaq?

Yup! I haven't gotten the time to write it up just yet but having checksums be per function is pretty much what we talked about

We landed on per type checksums + an overall ComponentInterface schema version of some sort

bendk · 2022-06-02T16:22:53Z

uniffi_bindgen/src/interface/function.rs

+            default: None,
+        }
+    }
+}


These conversion functions seem like a good starting point, but it makes me wonder if we could eventually use the uniffi_meta types directly. The fact that they implement Serialize opens the door to using them with other templating engines like handlebars or terra.

My thinking was that this would be a good starting point that would work well while the UDL stuff still exists as well. I think it makes sense to remove the other types after the proc-macro has fully replaced UDL.

bendk · 2022-06-02T16:35:50Z

uniffi_bindgen/src/interface/mod.rs

+        // influenced by things added later from proc-macro metadata. Those have their own
+        // namespacing mechanism.
+        assert!(!ci.namespace.is_empty());
+        ci.ffi_namespace = format!("{}_{:x}", ci.namespace, ci.checksum());


Maybe it's time to drop the interface-wide namespace?

For user functions/methods/callback interfaces, I like your system of using a checksum that's just based on the function args. It seems like we could do that regardless of if we're using UDL or a proc-macro.

For the uniffi functions like rustbuffer_alloc, maybe we can skip the checksum? I doubt we're going to need to change the signatures for those much and if we do we can pick a new name. BTW, I like you were prefixing everything with __uniffi, maybe we could use names like __uniffi__rustbuffer_alloc

If there's no interface-wide namespace, then the derive_ffi_funcs() methods might not be needed since the names don't depend on the ci_prefix.

I think this could be done in a separate PR that could be merged now.

I think this could be done in a separate PR

Definitely. Doesn't really matter whether it would be merged before or after this PR, right?

After thinking about this more, the issue with dropping the interface-wide namespace is trying to combine multiple UniFFI crates in the same .so, like we currently do. In that case, you need each rustbuffer_alloc function to have a different name (for now at least, at some point maybe we could figure out a way to avoid this duplication). Also, if 2 functions from different UniFFIed crates have the same name and signature then we would get a collision.

It's unfortunate that derive_ffi_funcs() needs to stay and the way we generate the prefix differs between the UDL approach and the macro-based approach, but I think we can live with that.

uniffi_macros/src/util.rs

uniffi_macros/src/export/scaffolding.rs

badboy

Indeed exciting. And much more self-contained than I would have expected.
And even for those things where I thought it might be tricky (placement of the json files, ...) it doesn't seem to be an issue.
👏

Only a couple comments from my side, on top of what's already been reported.

uniffi_bindgen/src/macro_metadata.rs

uniffi_macros/Cargo.toml

uniffi_macros/src/export/metadata/function.rs

uniffi_macros/src/util.rs

bendk · 2022-06-03T14:24:05Z

uniffi_macros/src/export/metadata.rs

+            Span::call_site(),
+            "impl blocks are not yet supported",
+        )),
+        // FIXME: Support const / static?


What do you think about supporting a module here as well? That would be equivalent to wrapping each item in the module.

Some users like how the UDL lets you see the entire API in one place. In a proc-macro world, I think that would translate to a single ffi module with the uniffi:export attribute.

I guess it could work 🤷🏼

Doing it well (w.r.t. error handling) would be a decent amount of work though, I think.

uniffi_meta/src/lib.rs

jplatte · 2022-06-09T08:14:37Z

Regarding the nightly feature, I guess the JSON format being stable might not be that important? I could replace the mod_path key by crate_name and possibly even remove the nightly feature for now, if you prefer. There's really nothing to it other than future proofing, and reducing fs operations (which seem more hacky to me than what happens with the nightly feature turned on).

Is that the only thing that needs to be resolved before this can be merged?

bendk · 2022-06-09T23:28:05Z

Regarding the nightly feature, I guess the JSON format being stable might not be that important? I could replace the mod_path key by crate_name and possibly even remove the nightly feature for now, if you prefer. There's really nothing to it other than future proofing, and reducing fs operations (which seem more hacky to me than what happens with the nightly feature turned on).

I think it's fine if it evolves as you develop this feature.

Is that the only thing that needs to be resolved before this can be merged?

I think everyone needs to be on board with the plan of developing this in main rather than a branch. I'm more-or-less fine with that plan, but maybe I'd like to get agreement on the checksum change. I'll try to put together a separate PR for that, so it's clear exactly what the change is. I should be able to get that ready tomorrow.

That said, if not having it merged is holding you back, then I'd be okay with merging it right now.

bendk · 2022-06-10T13:58:53Z

So, the question is if it's okay to commit to merging this into main. The biggest risk is that we find out down the line that something isn't going to work out and need to back out the code. I think we've thought this one through pretty well, so that risk is quite small. Also, most of the changes are self-contained like @badboy mentioned. But I'm just going to flag some more devs just in case (@jhugman @skhamis @rfk)

The main question I have is the FFI naming change. I made #1271 which focuses on just that. Does that plan seem good to others?

Are there any other concerns that should block this from being merged?

`from_webidl` is more explicit and hardly less convenient.

jplatte · 2022-06-15T08:22:41Z

Rebased to fix merge conflicts. The last two commits are new and address more recent review feedback. I guess this is now waiting for #1271 either way though, right?

bendk · 2022-06-15T17:24:00Z

Rebased to fix merge conflicts. The last two commits are new and address more recent review feedback. I guess this is now waiting for #1271 either way though, right?

The feedback from #1271 has been positive, I think we can be pretty confident on the direction of the naming changes.

I don't see any concerns against this PR, so I'm just going to go ahead and merge it so that we can continue to iterate.

jplatte force-pushed the jplatte/proc-macro-init branch 4 times, most recently from a5cf590 to 07a8b2c Compare May 31, 2022 14:37

jplatte marked this pull request as ready for review May 31, 2022 14:52

This comment was marked as resolved.

Sign in to view

jplatte requested review from bendk, rfk, jhugman and mhammond May 31, 2022 14:54

jplatte force-pushed the jplatte/proc-macro-init branch 2 times, most recently from 8c61fa2 to 3b6c2cf Compare June 1, 2022 10:48

jplatte removed request for rfk, jhugman and bendk June 2, 2022 11:50

badboy self-requested a review June 2, 2022 13:29

bendk reviewed Jun 2, 2022

View reviewed changes

jplatte mentioned this pull request Jun 3, 2022

Make use of implicit format string arguments #1266

Merged

badboy reviewed Jun 3, 2022

View reviewed changes

uniffi_bindgen/src/macro_metadata.rs Outdated Show resolved Hide resolved

uniffi_macros/Cargo.toml Show resolved Hide resolved

uniffi_macros/src/export/metadata/function.rs Outdated Show resolved Hide resolved

uniffi_macros/src/util.rs Show resolved Hide resolved

bendk reviewed Jun 3, 2022

View reviewed changes

bendk reviewed Jun 7, 2022

View reviewed changes

uniffi_meta/src/lib.rs Outdated Show resolved Hide resolved

bendk mentioned this pull request Jun 10, 2022

[Propsal] Updating FFI naming and checksums #1271

Closed

jplatte added 2 commits June 15, 2022 10:07

Remove impl FromStr for ComponentInterface

8740014

`from_webidl` is more explicit and hardly less convenient.

Add #[uniffi::export] attribute with support for non-impl functions

11e0a45

jplatte added 5 commits June 15, 2022 10:10

Add proc-macro test fixture with simple function calls

2fba5ff

Centralize ffi function name building for proc-macro frontend

0f13e8c

Address some review feedback

5def1c5

Document nightly feature

501973c

Don't start symbol names with double underscores

500eb7f

jplatte force-pushed the jplatte/proc-macro-init branch from 9bf4f0e to 500eb7f Compare June 15, 2022 08:21

jplatte requested a review from a team as a code owner June 15, 2022 08:21

Add missing copyright headers

b8c82f9

jplatte force-pushed the jplatte/proc-macro-init branch from c72d06e to b8c82f9 Compare June 15, 2022 11:39

bendk approved these changes Jun 15, 2022

View reviewed changes

bendk merged commit 534499f into mozilla:main Jun 15, 2022

jplatte deleted the jplatte/proc-macro-init branch June 15, 2022 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial proc-macro frontend bits #1259

Add initial proc-macro frontend bits #1259

jplatte commented May 30, 2022 •

edited

Loading

This comment was marked as resolved.

jplatte commented May 31, 2022

jplatte commented Jun 2, 2022 •

edited

Loading

badboy commented Jun 2, 2022

bendk left a comment

bendk Jun 2, 2022

tarikeshaq Jun 3, 2022

bendk Jun 2, 2022

jplatte Jun 2, 2022

bendk Jun 2, 2022

jplatte Jun 3, 2022

bendk Jun 9, 2022

badboy left a comment

bendk Jun 3, 2022

jplatte Jun 3, 2022

jplatte commented Jun 9, 2022

bendk commented Jun 9, 2022

bendk commented Jun 10, 2022

jplatte commented Jun 15, 2022

bendk commented Jun 15, 2022

Add initial proc-macro frontend bits #1259

Add initial proc-macro frontend bits #1259

Conversation

jplatte commented May 30, 2022 • edited Loading

This comment was marked as resolved.

jplatte commented May 31, 2022

jplatte commented Jun 2, 2022 • edited Loading

badboy commented Jun 2, 2022

bendk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte commented Jun 9, 2022

bendk commented Jun 9, 2022

bendk commented Jun 10, 2022

jplatte commented Jun 15, 2022

bendk commented Jun 15, 2022

jplatte commented May 30, 2022 •

edited

Loading

jplatte commented Jun 2, 2022 •

edited

Loading