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

Add branch awareness to TypeScript types (auto-expanding) #435

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented Jul 8, 2020

In the past, I've always tried to use TypeScript to force type consistency onto users of the library: #374 has a good summary of this history.

To explain my motivation for this change, let's first introduce the idea of certain and uncertain Futures. I'll refer to a Future where one of the two branches is never as a "certain Future". For example, resolve (42) is a certain Future of a number, because the rejection type is never.

In #374, I looked for a way to allow TypeScript to infer types when chaining a certain Future. This is quite a common scenario, and one that leaves a user with a consistent type. For example chain (x => resolve (42)) (reject ('x')) is allowed, because Future String a is assignable to Future b Number. In TypeScript, which does not have type variables like that, the two Futures are typed as Future<String, never> and Future<never, Number>. These are not assignable both ways, in TypeScript. To fix this problem, I decided to relax the types of chain, and chainRej functions, so that the resulting Future would be Future<String | never, never | Number>, which works because T | never is T. The downside is that forced type consistency was lost for these functions: chain (x => reject (42)) (reject ('x')) would come out as Future<number | string, never> without it being considered a type error.

A few weeks ago, I realized that this same problem plagues many more functions in the Fluture library. It's not just chain and chainRej, but also alt, and, ap, or any other function that combines two or more Futures.

This PR will relax the types of all of those combinators, hugely improving type inference when using typed Fluture, at the cost of forced type consistency across the library. I managed to find a middle-ground though, where type consistency is lost only when a Future is uncertain. I achieved this by essentially encoding the branching rules for each combinator into its TypeScript type. So whenever a Future is certain, TypeScript can figure out which branch will end up where, and it won't have to merge the types.

I have many more types to go over, but I'm opening this as a draft because from here on out, each new commit will just repeat the same pattern, so it's a good time to gather feedback.


I am quite excited about this change. When this is released, TypeScript users will find a massive improvement to the convenience of working with Fluture in TypeScript. Inference will take away most of the manual typing work which was needed, and the new branch awareness will seem like magic: "How does TypeScript know that this long chain of Futures will end up in a resolved state?!"

I think the middle ground I've found gives users the best of two worlds. Incorrect use of the API under uncertain branching conditions will be expressed as a mixed type, rather than a TypeError. Users will know they need to deal with the mixed type, either by using type guards at a later stage, or changing the way to use Fluture's API. But when branching conditions are certain, types can remain consistent.

/cc @codingedgar, @tetsuo, @gcanti

Avaq added 9 commits July 8, 2020 10:34
The type for 'alt' did not allow users to supply a future with a
predetermined branch (when one of the branches is never).

This commit adds overloads for alt that allow TypeScript to understand
how our code recovers when there is no concrete type in either of the
branches of the first Future. This makes the type inference much better,
but it also means that mixed types are no longer considered a type error.

Finally, this commit adds tests for the TypeScript type of alt.
The type for 'and' did not allow users to supply a future with a
predetermined branch (when one of the branches is never).

This commit adds overloads for 'and' that allow TypeScript to understand
how our code recovers when there is no concrete type in either of the
branches of the first Future. This makes the type inference much better,
but it also means that mixed types are no longer considered a type error.

Finally, this commit adds tests for the TypeScript type of 'and'.
Makes the typescript types for ap branch aware and adds type tests.

This improves type inference, but makes it allowed to combine two
Futures with different rejection types.
Makes the typescript types for bichain branch aware and adds type tests.

This improves type inference, but makes it allowed to combine two
Futures with different rejection or resolution types.
@codecov

This comment has been minimized.

@codingedgar
Copy link
Contributor

Seems nice! I find fp-ts TaskEither to work like magic, never had to add a single type EVER. Maybe @gcanti has the clue for this issues.

@gcanti
Copy link
Contributor

gcanti commented Jul 8, 2020

@codingedgar maybe that's because with fp-ts you are supposed to use pipe everywhere and type inference is optimized accordingly

import * as E from 'fp-ts/lib/Either'
import { pipe } from 'fp-ts/lib/function'

const x1 = E.chain(() => E.right(42))(E.left('x')) // <= error

const x2 = pipe( // <= ok
  E.left('x'),
  E.chain(() => E.right(42))
)

left and right definitions:

export declare function left<E = never, A = never>(e: E): Either<E, A>
export declare function right<E = never, A = never>(a: A): Either<E, A>

@Avaq
Copy link
Member Author

Avaq commented Jul 8, 2020

with fp-ts you are supposed to use pipe everywhere -- @gcanti

Ahh, so in fp-ts you consciously work around the "T is not assignable to never" problem by asking users to flip the order of the operation using pipe, making it assign never to T, which the type system will happily do.

The same is currently true for Fluture (with the exception of the chain and chainRej functions, because they have already been relaxed in #374):

import * as fl from 'fluture'

const x1 = fl.bichain(() => fl.resolve(42))(fl.resolve)(fl.reject('x')) // <= error

const x2 = fl.reject('x').pipe( // <= ok
  fl.bichain(() => fl.resolve(42))(fl.resolve)
)

I am curious about your opinion now, @gcanti. Do you think it's worth the trade-off? Essentially making it so:

E.chain(() => E.right(42))(E.left('x')) // <= no longer an error :)

E.chain(() => E.left(42))(E.left('x')) // <= no longer an error too :(

@gcanti
Copy link
Contributor

gcanti commented Jul 8, 2020

The same is currently true for Fluture

Not always if I'm not mistaken, take alt for example

import * as fl from 'fluture'

const x = fl.reject('a').pipe(fl.alt(fl.resolve(42))) // error

that's why I'm using defaults for the constructors type parameters

const reject: <L = never, R = never>(reason: L) => fl.FutureInstance<L, R> = fl.reject
const resolve: <L = never, R = never>(value: R) => fl.FutureInstance<L, R> = fl.resolve

const y = reject('a').pipe(fl.alt(resolve(42))) // ok

Do you think it's worth the trade-off?

Well, it really depends on the DX you want to provide, some users may be happy with an auto-widening API.
In fp-ts I ended up with two different API: the standard chain (no widening) that comes from the Monad type class

<E, A, B>(f: (a: A) => E.Either<E, B>) => (ma: E.Either<E, A>) => E.Either<E, B>

and a chainW variant (auto-widening)

<D, A, B>(f: (a: A) => E.Either<D, B>) => <E>(ma: E.Either<E, A>) => E.Either<D | E, B>

@Avaq
Copy link
Member Author

Avaq commented Jul 8, 2020

Not always if I'm not mistaken, take alt for example

Oh, good eye. I hadn't realized that! I don't use TypeScript often enough to notice these things, I must admit. But I intend to do a bit more with Deno, so I might. :)

that's why I'm using defaults for the constructors type parameters

I don't understand how those defaults are making a difference. o.0
What is happening here?

const resolve: <R>(value: R) => fl.FutureInstance<never, R> = fl.resolve
const resolve_: <L = never, R = never>(value: R) => fl.FutureInstance<L, R> = fl.resolve

const x = fl.reject('a').pipe(fl.alt(resolve(42))) // not ok
const y = fl.reject('a').pipe(fl.alt(resolve_(42))) // ok

const a = resolve(42); // FutureInstance<never, number>
const b = resolve_(42); // FutureInstance<never, number>

const x = fl.reject('a').pipe(fl.alt(a)) // not ok
const y = fl.reject('a').pipe(fl.alt(b)) // not ok

I must be misunderstanding something about how TypeScript assigns generics. Is it that in the alt(resolve_(42)) case, the default value is overwritten by the parameter type supplied by alt?

I ended up with two different API: the standard chain (no widening) [...] and a chainW variant (auto-widening)

Oh, I can see that making a lot of sense for a TypeScript-first library! That gives an interesting perspective. I'm going to let that sink in. :)

@tetsuo
Copy link
Contributor

tetsuo commented Jul 8, 2020

const resolve: <R>(value: R) => fl.FutureInstance<never, R> = fl.resolve

L is not parameterized in here, so it will always be never.

const b = resolve_(42); // FutureInstance<never, number>

When you do the assignment on b using resolve_ you set L to the default type, which is again, never.

Try this:

const x = fl.reject('a').pipe(fl.alt(resolve(42))) // not ok
const y = fl.reject('a').pipe(fl.alt(resolve_(42))) // ok

or

pipe(fl.reject('a'), fl.alt(resolve_(42)))

@Avaq Avaq changed the title Improve type inference, and add branch awareness to TypeScript types Add branch awareness to TypeScript types (auto-expanding) Aug 6, 2020
@Avaq
Copy link
Member Author

Avaq commented Aug 6, 2020

I created a strict (non-auto-expanding) variant of this pull request, but it introduces new challenges: #438

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants