StreamSink recursive intodart_type

[SilverMira]

Changes

Fixes #1839

Checklist

• An issue to be fixed by this PR is listed above.
• New tests are added to ensure new features are working. Please refer to this page to see how to add a test.
• ./frb_internal precommit --mode slow (or fast) is run (it internal runs code generator, does auto formatting, etc).
• If this PR adds/changes features, documentations (in the ./website folder) are updated.
• CI is passing. Please refer to this page to see how to solve a failed CI. looks like there are flaky tests with integrate command

Remark for PR creator

• ./frb_internal --help shows utilities for development.
• If fzyzcjy does not reply for a few days, maybe he just did not see it, so please ping him.

[welcome]

Hi! Thanks for opening this pull request! 😄

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.27%. Comparing base (79b0746) to head (db55d0b).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1843   +/-   ##
=======================================
  Coverage   99.27%   99.27%           
=======================================
  Files         358      359    +1     
  Lines       15099    15127   +28     
=======================================
+ Hits        14989    15017   +28     
  Misses        110      110           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fzyzcjy]

Feel free to ping me when ready (or having any questions) :)

[SilverMira]

I couldn't get ./frb_internal precommit --mode slow to actually work. Went for ./frb_internal generate-internal-frb-example-pure-dart && ./frb_internal generate-run-frb-codegen-command-generate --package frb_example/pure_dart but turns out it wasn't quite enough.
Had to run ./frb_internal generate-run-frb-codegen-command-generate --package frb_example/pure_dart_pde as well, hopefully everything should be fine now :)

[SilverMira]

Also, while writing this fix, I found that there are also other edge cases with StreamSink<T> that doesn't quite work either. Particularly, I noticed StreamSink<HashSet<MirrorEnum>> and StreamSink<Map<X, MirrorEnum>> isn't generating compilable code either, there are probably other combinations with container types that don't get resolved correctly. We should probably open another issue for tracking these.

Will probably take a look at them at a later date since I plan to aggressively use StreamSink in one of my projects :)

[fzyzcjy]

For that precommit thing, one way may be ./frb_internal precommit-generate (looks like things are already done).

For the command integrate CI, just manually modify things in https://github.com/fzyzcjy/flutter_rust_bridge/tree/master/frb_codegen/assets/integration_template, because the "integrate" command indeed copy-and-paste things from there to the frb_examples (in order to let end users be fast).

We should probably open another issue for tracking these.
Will probably take a look at them at a later date since I plan to aggressively use StreamSink in one of my projects :)

Sure, take your time and looking forward!

[SilverMira]

@fzyzcjy, will need some advise here for the StreamSink<HashSet<>> scenario, is there something in the codegen already that can conditionally put #[derive()] on the wrapper types depending on whether certain types are being generated?

Say the user want to use StreamSink<HashSet<OpaqueStruct>> , I suppose the users themselves should implement Eq and Hash themselves on OpaqueStruct so they can use it in a HashSet to begin with. The problem comes when FRB wraps OpaqueStruct in the Local_Auto_Owned_... struct, we have to mark this wrapper struct with #[derive(PartialEq, Eq, Hash)] since creating the sink will need the wrapper type to implement Eq and Hash as well.

I have verified that things will work fine if we derive the 3 traits like such

Hence back to the initial question since we wouldn't want to derive these traits if users are not using HashSet<OpaqueStruct>

[fzyzcjy]

is there something in the codegen already that can conditionally put #[derive()] on the wrapper types depending on whether certain types are being generated?

Interesting! IIRC there may not have been such things.
One way is, as you mentioned, when seeing HashSet, letting its child have these derives.
Another way is (possibly easier to implement), if detect the #[derive()] macros on the original struct, letting the generated struct have the derives.

[SilverMira]

if detect the #[derive()] macros on the original struct, letting the generated struct have the derives.

I don't think that would work either, for all we know, users may do impl Hash for OpaqueStruct themselves somewhere in the crate, maybe due to what they wrap in the struct is not Hash therefore they can't derive it automatically.

Another idea I had was to have a blanket implementation for wrapper types, currently wrapper types are concrete types Local_Auto_... or mirror_..., I'm not sure whether there were any sort of technical reason behind this. But from the generated code, it looks to me we can actually have something like this.

pub struct frb_Wrapper<T>(T)

impl <T: Hash> Hash for frb_Wrapper<T> {
   // Delegate hashing to inner type
}

impl IntoIntoDart<frb_Wrapper<OpaqueStruct>> for OpaqueStruct {
  // Same implementation on concrete type as before
}

// And other generated impls...

[fzyzcjy]

users may do impl Hash for OpaqueStruct

Yes, then we may need to introduce some kind of #[frb(please_derive_for=Hash)] manual annotation as a complement if we want to do it this way...

Another idea I had was to have a blanket implementation for wrapper types
I'm not sure whether there were any sort of technical reason behind this.

Good idea! I am also not sure whether that will work (IIRC I made a ton of new structs instead of one generic struct because of some reasons, but those reasons may disappear during later refactors, so yes this may work!), thus it would be great to firstly make some lightweight attempts to see whether it breaks something or not. (Should be quite fast, since we are just changing Local_Auto_Something to Local_Auto<Something>)

[SilverMira]

Side note, I did manage to find some way to check whether there is HashSet before emitting the pub struct Wrapper by using context.ir_pack.distinct_types() and tag on the derives, but it does seem like having a generic wrapper type is the better way forward if we can do that.

[SilverMira]

@fzyzcjy, I think the CI with generate is picking up wrongly on pubspec.lock being changed and failing

[fzyzcjy]

No worries, just run that command (./frb_internal ... shown in the title of your screenshot) locally without set-exit-if-changed and commit the local changes, and I guess the CI will be happy.

[SilverMira]

Looks like ci is happy now, currently the pr includes the switch over from Local_Auto_Something and mirror_Something to frb_wrapper<Something> for the blanket implementation approach for Eq and Hash. Tests are passing and we have lesser code generated since there are no longer any concrete wrapper types. It's ready for review and hope things are fine 🚀

[fzyzcjy]

Good work! Ready to merge and only some quick nits

[fzyzcjy]

(no worries about the flaky ios failure)

[fzyzcjy]

LGTM! Just one tiny nit and you can choose not to modify it

[welcome]

Hi! Congrats on merging your first pull request! 🎉

[fzyzcjy]

@all-contributors please add @SilverMira for code

[allcontributors]

@fzyzcjy

I've put up a pull request to add @SilverMira! 🎉