Different handling of nested Futures

[lexaknyazev]

Consider this code:

foo.dart

void main() {
  foo(new Future.value(true)).then((result) {
    print(result.runtimeType);
  });
}

Future<R> foo<R>(R value) => new Future.value(value);

It gives no analyzer issues. However, running it with "legacy" VM and with Dart2 options results in different output:

$ ./dart-sdk/sdk/out/ReleaseX64/dart-sdk/bin/dart --version
Dart VM version: 2.0.0-edge.226c715542fa1bf9419e3f2e25f765ef33575d26 (Tue Jan 2 13:29:03 2018 +0000) on "linux_x64"

$ ./dart-sdk/sdk/out/ReleaseX64/dart-sdk/bin/dart foo.dart
bool

$ ./dart-sdk/sdk/pkg/vm/tool/dart2 foo.dart
_Future<bool>

Is it expected?

[mraleph]

Seems a bit suspicious. @mkustermann, could you check this out when you have time?

[kmillikin]

The inferred types look bad.

$ pkg/front_end/tool/fasta compile --strong --target=vm --dump-ir ~/programs/dart/future-mess.dart
Note: strong mode support is preliminary and may not work.
library;
import self as self;
import "dart:async" as asy;
import "dart:core" as core;

static method main() → void {
  self::foo<asy::Future<core::bool>>(asy::Future::value<core::bool>(true)).{asy::Future::then}<dynamic>((asy::Future<core::bool> result) → core::Null {
    core::print(result.{core::Object::runtimeType});
  });
}
static method foo<R extends core::Object>(self::foo::R value) → asy::Future<self::foo::R>
  return asy::Future::value<self::foo::R>(value);

Namely, foo can't return Future<R> when R is Future<bool>. And the function passed to Future.then can't ever get passed a Future<bool>.

But what should it actually be? @leafpetersen @stereotype441

[leafpetersen]

The inferred types are correct, I believe. The behavior here is basically a library issue.

Dart 2.0 should infer Future<Future<bool>> as the return type of foo. The constructor call Future.value<Future<bool>>(value) needs to either:

• not flatten the future beyond what is expected by the return type: that is, return a value of type `Future<Future>
• or cast the flattened Future<bool> to Future<Future<bool>>, which of course will fail.

Obviously, I think the former is friendlier.

For context, in strong mode before FutureOr, we did future flattening on substitutions to try to preserve the property that for any closed type Future<T>, T was not a subtype of Future. This means that future flattening needs to be done at runtime as well, adding quite a bit of complication, and I was never convinced that this was all fully correct. The only uses of this that I've found in real code are uses that were explicitly introduced to work around the lack of FutureOr. The analyzer currently still has this flattening code path, but I expect to remove it sometime in the next couple of dev releases.

cc @lrhn

[lrhn]

Agree. The specification has been updated to not flatten more than one level of Future or FutureOr, but the libraries have not been changed yet. It's a necessary change to make the strong mode type system work - the type inference here would must accept Future<bool> as argument type to foo (since that is what it is) and the return type must then be Future<Future<bool>>.

[kmillikin]

So the libraries will change. In Dart 1.0 you might have assumed that, due to flattening, Future.then never passed a Future to its argument. But it can and the workaround is:

  foo(new Future.value(true)).then((Future<bool> result) {
    result.then((bool result) {
      print(result.runtimeType);
    });
  });

I guess everything is working as intended here?

[lexaknyazev]

I guess everything is working as intended here?

I can't say for sure. With the workaround above, execution fails in Dart 1.0 on result.then. I assume it's expected:

Unhandled exception:
NoSuchMethodError: Class 'bool' has no instance method 'then'.
Receiver: true

In Dart 2.0, it works. However, if I change foo to not have generic arguments, it fails:

import 'dart:async';

void main() {
  foo(new Future.value(true)).then((result) {
    result.then((value) {
      print(value.runtimeType);
    });
  });
}

Future foo(value) => new Future.value(value);

$ ~/dart-sdk/sdk/pkg/vm/tool/dart2 foo.dart
Unhandled exception:
NoSuchMethodError: Class 'bool' has no instance method 'then'.
Receiver: true
Tried calling: then(Closure: (dynamic) => Null)

[lrhn]

The non-generic behavior is expected in both Dart 1 and Dart 2.
The foo function should infer types as:

Future<dynamic> foo(dynamic value) => new Future<dynamic>.value(value);

Rewritten, the program is then equivalent to:

var fooArg = new Future<dynamic>.value(true);  // `dynamic` inferred from context.
Future fooResult = new Future.value(fooArg);  // inlined `foo` call
fooResult.then((dynamic result) {
  result.then((dynamic value) {
    print(value.runtimeType);
  });
});

So, fooArg is a Future<dynamic> containing true.
That is then used as argument to new Future<dynamic>.value, which will create a Future<dynamic> with the value true. That's fooResult.
The following then call then binds result to true, with static type dynamic, and promptly fails at run-time when you try to call then on true.

[lexaknyazev]

Does new Future<dynamic>.value(fooArg) do flattening so it doesn't return Future<Future<dynamic>>?

[lrhn]

It does, yes. The signature of Future<T>.value is:

Future.value(FutureOr<T> result);

If the argument is a Future<T>, then it flattens. In this case, T is dynamic, so any Future is a Future<T>. It will keep doing this, but restricting itself to removing one level of Future, because otherwise the types will be wrong.

[leafpetersen]

It will keep doing this, but restricting itself to removing one level of Future, because otherwise the types will be wrong.

Just to be sure we're on the same page, it should probably only unwrap if required. That is, pseudo-code for Future.value should be something like:

  Future.value(FutureOr<T> result) {
   if (result is T) return new Future.reallyValue(result);
  else return (result as Future<T>).then((a) => a);
}

[lrhn]

Something like that, but probably testing against the future type first.
The current code in Future.value calls _Future.immediate, which calls _Future._asyncComplete.

It makes a difference for the case new Future<Object>.value(something). Here we probably do want to unwrap something if it's a future.

(Actually, looking at that code, it seems it has been updated to only unfold once, but I have to check some edge cases to be sure that it's not just skin deep).

[lexaknyazev]

Hi, has this issue been converged? I've run into something very similar with this code:

import 'dart:async';
import 'package:isolate/isolate_runner.dart';
import 'package:isolate/load_balancer.dart';

Future main() async {
  final balancer = await LoadBalancer.create(1, IsolateRunner.spawn);
  balancer.run(_run, null).then((result) {
    if (result) {
      print('OK');
    }
  });
}

Future<bool> _run(_) =>
    new Future<bool>.delayed(const Duration(seconds: 1), () => true);

On DartVM 2.0.0-edge.2fb377240a2ccee3fc160791d6345cc3c248926a it gives no analyzer issues but fails in Fasta:

Error: A value of type 'dart.async::Future<dart.core::bool>' can't be assigned to a variable of type 'dart.core::bool'.
Try changing the type of the left hand side, or casting the right hand side to 'dart.core::bool'.
    if (result) {
        ^

Changing _run's return type to FutureOr<bool>

FutureOr<bool> _run(_) =>
    new Future<bool>.delayed(const Duration(seconds: 1), () => true);

leads to runtime failure:

Unhandled exception:
type '_Future' is not a subtype of type 'Future<FutureOr<bool>>' where
  _Future is from dart:async
  Future is from dart:async
  FutureOr is from dart:async
  bool is from dart:core

#0      IsolateRunner.run (package:isolate/isolate_runner.dart:189:12)
#1      _LoadBalancerEntry.run (package:isolate/load_balancer.dart:271:10)
#2      LoadBalancer.run (package:isolate/load_balancer.dart:77:18)
#3      main (file:///home/alexey/demo/foo.dart:7:12)
<asynchronous suspension>
#4      _startIsolate.<anonymous closure> (dart:isolate:1257:19)
#5      _RawReceivePortImpl._handleMessage (dart:isolate:1147:12)

[lrhn]

The isolate library seems to be creating a _Future<dynamic> in a place where it needs a Future<bool>. It needs to be updated.

The Future<FutureOr<bool>> looks wrong, though.

[mraleph]

What is the status of this issue? Is this a VM issue, a library issue or a front-end issue?

/cc @lrhn @kmillikin

[lexaknyazev]

The behavior from the comment above could be replicated with the current master.
Moreover, sometimes dart process hangs indefinitely after throwing Exception.

[kmillikin]

@lrhn @lexaknyazev If the return type of _run is typed as Future<bool> then balancer.run(_run,null) will have type Future<Future<bool>>, the then callback will be passed a Future<bool> which explains your first problem.

If the return type of _run is typed as FutureOr<bool> then balancer.run(_run,null) will ultimately get into this function in isolate_runner.dart in the isolate library:

  Future<R> run<R, P>(R function(P argument), P argument,
      {Duration timeout, onTimeout()}) {
    return singleResultFuture((SendPort port) {
      _commandPort.send(list4(_RUN, function, argument, port));
    }, timeout: timeout, onTimeout: onTimeout);
  }

R is the return type of _run namely FutureOr<bool>. singleResultFuture is returning the implementation type _Future<dynamic> and we're trying to cast it to Future<FutureOr<bool>>.

We think this is a library issue.

[lexaknyazev]

@kmillikin

We think this is a library issue.

Does it also explain process hang? With about 50% chance, dart process goes into sleeping state after printing stack trace.

[lrhn]

"Hanging" is most likely just a symptom of the main isolate still having an open receive port, and never getting a message that it should stop listening.

If something fails in a spawned isolate, it may still print the error. It can then die or not (it probably doesn't), but if that means that the computation never completes, then the main isolate never gets told to stop listening.

You can use IsolateRunner.onExit to listen for the death of the runner isolate if you want to, and then close the runner. Something like:

  var runner = await IsolateRunner.spawn();
  runner.onExit.whenComplete(runner.close);

but that won't prevent the runner isolate from staying alive and doing nothing.

Arguably, the runner isolate should run every operation in a new error zone, and report the operation as failing if it has any uncaught errors, not just if the entry-point for the operation throws. That would prevent that particular issue.

[lexaknyazev]

Given that isolate package has been fixed, should this issue be closed? Also, could updated isolate be published to pub.dartlang.org?

[lrhn]

I've published isolate v2.0.0