Move object converters into a sibling FfiConverter class - #156
Conversation
| class $ffi_converter_name { | ||
| static $cls_name lift(Pointer<Void> ptr) { | ||
| return $cls_name.lift(ptr); | ||
| } |
There was a problem hiding this comment.
Important callout that this changes the generated API - Recorder.lower(x) becomes FfiConverterRecorder.lower(x). Code that a consumer writes by hand I wouldn't think normally call these statics, because they belong to the FFI layer. But if any code does, it would need to change.
| Exception lift(RustBuffer errorBuf) { | ||
| return $(cls_name).read(errorBuf.asUint8List()).value; | ||
| return $(ffi_converter_name).read(errorBuf.asUint8List()).value; | ||
| } |
There was a problem hiding this comment.
Note that the error handler now points at the converter - an [Error] interface emits an error handler class, and that handler called Recorder.read. It now calls FfiConverterRecorder.read.
|
@chavic For reference, the two test failures on here seem to be the ones which we discussed on #150 (the lint failure, and the flaky Would you be willing to review this in its current state, or would you like to see the linting issue get resolved first? I think other than the linting issue this should be ready for review from my end. |
Clippy 1.97 extended useless_borrows_in_formatting to flag these five borrows, and CI runners now ship stable 1.97.1, so the Lints (stable) job fails on main (see the run on #156, which flags these exact lines without touching them). Verified: clippy 1.98.0 reports five 'redundant reference' errors on main and none with this change. Display for &T forwards to Display for T, so output is byte-identical. The same change rode along in #149, #152, #157 (and was reverted from #150 when it could not be reproduced locally on an outdated toolchain). Landing it once on main lets those branches rebase clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDnKiUL8NDSpeaPvyKJeoR
Object classes carried their converter statics (`lift`/`lower`/`read`/ `write`/`allocationSize`) directly, so a Rust method named `write` collided with the static of the same name -- Dart forbids a static and an instance member sharing a name. Objects and trait objects now get a sibling `FfiConverter<Name>` class holding those statics, matching how records, enums and callback interfaces are already emitted.
6924c8d to
a982131
Compare
|
Rebased on top of latest |
chavic
left a comment
There was a problem hiding this comment.
Nice fix, thank you. On your API callout: the rename is safe to take. The generator emits these calls from one source, so no stale call site remains, and the hand-written Dart in payjoin-ffi and bdk-dart calls none of these statics.
Please add a small fixture with an object that has a method named read or lower. That is the collision this change corrects, and no test guards it yet.
FIxes #155
Summary
ffi_converter_namereturned the canonical name of the object forObjectImpl::StructandObjectImpl::Trait. The converter statics therefore landed on the object class itself, where a Rust method of the same name collided with them.The function now returns
FfiConverter<CanonicalName>for both variants, andgenerate_objectandgenerate_trait_objectemit that class as a sibling of the object. Records, enums and callback interfaces already follow this pattern.Effect on generated code
class Recorder implements RecorderInterface { - static Pointer<Void> lower(Recorder value) { /* ... */ } - static int allocationSize(Recorder value) { /* ... */ } - static LiftRetVal<Recorder> read(Uint8List buf) { /* ... */ } - static int write(Recorder value, Uint8List buf) { /* ... */ } - void write({required Uint8List chunk}) { /* ... */ } } + +class FfiConverterRecorder { + static Recorder lift(Pointer<Void> ptr) { /* ... */ } + static Pointer<Void> lower(Recorder value) { /* ... */ } + static int allocationSize(Recorder value) { /* ... */ } + static LiftRetVal<Recorder> read(Uint8List buf) { /* ... */ } + static int write(Recorder value, Uint8List buf) { /* ... */ } +}Tests
I opted to not add any explicit tests for this since the existing test suite covers all these paths quite well - it's exercising all these existing internal bindgen method calls. If you'd like though, I could add something which goes through the whole call cycle with a method named
write()on an object just to validate this 100% and protect against a regression.