Add name mangling for method and type names to fit foreign language naming conventions #2

jhugman · 2020-06-22T21:30:22Z

This PR may not be useful, but starts as a way of exploring the codebase. Absenting some issues ;)

The IDL may specify method names in camel case or snake case.
The generated kotlin method names are in camel case.
The generated python method names are in snake case.
The rust method names are in snake case.

Is this useful?

If the rust devs are delivering generated APIs to App developers, then yes.
If the rust devs are delivering handmade wrappers around the generated APIs, then no.

This should probably be a candidate for project specific/language specific uniffi config.

… for methods and types

rfk · 2020-06-22T21:55:55Z

Great, thanks for diving in!! I had heck in the list of dependencies here with the intention of experimenting with this sort of thing at some point, so very glad to have you take a look.

I think it makes sense to pursue the " rust devs are delivering generated APIs to App developers" philosophy, at least to start. So being able to generate bindings that feel like they fit in the target language will be important.

I'll try to carve out some time to take a detailed look later today.

rfk

This all looks pretty reasonable to me! Some thoughts below that it would be good to get your take on, but if you think it's useful and ready to merge, I'd be happy to take it 👍

rfk · 2020-06-23T07:07:35Z

examples/naming-conventions/src/naming-conventions.idl

+
+namespace naming_conventions {
+  snake_case_object snake_case_method(u32 id);
+  CamelCaseObject camelCaseMethod(u32 id);


Interesting philosophical point here about what to allow in the IDL, and I'm kind of in two minds about it. On one hand, it seems wrong to outright error out if something declared in this file does not match a particular naming convention, but on the other hand, allowing mixing conventions like this might lead to confusion. Perhaps it's the sort of thing that might one day be a candidate for a warning.

See discussion below. I think the chance of collision is too high here.

rfk · 2020-06-23T07:10:45Z

src/bindings/kotlin/gen_kotlin.rs

@@ -283,13 +284,13 @@ internal interface _UniFFILib : Library {
 // Public interface members begin here.

 {% for e in ci.iter_enum_definitions() %}
-    enum class {{ e.name() }} {
+    enum class {{ e.name()|decl_name_kt }} {


It's probably not clear from the code, but in my head I'd originally intended the "decl" prefix on these filter function to be read as "declare type". Something like "class_name_kt" might be a clearer name for this filter (and the existing "decl_kt" might be better as something like "type_kt" for clarity).

Ok, I'll change functions as you suggest.

rfk · 2020-06-23T07:20:17Z

src/scaffolding.rs

@@ -34,7 +35,7 @@ ffi_support::define_bytebuffer_destructor!({{ ci.ffi_bytebuffer_free().name() }}
 // of items as declared in the rust code, but no harm will come from it.

 {% for e in ci.iter_enum_definitions() %}
-    unsafe impl uniffi::support::ViaFfi for {{ e.name() }} {
+    unsafe impl uniffi::support::ViaFfi for {{ e.name()|decl_name_rs }} {


Rather than doing this conversion when generating the Rust scaffolding, what do you think about the following approach:

Decree that the naming convention in the IDL should be the same as the naming convention in the Rust code (but allow users to write it differently if they want, maybe with a warning in future).

When parsing the IDL, canonicalize all names to the rust convention as part of building the ComponentInterface struct (this would be the place where we warn about it in future).

Allow each foreign language binding to choose whether and how it wants to deviate from that convention to fit in with the norms of that language.

Alternately, we could just insist that the names in Rust and IDL must match exactly, however you've written them, and not try to do any conversion.

Keeping the IDL and the Rust more tightly bound makes sense to me in a vague kind of way, especially if we imagine that one day we might general definitions using inline rust macros rather than a separate IDL file. It would also mean there are no "control knobs" for callers to fiddle with when interfacing between the IDL and the Rust code, while allowing the possibility of such knobs when generating the bindings (which there is also a placeholder Config class for in the kotlin and python bindings generators).

Thoughts?

If I understand correctly, I agree with your:

insist that the names in Rust and IDL must match
while allowing the possibility of such knobs when generating the bindings

There will be some back pressure from the hand written rust from the linter or rustc so that the rust and WebIDL will naturally tend to conform to a rust naming convention— because the rust compiler will insist, rather than uniffi. If users really want to have funky rust identifiers, they'll have to add their own annotations in rust.

The foreign language bindings are a view onto the WebIDL, so I think they should have a little bit of leeway to convert/mangle names to suite the language/project. I don't think we should do it without configuration in the long run because:

collisions are still possible, (the webIDL could define foo_bar and fooBar, which would both convert to the same methods). This might correspond to a normalize + check step before generating any bindings.

generating source-backward-compatible bindings for existing projects may still be an aim for the project.

If all this is desirable, then we should also consider doing the same for the property names.

collisions are still possible, (the webIDL could define foo_bar and fooBar, which would both convert to the same methods).
This might correspond to a normalize + check step before generating any bindings.

This is a good point; I think I'd be interested in trying out a normalize+check step when processing the IDL. In the same way that we'd error out if you tried to define two methods with exactly the same name, we could error out if you try to define two methods that would be ambiguous under common case conversions. (And it seems easier to error out now and loosen the rules later, than to start with loose rules and try to tighten them up later).

Removed name mangling in scaffolding.rs and filed #13.

rfk · 2020-06-23T07:23:43Z

What I also need to do, is figure out some basic test infrastructure here. I'd like to be able to turn the existing example.py and example.kts files into actual tests with assertions that can be executed to make sure the generated bindings are working as expected. So far I've just been running them by hand and eyeballing the output, but that's clearly not going to scale.

…tches the IDL

rfk · 2020-07-05T05:07:56Z

(heads-up that I edited this PR to target the new main branch rather than master)

rfk · 2020-07-08T06:57:04Z

Thanks for pushing on this! Just wanted to comment to set expectations, I should be able to find time to look at it by end-of-week.

rfk

The naming cleanups/consistency really made a difference here, I like the way this reads. Thanks!

rfk · 2020-07-10T06:03:14Z

src/bindings/kotlin/gen_kotlin.rs

-                      throw RuntimeException("invalid enum value, something is very wrong!!")
-                  }
+            internal fun lift(n: Int) =
+                try { values()[n - 1] }


proc-macro: Silence UnwindSafe error without Mutex

jhugman added 4 commits June 21, 2020 19:15

Treat function/method names to match the language naming conventions

6d23e07

Treat type names names to match the language naming conventions

71bb6dd

Add u32 lifting and lowering for python

a85381e

Add example/test for following naming conventions in foreign language…

b342fe7

… for methods and types

rfk reviewed Jun 23, 2020

View reviewed changes

jhugman mentioned this pull request Jun 23, 2020

Add testing infrastructure for running examples as fixtures. #5

Closed

This was referenced Jun 24, 2020

Generate Swift bindings #6

Merged

Handle name collisions with internal helpers #10

Open

jhugman added 3 commits July 3, 2020 14:44

Rename decl_name_* to class_name_* and decl_* to type_*

2a782dd

Remove mangling of names for rust scaffolding - the rust names now ma…

1bcce00

…tches the IDL

Add enum conventions for kotlin and python

ccbe38b

jhugman changed the title ~~[tentative] Add name mangling for method and type names to fit host language naming conventions~~ Add name mangling for method and type names to fit foreign language naming conventions Jul 3, 2020

jhugman mentioned this pull request Jul 3, 2020

Handle collisions resulting from name mangling #13

Open

5 tasks

rfk changed the base branch from master to main July 5, 2020 05:07

jhugman requested a review from rfk July 7, 2020 15:21

rfk approved these changes Jul 10, 2020

View reviewed changes

rfk merged commit 602c1f2 into mozilla:main Jul 10, 2020

rfk mentioned this pull request Aug 10, 2020

uniffi-bindgen should snake-case and lower-case Rust-side methods #225

Closed

rfk added a commit that referenced this pull request May 27, 2021

rfk suggestions for ADR-0005 (#2)

b17d6db

data-sync-user mentioned this pull request Jun 22, 2021

Passwords should not Sync from a profile with a Primary Password to a profile without a Primary Password #571

Closed

bendk pushed a commit that referenced this pull request Feb 27, 2023

Merge pull request #2 from matrix-org/jplatte/assert-unwind-safe

f791a89

proc-macro: Silence UnwindSafe error without Mutex

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add name mangling for method and type names to fit foreign language naming conventions #2

Add name mangling for method and type names to fit foreign language naming conventions #2

jhugman commented Jun 22, 2020

rfk commented Jun 22, 2020

rfk left a comment

rfk Jun 23, 2020

jhugman Jun 23, 2020

rfk Jun 23, 2020

jhugman Jun 23, 2020

jhugman Jul 3, 2020

rfk Jun 23, 2020

jhugman Jun 23, 2020

rfk Jun 24, 2020

jhugman Jul 7, 2020 •

edited

Loading

rfk commented Jun 23, 2020

rfk commented Jul 5, 2020

rfk commented Jul 8, 2020

rfk left a comment

rfk Jul 10, 2020

Add name mangling for method and type names to fit foreign language naming conventions #2

Add name mangling for method and type names to fit foreign language naming conventions #2

Conversation

jhugman commented Jun 22, 2020

rfk commented Jun 22, 2020

rfk 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

Choose a reason for hiding this comment

jhugman Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

rfk commented Jun 23, 2020

rfk commented Jul 5, 2020

rfk commented Jul 8, 2020

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhugman Jul 7, 2020 •

edited

Loading