Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse futures #2189

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Parse futures #2189

wants to merge 18 commits into from

Conversation

vhdirk
Copy link
Contributor

@vhdirk vhdirk commented Jul 5, 2024

Close #2172 Close #2222 (<--- edit by @fzyzcjy to let github understand it)

Changes

I've been trying to get stuff like this to work:

    pub fn example_async_method(&mut self) -> impl Future<Output = Result<String, MyErr>> {
       async {
            "result_value".to_string()
        }
    }

    pub fn example_async_future(&mut self) -> DartFnFuture<String> {
        async {
            "result_value".to_string()
        }
    }

    pub fn example_async_future2(&mut self) -> Pin<Box<dyn Future<Output = String> + Send + 'static>> {
        async {
            "result_value".to_string()
        }
    }

Due to restrictions of other usages for my library, I can't use async traits, so I have to resort to Future trait objects.

I'm not sure I'm on the right track with this. Any help would be appreciated.

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.

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.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jul 5, 2024

Looks great!

Firstly, I wonder whether this is related: #2172

Secondly, my brainstorm on implementing this:

  • Code changes mainly are in parser/mir
  • Maybe try to parse types like Pin<Box<dyn Future<Output = String> + Send + 'static>> into (1) "let's fake it as if user is writing a String here" (2) "let's mark MirFunc.rust_async = true".
  • Then the code generator will do everything automatically. It will treat as if it is async fn f() -> String and generate code related to strings and async/await and done.

@vhdirk
Copy link
Contributor Author

vhdirk commented Jul 5, 2024

I think it is related to #2172, yes.

I was trying to do exactly what you propose, but when a dyn Future is encountered, it seems not to be part of src_traits here: https://github.com/vhdirk/flutter_rust_bridge/blob/2ae58f68ffd2e223199bec6c508c577a02ef49d5/frb_codegen/src/library/codegen/parser/mir/parser/ty/trait_def.rs#L16

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jul 6, 2024

but when a dyn Future is encountered, it seems not to be part of src_traits

One way (just brainstorm, may or may not be the best way):

Mimic what we do to parse, say, Vec<String> (https://github.com/vhdirk/flutter_rust_bridge/blob/2ae58f68ffd2e223199bec6c508c577a02ef49d5/frb_codegen/src/library/codegen/parser/mir/parser/ty/concrete.rs#L72). We may have a futures.rs that recognizes Pin<Box<dyn Future<Output = String> + Send + 'static>> and directly return something like MirType::Delegate(MirTypeDelegate::Future(String)). It can be done by matching the type to see "is it a Pin, and if so, is the inner type a Box, and if so, is inner dyn, and if so, is inner Future, and if so, ...". Similarly we can find impl Future as well.

And, we put this early on https://github.com/vhdirk/flutter_rust_bridge/blob/2ae58f68ffd2e223199bec6c508c577a02ef49d5/frb_codegen/src/library/codegen/parser/mir/parser/ty/path.rs#L36 (e.g. just above that line). Then, that will take precedence.

@vhdirk
Copy link
Contributor Author

vhdirk commented Aug 1, 2024

I've finally been able to find some time for this PR again. My latest commit makes it successfully parse Pin<Box<dyn Future<Output = String>. I haven't looked at the code generator yet, though.
I think impl Future is also supported, but it's getting late here and I can't really recall if I did or didn't do that yet :)

@fzyzcjy: could you have another look please? There's a bit of cleaning up to do still, but I was trying not to touch too much things all at once.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 2, 2024

Great job! I will find some time to review it, probably within a day

Copy link

codecov bot commented Aug 3, 2024

Codecov Report

Attention: Patch coverage is 79.56989% with 57 lines in your changes missing coverage. Please review.

Project coverage is 82.05%. Comparing base (9e5c6e5) to head (ad0b1c8).

Files Patch % Lines
...ibrary/codegen/parser/mir/parser/ty/unencodable.rs 61.53% 10 Missing ⚠️
...c/library/codegen/parser/mir/parser/ty/concrete.rs 87.30% 8 Missing ⚠️
...src/library/codegen/parser/mir/parser/ty/future.rs 82.05% 7 Missing ⚠️
..._codegen/src/library/codegen/ir/mir/ty/delegate.rs 50.00% 5 Missing ⚠️
.../codegen/generator/api_dart/spec_generator/info.rs 0.00% 4 Missing ⚠️
...library/codegen/generator/codec/sse/ty/delegate.rs 0.00% 4 Missing ⚠️
...odegen/parser/mir/sanity_checker/unused_checker.rs 71.42% 4 Missing ⚠️
...library/codegen/parser/mir/parser/ty/impl_trait.rs 85.00% 3 Missing ⚠️
...rt/spec_generator/codec/cst/encoder/ty/delegate.rs 0.00% 2 Missing ⚠️
...c/library/codegen/parser/mir/parser/ty/optional.rs 77.77% 2 Missing ⚠️
... and 6 more
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2189       +/-   ##
===========================================
- Coverage   99.06%   82.05%   -17.02%     
===========================================
  Files         488      468       -20     
  Lines       20186    18368     -1818     
===========================================
- Hits        19997    15071     -4926     
- Misses        189     3297     +3108     

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

@vhdirk
Copy link
Contributor Author

vhdirk commented Aug 3, 2024

Well, I haven't actually tested the generated code. Not even once. But if it doesn't work immediately, I probably need some additional advice anyway :)

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 3, 2024

Sure, take your time and ping me when needing any suggestion etc!

@vhdirk
Copy link
Contributor Author

vhdirk commented Aug 6, 2024

I think I'm at a point where a proper review might be welcome. I think I broke some existing tests, but TBH, I don't quite know why that is :/

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 6, 2024

Sure!

Copy link
Owner

@fzyzcjy fzyzcjy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! The main idea looks quite reasonable, and I only have some nits

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 6, 2024

I think I broke some existing tests, but TBH, I don't quite know why that is :/

Try to copy-paste it to frb_example/dart_minimal and play with it, which is often easier (and faster) to debug.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 6, 2024

Btw CI may fail b/c some Dart updates, but my recent several PRs will fix it (along with other new code), so no worries about that.

@vhdirk vhdirk marked this pull request as ready for review August 6, 2024 18:13
@vhdirk vhdirk force-pushed the futures branch 2 times, most recently from 72752a0 to 4620d7a Compare August 7, 2024 07:02
@vhdirk
Copy link
Contributor Author

vhdirk commented Aug 7, 2024

So couple of things:

  • apparently I've introduced a regression with struct/enum parsing where it starts to complain that enum unit variants are not supported. I've been looking at the diff and I can't really spot any obvious difference. So in depth debugging is needed, and I'd kindly ask your help for that.
  • I'm on rust 1.80 and I get compile errors due to #[cfg(wasm)]. This is new for 1.80, apparently: https://blog.rust-lang.org/2024/05/06/check-cfg.html

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 7, 2024

So in depth debugging is needed, and I'd kindly ask your help for that.

Maybe try to do sth like:

  • reproduce the bug in frb_example/dart_minimal
  • run the new (buggy) codegen once, and the old (bug-free) codgen once
  • look at what they differ. use debugger, or do printing may help. also the rust/target/frb_dump logs may also help

errors due to #[cfg(wasm)]

That one was meant to be "only compile when in wasm mode". Thus maybe try to change to #[cfg(target_family = "wasm")] etc.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 18, 2024

Cross-link: #2234 (reply in thread)

(since github does not auto cross-link between a discussion and a issue/pr)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skiped async_trait for different crates Pin<Box<dyn Future<Output = _>>> in Trait Object translate issue
2 participants