google / autocxx

Tool for safe ergonomic Rust/C++ interop driven from existing C++ headers
https://docs.rs/autocxx
Apache License 2.0
2.12k stars 137 forks source link

We do not uniqueify cxx::bridge names except for functions #486

Open adetaylor opened 3 years ago

adetaylor commented 3 years ago

This error encountered on an internal codebase:

"thread '<unnamed>' panicked at 'Unable to generate header and C++ code: Syn(Error("the nameKeyis defined multiple times"))', /src/main.rs:189:18"

Working on reducing now.

adetaylor commented 3 years ago

Minimized test case:

namespace a {
namespace spanner {
class Key;
}
} // namespace a
namespace spanner {
class Key {
public:
  bool b(a::spanner::Key &);
};
} // namespace spanner
adetaylor commented 3 years ago

This problem is understood. It's because, for functions, we're using a bridge_name_tracker to ensure that entries in the (flat) cxx::bridge namespace are unique. We're not doing that for typedefs (which is what goes wrong here) not for structs/enums.

adetaylor commented 2 years ago

I'm now thinking that we should "simply" mangle all names in the cxx::bridge mod to include namespaces. To avoid conflicts in the cxx::bridge output Rust code, we'd need to avoid using #[rust_name]. We can use RustRenameStrategy::RenameInOutputMod to get them back to the desired name.

It would be ideal if we could come up with a name mangling scheme which, for simple names, happens to match user expectations. This would significantly help with debugging when people were looking through complex call stacks across languages.

e.g.

but then we would need something different for a type literally called bar_foo or a struct foo nested inside a struct bar.

So the alternative is indeed to have a bridge_name_tracker as originally planned, which allocates a new name for each thing, and that new name is stored with the thing in the API list. This remains a bit fiddly, so if we can make a deterministic mangling scheme good enough, maybe we should.

VirxEC commented 1 year ago

Just wanted to say that I'm encountering this issue with multiple definitions of cxxbridge1$new_autocxx_autocxx_wrapper in gen3.cxx and gen1.cxx as well as cxxbridge1$GetState_autocxx_wrapper in gen10.cxx and gen7.cxx.

Is it possible for the names to be mangled based on the gen # in the file name? When I cargo clean and rebuild the names of the gen files are the same, so it seems like it would be a predictable way to mangle the names. Could just append _gen3 and _gen1 to the end maybe?

adetaylor commented 1 year ago

That feels to me like a slightly different problem than the one here - the current issue is about multiple conflicting names within a single include_cpp! whereas it sounds like you have a conflict between multiple include_cpp! macro invocations. Please could you raise a new issue to avoid muddling this one?

VirxEC commented 1 year ago

Sure haha