Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can strong mode inference for Future.then be improved? #25944

Closed
leafpetersen opened this issue Mar 8, 2016 · 11 comments

Comments

@leafpetersen
Copy link
Member

commented Mar 8, 2016

Noting these down for future consideration.

import 'dart:async';

//[warning] Unsound implicit cast from Future<dynamic> to Future<int> (/usr/local/google/home/leafp/tmp/future.dart, line 5, col 10)
Future<int> t1() {
  Future f;
  return f.then((_) async => await new Future<int>.value(3));
}

//[warning] Unsound implicit cast from Future<dynamic> to Future<int> (/usr/local/google/home/leafp/tmp/future.dart, line 10, col 10)
Future<int> t2() {
  Future f;
  return f.then((_) async {return await new Future<int>.value(3);});
}

//[warning] Unsound implicit cast from Future<dynamic> to Future<int> (/usr/local/google/home/leafp/tmp/future.dart, line 15, col 10)
Future<int> t3() {
  Future f;
  return f.then((_) async => 3);
}

//[warning] Unsound implicit cast from Future<dynamic> to Future<int> (/usr/local/google/home/leafp/tmp/future.dart, line 20, col 10)
Future<int> t4() {
  Future f;
  return f.then((_) async {return 3;});
}

//[warning] Unsound implicit cast from Future<dynamic> to Future<int> (/usr/local/google/home/leafp/tmp/future.dart, line 25, col 10)
Future<int> t5() {
  Future f;
  return f.then((_) => new Future<int>.value(3));
}

// [error] Type check failed: new Future<int>.value(3) (Future<int>) is not of type int (/usr/local/google/home/leafp/tmp/future.dart, line 30, col 29)
//[warning] The return type 'Future<int>' is not a 'int', as defined by the method ''. (/usr/local/google/home/leafp/tmp/future.dart, line 30, col 29)
Future<int> t6() {
  Future f;
  return f.then((_) {return new Future<int>.value(3);});
}
@hterkelsen

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

I'm running into this and having to do some ugly workarounds:

Future<Foo> getFoo(String key) {
  return doSomething(). then((FooCache cache) {
    return cache.get(key); // FooCache#get returns a Future<Foo>
  });
}

has to be rewritten as:

Future<Foo> getFoo(String key) {
  return doSomething(). then((FooCache cache) {
    return cache.get(key); // FooCache#get returns a Future<Foo>
  }).then((x) => x);
}
@leafpetersen

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2016

Can you rewrite that using async/await?

Another workaround might be:

Future<Foo> getFoo(String key) {
  var result = doSomething(). then((FooCache cache) {
    return cache.get(key); // FooCache#get returns a Future<Foo>
  });
 return result;
}
@hterkelsen

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

Thanks, I was able to just use async/await

@srawlins

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

👍 , I think for t5(). I have a test case that highlights my confusion:

import 'dart:async';

Future<int> a() => new Future<int>.value(2);
Future<int> b() => a().then((int i) => i);
int c(int i) => i;
Future<int> d() => a().then(c);

// Why strong mode errors on lines 9 (p) and 11 (r)?
Future<int> p() => a().then((a) => new Future<int>.value(2));
Future<int> q(int i) => new Future<int>.value(i);
Future<int> r() => a().then(q);

// If we help it out with a comment, no more warnings.
Future<int> x() => a().then/*<Future<int>>*/((a) => new Future<int>.value(2));
Future<int> y(int i) => new Future<int>.value(i);
Future<int> z() => a().then/*<Future<int>>*/(y);

I expect p and r not to violate strong mode, but I have to resort to x and z.

@leafpetersen

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2016

Without union (or intersection) types, we can't really express the type of Future.then. The current state is a poor compromise that makes inference work for things like your a, b, c, and d, at the expense of making inference fall down on your p and r.

At some point soon, I will implement ad-hoc inference for Future.then (basically pretending that we have union/intersection types for this specific case). Until then, we're stuck with the workarounds.

Where possible, I believe that using async/await is a better solution:

Future<int> p() async {
  var aresult = await a();
  var result = await (new Future<int>.value(2));
  return result;
} 
Future<int> r() async {
  var ar = await a();
  return await q(ar);
}

Another option is to write helpers to untangle the overloading:

  Future/*<S>*/ thenV/*<T, S>*/(Future/*<T>*/ f, /*=S*/callback(/*=T*/ x)) => f.then/*<S>*/(callback);
  Future/*<S>*/ thenF/*<T, S>*/(Future/*<T>*/ f, /*=Future<S>*/callback(/*=T*/ x)) => f.then/*<Future<S>>*/(callback);

This will allow you to write your examples above as follows:

Future<int> a() => new Future<int>.value(2);
Future<int> b() => thenV(a(), ((int i) => i));
int c(int i) => i;
Future<int> d() => thenV(a(), c);

Future<int> p() => thenF(a(), (a) => new Future<int>.value(2));
Future<int> q(int i) => new Future<int>.value(i);
Future<int> r() => thenF(a(), q);
@srawlins

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

Thanks much for the tips!

@jmesserly jmesserly self-assigned this May 12, 2016

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

I can take a look

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented May 12, 2016

Also related to #25322, which affects await

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented May 25, 2016

Well the good news: I've got a prototype with all of these cases working better, by introducing a Future<T> | T concept during inference. Because inference knows what is going on, it is able to reach better conclusions in pretty much all cases.

The bad news: we've got some tough choices to make before it can be checked in. Having special types that exist for inference, but do not exist elsewhere, is hard to get right.

The prototype does not have the boundary fully working yet. My attempts at improving the boundary has lead to other problems, like ErrorVerifier getting confused and either dropping errors/warnings or issuing spurious ones.

There are related questions like how to explain what is going on with Future.then/async/await in error messages. For example, an invalid return type is normally handled via the message: The return type '{0}' is not a '{1}', as defined by the method '{2}'. But consider this example: we're in a context where it can return either a int or Future<int>. We'll need to figure out what to say there.

(One case where inference is not as good is this mixture of Future.then and Iterable.fold, but the root cause there is the lack of unified upwards+downwards inference.)

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2016

https://codereview.chromium.org/2208953002/ has made some progress on this.

@srawlins

This comment has been minimized.

Copy link
Member

commented Aug 10, 2016

Wooo! Thanks @jmesserly !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.