Fix most strong mode warnings.

Fix most strong mode warnings.

The remaining warnings will need type-asserting wrappers to fix, which
are coming in a separate CL.

R=lrn@google.com

Committed: https://github.com/dart-lang/async/commit/783a5f6bb3d2a11e5923fbd7fc47ec89dcc1824c

[Lasse Reichstein Nielsen, 2016-03-29 21:53:59]

LGTM, even if I don't think it's an improvement.

https://codereview.chromium.org/1841223002/diff/1/lib/src/delegate/future.dart
File lib/src/delegate/future.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:10: final Future<T> _future;
I would have thought that this was a feature - you could use the delegating
future to cast a Future<Subclass> to a Future<Superclass> (or vice versa if you
happen to know that the result is still valid).

https://codereview.chromium.org/1841223002/diff/1/lib/src/result.dart
File lib/src/result.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/result.dart#newcode80
lib/src/result.dart:80: return future.then((value) => new ValueResult(value),
No <T> on the new ValueResult?

https://codereview.chromium.org/1841223002/diff/1/lib/src/result.dart#newcode99
lib/src/result.dart:99: /// Shorthand for transforming the stream using
[captureStreamTransformer].
That is not longer true, so remove this comment line.

It's also annoying that you have to create a new transformer for every call.
Type parameters suck! :)

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart
File lib/src/result/error.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart#n...
lib/src/result/error.dart:11: class ErrorResult<T> implements Result<T> {
I really dislike adding <T> here - it's completely unnecessary since the class
doesn't use it for anything.

An ErrorResult is a Result<T> for any <T>, which is why it was declared as
Result<dynamic>. That's actually accurate - if you get an error from a
Future<int>, you should be able to make a Future<String> with the same error
without the error enforcing the type of the future unnecessarily.

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart#n...
lib/src/result/error.dart:30: Future<T> get asFuture => new Future.error(error,
stackTrace);
Should this be  new Future<T>.error ?
(Then, at least, the type is used for something).

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/future.dart
File lib/src/result/future.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/future.dart#...
lib/src/result/future.dart:24: resultFuture = new ResultFuture._(() async {
new ResultFuture<T>._ ?

https://codereview.chromium.org/1841223002/diff/1/lib/src/single_subscription...
File lib/src/single_subscription_transformer.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/single_subscription...
lib/src/single_subscription_transformer.dart:25: // type parameter and avoid
this conversion.
The extra type parameter allows you to cast from one stream type to another -
e.g., for doing:

 Stream<Bar> bars = streamOfFoo
   .where((x) => x is Bar)
   .transform(new SingleSubscriptionTransformer<Foo,Bar>());

Maybe we should have a cast operation on streams themselves (which makes sense
if we have generic methods).

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_group.dart
File lib/src/stream_group.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_group.dart#n...
lib/src/stream_group.dart:15: /// If the `StreamGroup` is constructed using [new
StreamGroup], [stream] will be
While-you-are-here: long line.

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_queue.dart
File lib/src/stream_queue.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:504: _completer.completeError(event.asError.error,
event.asError.stackTrace);
Why not event.complete(_completer)?

Is it because the error-event is not a Result<int> but a Result<T>?
(So the extra type parameter on the error result breaks the usability in cases
like this)

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_splitter.dart
File lib/src/stream_splitter.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_splitter.dar...
lib/src/stream_splitter.dart:84: onCancel: () => _onCancel(controller));
Maybe:
 
  var controller = new StreamController<T>(
     onListen: ...,
     ...,
     onResume: ...);
  controller.onCancel = (() => _onCancel(controller));


I think I landed setters for the controller callbacks at some point.

https://codereview.chromium.org/1841223002/diff/1/lib/src/subscription_stream...
File lib/src/subscription_stream.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/subscription_stream...
lib/src/subscription_stream.dart:53: // Wrap the subscription to ensure correct
type parameter.
Definite no to having comments inside conditional expressions.
Keep the if-statement, even if you have to type the variable.
(Or maybe we should change strong mode so it can infer types from the initial
assignments to a variable, even if they are not as an initializer expression).

[nweiz, 2016-03-30 00:56:19]

Code review changes

[nweiz, 2016-03-30 00:57:19]

https://codereview.chromium.org/1841223002/diff/1/lib/src/delegate/future.dart
File lib/src/delegate/future.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/delegate/future.dar...
lib/src/delegate/future.dart:10: final Future<T> _future;
On 2016/03/29 21:53:59, Lasse Reichstein Nielsen wrote:
> I would have thought that this was a feature - you could use the delegating
> future to cast a Future<Subclass> to a Future<Superclass> (or vice versa if
you
> happen to know that the result is still valid).

You're still allowed to pass Future<Subclass> to a function that declares
Future<Superclass>, or vice versa, even in strong mode. What you aren't allowed
to do is pass in a value whose static type is Future<dynamic> or
Future<UnrelatedType>.

If you have special knowledge that your Future<dynamic> will actually emit a T,
that's what asserting wrappers like the ones I added to collection are for. I
intend to add those here as well in a future CL.

https://codereview.chromium.org/1841223002/diff/1/lib/src/result.dart
File lib/src/result.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/result.dart#newcode80
lib/src/result.dart:80: return future.then((value) => new ValueResult(value),
On 2016/03/29 21:53:59, Lasse Reichstein Nielsen wrote:
> No <T> on the new ValueResult?

It's inferred, but now that you mention it there should be one on the
ErrorResult since onError is pretty much untyped.

https://codereview.chromium.org/1841223002/diff/1/lib/src/result.dart#newcode99
lib/src/result.dart:99: /// Shorthand for transforming the stream using
[captureStreamTransformer].
On 2016/03/29 21:53:59, Lasse Reichstein Nielsen wrote:
> That is not longer true, so remove this comment line.
> 
> It's also annoying that you have to create a new transformer for every call.
> Type parameters suck! :)

Done.

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart
File lib/src/result/error.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart#n...
lib/src/result/error.dart:11: class ErrorResult<T> implements Result<T> {
On 2016/03/29 21:53:59, Lasse Reichstein Nielsen wrote:
> I really dislike adding <T> here - it's completely unnecessary since the class
> doesn't use it for anything.
> 
> An ErrorResult is a Result<T> for any <T>, which is why it was declared as
> Result<dynamic>. That's actually accurate - if you get an error from a
> Future<int>, you should be able to make a Future<String> with the same error
> without the error enforcing the type of the future unnecessarily.

I understand the logic behind this being a Result<dynamic>, but it just doesn't
work with strong mode. In strong mode, Result<dynamic> is! Result<T> for any T
other than dynamic.

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart#n...
lib/src/result/error.dart:30: Future<T> get asFuture => new Future.error(error,
stackTrace);
On 2016/03/29 21:53:59, Lasse Reichstein Nielsen wrote:
> Should this be  new Future<T>.error ?
> (Then, at least, the type is used for something).

That's inferred via downward inference.

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/future.dart
File lib/src/result/future.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/future.dart#...
lib/src/result/future.dart:24: resultFuture = new ResultFuture._(() async {
On 2016/03/29 21:53:59, Lasse Reichstein Nielsen wrote:
> new ResultFuture<T>._ ?

Inference! :)

https://codereview.chromium.org/1841223002/diff/1/lib/src/single_subscription...
File lib/src/single_subscription_transformer.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/single_subscription...
lib/src/single_subscription_transformer.dart:25: // type parameter and avoid
this conversion.
On 2016/03/29 21:53:59, Lasse Reichstein Nielsen wrote:
> The extra type parameter allows you to cast from one stream type to another -
> e.g., for doing:
> 
>  Stream<Bar> bars = streamOfFoo
>    .where((x) => x is Bar)
>    .transform(new SingleSubscriptionTransformer<Foo,Bar>());
> 
> Maybe we should have a cast operation on streams themselves (which makes sense
> if we have generic methods).

I think having an explicit means of casting is better than hijacking this class
for an unrelated purpose. I'm working on typed wrappers for async like the ones
I made for collection, but having built-in methods would be even better.

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_group.dart
File lib/src/stream_group.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_group.dart#n...
lib/src/stream_group.dart:15: /// If the `StreamGroup` is constructed using [new
StreamGroup], [stream] will be
On 2016/03/29 21:53:59, Lasse Reichstein Nielsen wrote:
> While-you-are-here: long line.

Done.

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_queue.dart
File lib/src/stream_queue.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:504: _completer.completeError(event.asError.error,
event.asError.stackTrace);
On 2016/03/29 21:53:59, Lasse Reichstein Nielsen wrote:
> Why not event.complete(_completer)?
> 
> Is it because the error-event is not a Result<int> but a Result<T>?
> (So the extra type parameter on the error result breaks the usability in cases
> like this)

Yes, that's why.

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_splitter.dart
File lib/src/stream_splitter.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/stream_splitter.dar...
lib/src/stream_splitter.dart:84: onCancel: () => _onCancel(controller));
On 2016/03/29 21:53:59, Lasse Reichstein Nielsen wrote:
> Maybe:
>  
>   var controller = new StreamController<T>(
>      onListen: ...,
>      ...,
>      onResume: ...);
>   controller.onCancel = (() => _onCancel(controller));
> 
> 
> I think I landed setters for the controller callbacks at some point.

Done.

https://codereview.chromium.org/1841223002/diff/1/lib/src/subscription_stream...
File lib/src/subscription_stream.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/subscription_stream...
lib/src/subscription_stream.dart:53: // Wrap the subscription to ensure correct
type parameter.
On 2016/03/29 21:53:59, Lasse Reichstein Nielsen wrote:
> Definite no to having comments inside conditional expressions.
> Keep the if-statement, even if you have to type the variable.
> (Or maybe we should change strong mode so it can infer types from the initial
> assignments to a variable, even if they are not as an initializer expression).

I'm actually just going to remove the wrapping, since the original parameter is
type-annotated anyway.

[nweiz, 2016-03-30 00:57:26]

Description was changed from

==========
Fix most strong mode warnings.

The remaining warnings will need type-asserting wrappers to fix, which
are coming in a separate CL.

R=lrn@google.com
==========

to

==========
Fix most strong mode warnings.

The remaining warnings will need type-asserting wrappers to fix, which
are coming in a separate CL.

R=lrn@google.com

Committed:
https://github.com/dart-lang/async/commit/783a5f6bb3d2a11e5923fbd7fc47ec89dcc...
==========

[nweiz, 2016-03-30 00:57:27]

Committed patchset #3 (id:40001) manually as
783a5f6bb3d2a11e5923fbd7fc47ec89dcc1824c (presubmit successful).

[Lasse Reichstein Nielsen, 2016-03-30 16:00:18]

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart
File lib/src/result/error.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart#n...
lib/src/result/error.dart:30: Future<T> get asFuture => new Future.error(error,
stackTrace);
That doesn't work in the VM or pure Dart.

This code should be Dart (non-strong-mode) compatible, and if the return type
says Future<T>, then it should return a Future<T>, which it doesn't as written.

[nweiz, 2016-03-30 19:11:54]

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart
File lib/src/result/error.dart (right):

https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart#n...
lib/src/result/error.dart:30: Future<T> get asFuture => new Future.error(error,
stackTrace);
On 2016/03/30 16:00:18, Lasse Reichstein Nielsen wrote:
> That doesn't work in the VM or pure Dart.
> 
> This code should be Dart (non-strong-mode) compatible, and if the return type
> says Future<T>, then it should return a Future<T>, which it doesn't as
written.
> 

I'm not sure what you mean by "pure Dart", but this does work in the VM. In
checked mode, `Future<generic> is Future<T>`.

[Lasse Reichstein Nielsen, 2016-03-30 22:18:11]

On 2016/03/30 19:11:54, nweiz wrote:
> https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart
> File lib/src/result/error.dart (right):
> 
>
https://codereview.chromium.org/1841223002/diff/1/lib/src/result/error.dart#n...
> lib/src/result/error.dart:30: Future<T> get asFuture => new
Future.error(error,
> stackTrace);
> On 2016/03/30 16:00:18, Lasse Reichstein Nielsen wrote:
> > That doesn't work in the VM or pure Dart.
> > 
> > This code should be Dart (non-strong-mode) compatible, and if the return
type
> > says Future<T>, then it should return a Future<T>, which it doesn't as
> written.
> > 
> 
> I'm not sure what you mean by "pure Dart", but this does work in the VM. In
> checked mode, `Future<generic> is Future<T>`.

I mean non-strong-mode Dart, and while `Future<dynamic> is Future<T>` I want the
result to also satisfy that it `is! Future<not-T>`, and fail in checked mode if
you call `.then((notT x) => something)` on it because it expects a T->something
function.

[nweiz, 2016-03-30 22:52:29]

On 2016/03/30 22:18:11, Lasse Reichstein Nielsen wrote:
> I mean non-strong-mode Dart, and while `Future<dynamic> is Future<T>` I want
the
> result to also satisfy that it `is! Future<not-T>`, and fail in checked mode
if
> you call `.then((notT x) => something)` on it because it expects a
T->something
> function.

Why? It's extremely non-conventional to use is checks on generics in general,
and even more so on futures in particular. The same is true for typing the
callback argument to then(), especially now that async/await exists.

Besides, we're moving towards a strong-mode-only world. I don't think it makes
sense to add extra annotations that only make sense in checked or unchecked
modes when they're going away anyway.

[Lasse Reichstein Nielsen, 2016-03-30 23:54:22]

On 2016/03/30 22:52:29, nweiz wrote:
> On 2016/03/30 22:18:11, Lasse Reichstein Nielsen wrote:
> > I mean non-strong-mode Dart, and while `Future<dynamic> is Future<T>` I want
> the
> > result to also satisfy that it `is! Future<not-T>`, and fail in checked mode
> if
> > you call `.then((notT x) => something)` on it because it expects a
> T->something
> > function.
> 
> Why? It's extremely non-conventional to use is checks on generics in general,
> and even more so on futures in particular. The same is true for typing the
> callback argument to then(), especially now that async/await exists.

For the same reason we have type parameters everywhere else, and why we make
sure to return a List<int> from `new List<int>` instead of a just a
`List<dynamic>` - because it is a more precise type, and it allows catching
errors earlier - and it prevents errors in the few cases where someone does do
an `is List<String>` on what was intended to be just a `List<int>`.

We generally try to have the correct type parameters on objects we create.

Otherwise we could just remove all type parameters at runtome, which we are not
doing (and as far as I know are not (yet) planning to do).


> Besides, we're moving towards a strong-mode-only world. I don't think it makes
> sense to add extra annotations that only make sense in checked or unchecked
> modes when they're going away anyway.

We are moving towards that (or something like that), but we are not there yet.
Until then, I prefer to make things both Dart 1.0 and strong-mode compatible.
The annotations should be harmless in strong-mode.

[nweiz, 2016-03-31 22:52:40]

On 2016/03/30 23:54:22, Lasse Reichstein Nielsen wrote:
> On 2016/03/30 22:52:29, nweiz wrote:
> > On 2016/03/30 22:18:11, Lasse Reichstein Nielsen wrote:
> > > I mean non-strong-mode Dart, and while `Future<dynamic> is Future<T>` I
want
> > the
> > > result to also satisfy that it `is! Future<not-T>`, and fail in checked
mode
> > if
> > > you call `.then((notT x) => something)` on it because it expects a
> > T->something
> > > function.
> > 
> > Why? It's extremely non-conventional to use is checks on generics in
general,
> > and even more so on futures in particular. The same is true for typing the
> > callback argument to then(), especially now that async/await exists.
> 
> For the same reason we have type parameters everywhere else, and why we make
> sure to return a List<int> from `new List<int>` instead of a just a
> `List<dynamic>` - because it is a more precise type, and it allows catching
> errors earlier - and it prevents errors in the few cases where someone does do
> an `is List<String>` on what was intended to be just a `List<int>`.
> 
> We generally try to have the correct type parameters on objects we create.
> 
> Otherwise we could just remove all type parameters at runtome, which we are
not
> doing (and as far as I know are not (yet) planning to do).

This may be true for collections, but it's definitely not true for futures. In
spec-mode, futures almost universally have dynamic as their reified type because
that's what all the methods as well as async/await produced. It's categorically
unsafe to rely on the reified types of futures, and there are no
style-guide-compliant situations in which this refied type will help catch
errors.

> > Besides, we're moving towards a strong-mode-only world. I don't think it
makes
> > sense to add extra annotations that only make sense in checked or unchecked
> > modes when they're going away anyway.
> 
> We are moving towards that (or something like that), but we are not there yet.
> Until then, I prefer to make things both Dart 1.0 and strong-mode compatible.
> The annotations should be harmless in strong-mode.

I agree that we should write code to be spec-mode compatible for now—and the
existing code without the extra annotation is that. What I don't think we should
do is add extra code that will be useless in strong mode just to get slightly
better spec-mode checking in a situation that's very unlikely to occur in
practice.