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

Type inference algorithm depends on function argument order? (bug?) #17237

Closed
benny-medflyt opened this issue Jul 17, 2017 · 4 comments
Closed
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@benny-medflyt
Copy link

TypeScript Version: 2.4.1

I have a function that has 3 parameters:

function f(a: A, b: B, c: C) {
    // ...
}

I was surprised to discover that my program suddenly breaks (fails to compile) if I reorder the parameters:

function f(a: A, c: C, b: B) {
    // Body is completely identical
}

Of course the arguments were swapped at the call site of f as well. The failure is due to type inference working differently.

Below is a complete example showing the issue.

leftJoin1 and leftJoin2 are identical functions, but with their parameter order swapped.

test1 and test2 call leftJoin1 and leftJoin2 respectively (with the appropriate argument ordering).

Notice that the pair of leftJoin1 + test1 functions works fine, while the pair of leftJoin2 + test2 functions is broken.

To really let this sink in: delete the functions leftJoin2 and test2. Then manually swap the parameters of leftJoin1 together with swapping the arguments to it in test1, and observe how the program will have changed from valid to broken.

// test1 + leftJoin1
//
// Both these functions compile great!
//
export function leftJoin1<s, a extends object>(
        q: Q<s>,
        s: (q: Q<Inner<s>>) => MakeCols<Inner<s>, a>,
        pred: (p: MakeCols<s, a>) => Col<s, boolean>
    ): LeftCols<s, a> {
    q;
    pred;
    throw new Error("not implemented");
}
export function test1<s>(q: Q<s>) {
    leftJoin1(q,
        q => select(q, addressTable),
        address => stub(address.name)  // Works fine!  address: MakeCols<s, Address>
    );
}

// test2 + leftJoin2
//
// Swap the parameters and now there is an error!
//
export function leftJoin2<s, a extends object>(
        q: Q<s>,
        pred: (p: MakeCols<s, a>) => Col<s, boolean>,
        s: (q: Q<Inner<s>>) => MakeCols<Inner<s>, a>
    ): LeftCols<s, a> {
    q;
    pred;
    throw new Error("not implemented");
}
export function test2<s>(q: Q<s>) {
    leftJoin2(q,
        address => stub(address.name),  // Error here!  address: MakeCols<s, {}>
        q => select(q, addressTable)
    );
}

// --------------------------------------------------------------------

export class Q<s> {
    private constructor() { }
    "Q": Q<s>;
    s: s;
}

export interface Table<a> {
    "Table": Table<a>
    a: a;
}

export class Inner<s> {
    private constructor() { }
    "Inner": Inner<s>;
    s: s;
}

export type MakeCols<s, T extends object> = {
    [P in keyof T]: Col<s, T[P]>;
}

export class Col<s, a> {
    private constructor() { }
    "Col": Col<s, a>;
    private s: s;
    private a: a;
}

export type LeftCols<S, A> = {
    [P in keyof A]: Col<S, A[P] | null>;
}


export interface Address {
    readonly name: string;
    readonly city: string;
}

export const addressTable = declareTable<Address>("address");

export function declareTable<a>(tableName: string): Table<a> {
    tableName;
    throw new Error("not implemented");
}

export function select<s, a extends object>(q: Q<s>, table: Table<a>): MakeCols<s, a> {
    q;
    table;
    throw new Error("not implemented");
}

export function stub<s, a>(e: Col<s, a>): Col<s, boolean> {
    e;
    throw new Error("not implemented");    
}

tsconfig.json

{
    "compilerOptions": {
        "sourceMap": true,
        "inlineSources": true,
        "declaration": false,
        "removeComments": false,
        "newLine": "LF",
        "preserveConstEnums": false,
        "allowUnreachableCode": false,
        "allowUnusedLabels": false,
        "forceConsistentCasingInFileNames": true,
        "noFallthroughCasesInSwitch": true,
        "noImplicitAny": true,
        "noImplicitReturns": true,
        "noImplicitThis": true,
        "noUnusedLocals": true,
        "noUnusedParameters": true,
        "strictNullChecks": true,
        "suppressExcessPropertyErrors": false,
        "suppressImplicitAnyIndexErrors": false,
        "lib": [
            "dom",
            "es2015.collection",
            "es2015.promise",
            "es2015.symbol",
            "es5",
            "es6"
        ],
        "noLib": false,
        "target": "es2015",
        "module": "commonjs",
        "moduleResolution": "node",
        "isolatedModules": false
    }
}
@ikatyang
Copy link
Contributor

There is no enough information for a in leftJoin2's pred, thus TS just inferred it as {}.

This is a consequence of inference working left-to-right for contextually typed arguments.

See #15680 (comment).

@benny-medflyt
Copy link
Author

Thank you. That comment does explain it. And @ahejlsberg is right: turns out that leftJoin1 is indeed more "ergonomic", giving helpful autocomplete.

So type "inference working left-to-right for contextually typed arguments" is by design? It might be worthwhile to document this, since it is (at least for me) surprising behaviour that I haven't seen in any other language. Maybe in the Advanced Types section of the handbook?
Also might be worth going into more depth on what exactly a "Contextual Type" is.

Thank you

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Jul 17, 2017
@RyanCavanaugh
Copy link
Member

Ideally order would not matter, but we have architectural constraints that prevent a fully-optimal solution. The TL;DR version is that we're not set up to contextually type a function, typecheck its body, undo that work, and then try again with a different contextual type (this could also potentially be very, very slow in certain edge cases)

@mhegazy
Copy link
Contributor

mhegazy commented Aug 17, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Aug 17, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants