Enums that contain mirrored structs `Enum1(MyStruct)` and Unit Tests

alexthe2 commented 1 year ago

Changes

Please list issues fixed by this PR here, using format "Fixes #the-issue-number".

Checklist

[ ] An issue to be fixed by this PR is listed above.
[x] New tests are added to ensure new features are working. End-to-end tests are usually in the ./frb_example/pure_dart example, more specifically, rust/src/api.rs and dart/lib/main.dart.
[ ] The code generator is run and the code is formatted (via just precommit).
[ ] If this PR adds/changes features, documentations (in the ./book folder) are updated.
[ ] CI is passing.

Remark for PR creator

Justfile is a task runner for the command line interface that allows you to run shell commands from a file named justfile in your project directory. You can use Justfile after installing it. Note that commands written in justfile of this repository are expected to be run in bash, not cmd or powershell. Running just ... commands in cmd or powershell will produce errors as the syntax is not compatible. On Windows, you can use git bash if you have Git installed.
If fzyzcjy does not reply for a few days, maybe he just did not see it, so please ping him.

alexthe2 commented 1 year ago

I got some issues in one of my projects, that I can’t reproduce in the bridge, but maybe one of these tests will fail

alexthe2 commented 1 year ago

If they don’t fail I just added more tests, but I’ll try a bit more, so don’t merge it yet

alexthe2 commented 1 year ago

found the broken part, mirrors. They seem to break everything

alexthe2 commented 1 year ago

If you don’t mind @fzyzcjy , can you explain me quickly how mirrors work exactly / is there some docs about them

fzyzcjy commented 1 year ago

https://cjycode.com/flutter_rust_bridge/feature/lang_external.html

Also cc @Unoqwy who implemented the good mirror feature

alexthe2 commented 1 year ago

Finally got the tests to reproduce exactly my issue, will try to fix it now

alexthe2 commented 1 year ago

So the problem is that when an Enum is mirrored, its data types are stayed as regular, not the mirrored structs

alexthe2 commented 1 year ago

I got a first draft that seems to work, but still have 2 edge cases I need to figure out, list of enums and enums in api.rs

alexthe2 commented 1 year ago

Ok so in the current master version list of mirrored enums don’t work either, meaning that my changes right now have at least fixed enums that contain mirrored structs, I still have to do some refactoring but overall I’m finished with this part, list of enums will be a seperate pull request, let’s approach this one at a time

fzyzcjy commented 1 year ago

Looks great! I will probably review today :)

alexthe2 commented 1 year ago

Looks great! I will probably review today :)

I need to sleep 😴 , and some of the code requires a bit of rethinking, so when u review ignore this function:

fn extract_type(fields: &Fields) -> Option<String> {
        let f = match fields {
            Fields::Named(_) => None,
            Fields::Unnamed(u) => Some(u.to_token_stream()),
            Fields::Unit => None,
        }
        .map(|s| {
            s.to_string()
                .trim_start_matches('(')
                .trim_end_matches(')')
                .to_string()
        });

        if f.is_some()
            && vec!["String", "bool", "i32", "i64", "f32", "f64"]
                .contains(&f.as_ref().unwrap().as_str())
        {
            return None;
        }

        f
    }

I need to reorder it

alexthe2 commented 1 year ago

Nvm, I guess. It was a very short refactor. If you can review it, it would be great. I can then change it later today (after I finally sleep).

If there are no nits and all tests pass(which I don't believe), then for the changelog would be:

Bumped up syn, to latest v1 version
Fixed enum containing structs
Added a bunch of unit tests to make sure it works for the future (because it worked before, so some feature/refactor must have broken it)

fzyzcjy commented 1 year ago

Sure, take your time! Btw for the changelog - could you please modify the PR title, such that it is easier to read for future readers :)

alexthe2 commented 1 year ago

how does that look?, not happy with the hardcoding though

fzyzcjy commented 1 year ago

Quite busy now, I will check it soon (probably within a few hours) :)

alexthe2 commented 1 year ago

Quite busy now, I will check it soon (probably within a few hours) :)

don’t worry, not pressing (maybe a bit because everything is broken on my side in the project)

fzyzcjy commented 1 year ago

maybe a bit because everything is broken on my side in the project

Ah then I should do it quicker!

alexthe2 commented 1 year ago

Ohh, I get what you mean. I can change that very quickly

On Tue, 28 Mar 2023, 01:56 fzyzcjy, @.***> wrote:

@.**** commented on this pull request.

In frb_codegen/src/generator/rust/ty_enum.rs https://github.com/fzyzcjy/flutter_rust_bridge/pull/1144#discussion_r1149893020 :

let struct_name = field.ty.mirrored_nested();

if field.mirrored_enum && struct_name.is_some() {

format!(

"mirror_{}({}).into_dart()",

struct_name.unwrap(),

field.name.rust_style().to_owned()

Hmm I am still a bit confused.

The current code is: For each field, if the type of the field is indeed a mirror, do extra log to code generation (addd "mirror_").

However, we know it is type that can be a mirror. So what I proposes is: Is it possible we add is_mirror or something like that onto the type, instead of adding it as a flag for the field?

— Reply to this email directly, view it on GitHub https://github.com/fzyzcjy/flutter_rust_bridge/pull/1144#discussion_r1149893020, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIBZJB6M26VUXHB3FFJ2AQDW6ISJDANCNFSM6AAAAAAWG6KU3E . You are receiving this because you authored the thread.Message ID: @.***>

fzyzcjy commented 1 year ago

(notes for myself)

Goal: When generating into-dart logic, if the type of a field is a mirror, we should add mirror_SomeType(...) wrapper around it.

Current code: Add "is-mirror" information into the IrField struct.

Suggestion: Derive "is-mirror" information from the IrType.

alexthe2 commented 1 year ago

(notes for myself)

Goal: When generating into-dart logic, if the type of a field is a mirror, we should add mirror_SomeType(...) wrapper around it.

Current code: Add "is-mirror" information into the IrField struct.

Suggestion: Derive "is-mirror" information from the IrType.

small extension to this: This became a new bug, I don’t exactly know what version caused it, my guess is either 1.70, 1.71 or 1.72, because it worked before

fzyzcjy commented 1 year ago

I see... Then maybe hack like this temporarily :( Since I do not have more time for this edge case

alexthe2 commented 1 year ago

I see... Then maybe hack like this temporarily :( Since I do not have more time for this edge case

any nits or can we merge?

fzyzcjy commented 1 year ago

No nits, good job!

alexthe2 commented 1 year ago

Cool, the release will be out once the release pipelines pass, won't it?

fzyzcjy commented 1 year ago

Executing release pipeline on my machine :)

fzyzcjy commented 1 year ago

Ping me if it does not appear after an hour since this one usually takes only a dozen (?) minutes

alexthe2 commented 1 year ago

Oh ok, thank you

fzyzcjy commented 1 year ago

Indeed done now.

You are welcome :)

fzyzcjy / flutter_rust_bridge