make mod_path() consistent with/without nightly feature

[bobozaur]

The nightly version of the mod_path() function does not do the same thing as the one reading Cargo.toml.

We'll only ever get the crate name from Cargo.toml, but the core::module_path!() macro will return the module path, with components separated by ::, as the Rust convention goes.

The problem is that the crate name is the first element of core::module_path!() macro, and the current impl does not consider that. This might not have been visible if tested from the lib.rs of a crate, since the module_path == crate_name, but UniFFI objects in nested modules will pose issues.

If the desire is to hold on to the full module path, then we could at least replace the :: with _ as otherwise syn complains that identifiers are not valid. Let me know if that's a better approach (though I think the behavior should be consistent with the non-nightly version).

[jplatte]

The current implementation returns a module path with that feature on purpose, that's why the function is named as it is named. It should not be changed to flatten that into a single identifier.

If the scaffolding generation doesn't work with the nightly feature, that's a bug. If some backend doesn't work with the nightly feature, you can try to adjust the backend to support it.

The intent with this feature was to allow uniffi to generate bindings that consist of multiple modules, mirroring the Rust-side structure.
Maybe there should be an extra codegen-time runtime flag for this though, so users have to opt in to this more explicitly.

[bobozaur]

The current implementation returns a module path with that feature on purpose, that's why the function is named as it is named. It should not be changed to flatten that into a single identifier.

That's why I provided the alternative. It's not a backend issue, but a scaffolding issue. As I described, identifiers seem to be created through the syn crate and this will especially fail on function names because the mod_path is used as namespace and something like format!("uniffi_{namespace}_fn_init_callback_vtable_{callback_interface_name}") won't play nice with my_crate::some_module::some_other_module.

I'll adjust the PR to use the proposed alternative, which is replacing the columns with an underscore.

[jplatte]

Sorry, but this still changes the function when the bug is in some of its usages. Specifically it seems like ffi_names.rs is treating the module path like something that can be interpolated into a symbol name verbatim, and the same seems to be true in setup_scaffolding.rs for some format_ident! invocations.

All of the other uses of this function are correct and should stay the way they are.

I think there used to be some code there that replaced :: with __ (double underscore), to make it a little more bullet-proof against name collisions. I'm guessing a refactoring changed that.

[bobozaur]

@jplatte This seems to be the commit that changed this. The return type used to be a Vec of path components and was changed to a regular String.

I can take this on but since my ideas do not seem right, what's the expected behavior here? Should mod_path() go back to return a Vec? If so, should the items then be concatenated with underscores to create the namespace?

How certain are you that the other usages of the function are correct?

[jplatte]

I don't think it would be necessary to go back to Vec<_>, you could also .replace("::", "__") in the relevant places.

Anyways, let's wait to hear what @bendk thinks. Good thing he authored that change, because he should be the most knowledgable person on the scaffolding generation in general :)

[bobozaur]

@jplatte I followed your advice and only normalized the usage of mod_path() in function names. Hopefully this is satisfactory 😄 .

I assume the reason the module path is meant to be used as-is is that the MetadataBuffer is meant to display the module path with the double colon notation. To be fair, I did encounter this for function names, so this makes sense.

Thanks for the guidance! Let me know if there's something else to change.