feat: Add oxidized Result support for fallible functions#2950
feat: Add oxidized Result support for fallible functions#2950rlch wants to merge 9 commits intofzyzcjy:masterfrom
Conversation
When oxidized package is detected in pubspec.yaml dependencies: - Fallible Rust functions return Future<Result<T, E>> instead of Future<T> - Errors are returned as Err(...) instead of throwing exceptions - Panics still throw PanicException (bugs, not expected errors) Runtime (frb_dart): - Add decodeAsResult() to SimpleDecoder for Result-based decoding - Add decodeObjectAsResult/decodeWireSyncTypeAsResult to BaseCodec - Add executeNormalAsResult/executeSyncAsResult to BaseHandler - Export Result, Ok, Err from flutter_rust_bridge_for_generated_common.dart Codegen (frb_codegen): - Add has_dependency() to DartRepository - Add detect_oxidized_dependency() in generator_parser.rs - Add use_oxidized field to GeneratorApiDartInternalConfig - Wrap fallible function return types in Result<T, E> - Use executeNormalAsResult/executeSyncAsResult for fallible functions Tested with dart_minimal example: - Async functions: Future<Result<T, E>> - Sync functions: Result<T, E>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2950 +/- ##
==========================================
- Coverage 98.57% 6.19% -92.38%
==========================================
Files 464 460 -4
Lines 19202 16242 -2960
==========================================
- Hits 18928 1007 -17921
- Misses 274 15235 +14961 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
When a type alias like \`type Result<T> = std::result::Result<T, MyError>\` is used, FRB now correctly extracts the error type from the alias definition instead of falling back to AnyhowException. - Store generic type aliases separately in HirFlatTypeAlias - Look up generic Result aliases when parsing function return types - Substitute type parameters to resolve the actual error type
- Add example error types and functions using type aliases - Update generated Dart/Rust bindings with new test cases
|
You are welcome and thanks for the great job! |
fzyzcjy
left a comment
fzyzcjy
left a comment
There was a problem hiding this comment.
tiny nits (I will review more ASAP when having time)
|
Thanks @fzyzcjy ! Addressed changes. |
|
Looks great! I will review ASAP, but have a tight ddl now thus may not have time very recently :( |
|
Hey @fzyzcjy would you be able to review this when you get some time? Got like 3 projects using this fork and it's getting a little annoying lol. No rush, I understand you're busy. Just thought I'd remind! |
|
Hi, happy to see your work is being used by multi projects, and I also think this is a great feature! I am really sorry for being super busy w/ a very emergent and important ddl (or indeed, multiple overlapped deadlines, that's why I have been non resting for a long time) :( I will review as soon as the current ddl ends, potentially it will not be overlapped with another emergent ddl. By the way, if you (or maybe Cursor / Claude Code etc) put the tests in pure_dart etc instead of the dart_minimal, then I do not need to manually do the moving / refactoring / checking, then I will potentially just merge it with a lightweight review, since it is does not change current code a lot, and can be refactored and further reviewed later. |
|
No worries at all mate. Sorry to hear you're so busy, best of luck with it! I'll handle the tests, lmk if I've done anything wrong when I get around to it. Again no stress at all, take your time. |
|
Thank you! |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that introduces a significant and valuable feature: support for oxidized Result types. The implementation is thorough, touching the code generator, runtime, and adding comprehensive tests and documentation. The approach to handle generic type aliases is particularly well-done. I have one minor suggestion to improve a test, but overall, this is a high-quality contribution.
| #[test] | ||
| fn test_is_result_type_alias() { | ||
| // Simulate: type WResult<T> = std::result::Result<T, WakuError>; | ||
| let alias = HirFlatTypeAlias { | ||
| ident: "WResult".to_string(), | ||
| target: syn::parse_str("std::result::Result<T, WakuError>").unwrap(), | ||
| generics: Some(syn::parse_str("<T>").unwrap()), | ||
| }; | ||
|
|
||
| let mut src_generic_types = std::collections::HashMap::new(); | ||
| src_generic_types.insert("WResult".to_string(), &alias); | ||
|
|
||
| // Create a mock TypeParser with just the generic types | ||
| // We can't easily create a full TypeParser, but we can test the logic directly | ||
| if let syn::Type::Path(type_path) = &alias.target { | ||
| if let Some(last_segment) = type_path.path.segments.last() { | ||
| assert_eq!(last_segment.ident, "Result"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This test doesn't directly test the is_result_type_alias function. Instead, it re-implements part of its logic. It would be better to call the function directly to ensure it behaves as expected. You can construct a TypeParser with default values for fields not used by is_result_type_alias to make the test more direct and robust.
#[test]
fn test_is_result_type_alias() {
// Simulate: type WResult<T> = std::result::Result<T, WakuError>;
let alias = HirFlatTypeAlias {
ident: "WResult".to_string(),
target: syn::parse_str("std::result::Result<T, WakuError>").unwrap(),
generics: Some(syn::parse_str("<T>").unwrap()),
};
let alias_not_result = HirFlatTypeAlias {
ident: "WOption".to_string(),
target: syn::parse_str("std::option::Option<T>").unwrap(),
generics: Some(syn::parse_str("<T>").unwrap()),
};
let mut src_generic_types = std::collections::HashMap::new();
src_generic_types.insert("WResult".to_string(), &alias);
src_generic_types.insert("WOption".to_string(), &alias_not_result);
let type_parser = TypeParser::new(
&Default::default(),
&Default::default(),
Default::default(),
Default::default(),
Default::default(),
Default::default(),
src_generic_types,
Default::default(),
Default::default(),
);
assert!(is_result_type_alias(&type_parser, "WResult"));
assert!(!is_result_type_alias(&type_parser, "WOption"));
assert!(!is_result_type_alias(&type_parser, "NonExistent"));
}
|
thanks for the PR! |
Traits marked with #[frb(ignore)] were being collected into src_traits but the ignore attribute was never checked, causing panics when the trait was used in Option<T> contexts. This mirrors how structs/enums handle the ignore attribute via parse_struct_or_enum_should_ignore. Amp-Thread-ID: https://ampcode.com/threads/T-019bf8ee-f69c-75eb-981d-55743289c99e Co-authored-by: Amp <amp@ampcode.com>
Add first-class support for serde_json::Value as a delegate type that serializes via JSON string across the FFI boundary, mapping to Dart's Object?. Supports all codecs (SSE, DCO, CST) and composite types (Option, Vec, HashMap, struct fields). Includes tests, documentation, and feature flag gated behind `serde_json`.
3 participants
Hey @fzyzcjy
This PR adds support for serializing
Result<T, E>to the equivalent exposed by oxidized. I think the package's API has stabilized, obviously idiomatic w.r.t. Rust conventions; it's well-tested and doesn't pull in a crap-ton of API's like a lot of the sibling packages.Thanks again for this package, I hope to contribute more over the coming years; what you've done is magnificent and IMO the best addition to the Flutter ecosystem thus far.
Changes
Fixes #2683
Procedure and Checklist
In order to quickly iterate and avoid slowing down development pace by making CI pass, only the following simplified steps are needed, and I (fzyzcjy) will handle the rest of CI / moving the tests currently (will have more automation in the future).
frb_example/dart_minimal.dart_minimal's CI green.Utility commands
cargo run --manifest-path /path/to/flutter_rust_bridge/frb_codegen/Cargo.toml -- generate./frb_internal test-dart-native --package frb_example/dart_minimal