Change and documentat how completer handles futures.

Change and documentat how completer handles futures.

Completer.complete changes signature to accept 'var' and chain if the
argument is a future.
Completer.completeError throws if error is a future or null.
Documentation updated to match.
Fix error in implementation of completeError which dropped listeners when
chained. If the chained-to future is not a _Future, the listeners cannot
be forwarded.

BUG= http://dartbug.com/14211 http://dartbug.com/14212 http://dartbug.com/14435
R=floitsch@google.com

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

[floitsch, 2013-10-29 18:13:35]

I'm not convinced that we should reject Future errors, but otherwise LGTM.

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

https://codereview.chromium.org/50373002/diff/30001/sdk/lib/async/future.dart...
sdk/lib/async/future.dart:508: * The argument [error] must not be `null`, and it
must not be a future.
Why this restriction?

I'm ok with it being a future. We are just not going to wait for it.

https://codereview.chromium.org/50373002/diff/30001/sdk/lib/async/future_impl...
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/50373002/diff/30001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:34: if (error is Future) {
ditto. I don't think this restriction is necessary.

https://codereview.chromium.org/50373002/diff/30001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:52: if (error is Future) {
ditto

https://codereview.chromium.org/50373002/diff/30001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:243: _Future target = this;
spurious change?

[Lasse Reichstein Nielsen, 2013-10-30 09:22:16]

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

https://codereview.chromium.org/50373002/diff/30001/sdk/lib/async/future.dart...
sdk/lib/async/future.dart:508: * The argument [error] must not be `null`, and it
must not be a future.
If the result of a future cannot be a future, I assumed that the error result
couldn't either. In that case, we don't have a value to use here.
Since you say that we won't wait for it, I assume you mean that the future will
be the error value. 
I'm not sure I like the difference, *but* we do allow futures elsewhere if we
throw them, e.g.,  
  new Future(() { throw new Future.value(42); }).catchError((Future f) =>
f).then(print);  // Prints 42

I'll change it back to allowing futures, and document it as such.
Why not null, btw?

https://codereview.chromium.org/50373002/diff/30001/sdk/lib/async/future_impl...
File sdk/lib/async/future_impl.dart (right):

https://codereview.chromium.org/50373002/diff/30001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:34: if (error is Future) {
Removing.

https://codereview.chromium.org/50373002/diff/30001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:52: if (error is Future) {
ditto too.

https://codereview.chromium.org/50373002/diff/30001/sdk/lib/async/future_impl...
sdk/lib/async/future_impl.dart:243: _Future target = this;
ack, yes. Left over from debugging.
Removed.

[Lasse Reichstein Nielsen, 2013-10-30 09:22:37]

Committed patchset #2 manually as r29518 (presubmit successful).

[floitsch, 2013-10-30 18:50:53]

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

https://codereview.chromium.org/50373002/diff/30001/sdk/lib/async/future.dart...
sdk/lib/async/future.dart:508: * The argument [error] must not be `null`, and it
must not be a future.
On 2013/10/30 09:22:17, Lasse Reichstein Nielsen wrote:
> If the result of a future cannot be a future, I assumed that the error result
> couldn't either. In that case, we don't have a value to use here.
> Since you say that we won't wait for it, I assume you mean that the future
will
> be the error value. 
> I'm not sure I like the difference, *but* we do allow futures elsewhere if we
> throw them, e.g.,  
>   new Future(() { throw new Future.value(42); }).catchError((Future f) =>
> f).then(print);  // Prints 42
> 
> I'll change it back to allowing futures, and document it as such.
> Why not null, btw?
> 
Error handlers are frequently piped (like for example in streams: listen(...,
onDone: completer.complete(), onError: completer.completeError).
It would be annoying if they needed to look out for futures.
That said: it is clearly a really bad idea to make a future an error (especially
if the future completes with an error)

I'm not sure why we disallow null. I would be ok with allowing it as well.