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

Future.wait([b, c]) does not have the correct return type #27151

Closed
scheglov opened this issue Aug 25, 2016 · 14 comments

Comments

@scheglov
Copy link
Contributor

commented Aug 25, 2016

This might be an extension of #27134.
But in contrast to it, this is not the code I have seen in internal codebase.
Just accidentally occurred when I was testing after you fixed #27134.

In the code below result has the static type List<A>.
But result2 is List<dynamic>.

Even if b and c are both Future<A>, result2 and await don't get the ideal type.

import 'dart:async';

main() async {
  var b = new Future<B>.value(new B());
  var c = new Future<C>.value(new C());
  var lll = [b, c];
  var result = await Future.wait(lll);
  var result2 = await Future.wait([b, c]);
}

class A {}

class B extends A {}

class C extends A {}
@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2016

ah great catch!

One thing I noticed while investigating another bug, is we push down dynamic as a context type sometimes, and alsodynamic | Future<dynamic>. Neither are useful to push down. But some of our inference may prefer (any) downwards type over an upwards type. List literals are that way, I think. We should probably use the same logic for them we'd use for generic function/methods/constructor inference.

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2016

Filed #27155 to track the "push down dynamic" issue, but leaving this one open because I think it's related to ListLiteral specifically (and MapLiteral probably has a similar problem).

@jmesserly jmesserly self-assigned this Aug 25, 2016

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2016

@leafpetersen want to run this by you. So we'd currently perform the following inference:

List/*<T>*/ f/*<T>*/(/*=T*/ a,/*=T*/ b) => null;
class C<T> {
  C(T a, T b);
}
main() {
  List<num> x =     f/*infer <int>*/(123, 456); // 1
  C<num> y    = new C/*infer <int>*/(123, 456); // 2
  List<num> z =      /*infer <num>*/[123, 456]; // 3
}

My inclination would be to make 3 consistent with 1 and 2: use the most precise type, let someone weaken it if they want to via explicit annotation.

Why this relates to the bug: await creates a dynamic | Future<dynamic> context, which leads generic method inference to create a context of Iterable<Future<dynamic>> for the argument, that leads to List<Future<dynamic>> instead of List<Future<A>> in the example.

but you should be able reproduce it with Future<List> f = Future.wait([b, c]);, so it's not caused by future union, but rather because we let downward inference take over.

@scheglov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2016

I agree, consistency between all 1, 2 and 3 would be good.

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

Agreed that they should all be consistent. I'm not sure what the best thing to choose is when you have a less precise type from context. Choosing the downwards type ('num' here) is the safest option, since any uses of the value will be at the downwards type (or will be mediated by a cast). You can certainly have code that looks like:

void addStuffToList(List<num> l) {
  l.add(3.0);
  l.add(4);
}

addStuffToList([1]);

where choosing the more precise type will cause a runtime failure. Of course, if you do:

List<num> l = b ? [1] : [1.0];
....
if (b) {
  List<int> li = l as List<int>
}

You get the runtime failure if you choose the less precise type.

Is the choice of int in the first two cases the result of our co/contravariant heuristic, or of something else?

Concretely on the await case, do we need to push down the dynamic | Future<dynamic> context, or can we uniformly avoid pushing down dynamic contexts by making the InferenceContext just ignore them?

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

Concretely on the await case, do we need to push down the dynamic | Future context, or can we uniformly avoid pushing down dynamic contexts by making the InferenceContext just ignore them?

agreed we should not push Future<*> | * down. that's tracked in #27155, and fixing that will fix the original reproduction here.

Is the choice of int in the first two cases the result of our co/contravariant heuristic, or of something else?

yeah, it's a result of the heuristic, which we weren't sure we had right.

Perhaps when we have a return context, we should choose the type parameter in such a way that it ends up as close as possible to the return context. Concretely, if the type parameter appears in an return position (e.g. return type is T or List<T> or () -> T) we'll chose the upper bound, if it's in an parameter position (e.g. return type is (T) -> *) we'll chose the lower bound.

That is backwards the normal heuristic, which tries to pick the tightest type, but seems reasonable because as you noted, accesses are mediated by the (weaker) return context type.

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

@leafpetersen here's a straw man proposal for this issue: I'll have list/map literals (3) go through the same code path as generic functions (1) and constructors (2). But I'll change the heuristic on all of them, as described above, to pick a type closer to the return context. WDYT?

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

I'm on board with making list/map literals use the same code path. I'd like to think through the details of the heuristic change a bit before committing to it.

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

thanks! sounds great.

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

clarification to one of my comments:

agreed we should not push Future<*> | * down. that's tracked in #27155, and fixing that will fix the original reproduction here.

This was not enough to fix it. We still instantiate Future.wait's type parameter as dynamic which makes the parameter context List<Future<dynamic>>.

@leafpetersen I think that's the other reason the current heuristic in generic methods is necessary for now. We don't have unified upwards+downwards so our downwards types are too weak for parameters to generic methods. Hence we benefit from finding the tightest type. Once we get unified inference that won't matter, but it's a useful stopgap.

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

another interesting find: unifying results in a worse errors, because of #26992 (inference failures silently result in "dynamic").

if nothing else this has been a good exercise in finding some rough spots in the current system :)

@jmesserly jmesserly added the blocked label Aug 27, 2016

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

i'm going to mark this blocked on that other bug. I have the CL working for lists, but I don't want to go further if it makes a bunch of precise downwards errors into "down cast" warnings. e.g. if you use [1, 'hello'] when a List<String> is expected, the error right now will be on the 1, not the whole list.

@jmesserly jmesserly removed the blocked label Sep 1, 2016

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2016

@jmesserly jmesserly closed this in 79a9409 Sep 14, 2016

@jmesserly

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2016

jmesserly added a commit that referenced this issue Sep 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.