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

Better static checking of erroneous API results #3165

Open
Mike278 opened this issue Jun 25, 2023 · 5 comments
Open

Better static checking of erroneous API results #3165

Mike278 opened this issue Jun 25, 2023 · 5 comments
Labels
request Requests to resolve a particular developer problem

Comments

@Mike278
Copy link

Mike278 commented Jun 25, 2023

I stole this issue title from @munificent's closing remarks here (with which I whole-heartedly agree)

At a high level, I think we are all on board with wanting better static checking of erroneous API results, but I don't think checked exceptions are the way to get them. Instead, I think a better approach is to go towards pattern matching and algebraic datatype style.

It's a couple years later (apparently?!), and Dart now supports pattern matching and sealed classes 🎉. These features dramatically improve expressiveness when both producing and consuming branching logic at the type level. However there are still some ergonomic issues that make sealed classes tricky to use for exception"unhappy path" handling when compared to throwing exceptions. And just to clarify: I'm specifically referring to first class "domain" exceptions that you typically build a UI/workflow to handle - not programmer errors.

There are some smaller things like #2025 but main issue I've come across is that sealed classes don't compose well because it's hard to "propagate" the subtypes you don't want to handle.

For a somewhat concrete example, in our app there are a bunch of workflows that can either be executed manually with the proper sequence of button taps, or automatically by scanning 1 or 2 QR codes. This means there are a set of functions which are sometimes called individually, one at a time by the UI (in the manual workflow case), and sometimes programmatically from a single entrypoint (in the automatic QR code case). For the latter, some errors are converted into default values, while others should bubble up to the UI where existing error-handling UI/workflows can be re-used. Writing down the sealed class hierarchy that describes the algebra of "this set + that set - 1 element" was not pretty!

From the small amount that I've searched about this topic:

I think a big part of this could be improved by existing proposals like #2364 and #3021, but I'd be curious to hear what other ideas are floating around. Something like Swift's best-attempt at exhaustive catch clauses? Or better ergonomics for adding existing classes to a new sealed hierarchy? The idea at the end of this comment also seems super interesting.

@Mike278 Mike278 added the request Requests to resolve a particular developer problem label Jun 25, 2023
@munificent
Copy link
Member

munificent commented Jun 27, 2023

This means there are a set of functions which are sometimes called individually, one at a time by the UI (in the manual workflow case), and sometimes programmatically from a single entrypoint (in the automatic QR code case). For the latter, some errors are converted into default values, while others should bubble up to the UI where existing error-handling UI/workflows can be re-used. Writing down the sealed class hierarchy that describes the algebra of "this set + that set - 1 element" was not pretty!

To make this a little more concrete, do you mean something like:

sealed class ConnectResult {}
class Connected extends ConnectResult {
  final Connection connection;
  Connected(this.connection);
}
class Offline extends ConnectResult {}

sealed class DownloadResult {}
class Downloaded extends DownloadResult {
  final String data;
  Downloaded(this.data);
}
class DownloadInterrupted extends DownloadResult {}

sealed class ConnectAndDownloadResult {}
class ConnectedAndDownloaded extends ConnectAndDownloadResult {
  final String data;
  ConnectedAndDownloaded(this.data);
}
class ConnectOffline extends ConnectAndDownloadResult {}
class ConnectAndDownloadInterrupted extends ConnectAndDownloadResult {}

/// Manual step 1:
ConnectResult connect() { ... }

/// Manial step 2:
DownloadResult download(Connection connection) { ... }

/// Automated:
ConnectAndDownloadResult connectAndDownload() {
  return switch (connect()) {
    Connected(:var connection) =>
      switch (download(connection)) {
        Downloaded(:var data) => ConnectedAndDownloaded(data),
        DownloadInterrupted() => ConnectAndDownloadInterrupted(),
      },
    Offline() => ConnectOffline(),
  };
}

Assuming I have this right, yes, it's annoying to compose a pipeline of functions that return different sealed types.

One thing you can do is take advantage of the fact that sealed types don't have to be a single hierarchy. So you can reuse some of the classes like:

sealed class ConnectAndDownloadResult {}

sealed class ConnectResult implements ConnectAndDownloadResult {}
class Connected extends ConnectResult {
  final Connection connection;
  Connected(this.connection);
}
class Offline extends ConnectResult {}

sealed class DownloadResult implements ConnectAndDownloadResult {}
class Downloaded extends DownloadResult {
  final String data;
  Downloaded(this.data);
}

class DownloadInterrupted extends DownloadResult {}

/// Manual step 1:
ConnectResult connect() { ... }

/// Manial step 2:
DownloadResult download(Connection connection) { ... }

/// Automated:
ConnectAndDownloadResult connectAndDownload() {
  return switch (connect()) {
    Connected(:var connection) => download(connection),
    var result => result,
  };
}

@rubenferreira97
Copy link

rubenferreira97 commented Jun 28, 2023

I believe what @Mike278 is proposing (please correct me if I am mistaken) is an effortless method to propagate errors that may occur in a sequence of calls that could potentially produce errors. Let's consider the divideNonNegativeThenAdd function as an example:

sealed class Result<T, E> {}

class Ok<T> extends Result<T, Never> {
  Ok(this.value);
  final T value;
}

class Err<E> extends Result<Never, E> {
  Err(this.error);
  final E error;
}

sealed class DivideError {}

class DivideByZero extends DivideError {}

class DivideByNegative extends DivideError {}

Result<int, DivideError> divideNonNegative(int a, int b) {
  if (b == 0) return Err(DivideByZero());
  if (b < 0) return Err(DivideByNegative());
  return Ok(a ~/ b);
}

sealed class AddError {}

class OverflowError extends AddError {}

const int intMaxValue = 9223372036854775807;

Result<int, AddError> add(int a, int b) {
  if (a > 0 && b > 0 && a > intMaxValue - b) return Err(OverflowError());
  return Ok(a + b);
}

sealed class DivideNonNegativeThenAddError implements DivideError, AddError {}

Result<int, DivideNonNegativeThenAddError> divideNonNegativeThenAdd(int number, int divideBy, int addBy) {
  final res = divideNonNegative(number, divideBy)?; // res should be int, ? propagates error if Result is DivideByZero | DivideByNegative
  final res2 = add(res, addBy)?; // res2 should be int, ? propagates error if Result is OverflowError
  return res2;
}

Here we have a function divideNonNegativeThenAdd that can produce a int result if it "succeeds" or a DivideNonNegativeThenAddError that can be instantiated with DivideByZero | DivideByNegative | OverflowError.
This function involves two function calls (divideNonNegative and add) that have the potential to produce their respective errors.

With a "propagate operator," we can avoid manual checks to return early from the function, reducing boilerplate.

And example of this operator is Rust ? operator (the example was inspired by that syntax):

The ? operator can only be used in functions whose return type is compatible with the value the ? is used on. This is because the ? operator is defined to perform an early return of a value out of the function...

@Mike278
Copy link
Author

Mike278 commented Jun 28, 2023

Assuming I have this right,

Yep that's a great example - even managed to capture the little details like Offline() => ConnectOffline() and having to pick names for states like "ConnectedAndDownloaded" and "ConnectAndDownloadInterrupted". And you can imagine how it explodes the deeper the call stack is.

One thing you can do is take advantage of the fact that sealed types don't have to be a single hierarchy. So you can reuse some of the classes like:

Oh of course 🤦‍♂️! That definitely improves things, I must have overlooked it because my mental model when writing tagged unions is "name: list, of, options" but the syntax is "option is part of lists name1, name2".

I tried adjusting the 2nd example a bit by tagging only the classes that may actually be returned, and that also works well. (Removing Connected from the return type is an example of what I was referring to with "this set + that set - 1 element".)

This works well for classes you own since you can just keep accumulating callers like:

sealed class DownloadResult implements 
    ConnectAndDownloadResult, 
    AnotherCallerResult, 
    SomeTransitiveCallerResult 
    /* ... more ... */ 
    {}

But I wonder what would be the implications of something like:

sealed class ConnectAndDownloadResult = DownloadResult, Offline
sealed class SomeCallerResult = DownloadResult, Offline, AuthRequired
sealed class TransitiveCallerResult = DownloadResult, SomeTypeFromAnotherLibrary // maybe disallow this one?

(Is it just me or does this seem oddly similar to The Expression Problem?)


Another approach could be to take advantage of the fact this is primarily for exceptions and add support for opting-in to an "exhaustive try-catch". Is it even possible to infer the set of errors that a function could throw? Maybe with enough restrictions like "only applies to final subclasses of Exception"? Something like:

bool someCondition() => DateTime.now().millisecondsSinceEpoch > 1;

/// Manual step 1:
class Connection {}
final class Offline implements Exception {}
final class BadLuck implements Exception {}
Connection connect() { 
  if (someCondition()) {
    return Connection();
  } else if (someCondition()) {
    throw BadLuck();
  } else {
    throw Offline();
  }
}

/// Manual step 2:
final class DownloadInterrupted implements Exception {}
final class AuthExpired implements Exception {}
String download(Connection connection) { 
  if (someCondition()) {
    return 'the data';
  } else if (someCondition()) {
    throw DownloadInterrupted();
  } else {
    throw AuthExpired();
  }
}

/// Automated:
String connectAndDownload() {
  try {
    final connection = connect();
    return download(connection);
  } on Offline {
    return 'some default data';
  }
}

void main() {
  try really hard { // opt-in
    print(connectAndDownload());
    // ERROR:
    // Exceptions thrown by the function 'connectAndDownload' are not exhaustively 
    // matched by the catch patterns since it doesn't match 'BadLuck'. 
    // Try adding a wildcard pattern or a pattern that matches 'BadLuck'.
  } on AuthExpired {
    print('re-auth');
  } on DownloadInterrupted {
    print('cancelled');
  } on Offline {
    // LINT: dead code, `connectAndDownload` doesn't throw `Offline`
  }
}

@Mike278
Copy link
Author

Mike278 commented Jun 28, 2023

I believe what @Mike278 is proposing (please correct me if I am mistaken) is an effortless method to propagate errors that may occur in a sequence of calls that could potentially produce errors.

Yeah I'm definitely interested in the "sequence of calls" aspect. Yours is another good example - especially because it looks like your mental model for writing tagged unions matches mine :)

sealed class DivideNonNegativeThenAddError implements DivideError, AddError {}

initially seems like it'd do what you want, but its putting DivideNonNegativeThenAddError underneath DividerError and AddError in the hiearchy instead of above.

With a "propagate operator," we can avoid manual checks to return early from the function, reducing boilerplate.

I think #2025 could help with this too. But one of the benefits of the "exhaustive try-catch" approach mentioned above is that Dart already has the shortest-possible "propagate operator": nothing!

@rubenferreira97
Copy link

rubenferreira97 commented Jun 28, 2023

initially seems like it'd do what you want, but its putting DivideNonNegativeThenAddError underneath DividerError and AddError in the hiearchy instead of above.

Yes, that was an example of how everything would work if it matched my expectations 😁. Today, I believe we would require a different approach to represent this. Although I generally love sealed classes, I really dislike the fact that, as far as I know, sealed classes in Dart cannot be composed by other sealed classes without modifying and thus polluting the definition of the top-level sealed class that we intend to compose.

Currently, in Dart, you can achieve composition of sealed classes using the following approach:

class DivideByZero extends DivideError implements DivideNonNegativeThenAddError {}

class DivideByNegative extends DivideError implements DivideNonNegativeThenAddError {}

class OverflowError extends AddError implements DivideNonNegativeThenAddError {}

sealed class DivideNonNegativeThenAddError {}

DivideNonNegativeThenAddError a = DivideByZero(); // Works
DivideNonNegativeThenAddError b = DivideByNegative(); // Works
DivideNonNegativeThenAddError c = OverflowError(); // Works

However, in my opinion, this approach feels incorrect here because it adds types from the top level instead of the bottom. In this case, we genuinely want to create a completely new sealed class.

Ideally, I would expect a way to achieve this without modifying the top-level sealed class definition, like so:

// Ignore the obnoxious syntax, this is kinda pseudocode
sealed class DivideNonNegativeThenAddError {
  hierarchy DivideError,
  hierarchy AddError,
  class AnotherError(),
  ...
}

// I think the closest approach using unions would be:
// type DivideNonNegativeThenAddError = DivideError | AddError | AnotherError

DivideNonNegativeThenAddError a = DivideByZero(); // Works
DivideNonNegativeThenAddError b = DivideByNegative(); // Works
DivideNonNegativeThenAddError c = OverflowError(); // Works
DivideNonNegativeThenAddError c = AnotherError(); // Works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request Requests to resolve a particular developer problem
Projects
None yet
Development

No branches or pull requests

3 participants