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

Use a conditional type to represent the return value of map #403

Merged
merged 2 commits into from
Dec 9, 2019

Conversation

wms
Copy link
Contributor

@wms wms commented Dec 4, 2019

Although #401 solved the case for using map on a ConcurrentFutureInstance, it broke FutureInstance#pipe(map(...)):

import { ConcurrentFutureInstance, FutureInstance, map } from 'fluture';

declare const x: FutureInstance<Error, number>;
declare const y: ConcurrentFutureInstance<Error, number>;
declare const double: (x: number) => number;

const v1 = map(double)(x); // ok; FutureInstance<Error, number>
const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
const v2 = x.pipe(map(double)) // error

This could potentially be down to TypeScript selecting the wrong overload (maybe because ConcurrentFutureInstance is the more restrictive type?)

So, I came up with this version instead, which uses a Conditional type to represent the mapper function instead. It's not ideal from an ergonomics perspective, as you end up with the whole conditional type appearing in Intellisense information. I've never been fond of that. But, it does fix (and preserve all type information) for the three forms above:

import { ConcurrentFutureInstance, FutureInstance, /*map*/ } from 'fluture';

declare function map<RA, RB>(mapper: (value: RA) => RB): <T>(source: T) =>
    T extends FutureInstance<infer U, infer V> ?
    FutureInstance<U, V> :
    T extends ConcurrentFutureInstance<infer U, infer V> ?
    ConcurrentFutureInstance<U, V> :
    never;

declare const x: FutureInstance<Error, number>;
declare const y: ConcurrentFutureInstance<Error, number>;
declare const double: (x: number) => number;

const v1 = map(double)(x); // ok; FutureInstance<Error, number>
const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
const v2 = x.pipe(map(double)) // ok; FutureInstance<Error, number>

I did also experiment with simply swapping the order of the overloads around:

import { ConcurrentFutureInstance, FutureInstance, /*map*/ } from 'fluture';

declare function map<RA, RB>(mapper: (value: RA) => RB): {
    <L>(source: ConcurrentFutureInstance<L, RA>): ConcurrentFutureInstance<L, RB>;
    <L>(source: FutureInstance<L, RA>): FutureInstance<L, RB>;
}

declare const x: FutureInstance<Error, number>;
declare const y: ConcurrentFutureInstance<Error, number>;
declare const double: (x: number) => number;

const v1 = map(double)(x); // ok; FutureInstance<Error, number>
const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
const v2 = x.pipe(map(double)) // almost ok; FutureInstance<unknown, number>

Observe that the left type of v2 is unknown when it should be Error. I suspect this is down to that hanging <L> type parameter, and the only way to eliminate that (afaik) is to use a conditional type instead. Which, coincidentally, is where we came in.

Although fluture-js#401 solved the case for using `map` on a `ConcurrentFutureInstance`, it broke `FutureInstance#pipe(map(...))`:

```typescript
import { ConcurrentFutureInstance, FutureInstance, map } from 'fluture';

declare const x: FutureInstance<Error, number>;
declare const y: ConcurrentFutureInstance<Error, number>;
declare const double: (x: number) => number;

const v1 = map(double)(x); // ok; FutureInstance<Error, number>
const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
const v2 = x.pipe(map(double)) // error
```

This could potentially be down to TypeScript selecting the wrong overload (maybe because `ConcurrentFutureInstance` is the more restrictive type?)

So, I came up with this version instead, which uses a Conditional type to represent the mapper function instead. It's not ideal from an ergonomics perspective, as you end up with the whole conditional type appearing in Intellisense information. I've never been fond of that. But, it does fix (and preserve all type information) for the three forms above:

```typescript
import { ConcurrentFutureInstance, FutureInstance, /*map*/ } from 'fluture';

declare function map<RA, RB>(mapper: (value: RA) => RB): <T>(source: T) =>
    T extends FutureInstance<infer U, infer V> ?
    FutureInstance<U, V> :
    T extends ConcurrentFutureInstance<infer U, infer V> ?
    ConcurrentFutureInstance<U, V> :
    never;

declare const x: FutureInstance<Error, number>;
declare const y: ConcurrentFutureInstance<Error, number>;
declare const double: (x: number) => number;

const v1 = map(double)(x); // ok; FutureInstance<Error, number>
const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
const v2 = x.pipe(map(double)) // ok; FutureInstance<Error, number>
```

I did also experiment with simply swapping the order of the overloads around:

```typescript
import { ConcurrentFutureInstance, FutureInstance, /*map*/ } from 'fluture';

declare function map<RA, RB>(mapper: (value: RA) => RB): {
    <L>(source: ConcurrentFutureInstance<L, RA>): ConcurrentFutureInstance<L, RB>;
    <L>(source: FutureInstance<L, RA>): FutureInstance<L, RB>;
}

declare const x: FutureInstance<Error, number>;
declare const y: ConcurrentFutureInstance<Error, number>;
declare const double: (x: number) => number;

const v1 = map(double)(x); // ok; FutureInstance<Error, number>
const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
const v2 = x.pipe(map(double)) // almost ok; FutureInstance<unknown, number>
```

Observe that the left type of `v2` is `unknown` when it should be `Error`. I suspect this is down to that hanging `<L>` type parameter, and the only way to eliminate that (afaik) is to use a conditional type instead. Which, coincidentally, is where we came in.
@Avaq
Copy link
Member

Avaq commented Dec 5, 2019

Hi @wms! Thank you for bringing this up. From what I understand about your proposed type...

declare function map<RA, RB>(mapper: (value: RA) => RB): <T>(source: T) =>
    T extends FutureInstance<infer U, infer V> ?
    FutureInstance<U, V> :
    T extends ConcurrentFutureInstance<infer U, infer V> ?
    ConcurrentFutureInstance<U, V> :
    never;

...the mapper will not affect the output type. This is visible when changing the type of your double definition to (x: number) => string. We would expect v1 and v1p to change to FutureInstance<Error, string> and ConcurrentFutureInstance<Error, string> respectively, but they keep the number type on the right side. That's because your conditional type just forwards the inner types inferred from T into the output type of the function.

Maybe it would work to change the function declaration to the following:

declare function map<RA, RB>(mapper: (value: RA) => RB): <T>(source: T) =>
    T extends FutureInstance<infer U, infer RA> ?
    FutureInstance<U, RB> :
    T extends ConcurrentFutureInstance<infer U, infer RA> ?
    ConcurrentFutureInstance<U, RB> :
    never;

In my own experiments, this appears to be giving the desired results.

@Avaq
Copy link
Member

Avaq commented Dec 5, 2019

Ah, and since RA can be inferred from the passed-in function, I think we can stop inferring it from T:

declare function map<RA, RB>(mapper: (value: RA) => RB): <T>(source: T) =>
    T extends FutureInstance<infer U, RA> ?
    FutureInstance<U, RB> :
    T extends ConcurrentFutureInstance<infer U, RA> ?
    ConcurrentFutureInstance<U, RB> :
    never;

@Avaq
Copy link
Member

Avaq commented Dec 5, 2019

And I would prefer to name the left type L instead of U, for the sake of consistency.

@wms
Copy link
Contributor Author

wms commented Dec 5, 2019

Hi @Avaq - thanks for your comments. Indeed, it occurred to me this morning that I could probably simplify the conditional by not inferring the right value's type and just re-using an existng parameter.

But, as you've just pointed out, it's acutally more than just an optimization and the semantically correct thing to do.

Give me ~20 minutes to update this. While I'm there I'll rename U to L as well.

@Avaq
Copy link
Member

Avaq commented Dec 5, 2019

The downside of this approach is that the user doesn't get a clear error message when providing inconsistent types. Instead they just get a return value of type never:

declare const x: FutureInstance<Error, boolean>;
declare const double: (x: number) => string;
const v1 = map(double)(x);

v1 // never

@wms
Copy link
Contributor Author

wms commented Dec 5, 2019

@Avaq Good point, I think I've solved for that case too. Additional commit incoming.

1. The previous commit did not solve for the case where the mapper function changes the type of the right value:

```typescript
declare const toString: (x: number) => string;

const v3 = map(toString)(x); // ok but incorrect; FutureInstance<Error, number>
const v3p = map(toString)(y); // ok but incorrect; ConcurrentFutureInstance<Error, number>
const v4 = x.pipe(map(toString)) // ok but incorrect; FutureInstance<Error, number>
```

This is addressed by re-using the `RA` and `RB` type parameters instead of inferring:

```typescript
declare function map<RA, RB>(mapper: (value: RA) => RB): <T>(source: T) =>
    T extends FutureInstance<infer L, RA> ?
    FutureInstance<L, RB> :
    T extends ConcurrentFutureInstance<infer L, RA> ?
    ConcurrentFutureInstance<L, RB> :
    never;

const v3 = map(toString)(x); // ok; FutureInstance<Error, string>
const v3p = map(toString)(y); // ok; ConcurrentFutureInstance<Error, string>
const v4 = x.pipe(map(toString)) // ok; FutureInstance<Error, string>
```

2. However, this introduces a potentially hard to track down error when using the wrong type:

```typescript
const v5 = map(toString)(z); // never w/ no error at callsite; sub-optimal
const v5p = map(toString)(z); // never w/ no error at callsite; sub-optimal
const v6 = z.pipe(map(toString)) // never w/ no error at callsite; sub-optimal
```

To address this, we place a constraint on `T`:

```typescript
declare function map<RA, RB>(mapper: (value: RA) => RB): <T extends FutureInstance<any, RA> | ConcurrentFutureInstance<any, RA>>(source: T) =>
    T extends FutureInstance<infer L, RA> ?
    FutureInstance<L, RB> :
    T extends ConcurrentFutureInstance<infer L, RA> ?
    ConcurrentFutureInstance<L, RB> :
    never;

const v1 = map(double)(x); // ok; FutureInstance<Error, number>
const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
const v2 = x.pipe(map(double)) // ok; FutureInstance<Error, number>

const v3 = map(toString)(x); // ok; FutureInstance<Error, string>
const v3p = map(toString)(y); // ok; ConcurrentFutureInstance<Error, string>
const v4 = x.pipe(map(toString)) // ok; FutureInstance<Error, string>

const v5 = map(toString)(z); // error at callsite; that's good
const v5p = map(toString)(z); // error at callsite; that's good
const v6 = z.pipe(map(toString)) // error at callsite; that's good
```
@Avaq
Copy link
Member

Avaq commented Dec 5, 2019

I went back to understand what's happening with the current definition for map:

//index.ts
import {map, FutureInstance} from 'fluture'

declare const x: FutureInstance<Error, number>;
declare const transform: (x: number) => string;

const mapTransform = map(transform);
const v2 = x.pipe(mapTransform);
  • mapTransform was correctly assigned the type:

    const mapTransform: {
        <L>(source: FutureInstance<L, number>): FutureInstance<L, string>;
        <L>(source: ConcurrentFutureInstance<L, number>): ConcurrentFutureInstance<L, string>;
    }
  • x.pipe was incorrectly assigned the type:

    (method) FutureInstance<Error, number>.pipe<ConcurrentFutureInstance<unknown, string>>(fn: (future: FutureInstance<Error, number>) => ConcurrentFutureInstance<unknown, string>): ConcurrentFutureInstance<unknown, string>

And so the TypeError that occurs is as follows:

$ tsc index.ts 
index.ts:14:19 - error TS2345: Argument of type '{ <L>(source: FutureInstance<L, number>): FutureInstance<L, string>; <L>(source: ConcurrentFutureInstance<L, number>): ConcurrentFutureInstance<...>; }' is not assignable to parameter of type '(future: FutureInstance<Error, number>) => ConcurrentFutureInstance<unknown, string>'.
  Property 'sequential' is missing in type 'FutureInstance<any, string>' but required in type 'ConcurrentFutureInstance<unknown, string>'.

14 const v2 = x.pipe(mapTransform);
                     ~~~~~~~~~~~~

It seems that TypeScript is telling us that the type it assigned to x.pipe based on information derived from mapTransform is not compatible with mapTransform itself. This looks to me like TypeScript is misbehaving. It should have never thought that x.pipe would expect a function of type (future: FutureInstance<Error, number>) => ConcurrentFutureInstance<unknown, string> based on our overloaded mapTransform function.

@wms
Copy link
Contributor Author

wms commented Dec 5, 2019

Is it possible that microsoft/TypeScript#30369 is the culprit here?

@Avaq
Copy link
Member

Avaq commented Dec 5, 2019

Based on the title of that issue, yes. Let me look into that. :)

@Avaq
Copy link
Member

Avaq commented Dec 5, 2019

In general we can't do overload resolution at the same time as inference and will usually be working with either the first or the last overload, depending on the situation -- comment

Interestingly, what seems to be happening in our case is that it appears to have merged the two overloads into one single new function type, whose domain is taken from the first overload and codomain taken from the second.

@wms
Copy link
Contributor Author

wms commented Dec 5, 2019

@Avaq I've just added a commit to this branch that solves for the original cases plus the ones you brought up. I shall repeat it here for future reference:

  1. The previous commit did not solve for the case where the mapper function changes the type of the right value:
declare const toString: (x: number) => string;

const v3 = map(toString)(x); // ok but incorrect; FutureInstance<Error, number>
const v3p = map(toString)(y); // ok but incorrect; ConcurrentFutureInstance<Error, number>
const v4 = x.pipe(map(toString)) // ok but incorrect; FutureInstance<Error, number>

This is addressed by re-using the RA and RB type parameters instead of inferring:

declare function map<RA, RB>(mapper: (value: RA) => RB): <T>(source: T) =>
    T extends FutureInstance<infer L, RA> ?
    FutureInstance<L, RB> :
    T extends ConcurrentFutureInstance<infer L, RA> ?
    ConcurrentFutureInstance<L, RB> :
    never;

const v3 = map(toString)(x); // ok; FutureInstance<Error, string>
const v3p = map(toString)(y); // ok; ConcurrentFutureInstance<Error, string>
const v4 = x.pipe(map(toString)) // ok; FutureInstance<Error, string>
  1. However, this introduces a potentially hard to track down error when using the wrong type:
const v5 = map(toString)(z); // never w/ no error at callsite; sub-optimal
const v5p = map(toString)(z); // never w/ no error at callsite; sub-optimal
const v6 = z.pipe(map(toString)) // never w/ no error at callsite; sub-optimal

To address this, we place a constraint on T:

declare function map<RA, RB>(mapper: (value: RA) => RB): <T extends FutureInstance<any, RA> | ConcurrentFutureInstance<any, RA>>(source: T) =>
    T extends FutureInstance<infer L, RA> ?
    FutureInstance<L, RB> :
    T extends ConcurrentFutureInstance<infer L, RA> ?
    ConcurrentFutureInstance<L, RB> :
    never;

const v1 = map(double)(x); // ok; FutureInstance<Error, number>
const v1p = map(double)(y); // ok; ConcurrentFutureInstance<Error, number>
const v2 = x.pipe(map(double)) // ok; FutureInstance<Error, number>

const v3 = map(toString)(x); // ok; FutureInstance<Error, string>
const v3p = map(toString)(y); // ok; ConcurrentFutureInstance<Error, string>
const v4 = x.pipe(map(toString)) // ok; FutureInstance<Error, string>

const v5 = map(toString)(z); // error at callsite; that's good
const v5p = map(toString)(z); // error at callsite; that's good
const v6 = z.pipe(map(toString)) // error at callsite; that's good

@Avaq
Copy link
Member

Avaq commented Dec 5, 2019

I'd like to run this by @gcanti, who was the driving force behind #401. Am I correct in assuming this will not pose a problem for existing code using map, and can therefore release it as a patch?

@wms
Copy link
Contributor Author

wms commented Dec 5, 2019

I think I've covered all use-cases in my tests, but this is my first time out with Fluture and fixed this as I found it. It would be really helpful to get some input from people who are already using this pattern in TypeScript.

@gcanti
Copy link
Contributor

gcanti commented Dec 5, 2019

@Avaq sorry, I didn't notice the issue with pipe. @wms 's solution looks good to me

@Avaq Avaq merged commit d01ff1d into fluture-js:master Dec 9, 2019
Avaq added a commit that referenced this pull request Dec 9, 2019
- #403 Fix an issue in TypeScript when using .pipe on map(f)
@Avaq
Copy link
Member

Avaq commented Dec 9, 2019

Released as 12.0.2.

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.

3 participants