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

Support for Result<MyType, MyError> with exception throwing on Dart #582

Closed
wants to merge 89 commits into from

Conversation

lattice0
Copy link
Contributor

@lattice0 lattice0 commented Jul 18, 2022

Close #533

Adds support for Rust's std::Result, where you can return either a value or an error.

Example

pub enum CustomError {
    Error0(String),
    Error1(u32),
}

pub fn return_err_custom_error() -> Result<u32, CustomError> {
    Err(CustomError::Error1(3))
}

Becomes something that can be used like this:

try {
      final r = await api.returnErrCustomError();
      print("received $r");
    } catch (e) {
      print('dart catch e: $e');
      expect(e, isA<CustomError>());
    }
}

Checklist

  • New tests are added to ensure new features are working (probably by modifying the frb_example/pure_dart example).
  • All existing and new tests are passing.
  • If this PR adds/changes features, documentations (in book folder) are modified as well.

@lattice0 lattice0 changed the title Error Support for Result<MyType, MyError> with exception throwing on Dart Jul 18, 2022
@lattice0
Copy link
Contributor Author

PS: Should fail until we get anyhow support for IntoDart as done here shekohex/allo-isolate#23

@lattice0 lattice0 marked this pull request as ready for review July 18, 2022 20:17
.gitmodules Outdated Show resolved Hide resolved
book/src/feature/result.md Outdated Show resolved Hide resolved
frb_example/pure_dart/dart/lib/bridge_generated.dart Outdated Show resolved Hide resolved
frb_example/pure_dart/dart/lib/bridge_generated.dart Outdated Show resolved Hide resolved
frb_example/pure_dart/dart/lib/main.dart Outdated Show resolved Hide resolved
frb_example/pure_dart/dart/lib/main.dart Outdated Show resolved Hide resolved
@fzyzcjy
Copy link
Owner

fzyzcjy commented Jul 18, 2022

Great job!

Since CI is not functioning before the upstream release, I just comment partially currently.

I am also wondering: Can we make this breaking change as little as possible? I want to cheat a little bit and do not bump the major version (i.e. release 2.0.0), since this feature though great is not as huge as a major version bump.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jul 24, 2022

Well I mean:

class FrbException implements Exception {} // no fields

abstract class FrbBacktracedException implements FrbException {
  Backtrace get backtrace;
}

class FrbAnyhowException extends FrbBacktraceException {
  final String description; // or something else etc
  final String backtrace;

  FrbAnyhowException(this.description, this.backtrace);
}

@freezed
class CustomNestedError2 with _$CustomNestedError2 {
  // NOTE for data *without* field "backtrace", just implement FrbException
  @Implements<FrbException>()
  const factory CustomNestedError2.customNested2(
    String field0,
  ) = CustomNested2;

  // NOTE **only** implements FrbBacktracedException **if** there is **really** a "backtrace" field
  @Implements<FrbBacktracedException>()
  const factory CustomNestedError2.customNested2Number(
    int field0,
    String backtrace,
  ) = CustomNested2Number;
}

@lattice0
Copy link
Contributor Author

Why limit to errors with backtrace if we can always put a backtrace? Most people won't put they own backtraces so we should just put it something that is always received

@franklinblanco
Copy link

@fzyzcjy any hints on what would it take to finish this, as this is a pretty useful feature!

@fzyzcjy
Copy link
Owner

fzyzcjy commented Sep 19, 2022

@franklinblanco Just fix the CI, and I am going to merge it :)

@franklinblanco
Copy link

@franklinblanco Just fix the CI, and I am going to merge it :)

So it would just be the timeout issue and upgrading the project, right?

@fzyzcjy
Copy link
Owner

fzyzcjy commented Sep 20, 2022

@franklinblanco Yes, basically speaking just that :)

Would be great to resolve all unresolved conversations, but that can also be a future work.

@lattice0
Copy link
Contributor Author

lattice0 commented Oct 11, 2022 via email

@fzyzcjy
Copy link
Owner

fzyzcjy commented Oct 11, 2022

@lattice0 The bug on valgrind is going to take some more days 😔

Take your time!

@lattice0
Copy link
Contributor Author

Something is wrong with the mail system, these messages were supposed to be from months ago. I currently have no time to fix the problems :c

@lattice0 lattice0 mentioned this pull request Oct 11, 2022
@fzyzcjy
Copy link
Owner

fzyzcjy commented Oct 11, 2022

@lattice0 Something is wrong with the mail system, these messages were supposed to be from months ago. I currently have no time to fix the problems :c

No worries, take your time, and looking forward to your future contribution :)

@stale
Copy link

stale bot commented Dec 10, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Dec 10, 2022
@stale stale bot closed this Dec 17, 2022
@fzyzcjy fzyzcjy reopened this Dec 17, 2022
@stale stale bot removed the wontfix This will not be worked on label Dec 17, 2022
@lattice0
Copy link
Contributor Author

I wish someone finished this last annoying part, I have no time :(

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jan 14, 2023

@lattice0 Sad to hear that :( Also looking forward to seeing someone implementing it, since this can be quite helpful

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jan 14, 2023

I have added a paragraph b71004c#diff-a10cdc248927ef9888ef0fbb35fdc60697eb7528199fd78c38ef5d1294852a5aR5 pointing to this PR for who is interested :)

  1. #582 adds support for error hierarchy, or arbitrary error types. For example, you can create your own CustomError (such as using thiserror), and it will automatically be converted to a new Dart class. The PR is already almost completed (with the hard work done), and the missing piece is fixing the test errors. I have provided some guidance about how to fix it at the PR comments as well.

@lattice0
Copy link
Contributor Author

Run dart test, without valgrind, once. Does it work? Or does it also timeout? If it timeouts, then we are pretty sure it is our bug.

it does not timeout, only valgrind timeouts. Locally the error runs fine.

I can't continue my app without this feature but I have no time to do complicated fixes, trying to look into the easy paths for solving this

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jan 23, 2023

it does not timeout, only valgrind timeouts. Locally the error runs fine.

Hmm ok. Then we are still unsure whether it is valgrind bug or valgrind helps us to realize a bug in our code (e.g. undefined behavior).

By the way, given the main codebase has changed so much, maybe by merging upstream code something can change.

I can't continue my app without this feature but I have no time to do complicated fixes, trying to look into the easy paths for solving this

:/

@stale
Copy link

stale bot commented Mar 25, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Mar 25, 2023
@lattice0
Copy link
Contributor Author

lattice0 commented Mar 25, 2023 via email

@stale stale bot removed the wontfix This will not be worked on label Mar 25, 2023
@stale
Copy link

stale bot commented May 24, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label May 24, 2023
@stale stale bot closed this Jun 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Result<(),MyCustomError> instead of anyhow?
5 participants