Reimplement Python bindings using the pipeline code #2537
Conversation
bendk
force-pushed
the
push-xyuwnlsxurkk
branch
3 times, most recently
from
76c7085 to
3c000af
Compare
| #[derive(Debug, Clone, Node)] | ||
| pub struct Callable { | ||
| pub name: String, | ||
| pub is_async: bool, |
There was a problem hiding this comment.
why do all the types above have a Callable but also their own copies of the fields?
There was a problem hiding this comment.
I've gone back and forth one this a lot, I'm not sure what's best.
The initial -> general pass is where Callable is added. We need the extra copies to generate the Callable data. In a previous version of the pipeline code, I had another general pass which dropped the unneeded fields after we had created Callable from them. However, it felt to me like the extra pass was adding more complexity than it was worth and it was better to have a policy that passes added fields, but didn't remove them.
For the general -> python pass, we could easily drop the fields but I didn't because it felt consistent with the "policy". However, I'm not loving this policy anymore, it seems kind of silly to keep these fields around in the python pass and also introduces the possibility of errors if a pass changes one of the fields but not the other.
There was a problem hiding this comment.
introduces the possibility of errors if a pass changes one of the fields but not the other.
yeah, this is exactly my concern - it's not clear which is the correct one to use. I admit I don't have an understanding of what the complexity cost is, but allowing redundant fields (or, eg, as noted above, fields which might be objectively wrong to use in bindings) does seem like a footgun.
There was a problem hiding this comment.
That makes sense, I removed those fields. This seems like the best balance.
Note: there's still some duplication here, for fields added in the Python passes. For example, Interface.methods and Interface.protocol.methods are the same list. It's not great, but I can't see a simple way to avoid this. I think the best way would be adding module like nodes2.rs, which doesn't seem great. I'm thinking the general rule is that it's okay to have duplicates when your pass adds new fields, but the pass after that should drop the duplicates. In general, I guess we should try to be practical and not try for a one-size-fits-all solution.
There was a problem hiding this comment.
I don't really fully understand the implications of all this yet, but I agree entirely with your "rule" above and the need to be flexible/pragmatic - thanks!
bendk
force-pushed
the
push-xyuwnlsxurkk
branch
2 times, most recently
from
cbd0063 to
c19a79c
Compare
This is the start of work to switch the Python bindgen to use the new IR pipeline system. I split this up so the diffs are easier to read and maybe the commits can serve as breadcrumbs for implementing other bindings using the pipeline code. The first step is wiring the uniffi-bindgen CLI to an basically empty pipeline. I also removed a bunch of the older python code.
* copied `pipeline/general/nodes.rs` to `python/pipeline/nodes.rs` * Removed the `#[wraps]` attributes to avoid wrapping nodes twice. * Updated the pipeline to add a general IR -> python IR conversion pass.
bendk
force-pushed
the
push-xyuwnlsxurkk
branch
2 times, most recently
from
4c0312c to
3dc2f92
Compare
Added pipeline code and updated the templates to work with the new
system.
My main workflow loop for this was:
- Commenting out the template code using `{#` and `#}`
- Running `cargo test py` on one of the examples/fixtures.
- Uncommenting/upating the template code and implementing any pipeline
passes needed to make the tests pass.
I also removed some of the redundant fields. `Function.inputs` field
is redundant when there's `Function.callable.inputs`.
Along the way, markh pointed out that the `AsRef` macros and system was
not really necessary. I removed that functionality from
`uniffi_internal_macros`. The fewer concepts needed to understand the
pipeline the better, IMO.
This shows how the new pipeline code affects an existing bindings implementation. Before I was linking to the Gecko-JS bindings, but these have a bunch of extra complexity.
This is built on top of #2536, which contains the changes to the general pipeline that I realized needed to be made when implementing Python.
I split this up so the diffs are easier to read and maybe the commits can serve as breadcrumbs for implementing other bindings using the pipeline code. I recommend reading this PR one commit at a time.