Add second argument to Future error handlers.

Add second argument to Future error handlers.

R=lrn@google.com, nweiz@google.com

Committed: https://code.google.com/p/dart/source/detail?r=28510

[Lasse Reichstein Nielsen, 2013-10-04 09:17:39]

lgtm

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/async_error....
File sdk/lib/async/async_error.dart (right):

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/async_error....
sdk/lib/async/async_error.dart:30: // want to make the class public.
We can implement Error without problems. We have the stackTrace getter, so it's
fine.

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/async_error....
sdk/lib/async/async_error.dart:44: if (trace == null) trace =
getAttachedStackTrace(error);
This looks wrong to me. If anything, the attached stack trace should be detached
when the object is created, not when the toString is called, and it should only
be used if an explicit stacktrace isn't provided in the constructor.
Something like:
  _UncaughtAsyncError(error, StackTrace stackTrace) 
    : super(error, stackTrace != null ? stackTrace :
getAttachedStackTrace(error)) {
     _attachStackTrace(error, null);
  }

Then we can do:
  if (trace == null && error is Error) trace = error.stackTrace;
to get at the third way a stack trace can be attached.

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/future.dart#...
sdk/lib/async/future.dart:269: * `onError(error, StackTrace stackTrace)`.
Depending on the function type
Per stream-comments, how about:
  "If [onError] is of the latter type, it is called with both the
   error and the stackTrace as arguments, otherwise it's called with
   just the error."

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:9: typedef dynamic ZoneBinaryCallback(arg1, arg2);
Maybe make a
 typedef ZoneErrorCallback(arg1, StackTrace arg2);
and use that for testing. That way we won't call any binary function with a
stackTrace.

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:22: typedef ZoneUnaryCallback
RegisterBinaryCallbackHandler(
return type should be ZoneBinaryCallback.

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:775: if (onError is ZoneBinaryCallback)
Braces for if-body.

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:783: // trace?
No.
You caught the error and handled it, and then threw another error.
If the old stack-trace was needed, so was the old error.

https://codereview.chromium.org/25027004/diff/6001/tests/lib/async/zone_debug...
File tests/lib/async/zone_debug_test.dart (right):

https://codereview.chromium.org/25027004/diff/6001/tests/lib/async/zone_debug...
tests/lib/async/zone_debug_test.dart:51: debugUncaughtHandler(
void return type?

[floitsch, 2013-10-05 17:22:59]

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/async_error....
File sdk/lib/async/async_error.dart (right):

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/async_error....
sdk/lib/async/async_error.dart:30: // want to make the class public.
On 2013/10/04 09:17:39, Lasse Reichstein Nielsen wrote:
> We can implement Error without problems. We have the stackTrace getter, so
it's
> fine.

Done.

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/async_error....
sdk/lib/async/async_error.dart:44: if (trace == null) trace =
getAttachedStackTrace(error);
On 2013/10/04 09:17:39, Lasse Reichstein Nielsen wrote:
> This looks wrong to me. If anything, the attached stack trace should be
detached
> when the object is created, not when the toString is called, and it should
only
> be used if an explicit stacktrace isn't provided in the constructor.
> Something like:
>   _UncaughtAsyncError(error, StackTrace stackTrace) 
>     : super(error, stackTrace != null ? stackTrace :
> getAttachedStackTrace(error)) {
>      _attachStackTrace(error, null);
>   }
> 
> Then we can do:
>   if (trace == null && error is Error) trace = error.stackTrace;
> to get at the third way a stack trace can be attached.

done.

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/future.dart#...
sdk/lib/async/future.dart:269: * `onError(error, StackTrace stackTrace)`.
Depending on the function type
On 2013/10/04 09:17:39, Lasse Reichstein Nielsen wrote:
> Per stream-comments, how about:
>   "If [onError] is of the latter type, it is called with both the
>    error and the stackTrace as arguments, otherwise it's called with
>    just the error."

Same as for Stream:
   * The [onError] callback must be of type `void onError(error)` or
   * `void onError(error, StackTrace stackTrace)`. If [onError] accepts
   * two arguments it is called with the stack trace (which could be `null` if
   * the stream itself received an error without stack trace).
   * Otherwise it is called with just the error object.
done.

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:9: typedef dynamic ZoneBinaryCallback(arg1, arg2);
On 2013/10/04 09:17:39, Lasse Reichstein Nielsen wrote:
> Maybe make a
>  typedef ZoneErrorCallback(arg1, StackTrace arg2);
> and use that for testing. That way we won't call any binary function with a
> stackTrace.

started doing this, but then decided against it. It is simpler if we just look
for 2 arguments. Since I wouldn't want to expose this typedef it also means that
other libraries can more easily do the same operation.

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:22: typedef ZoneUnaryCallback
RegisterBinaryCallbackHandler(
On 2013/10/04 09:17:39, Lasse Reichstein Nielsen wrote:
> return type should be ZoneBinaryCallback.

Good catch. thanks.
(Not tested yet because of bug 13406 which means that the types are not yet used
everywhere).

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:775: if (onError is ZoneBinaryCallback)
On 2013/10/04 09:17:39, Lasse Reichstein Nielsen wrote:
> Braces for if-body.

Done.

https://codereview.chromium.org/25027004/diff/6001/sdk/lib/async/zone.dart#ne...
sdk/lib/async/zone.dart:783: // trace?
On 2013/10/04 09:17:39, Lasse Reichstein Nielsen wrote:
> No.
> You caught the error and handled it, and then threw another error.
> If the old stack-trace was needed, so was the old error.

ok.

https://codereview.chromium.org/25027004/diff/6001/tests/lib/async/zone_debug...
File tests/lib/async/zone_debug_test.dart (right):

https://codereview.chromium.org/25027004/diff/6001/tests/lib/async/zone_debug...
tests/lib/async/zone_debug_test.dart:51: debugUncaughtHandler(
On 2013/10/04 09:17:39, Lasse Reichstein Nielsen wrote:
> void return type?

The return-type is actually specified as being dynamic.
This is used when the uncaught-handler is used like a try/catch.

[floitsch, 2013-10-05 17:23:59]

Feel free to take another look.
I will commit together with the follow-up CLs.

[floitsch, 2013-10-05 18:09:47]

Adding Nathan and Bob for the scheduled test and barback changes.

[floitsch, 2013-10-05 18:10:18]

Adding Nathan and Bob for the scheduled test and barback changes.
(this time really...)

[Lasse Reichstein Nielsen, 2013-10-07 12:39:20]

still LGTM.

https://codereview.chromium.org/25027004/diff/14001/sdk/lib/async/async_error...
File sdk/lib/async/async_error.dart (right):

https://codereview.chromium.org/25027004/diff/14001/sdk/lib/async/async_error...
sdk/lib/async/async_error.dart:65: Error e = error;
Soon to be redundant cast! :)

[nweiz, 2013-10-07 20:30:39]

One change, otherwise pub/pkg changes lgtm.

https://codereview.chromium.org/25027004/diff/14001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/error_group.dart (right):

https://codereview.chromium.org/25027004/diff/14001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/error_group.dart:182: Future then(onValue(value),
{ Function onError }) {
No spaces before "Function" or after "onError".

[floitsch, 2013-10-10 13:47:33]

https://codereview.chromium.org/25027004/diff/14001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/error_group.dart (right):

https://codereview.chromium.org/25027004/diff/14001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/error_group.dart:182: Future then(onValue(value),
{ Function onError }) {
On 2013/10/07 20:30:39, nweiz wrote:
> No spaces before "Function" or after "onError".

Done.

[Lasse Reichstein Nielsen, 2013-10-10 13:49:14]

https://codereview.chromium.org/25027004/diff/14001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/error_group.dart (right):

https://codereview.chromium.org/25027004/diff/14001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/error_group.dart:182: Future then(onValue(value),
{ Function onError }) {
I actually prefer the spaces.

[nweiz, 2013-10-10 15:09:20]

https://codereview.chromium.org/25027004/diff/14001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/error_group.dart (right):

https://codereview.chromium.org/25027004/diff/14001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/error_group.dart:182: Future then(onValue(value),
{ Function onError }) {
On 2013/10/10 13:49:14, Lasse Reichstein Nielsen wrote:
> I actually prefer the spaces.

It's contrary to the style guide: "DON’T use a space after (, [, and {, or
before ), ], and }".

[floitsch, 2013-10-11 11:43:03]

Committed patchset #5 manually as r28510 (presubmit successful).