Future.then((value) {}) does not have a type.

[scheglov]

@jmesserly @leafpetersen

Test case:

import 'dart:async';

main() {
  getA().then((a) => a.getItems()).then((items) => items.length);
}

Future<A> getA() async => new A();

class A {
  Future<List<int>> getItems() async => <int>[];
}

We know the type of a.getItems(), but later in (items) => items.length the type of items is not set.

Is it the same as #25944 ?

[jmesserly]

by "propagated type" did you mean the inferred static type?

[scheglov]

a type :-)

[jmesserly]

ah gotcha. Yeah I think this was related to the fix for #25944. What happens now is the return type of the Future.then first parameter is Future<S> | S, however it is declared as dynamic in the SDK. We know to "pretend" Future<S> | S during downwards inference, but not during upwards inference. I suspect it wouldn't be all that hard to fix.

(The reason it's so tricky is we don't want our Future<S> | S type to ever end up in the Element model or in a FunctionType or staticType field. So we have to be really careful about introducing it, even though it does help out inference.)

[bwilkerson]

Analyzer tried adding union types at one point as an experiment, but the confusion it caused users more than offset any value we got for it. If we're only using this for a very limited (and local) form of inference and if we're being really careful about how we're reporting errors to the user, then we might be able to avoid the problems we ran into earlier, but we need to tread very carefully.

[jmesserly]

hah, yeah that was exactly our fear. So far we've made sure these types aren't user-visible, we always pick one or the other before we'd report anything about them (impl details: it only exists inside InferenceContext).

[jmesserly]

fwiw, I think I can fix this bug without any user-visible side effects.

[scheglov]

Another test case related to Future.then().

import 'dart:async';

main() {
  var fff = foo().then((_) => 2.3);
}

Future<int> foo() async => 1;

I'd expect that the static type of fff is Future<double>, but actually it's Future<dynamic>.

[jmesserly]

strange. I would expect at least Future<dynamic>.

[scheglov]

I've updated my comment.

You're right, it's Future<dynamic>.
But it still does not seem right.

[jmesserly]

Definitely not right :)

We used to say the signature of Future<T>.then<S> was <S>(T -> S) -> Future<S> which is how these were working. But that broke a lot of things that returned Future<S> from the body. So now the type signature is <S>(T -> dynamic) -> Future<S> and we'll have to special case that dynamic during upwards inference for generic methods (somewhere inside StaticTypeAnalyzer._inferGenericInvoke or when it calls inferGenericFunctionCall)

[kevmoo]

@jmesserly @scheglov Do we consider this a regression? Or a crazy gnarly issue?

Just want to be super clear if it's a 1.19 blocker.

CC @mit-mit

[scheglov]

It breaks rolling 1.19 internally.
AFAIK @keertip sees issues with existing code.

[jmesserly]

yeah it's a regression

[jmesserly]

I'm almost ready to send a CL out

[jmesserly]

Oh wow, I just discovered why this regression happened ... we had a test for it, but there's more than one mock SDK for Analyzer tests. I'd updated one of them without discovering the other, so the test didn't catch the change in Future.then's signature =(

[bwilkerson]

There are three classes named MockSdk (spread across three of our projects; primarily because it's hard to share test support code) and AnalysisContextFactory creates yet another mock SDK. Which one was updated?

[jmesserly]

AnalysisContextFactory was the problematic one. Here's the fix: https://codereview.chromium.org/2253923002/

I don't generally work outside package:analyzer so if the other ones are outside of it, I didn't see them. Suggestion: move mock_sdk under lib/src so other pkgs can use it.

[bwilkerson]

Yep. I think that's the pattern we agreed on, it's just not been high enough priority to actually get done yet. So far we've been putting this kind of code in analyzer/lib/src/generated/testing (though it really ought to get moved out of generated).

[kevmoo]

There are three classes named MockSdk (spread across three of our projects; primarily because it's hard to share test support code) and AnalysisContextFactory creates yet another mock SDK.

@bwilkerson since these are all in the SDK repo, could we just create a pkg/_analyzer_test_shared pseudo-package for all of this logic?

[bwilkerson]

We could, and we discussed that possibility, but we decided it was better for the test support to be carried along with the analyzer package (less chance of it getting out of sync, and better access to internal code).