-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minimal support for Rust type aliases #1181
base: master
Are you sure you want to change the base?
Conversation
The intention is to provide support to re-import Rust types already exported elsewhere into other bridges, creating equivalence of `extern "Rust"` types on the C++ side. Currently, the support is limited to re-exporting the type under the same name and the C++ namespace must be explicitly specified. Then, C++ functions can use the same type.
@@ -6,9 +6,10 @@ impl Api { | |||
match self { | |||
Api::CxxFunction(efn) | Api::RustFunction(efn) => &efn.name.namespace, | |||
Api::CxxType(ety) | Api::RustType(ety) => &ety.name.namespace, | |||
Api::TypeAlias(ety) => &ety.name.namespace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge the branch with the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, that's not possible, since those have different inner types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged.
if let RustType::Path(path) = &ty { | ||
// OK, we support path | ||
if let Some(last) = path.path.segments.last() { | ||
if last.ident != alias.name.rust { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we really need to prevent this - some may really want to use different names for the same Rust type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is the C++ side. Effectively, what we want to do is to publish a forward declaration of the struct in question in the C++ code in the proper namespace (that's why also the namespace should match, otherwise the types are different on C++ side - currently, this is not checked, but probably should be).
If we'd like to support renaming the object, that would be possible, but again, that would require materializing both the forward declaration and some kind of type alias (e.g., using
statement), so we would have both. The name would be accessible by both names and the original name taken anyway.
I.e., someone trying to rename the type to prevent name clashes would still end up with name clashes. Therefore, better to simply not allow renames.
The only way to do this properly would be to expose the original type in the original namespace and the alias name in a new namespace. However, due to the restriction on the Rust side, we only have access to tokens within our bridge, so there is no access to the original namespace (that's also the reason why the namespace must be re-specified).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks - I misunderstood the intention.
The intention is to provide support to re-import Rust types already exported elsewhere into other bridges, creating equivalence of
extern "Rust"
types on the C++ side.Currently, the support is limited to re-exporting the type under the same name and the C++ namespace must be explicitly specified. Then, C++ functions can use the same type.