Future.then inference does not work for classes that implement Future

[jmesserly]

Future.then now has really nice inference, in particular we understand that the "real" signature is one of:

Future<T>.then: <S>(T -> S) -> Future<S>
Future<T>.then: <S>(T -> Future<S>) -> Future<S>

We special case it in both downwards and upwards inference. However it does not work for a user-defined type that implements Future (often to simply forward).

original context https://codereview.chromium.org/2250513003/

[jmesserly]

CC @nex3 @leafpetersen -- could y'all discuss and decide what to do here?

I'll mark P1 in the meantime since this is blocking-ish

[leafpetersen]

I looked at the code briefly. It looked like it might not be too bad to extend this inference to classes which extend or implement Future, and which don't change the signature of .then. @jmesserly does that seem reasonable to you as well?

@nex3 mentioned that there were similar patterns that currently sort of work because of Future flattening. Do you have concrete examples I can look at? Generally speaking, we can only take ad hoc inference so far, and extending it to other types that are unrelated to Future I think is too far. Future flattening sometimes does the right thing, but it is very confusing to users, and it's very problematic in the type system. I'd really prefer to move away from it. @jmesserly did you already rip it out, or is it still there for the time being?

Until we get union types and/or overloading, APIs that both want to get good inference and provide overloading like functionality are probably going to have to do so via explicit overloads (that is, provide the equivalent of a MyFutureLikeClass.thenLikeMethodForFuture : T -> Future<S> -> Future<S> and MyFutureLikeClass.thenLikeMethodForValue : T -> S -> Future<S>.

[jmesserly]

@jmesserly does that seem reasonable to you as well?

yeah that sounds right to me.

@nex3 mentioned that there were similar patterns that currently sort of work because of Future flattening.

well in general, any you have a type parameter T and something that returns a Future<T>, you benefit from flattening. For example Stream<T>.first but there are many other examples. The reason this one got worse is we removed the S type parameter from the signature of Future.then's first param, so upwards inference can only kick in from the ad hoc code, and our override checking forces anyone subclassing Future to also remove the S

[nex3]

@nex3 mentioned that there were similar patterns that currently sort of work because of Future flattening. Do you have concrete examples I can look at?

schedule() in scheduled_test and Pool.withResource() in pool are major public examples. There are a number of other internal functions that use similar patterns.

Until we get union types and/or overloading, APIs that both want to get good inference and provide overloading like functionality are probably going to have to do so via explicit overloads (that is, provide the equivalent of a MyFutureLikeClass.thenLikeMethodForFuture : T -> Future<S> -> Future<S> and MyFutureLikeClass.thenLikeMethodForValue : T -> S -> Future<S>.

This would force packages to make major breaking changes to their public APIs, to the detriment of users. schedule() in particular is called constantly. The pain of upgrading would be huge, and the resulting code would look a lot worse. I have a hard time believing that this would be better than the current system—as hacky as Future flattening is, it works most of the time for everyone's code, not just for the core libraries.

[jmesserly]

@leafpetersen -- well maybe this is a terrible idea, but we could consider something like: whenever we see a Future<T> out (from any out position) of a generic function, we treat every in-position T as unflatten(T), the inverse of flatten, which results in our special union type T | Future<T>. That would essentially give all matching functions the Future.then downward inference magic.

that's probably way too crazy, so a less crazy idea is: we put Future<T>.then back to its old signature (T -> S) -> Future<S>. Then revert the change I made earlier today and replace it with some special type checking logic that allows the lambda to return anything without checking it (because we know it's checked at runtime). So the "S" is kind of a pretend type for inference.

[leafpetersen]

Correct me if I'm wrong, but I think that Future.then is currently in a good place.

Subclasses of Future.then can also be put into a good place, at some (but not too much) cost.

The issue which @nex3 is concerned about is the set of other "Future.then like" methods that are out there for which we don't have ad hoc inference. I believe that these currently will still work as before because Future flattening is still in the type system (correct me if I'm wrong). So assuming we generalize the current inference to subclasses of Future, I think we're in an ok place for the short term.

However, future flattening is a very expensive feature. It does quite a bit of violence to the static type system (which I could live with), and it also reaches into the dynamic type system. All of the implementations (VM, dart2js, DDC) will have to bake this into their reified type system. This isn't out of the question (DDC does it, though I have low confidence that it's 100% correct), but it's a big cost in complexity, and I think a likely source of soundness holes. I'd really like to avoid supporting this in the long term if at all possible.

There are actually two parts of the Future.then ad hoc inference, I think. The main one is that inference essentially treats Future.then as having an overloaded signature: it tries to infer based on the type (T -> Future<S>) -> Future<S>, and then if that fails based on the type (T -> S) -> Future<S>. A secondary part (that I think we implement? correct me if I'm wrong) is that it can also do downward inference on the lambda parameter using the Future union type.

I believe that @nex3 is mostly concerned with the first part (since the second part didn't exist until just recently anyway). Assuming so, I wonder if we could address this by generalizing slightly to a notion of an "inference pattern" which generalizes the ad hoc "try this signature and then this other one". So the idea would be to use some sort of annotation or comment syntax to allow methods to be marked as essentially having an overloaded type strictly for the purpose of doing inference. This could potentially help with a fairly wide range of API surfaces: it might be enough to help address some of the other async functions we've discussed elsewhere.

Thoughts?

[nex3]

So the idea would be to use some sort of annotation or comment syntax to allow methods to be marked as essentially having an overloaded type strictly for the purpose of doing inference. This could potentially help with a fairly wide range of API surfaces: it might be enough to help address some of the other async functions we've discussed elsewhere.

This would work for me, but would the other implementation teams go for it?

[jmesserly]

@leafpetersen -- regarding your "Correct me if I'm wrong" ... all of those statements are correct :)

"try this signature and then this other one"

yeah I love that idea. +1 from me.

RE: other implementations, I think type inference stuff only needs to be implemented once in the "shared front end"

[keertip]

Some more context, this is causing test and build failures due to strong mode errors

third_party/dart/async/lib/src/delegate/future.dart:30: [ERROR] Invalid override. The type of

DelegatingFuture.then (<S>((T) → S, {onError: Function}) → Future<S>) 

is not a subtype of

Future<T>.then (<S>((T) → dynamic, {onError: Function}) → Future<S>).

[STRONG_MODE_INVALID_METHOD_OVERRIDE]

third_party/dart/async/lib/src/typed/future.dart:17: [ERROR] Invalid override. The type of

TypeSafeFuture.then (<S>((T) → S, {onError: Function}) → Future<S>)

is not a subtype of

Future<T>.then (<S>((T) → dynamic, {onError: Function}) → Future<S>). 

[STRONG_MODE_INVALID_METHOD_OVERRIDE]

There are also other classes in internal code that implement Future which have the same failures.

And a cl - https://codereview.chromium.org/2250513003/

[leafpetersen]

The original change improves inference dramatically. Lots of places where users currently have to write .then<Future<T>> will now become inferred. This is a major point of confusion for users which can now be fixed. Unfortunately as a consequence, some places where we currently get inference will now need to be made explicit.

This change should be landed, and then we can see what the impact of the regression actually is. I will look into extending the ad hoc inference to subclasses of Future. If the regression is small, I suggest we live with the regression for now and land any extensions in the next roll.

[jmesserly]

Now that Keerti's CL landed can we lower the priority to P1?

[nex3]

This is a backwards incompatibility that affects real code. I'd like to be able to discuss it without the time pressure of an impending release, so I think we should roll back the breaking change for 1.19.

[jmesserly]

I'm a little confused about context. @nex3 what is the backwards incompatible change that you're referring to?

[jmesserly]

By the way, if Leaf doesn't beat me to it, I should be able to have a look at this bug tomorrow. In other words, the request for downwards inference on Future.then subtypes.

(the change to match the new Future.then signature is still needed in subtypes)

[nex3]

I'm a little confused about context. @nex3 what is the backwards incompatible change that you're referring to?

Changing the type of Future.then() is backwards-incompatible. If Future<Future> folding gets removed, that will be backwards-incompatible as well.

[jmesserly]

folding is still there, so that's good :)

Future.then signature change was intentional. It's backwards-incompatible to strong mode, but we aren't done with strong mode, so breaking changes do happen from time to time. There are quite a few in the 1.19 release, as a consequence of adding many inference improvements. (and even making Future.then generic at all was a breaking change w.r.t. Dart 1, but we need to do these sort of things or folks would be passing explicit types everywhere)

I know it's not fun to pass generic method types explicitly, especially with the annoying comment syntax, but what we gained on this one was a lot more than we lost. And I think we can fix this issue before 1.19 meaning the net loss will be zero :)

[jmesserly]

@leafpetersen i'm assigning to you because I think you were working on it based on our conversation yesterday

[leafpetersen]

I'm mostly done with this - just triaging a couple of test cases to be sure I understand a couple of corner cases.

[jmesserly]

@scheglov would you mind filing a new issue for that? I don't think it is related to this one.

[scheglov]

OK, I've opened a new issue.
#27113

[nex3]

Future.then signature change was intentional. It's backwards-incompatible to strong mode, but we aren't done with strong mode, so breaking changes do happen from time to time.

I'm not arguing for no breaking changes, just for being careful and working to mitigate the problems they cause. Ideally they'd be introduced early in the dev cycle for a new version, and we'd have a strong process in place for figuring out what concrete problems they cause and figuring out a way to ameliorate those problems.

[jmesserly]

Definitely. In an ideal world we'd be able to develop against our internal repository and see the complete effect of inference changes. Unfortunately we aren't set up for that yet. But perhaps we will be soon.