Type error at runtime when passing Stream.sink as an argument after null safety migration

[annagrin]

We encountered some cryptic and hard to solve errors during null safety migration of dwds.

I suspect the issue is related to the variance issue in dart, am I correct (dart-lang/language#524)? Is there a way to annotate the parameter in addFoo to make sure we get a compilation error on callsite?

Simplified example

This code stops working after null safety migration:

import 'dart:async';

// Code in library being migrated
void main() {
  // Should be StreamController<Map<String, Object?>>() after null safety
  // migration but it's hard to detect due to absence of compilation errors. 
  // Runtime error does not help detect the problem.
  final controller = StreamController<Map<String, Object>>(); 
  controller.stream.listen(print);
  addFoo(controller.sink); // No compilation error here
  print('done');
}

// Code in another library, already migrated
void addFoo(StreamSink<Map<String, Object?>> sink) { 
  sink.add({'foo': 'bar'}); // Runtime type error here
}

Expected

Compilation fails at addFoo(controller.sink);

Actual

No compilation error shown by the analyzer. Runtime error at sink.add({'foo': 'bar'});:

Uncaught Error: TypeError: Instance of 'JsLinkedHashMap<String, Object?>': type 'JsLinkedHashMap<String, Object?>' is not a subtype of type 'Map<String, Object>'

In our dwds CI (https://github.com/dart-lang/webdev/runs/7622869869?check_suite_focus=true), the error happens far from the creation of the stream controller (another library) and traces look like:

❌ test/handlers/asset_handler_test.dart: Asset handler can read large number of resources simultaneously (failed)
  Retry: Asset handler can read large number of resources simultaneously
  type '_InternalLinkedHashMap<String, Object?>' is not a subtype of type 'Map<String, Object>' of 'data'
  dart:async                                      _StreamSinkWrapper.add
  package:vm_service/src/vm_service.dart 1732:21  VmServerConnection._delegateRequest

Dart SDK Version (dart --version)

version: 2.19.0-36.0.dev

[annagrin]

/cc @elliette

[eernstg]

I suspect the issue is related to the variance issue in dart

Yes, the run-time error occurs because we're using dynamically checked covariance, and the upcast that introduces a ? on Object is the step where the covariance becomes non-trivial.

We won't directly be able to eliminate this problem by introducing declaration-site variance (dart-lang/language#524), because it is unlikely that we would be able to make widely used platform classes like Map use statically checked variance. We could do it with use-site invariance, though (dart-lang/language/issues/753):

import 'dart:async';

// Code in library being migrated
void main() {
  final controller = StreamController<Map<String, Object>>(); 
  controller.stream.listen(print);
  addFoo(controller.sink); // Compile-time error.
  print('done');
}

// Code in another library, already migrated
void addFoo(StreamSink<exactly Map<String, Object?>> sink) { 
  sink.add({'foo': 'bar'});
}

We would get the compile-time error at the invocation of addFoo because we have an inferred type of controller which is a little bit more precise than the type that we're currently inferring: StreamController<exactly Map<String, Object>>. If we had obtained the value of controller in a more general manner (like final controller = myFunction(some, arguments);) then that variable might very well have gotten the type StreamController<Map<String, Object>> (no invariance), and in that case the invocation of addFoo would pass an actual argument whose type is a proper supertype of the declared type, and then we would get a "downcast needed" error.

So there will be some considerations about whether or not that downcast should be written into the code (because we think it will succeed), but we will have the feedback from the type system which allows us to eliminate the dynamically checked covariance at the call site of addFoo, that is, we do get to see the error statically, and we can pull the error back from sink.add() to the invocation of addFoo, and potentially further back in the control flow, as long as we have the ability to edit the code.

[eernstg]

Actually, I think we should close this as 'working as intended', and recommend that further discussion is taken in the issues about variance related proposals, dart-lang/language#524 resp. dart-lang/language#753, or by creating new issues as needed in the language repository.

[annagrin]

Thanks @eernstg! Would be great to try the use-site invariance when we get there.

I tried to invent a type check I could use for now to move the runtime error closer to the callsite (by checking the type of the argument in addFoo, which will give better runtime errors but of course does not have the benefits of statically propagating the type errors to other functions), but the only check I came up with looks a little... fragile:) I appreciate pointers to better checks if possible:

import 'dart:async';

// Code in library being migrated
void main() {
  final controller = StreamController<Map<String, Object>>(); 
  controller.stream.listen(print);
  addFoo(controller.sink); // No compile-time error.
  print('done');
}

// Code in another library, already migrated
void addFoo(StreamSink<Map<String, Object?>> sink) { 
  if (!sink.runtimeType.toString().contains('<Map<String, Object?>>')) {
    throw 'sink needs to allow nullable values'; // Runtime error with the stack that shows the callsite
  }
  // Somewhere deep inside the library
  sink.add({'foo': 'bar'}); // Not reaching here
}

[annagrin]

Update: found a better way (and a crash in dart2js compiled code: #49588):

// @dart = 2.17
import 'dart:async';

// Code in library being migrated
void main() {
  final controller = StreamController<Map<String, Object?>>();
  controller.stream.listen(print);
  addFoo(controller.sink);
  print('done');
}

// Code in another library, already migrated
void addFoo(StreamSink<Map<String, Object?>> sink)   {
  if (sink is StreamSink<Map<String, Object>>) {
    throw 'bad sink'; // Runtime error with the stack that shows the callsite
  }
  // Somewhere deep inside the library
  sink.add({'foo': 'bar'}); // Not reaching here
}

[eernstg]

Type.toString() is really dangerous: There is no guarantee about what it returns. For instance, function types have been written approximately as (ArgumentTypes) => ReturnType for a long, long time, and in the meantime we introduced the syntax ReturnType Function(ArgumentTypes) in the language, and it is very tempting to change Type.toString() such that it uses the "new" syntax. So anything else is better than Type.toString(). ;-)

The test sink is StreamSink<Map<String, Object>> is difficult to generalize, because there are so many different ways to be a subtype of a given type.

If we need the actual type argument (let's call it A) of sink at StreamSink to be exactly Map<String, Object?>, we can rely on one direction immediately: A is a subtype of Map<String, Object?>, based on the declared type. But we don't have an easy way to confirm that A is a supertype of Map<String, Object?>, and the problem is that there is no set of type arguments that we can pass to Map such that we get a complete set of types {T1, ... Tk} such that every type which is a subtype of Map<String, Object?> and which is different from that type will be a subtype of at least one of T1 .. Tk. Going to the non-nullable subtype is one possible candidate to be in that set, but we can't find {T1 .. Tk} in the general case.

For instance, sink could be a StreamSink<Map<Never, Object?>>, or StreamSink<Map<String, int?>>, etc. These types wouldn't be detected by sink is StreamSink<Map<String, Object>>, but they would still cause the dynamic covariance check to fail later on.

It looks like there is no option which is better than just having the dynamic error where it occurs today, and then tell everybody that we want safer variance, soon! ;-)

[eernstg]

Oh, I could mention one more thing: We can implement a manual covariance check as follows, if we have the ability to add a helper method to the class for each type variable that we wish to cover:

extension CheckCovarianceOfC<X, Y> on C<X, Y> {
  bool get isCovariant => 
      _isNotSubtypeOfX<X>() || _isNotSubtypeOfY<Y>();
}

class C<X, Y> {
  bool _isNotSubtypeOfX<X1>() => <X1>[] is! List<X>;
  bool _isNotSubtypeOfY<Y1>() => <Y1>[] is! List<Y>;
}

void main() {
  C<String, Object> c1 = C();
  C<String, Object?> c2 = c1;
  print(c1.isCovariant); // 'false'.
  print(c2.isCovariant); // 'true'.
}

[annagrin]

@eernstg this is a great suggestion, thanks!

Update: tried to play with this but got stuck on C being a StreamSink<Map<X,Y>> in our case, so we cannot add _isNotSubtypeOf... methods to it.

I came up with this (a bit complicated and does not always work):

import 'dart:async';

extension CheckCovarianceOf<X> on CovarianceCheckWrapper<X> {
  bool get isCovariant => _isNotSubtypeOfX<X>();
}

// wrapper for X since we cannot modify X
class CovarianceCheckWrapper<X>  {
 final X x;
 CovarianceCheckWrapper(this.x);
  
 bool _isNotSubtypeOfX<X1>() => <X1>[] is! List<X>;
}

// our library
void main() {
  {
    // bad controller
    var controller = StreamController<Map<String, Object>>();
    var c1 = CovarianceCheckWrapper(controller.sink);

    // test
    CovarianceCheckWrapper<StreamSink<Map<String, Object?>>> c2 = c1;
    print(c1.isCovariant); // 'false'.
    print(c2.isCovariant); // 'true'.

    // call a function that needs to do a covariance check
    // covariance check works as expected here
    try {
     addFoo(c1);
    } catch (e) {
      print(e); // 'Bad state: Bad sink'
    }

    // covariance check does not work as expected here
    try {
     addFoo(CovarianceCheckWrapper(controller.sink));
    } catch (e) {
      print(e); // TypeError: Instance of 'JsLinkedHashMap<String, Object?>': type 'JsLinkedHashMap<String, Object?>' is not a subtype of type 'Map<String, Object>'
    }
  }
  {
    // good controller
    var controller = StreamController<Map<String, Object?>>();
    var c1 = CovarianceCheckWrapper(controller.sink);

    // test
    CovarianceCheckWrapper<StreamSink<Map<String, Object?>>> c2 = c1;
    print(c1.isCovariant); // 'false'.
    print(c2.isCovariant); // 'false'.

    // call a function that needs to do a covariance check
    // covariance check works as expected here
    try {
      addFoo(c1); // 'false'
    } catch (e) {
      print(e);
    }

    // covariance check works as expected here
    try {
     addFoo(CovarianceCheckWrapper(controller.sink)); // 'false'
    } catch (e) {
      print(e);
    }
  }
}

// another library that we can modify
void addFoo(CovarianceCheckWrapper<StreamSink<Map<String, Object?>>> c) {
  print(c.isCovariant); // Would like 'true' iff the sink does not allow nullable objects.
  if (c.isCovariant) throw StateError('Bad sink');
  c.x.add({'foo': 'bar'});
}

output:

false
true
true
Bad state: Bad sink
false
TypeError: Instance of 'JsLinkedHashMap<String, Object?>': type 'JsLinkedHashMap<String, Object?>' is not a subtype of type 'Map<String, Object>'
false
false
false
false

For now we might just add a runtime check as I mentioned before to guard against the future problems:

if (sink is StreamSink<Map<String, Object>>) {
    throw 'bad sink'; // Runtime error with the stack that shows the callsite
}

[eernstg]

The problem is that the type argument of the wrapper is determined by the expectations of the enclosing expression (the 'context type') in the invocation of addFoo. So we've got the following code:

    // covariance check does not work as expected here
    try {
     addFoo(CovarianceCheckWrapper(controller.sink));
    } catch (e) {
      print(e); // TypeError: Instance of 'JsLinkedHashMap<String, Object?>': ...
    }

If you change it as follows then the type argument is determined by the type of controller.sink, because there is no (non-trivial) context type:

    try {
      var wrapper = CovarianceCheckWrapper(controller.sink);
      addFoo(wrapper);
    } catch (e) {
      print(e); // Now prints 'Bad state: Bad sink'.
    }

It should be possible to use those wrappers if it is possible to keep track of the exact type argument of every object which will be wrapped (otherwise you'll just get a covariant relation between the run-time type of the wrapped object and the type argument of the wrapper).

Of course, there will be no shortage of warnings about allocating extra objects in order to help the type checker. It would be much better to have sound variance (both kinds) directly in the language, but I guess the wrappers could be helpful for very specialized purposes (such as debugging) until we get there. ;-)

[annagrin]

Thank you @eernstg! We will add something along these lines temporarily until the sound variance is in the language.