dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.09k stars 1.56k forks source link

StreamTransformer is difficult to use in Dart 2.0 #29878

Open vsmenon opened 7 years ago

vsmenon commented 7 years ago

This has come up several times, but I don't see a bug for it.

Here's a sample use that highlights the problem:

import 'dart:async';

Stream<int> change(Stream<Iterable> stream) {
  return stream.transform(new StreamTransformer<Iterable, int>.fromHandlers(
      handleData: (Iterable iter, EventSink<int> sink) {
    sink.add(iter.length);
  }));
}

void main() {
  Stream<Iterable> stream = new Stream<List<String>>.fromIterable([
    ["hello"],
    ["world"]
  ]);
  var result = change(stream);
  result.listen(print);
}

The change method looks just fine from a typing perspective. However, if we invoke it with Stream<T> where T is a proper subtype of Iterable, it will trigger a confusing runtime error:

Type '_StreamHandlerTransformer<Iterable, int>' is not a subtype of type 'StreamTransformer<List<String>, int>'

Intuitively, StreamTransformer should be contravariant on it's first type parameter (Iterable here). But, Dart does not support that.

vsmenon commented 7 years ago

A workaround is to modify the reified runtime type on the stream:

var result = change(stream.map<Iterable>((x) => x));

should do it.

lrhn commented 7 years ago

True, it's annoying, but a direct consequence of Dart's covariant generic classes.

A _StreamHandlerTransformer<Iterable, int> is not a subtype of StreamTransformer<List<String>,int> because Iterable is not a subtype of List<String> - it's the other way around.

We don't have a way to specify, or detect, that a type parameter of a class occurs only contravariantly and should be treated as such.

vsmenon commented 7 years ago

@donny-dont

Here's another case we're hitting in package:http:

Simplified, we end up with something like this:

import 'dart:async';
import 'dart:convert';
import 'dart:typed_data';

final codec = new AsciiCodec();

Stream<List<int>> read() {
  // Reified as a Stream<Uint8List>
  var stream = new Stream.fromIterable(<Uint8List>[codec.encode("hello"), codec.encode("world")]);
  return stream;
}

void main() {
  // This throws: Type 'AsciiDecoder' is not a subtype of type 'StreamTransformer<Uint8List, String>'
  codec.decodeStream(read()).then(print);
}
vsmenon commented 7 years ago

@floitschG @kasperl

Is there something actionable here the language/sdk team can do to improve? We know what the problem is. How do we fix?

I've heard ideas including language changes or new APIs for this functionality, but not clear if we're proceeding on any front.

matanlurey commented 7 years ago

+1 this is a major pain point. Honestly I'd love to see this just become a function definition:

typedef StreamTransformer = T Function<S, T>(Stream<S> input);
floitschG commented 7 years ago

The correct solution is to introduce contravariant generics. We currently have no plans to add those.

Making StreamTransformer a typedef works in simple cases, but all important StreamTransformers are also Converters. Converters aren't simple functions that take one element and return another. They are a group of functions (convert, fuse, bind, ...). That screams for a "container" class to bundle them together (call it Converter) and you are back to square one.

vsmenon commented 7 years ago

@floitschG How do you want to proceed? I don't think the status quo is working.

matanlurey commented 7 years ago

FWIW I have almost never used Converter, but I don't see why we couldn't support:

abstract class Converter<S, T> {
  const factory Converter.fromTransformer(StreamTransformer<S, T> transformer);
}

To handle this case.

vsmenon commented 7 years ago

E.g., a fairly minimal change:

https://codereview.chromium.org/2954383002/diff/1/sdk/lib/async/stream.dart

gets this last example running (and perhaps the rest?).

I'm not fond of writing dynamic (or Object if you prefer), but T is/was just incorrect.

lrhn commented 7 years ago

@matanlurey The problem is not to make a StreamTransformer into a Converter, but that many transformers are already converters, because all converters are tranformers:

abstract class Converter<S, T> implements StreamTransformer<S, T> { 

That means that they will have the same co-/contra-variance problems as you are seeing with StreamTransformer, it's not just about that class.

Even if we make StreamTransformer into a function type so it's contravariant in the source type, that won't help Converter. you could extract a transformer from a converter by tearing off the bind method, but it wouldn't be a transformer unless we also add a call method that forwards to bind, and then you have function subtyping and class subtyping being at odds with each other. That's not going to be ... at all understandable.

class C<T> {
   void call(T x);
}
typedef F = void Function<T>(T);
main() {
   var i = new C<int>();
   var n  = new C<num>();
   print(n is C<int>);  // false
   print(n is F<int>);  // true
   print(n is C<Object>);  // true
   print(n is F<Object>);  // false
   C<Object> o = n;
   F<Object> fo = o;  // No warning, strong mode runtime error
}

So, we can perhaps fix the problem for transformer (if we make it a function type, not a class type), but that won't help Converter.

matanlurey commented 7 years ago

FWIW I'm very ok with breaking Converter. I don't see it used much outside of JSON or UTF8 in client code and it seems easy enough to wrap those two in the core library.

On Mon, Jun 26, 2017, 10:52 PM Lasse R.H. Nielsen notifications@github.com wrote:

@matanlurey https://github.com/matanlurey The problem is not to make a StreamTransformer into a Converter, but that many transformers are already converters, because all converters are tranformers:

abstract class Converter<S, T> implements StreamTransformer<S, T> {

That means that they will have the same co-/contra-variance problems as you are seeing with StreamTransformer, it's not just about that class.

Even if we make StreamTransformer into a function type so it's contravariant in the source type, that won't help Converter. you could extract a transformer from a converter by tearing off the bind method, but it wouldn't be a transformer unless we also add a call method that forwards to bind, and then you have function subtyping and class subtyping being at odds with each other. That's not going to be ... at all understandable.

class C { void call(T x); }typedef F = void Function(T);main() { var i = new C(); var n = new C(); print(n is C); // false print(n is F); // true print(n is C); // true print(n is F); // false C o = n; F fo = o; // No warning, strong mode runtime error }

So, we can perhaps fix the problem for transformer (if we make it a function type, not a class type), but that won't help Converter.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/29878#issuecomment-311260007, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKQ7jv7xkCTU_Em9P9650kwSSsT0p4_ks5sIJiAgaJpZM4N6jMu .

franklinyow commented 4 years ago

@munificent @lrhn Please re-evaluate this and do one of the following:

  1. Add it to a milestone if it is still P1
  2. Change priority label and add milestone if no longer P1
  3. Close, if this issue is no longer needed
munificent commented 4 years ago

Definitely not a P1. I think this is worth revisiting once we ship the variance changes.

lrhn commented 4 years ago

Agree. There isn't much we can do with the current API and feature-set. Changing the API, say to it being a function type, would be breaking. The variance feature would allow us to mark the input type of the transformer as contravariant, which would likely solve most of the issues, and there'd be a migration for that feature anyway.