[Breaking change request] The runZoned method will now not catch synchronously thrown errors even when given an error handler.

[lrhn]

2020-03-23 - Rewritten to state the actual change decided upon.

Summary

The runZoned method will not have an onError parameter. A runZonedGuarded function is added which has an onError parameter.

The IOOverrides and HttpOverrides static run* methods will not have onError or zoneSpecification parameters.

What is changing:

The runZoned method is declared as

R runZoned<R>(
  R body(), {
  Map zoneValues,
  ZoneSpecification zoneSpecification
})

and we add:

R? runZonedGuarded<R>(
  R body(), 
  void onError(Object error, StackTrace stack), {
  Map zoneValues,
  ZoneSpecification zoneSpecification,
})

The static methods from dart:io have their zoneSpecification and onError parameters removed.

Why is this changing?

The existing uses of runZoned can be split into two groups: Those with onError and those without.
Those without an onError always return the result of calling body. Those with onError return either that or null.

In Null Safe code, using the same function for both operations would require the return type to always be nullable, which makes the first group unnecessarily cumbersome.
Other workarounds are either fragile or surprising (for example, as originally proposed, throwing if the R type is not nullable and returning null if not).

This change is larger, but ends up with a better API for Null Safety.

The dart:io override classes would have to split their static run* methods in the same way to be compatible. Instead it was decided that the onError and zoneSpecification parameters were not used often enough for them to carry their own weight. Removing them gives a better API, and users can still use the dart:async runZoned/runZonedGuarded methods as well, if necessary.

Expected impact

Code which uses runZoned with an onError argument to catch synchronous errors will need to call runZonedGuarded instead.

Migration

For now, the onError parameter is only @deprecated, and in the Null Safe SDK is made to throw if catching a synchronous error and the return type is not nullable. Existing code should keep running until it is migrated to using runZonedGuarded.
When enough code has been migrated off the deprecated member, it will be removed entirely in a separate CL.

[franklinyow]

cc @Hixie @matanlurey @dgrove @vsmenon for review and approval.

[Hixie]

It's not really clear to me when you would intentionally use onError with runZoned and expect a synchronous return value, other than the specific case of wanting to have the handler handle the error and the call to return null.

[lrhn]

@Hixie Not sure I understand.

The current uses of runZoned fall into a number of use-cases:

• Non-error zone: No onError and no handleUncaughtError in the zone specification (if any). The call just returns the result of calling body in a zone with some other non-error zone features used.
• Error zone for async errors: The body always returns a value, or it is even void, and the error handler is just for async errors. The onError is used as a shorthand if no other zone feature in the zone specification needs to be set.
• Preventing all errors from escaping: The onError is used to catch all errors, synchronous and asynchronous, so that the call to runZoned(body ...) never causes an error.

The third item is the one that we cannot support with null safe types without making the return type nullable (R? instead of R). Because we might not be able to return null, we might have to throw synchronously anyway, which breaks that use-case.
The second use-case is unaffected because it never needs to return null as a default value.

If we change the return type to R? then we make the first two use-cases more complicated even though they don't use the synchronous error capture. That's a bad design because it puts the cost of the feature on the code that doesn't use that feature.

In languages with overloading, it would not be a problem because they would just have a version without onError returning R and a version with onError returning R?. We can't do that.

We could introduce a second R? runZonedGuarded<R>(...same arguments...), possibly onError as positional required parameter, which works like the current runZoned and captures sync errors and returns null. Then we can migrate code needing the behavior to that function instead. That can be used as breakage mitigation, it doesn't change that we are breaking the behavior of the existing runZoned.

[Hixie]

If it was up to me I think my proposal would be to deprecate runZoned entirely and have two alternatives, one without error handling that returns a non-nullable value, and one with error handling that returns a nullable value.

That said, it seems Zone already has a more elaborate API here. Indeed, runZoned and Zone.run, Zone.runGuarded, et al, are quite different in semantics and that seems unnecessarily inconsistent. I think we should probably just rationalize these two APIs so that they are equivalent. If we do that, then we can simultaneously make sure that our new API is rational in an NNBD world (e.g. looks like Zone.runGuarded returns void, not R, and so side-steps the problem; why can't we do the same here?).

[lrhn]

We can do a lot of things if we want to change or replace runZoned.

The one thing we cannot do is to keep it working like it does now (sometimes returning null) without making the API bad for users who just want the result of their non-throwing body function.

That does suggest another approach: Make the return type R?, which is highly annoying for people who don't need the null, but provide an alternative function for those that they can migrate to instead.

So:

/// Keeps working like now. Perhaps deprecate it.
R? runZoned<R>(R body(), { ... }) ...

/// Captures all errors, sync or async. Returns nothing.
void runZonedGuarded(void body(), {..., void onError(Object error, StackTrace trace)})  ...

/// Captures async errors only
R runZonedWith(R body(), { ..., void onError(Object error, StackTrace trace)}) ...

[Hixie]

runZonedWith would be inconsistent with the names on Zone, which seems like something to avoid. Also it seems weird to only catch async errors. I don't understand that use case. As far as I can tell, that's also not something any of the Zone methods do.

How about:

/// Captures all errors, sync or async. Returns nothing.
void runZonedGuarded(void body(), {..., void onError(Object error, StackTrace trace)})  ...

/// Does not capture errors (no error handler).
R runZoned(R body(), { ... }) ...

If I understand the Zone API correctly, that would at least be consistent with Zone's runGuarded and run.

[lrhn]

I like the APIs. It's what I was going for above, but without keeping the legacy function around. That's also the biggest issue.

This would be a hard breaking change since the runZoned function can no longer be called with onError. All existing code calling runZoned with an onError needs to be migrated.

That suggests a two-step migration:

• First introduce runZoneGuarded as defined above in both legacy and NNBD libraries (probably make onError a required positional parameter, because it isn't really guarded without one). Deprecate onError on runZoned.
• In the NNBD SDK only, remove the onError from runZoned.

The we won't remove the onError until we release the unforked core librarires. That gives people time to migrate their code, and if they don't, they will be forced to when we unfork the libraries.

(The runZoned here would be able to catch only async errors by providing a ZoneSpecifiction with an uncaughtErrorHander. That's how error zones usually work. I also think we should remove the error zone concept.)

[Hixie]

LGTM.