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

Higher order function type inference #30215

Merged
merged 20 commits into from Mar 8, 2019

Conversation

10 participants
@ahejlsberg
Copy link
Member

ahejlsberg commented Mar 4, 2019

With this PR we enable inference of generic function type results for generic functions that operate on other generic functions. For example:

declare function pipe<A extends any[], B, C>(ab: (...args: A) => B, bc: (b: B) => C): (...args: A) => C;

declare function list<T>(a: T): T[];
declare function box<V>(x: V): { value: V };

const listBox = pipe(list, box);  // <T>(a: T) => { value: T[] }
const boxList = pipe(box, list);  // <V>(x: V) => { value: V }[]

const x1 = listBox(42);  // { value: number[] }
const x2 = boxList("hello");  // { value: string }[]

const flip = <A, B, C>(f: (a: A, b: B) => C) => (b: B, a: A) => f(a, b);
const zip = <T, U>(x: T, y: U): [T, U] => [x, y];
const flipped = flip(zip);  // <T, U>(b: U, a: T) => [T, U]

const t1 = flipped(10, "hello");  // [string, number]
const t2 = flipped(true, 0);  // [number, boolean]

Previously, listBox would have type (x: any) => { value: any[] } with an ensuing loss of type safety downstream. boxList and flipped would likewise have type any in place of type parameters.

When an argument expression in a function call is of a generic function type, the type parameters of that function type are propagated onto the result type of the call if:

  • the called function is a generic function that returns a function type with a single call signature,
  • that single call signature doesn't itself introduce type parameters, and
  • in the left-to-right processing of the function call arguments, no inferences have been made for any of the type parameters referenced in the contextual type for the argument expression.

For example, in the call

const f = pipe(list, box);

as the arguments are processed left-to-right, nothing has been inferred for A and B upon processing the list argument. Therefore, the type parameter T from list is propagated onto the result of pipe and inferences are made in terms of that type parameter, inferring T for A and T[] for B. The box argument is then processed as before (because inferences exist for B), using the contextual type T[] for V in the instantiation of <V>(x: V) => { value: V } to produce (x: T[]) => { value: T[] }. Effectively, type parameter propagation happens only when we would have otherwise inferred the constraints of the called function's type parameters (which typically is the dreaded {}).

The above algorithm is not a complete unification algorithm and it is by no means perfect. In particular, it only delivers the desired outcome when types flow from left to right. However, this has always been the case for type argument inference in TypeScript, and it has the highly desired attribute of working well as code is being typed.

Note that this PR doesn't change our behavior for contextually typing arrow functions. For example, in

const f = pipe(x => [x], box);  // (x: any) => { value: any[] }

we infer any (the constraint declared by the pipe function) as we did before. We'll infer a generic type only if the arrow function explicitly declares a type parameter:

const f = pipe(<U>(x: U) => [x], box);  // <U>(x: U) => { value: U[] }

When necessary, inferred type parameters are given unique names:

declare function pipe2<A, B, C, D>(ab: (a: A) => B, cd: (c: C) => D): (a: [A, C]) => [B, D];

const f1 = pipe2(list, box);  // <T, V>(a: [T, V]) => [T[], { value: V }]
const f2 = pipe2(box, list);  // <V, T>(a: [V, T]) => [{ value: V }, T[]]
const f3 = pipe2(list, list);  // <T, T1>(a: [T, T1]) => [T[], T1[]]

Above, we rename the second T to T1 in the last example.

Fixes #417.
Fixes #3038.
Fixes #9366.
Fixes #9949.

@jack-williams

This comment has been minimized.

Copy link
Contributor

jack-williams commented Mar 4, 2019

Is this the correct behaviour?

function ap<A>(fn: (x: A) => A, x: A): () => A {
    return () => fn(x);
}
declare function id<A>(x: A): A
const w = ap(id, 10); // Argument of type '10' is not assignable to parameter of type 'A'. [2345]

I think the logic that floats type parameters out when the return value is a generic function might be too eager. In existing cases where {} would have been inferred it might not be because the application is sufficiently generic, rather inference didn't produce anything sensible but that still type-checked.

I haven't looked into this much, so apologies in advance if I've got something wrong.

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Mar 4, 2019

@jack-williams The core issue here is the left to right processing of the arguments. Previously we'd get it wrong as well (in that we'd infer {} instead of number), but now what was a loss of type safety instead has become an error. Ideally we ought to first infer from arguments that are not generic functions and then process the generic functions afterwards, similar to what we do for contextually typed arrow functions and function expressions.

@jack-williams

This comment has been minimized.

Copy link
Contributor

jack-williams commented Mar 5, 2019

@ahejlsberg Ah yes, that makes sense. I agree that it was previously 'wrong', but I disagree that it was a loss of type safety (in this example). The call is perfectly safe, it's just imprecise. Previously this code was fine:

const x = ap(id, 10)();
if (typeof x === "number" && x === 10) {
    const ten: 10 = x;
}

but it will now raise an error. Could this be a significant issue for existing code?

I'll just add the disclaimer that this is really cool stuff and I'm not trying to be pedantic or needlessly pick holes. I'd like to be able to contribute more but I really haven't got my head around the main inference machinery, so all I can really do is try out some examples.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Mar 5, 2019

The safe/imprecise distinction is sort of lossy given certain operations that are considered well-defined in generics but illegal in concrete code, e.g.

function ap<A>(fn: (x: A) => A, x: A): (a: A) => boolean {
    return y => y === x;
}
declare function id<A>(x: A): A
let w = ap(id, 10)("s");

or by advantage of covariance when you didn't intend to

function ap<A>(fn: (x: A) => A, x: A[]): (a: A) => void {
  return a => x.push(a);
}
function id<A>(x: A): A { return x; }
const arr: number[] = [10];
ap(id, arr)("s");
// prints a string from a number array
console.log(arr[1]);
@jack-williams

This comment has been minimized.

Copy link
Contributor

jack-williams commented Mar 5, 2019

I think being lossy in isolation is ok; it's when you try and synthesise information that you get into trouble. The first example is IMO completely fine. The conjunction of union types, and equality over generics makes comparisons like that possible.

function eq<A>(x: A, y: A): boolean {
    return x === y;
}
eq<number | string>(10, "s");
eq(10, "s"); // error

It's just the behaviour of type inference that rejects the second application because it's probably not what the user intended. I accept that my definition of 'fine' here comes from a very narrow point-of-view, and that being 'fine' and unintentional is generally unhelpful.

I agree that the second example is very much borderline, though stuff like that is going to happen with covariance because you end up synthesising information.

I'm not trying to defend the existing behaviour; I'm all in favour of being more explicit. My only concern would stem from the fact that the new behaviour is somewhat unforgiving in that it flags the error immediately at the callsite. Users that previously handled the {} type downstream (either safely, or not), will now have to fix upstream breaks. I have no idea whether this is a practical problem, though.

On a slightly different note I wonder if the new inference behaviour could come with some improved error messages. There is a long way to go until TypeScript reaches the cryptic level of Haskell, but with generics always comes confusion. In particular, I wonder in the case of:

const w = ap(id, 10);

The error message is Argument of type '10' is not assignable to parameter of type 'A'. I wonder if there is a way to mark the parameter A as rigid and when relating to that type, suggest that the user given an instantiation for the call, and say that inference flows left-to-right.

@tycho01

This comment has been minimized.

Copy link
Contributor

tycho01 commented Mar 5, 2019

@aleksey-bykov

This comment has been minimized.

Copy link

aleksey-bykov commented Mar 5, 2019

best day ever! thank you so much @ahejlsberg and the team you made my day! i don't really know what to wish for now (except HKT's 😛 )

@tycho01

This comment has been minimized.

Copy link
Contributor

tycho01 commented Mar 6, 2019

would you have a link to the ramda/ramda-tests.ts result?

@j-oliveras

This comment has been minimized.

Copy link
Contributor

j-oliveras commented Mar 6, 2019

@tycho01 on the message of typescript-bot about testing Definitely Typed you have the link: https://typescript.visualstudio.com/TypeScript/_build/results?buildId=24299

Show resolved Hide resolved src/compiler/checker.ts
@@ -22160,7 +22186,7 @@ namespace ts {
checkNodeDeferred(node);

// The identityMapper object is used to indicate that function expressions are wildcards
if (checkMode === CheckMode.SkipContextSensitive && isContextSensitive(node)) {
if (checkMode && checkMode & CheckMode.SkipContextSensitive && isContextSensitive(node)) {

This comment has been minimized.

@weswigham

weswigham Mar 6, 2019

Member
Suggested change
if (checkMode && checkMode & CheckMode.SkipContextSensitive && isContextSensitive(node)) {
if (checkMode! & CheckMode.SkipContextSensitive && isContextSensitive(node)) {

is preferable here, IMO.

This comment has been minimized.

@ahejlsberg

ahejlsberg Mar 6, 2019

Author Member

Why is that preferable? I'm not particularly comfortable reasoning about bitwise logical operators applied to undefined values. Better to first get rid of anything falsy so we know we're just dealing with proper numeric values.

This comment has been minimized.

@weswigham

weswigham Mar 6, 2019

Member

undefined coerces to zero. We make use of this fact for objectFlags (and some other optional flags fields) all the time, AFAIK.

@@ -22200,7 +22226,7 @@ namespace ts {
const signature = getSignaturesOfType(type, SignatureKind.Call)[0];
if (isContextSensitive(node)) {
const contextualMapper = getContextualMapper(node);
if (checkMode === CheckMode.Inferential) {
if (checkMode && checkMode & CheckMode.Inferential) {

This comment has been minimized.

@weswigham

weswigham Mar 6, 2019

Member
Suggested change
if (checkMode && checkMode & CheckMode.Inferential) {
if (checkMode! & CheckMode.Inferential) {

is preferable here, IMO.

@@ -23341,22 +23365,127 @@ namespace ts {
}

function instantiateTypeWithSingleGenericCallSignature(node: Expression | MethodDeclaration | QualifiedName, type: Type, checkMode?: CheckMode) {
if (checkMode === CheckMode.Inferential) {
if (checkMode && checkMode & (CheckMode.Inferential | CheckMode.SkipGenericFunctions)) {

This comment has been minimized.

@weswigham

weswigham Mar 6, 2019

Member
Suggested change
if (checkMode && checkMode & (CheckMode.Inferential | CheckMode.SkipGenericFunctions)) {
if (checkMode! & (CheckMode.Inferential | CheckMode.SkipGenericFunctions)) {

is preferable here, IMO.

@@ -15,8 +15,8 @@ var e = <K>(x: string, y?: K) => x.length;
>length : number

var r99 = map(e); // should be {}[] for S since a generic lambda is not inferentially typed

This comment has been minimized.

@weswigham

weswigham Mar 6, 2019

Member

Looks like this comment (and the one below on GH) need to be updated,

@@ -1798,7 +1798,7 @@ namespace ts {
const span = createTextSpanFromBounds(start, end);
const formatContext = formatting.getFormatContext(formatOptions);

return flatMap(deduplicate(errorCodes, equateValues, compareValues), errorCode => {
return flatMap(deduplicate<number>(errorCodes, equateValues, compareValues), errorCode => {

This comment has been minimized.

@weswigham

weswigham Mar 6, 2019

Member

Is this type parameter addition required? Seems strange to now need it.

This comment has been minimized.

@ahejlsberg

ahejlsberg Mar 6, 2019

Author Member

Here's what's happening: Previously we'd infer number from errorCodes, then fix that inference as we contextually typed equateValues, and finally ignore inferences from compareValues because of fixing. Now we defer processing of equateValues to get the best possible contextual type, which means we'll infer from both errorCodes and compareValues. And from compareValues we get the candidate number | undefined which is technically contravariant, but we ignore that because we're not in --strictFunctionTypes mode. So, we end up with a worse inference. The real fix here would be for us to do the work to use --strictFunctionTypes in the compiler, the legacy rules just aren't right but it is a breaking change (in other places) to fix them.

This comment has been minimized.

@weswigham

weswigham Mar 6, 2019

Member

It's probably too far gone to change; but is there a good compat reason we can't perform inferences the same way outside and inside strictFunctionTypes mode?

But aight, if that's the case I guess it can't be avoided. :(

@tycho01

This comment has been minimized.

Copy link
Contributor

tycho01 commented Mar 6, 2019

ramda test errors:

Error: /home/vsts/work/1/s/dtslint-runner/DefinitelyTyped/types/ramda/ramda-tests.ts:963:26
ERROR: 963:26   expect  TypeScript@local compile error: 
Argument of type '<T>(obj: Record<"id", T>) => T' is not assignable to parameter of type '(a: Record<"id", T>) => string'.
  Type 'T' is not assignable to type 'string'.

This looks like an improvement -- we should update the expected value to reflect the improved inference.

ERROR: 1008:51  expect  TypeScript@local compile error: 
Argument of type '{ (xs: string): string; (xs: readonly {}[]): {}[]; }' is not assignable to parameter of type '(x0: readonly {}[], x1: {}, x2: {}) => readonly number[]'.
  Types of parameters 'xs' and 'x0' are incompatible.
    Type 'readonly {}[]' is not assignable to type 'string'.

???, line, compose type

ERROR: 1410:24  expect  TypeScript@local compile error: 
Argument of type '<T>(a: T) => T' is not assignable to parameter of type '(i: number) => T'.
  Type 'number' is not assignable to type 'T'.

This seems like R.times had a sloppy type -- the number type should have been applied to the function's generic (if possible without invocation).

ERROR: 1435:51  expect  TypeScript@local compile error: 
Argument of type '{ (xs: string): string; (xs: readonly {}[]): {}[]; }' is not assignable to parameter of type '(x0: readonly {}[], x1: {}, x2: {}) => readonly number[]'.

???, line. transduce type

ERROR: 1438:37  expect  TypeScript@local compile error: 
Argument of type 'number[]' is not assignable to parameter of type 'readonly never[]'.
  Type 'number' is not assignable to type 'never'.

???, line. transduce type

ERROR: 1454:16  expect  TypeScript@local compile error: 
Argument of type '<T>(...items: T[]) => T[]' is not assignable to parameter of type '(a: number[][]) => T[]'.
  Type 'number[][][]' is not assignable to type 'T[]'.
    Type 'number[][]' is not assignable to type 'T'.

Type improved, should update expectation.

ERROR: 1609:11  expect  TypeScript@local compile error: 
Type '{ b: number; c: null; d: { e: number; }; }' is not assignable to type 'undefined'.

    at /home/vsts/work/1/s/dtslint-runner/node_modules/dtslint/bin/index.js:210:19
    at Generator.next (<anonymous>)
    at fulfilled (/home/vsts/work/1/s/dtslint-runner/node_modules/dtslint/bin/index.js:5:58)

???, line, evolve type giving undefined somehow

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Mar 7, 2019

@tycho01 Just pushed a change that fixes the errors on line 1008 and 1435, plus the follow on error on line 1438. I think that just leaves the error on line 1609 which appears to be an effect of passing the anyFunctionType wildcard we use when deferring function type arguments in type inference through conditional and/or mapped types. I'll be taking a closer look.

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Mar 7, 2019

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

typescript-bot commented Mar 7, 2019

Heya @ahejlsberg, I've started to run the Definitely Typed test suite on this PR at a9e924b. You can monitor the build here. It should now contribute to this PR's status checks.

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Mar 7, 2019

@tycho01 The issue on line 1609 has to do with the behavior of Ramda's Evolvable type:

type Evolvable<E extends Evolver> = {
    [P in keyof E]?: E[P] extends (value: infer V) => any ? V :
        E[P] extends Evolver ? Evolvable<E[P]> :
        never
};

The two conditional types aren't distributive (because their check types aren't naked type parameters) and don't work as intended when E[P] is a union type. For this reason, when resolving Evolvable<Evolver> we end up with { [x: string]: undefined } which is a very restrictive type. The Evolvable type should be rewritten to:

type Evolvable<E extends Evolver> = {
    [P in keyof E]?: Evolved<E[P]>;
};

type Evolved<T> =
    T extends (value: infer V) => any ? V :
    T extends Evolver ? Evolvable<T> :
    never;

This makes the template type distributable and Evolved<Evolvable> now properly resolves to { [x: string]: any }.

The Evolvable<Evolver> type ends up occurring during type inference because we now defer processing of the first argument in the test on line 1609, specifically because R.toString is a generic function (which, btw, it shouldn't be, but that's beside the point).

@RyanCavanaugh RyanCavanaugh added this to This Week in Design Meeting Docket Mar 8, 2019

@ahejlsberg ahejlsberg merged commit d59e51b into master Mar 8, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@ahejlsberg ahejlsberg deleted the higherOrderFunctionTypeInference branch Mar 8, 2019

@maciejw

This comment has been minimized.

Copy link

maciejw commented Mar 9, 2019

is this by design:

when I hover over

listBox('')

vs code is showing me function signature in a box

const listBox: <string>(a: string) => {
    value: string[];
}

when I place cursor inside parenthesis of listBox and press ctrl+shift+space, vs code is showing me list of overloades in a box

listBox(...args: [string]): { value: string[]; }

it would be nice to show a: string notation instead of tuple ...args: [string] notation in this box also.

@infctr infctr referenced this pull request Mar 9, 2019

Merged

[Ramda] Generic types for curry #33628

9 of 16 tasks complete
@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Mar 9, 2019

@maciejw That's actually an orthogonal issue resulting from #24897, but we should fix it.

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Mar 9, 2019

@maciejw Now fixed in #30299.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.