General pipeline updates #2536
General pipeline updates #2536
Conversation
| .. | ||
| } => { | ||
| match generate_ffi_type(builtin, current_module_name) { | ||
| // Fixup `module_name` for primitive types that lower to `RustBuffer`. |
There was a problem hiding this comment.
this and the module/crate name confusion from the CI makes me sad. Isn't the module name the crate name transformed by the bindings config?
Can the bindings somehow fix this in a later stage? I don't quite get the new module node in the same way.
There was a problem hiding this comment.
It's definitely sad. The corner case here is something like:
- Crate A defines a custom URL type that lowers to a string
- Crate B uses URL as an external type
- When determining the FFI type, we start by getting the FFI type for the builtin. In this case the builtin type is string, so the FfiType is
FfiType::RustBuffer(None). - However, since it's an external type used in crate B, we need to change that to
FfiType::RustBuffer(Some("A"))
The core of this issue is the fact that we're creating multiple RustBuffer types, which means the custom URL type has 2 different ffi types, depending on the module FfiType::RustBuffer(None) vs FfiType::RustBuffer(Some("A")). Part of this is that FfiTypeRustBuffer(None) is special cased to mean "the RustBuffer from current module", which doesn't work well with external types.
Maybe it would be slightly cleaner to make the module name non-optional for RustBuffer. However, that's still pretty confusing. It means that when we're determining FFI types, we always need to know the "current module" and that module will change once you recurse inside an external type.
I think the real solution to this is having a single RustBuffer type that all modules share. At this point I'm very confident we can do that with this pipeline code.
| ### Field Conversion order | ||
|
|
||
| Fields are converted in declaration order. | ||
| This matters when a field one uses `#[node(from(<name_of_the_another_field>)]`. |
There was a problem hiding this comment.
This is much clearer, thanks!
Maybe "This matters when a field is listed twice with one of them using #[node(from(<name_of_the_another_field>)]"?
(I'm still not quite sure what the semantics would be of the #[node(from(<name_of_the_another_field>)] field was listed second and the non-option item was listed first - how does the option get converted to the non-option there?
There was a problem hiding this comment.
That wording sounds good, thanks.
(I'm still not quite sure what the semantics would be of the #[node(from(<name_of_the_another_field>)] field was listed second and the non-option item was listed first - how does the option get converted to the non-option there?
That would be the same as the general case of converting an optional value into a non-option. It will always fail -- even if there were no None values (I'm not 100% sure on that, but I'm pretty sure that's how it should work).
This were the result of me implementing the Python bindings using the new pipeline system. There were a handful of things I realized I needed/wanted there that didn't come up when I was implementing the JS bindings. Add `FfiTypeNode`. I didn't originally have this as part of the general pipeline, since it wasn't needed there. However, virtually all language pipelines are going to want to add it so we may as well do it in shared code. Determine enum discriminants for the bindings. This uses basically the same algorithm as from `ComponentInterface`. The one difference is that for enums without a specified `repr`, this calculates the smallest integer width needed to store the variants. I used a new pipeline technique for the enum discriminents: the old/optional `discr` field gets renamed to `meta_discr` so that we can create a new/non-optional `discr` field. Adding the `Node::has_descendant` method. I've noticed I'm implementing this code again and again, so it feels a good time to generalize. Several other smaller changes/fixes.
bendk
force-pushed
the
push-uslvmlqkwwkx
branch
2 times, most recently
from
0944997 to
89db902
Compare
This were the result of me implementing the Python bindings using the new pipeline system. There were a handful of things I realized I needed/wanted there that didn't come up when I was implementing the JS bindings.
Add
FfiTypeNode. I didn't originally have this as part of the general pipeline, since it wasn't needed there. However, virtually all language pipelines are going to want to add it so we may as well do it in shared code.Determine enum discriminants for the bindings. This uses basically the same algorithm as from
ComponentInterface. The one difference is that for enums without a specifiedrepr, this calculates the smallest integer width needed to store the variants.I used a new pipeline technique for the enum discriminents: the old/optional
discrfield gets renamed tometa_discrso that we can create a new/non-optionaldiscrfield.Adding the
Node::has_descendantmethod. I've noticed I'm implementing this code again and again, so it feels a good time to generalize.Several other smaller changes/fixes.