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

Generic inference inside of pipe is incorrect when strict mode is disabled #25826

Closed
OliverJAsh opened this issue Jul 20, 2018 · 8 comments · Fixed by #30193
Closed

Generic inference inside of pipe is incorrect when strict mode is disabled #25826

OliverJAsh opened this issue Jul 20, 2018 · 8 comments · Fixed by #30193
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@OliverJAsh
Copy link
Contributor

TypeScript Version: 2.9.2

Search Terms:

Code

With strict mode disabled:

declare function pipe<V0, T1, T2, T3>(
    fn0: (x: V0) => T1,
    fn1: (x: T1) => T2,
    fn2: (x: T2) => T3,
): (x: V0) => T3;

type Option<T> = { t: T }
declare function optionOf<T>(value: T | null | undefined): Option<T>

declare function get(name: "set-cookie"): string[] | undefined;
declare function get(name: string): string | undefined;

// $ExpectType (x: {}) => Option<string>
// but got (x: {}) => Option<{}>
// Note: with strict mode enabled, we get the expected type.
const fn = pipe(
    ({}: {}) => 'foo',
    string => get(string),
    // Workaround: pass the function directly, instead of wrapping:
    // get,
    optionOf,
);

Related Issues:
This is similar to—but not exactly the same as—other issues I've filed when trying to use pipe:

@dardino
Copy link

dardino commented Jul 21, 2018

from #25637 :

class C1 { P1: string; }
class C2 { P2: string; }
class C3 { P3: string; }

function a(value: C1 | null): string;
function a(value: C2 | null): string;
function a(value: C1 | C2 | null): string {
    return "";
}

var c = (value?: C1 | C2 | C3 | null | undefined): string => {
    if (value instanceof C1 || value instanceof C2)        
        return a(value); // C1 | C2  --> not null or undefined, there is no reasons to give an error...
        // is a(C1) a valid call? YES
        // is a(C2) a valid call? YES
        // why error? 
    return value ? value.P3 : "";
}

var b = (value?: C1 | C2 | C3 | null | undefined): string => {
    if (value instanceof C1) return a(value);  // C1
    if (value instanceof C2) return a(value);  // C2
    return value ? value.P3 : "";
}

@OliverJAsh
Copy link
Contributor Author

OliverJAsh commented Jul 23, 2018

@dardino This issue is concerning overloads when used with a pipe function, which doesn't appear to be the case with your issue. Perhaps you could open a new issue to avoid conflating the issue being tracked here?

@dardino
Copy link

dardino commented Jul 23, 2018

Ok sorry

@OliverJAsh
Copy link
Contributor Author

@mhegazy How come this is marked as a suggestion? Isn't this a bug?

@ahejlsberg
Copy link
Member

@OliverJAsh It's an effect (or limitation) of our type inference algorithm.

In the call to pipe we first make inferences from the first and third arguments because they are not "contextually sensitive" (i.e. they're not function expressions with parameters that have contextual types), and then finally from the contextually sensitive second argument. However, as we are inferring from the third argument, we end up fixing (i.e. freezing) inferences for T2 so that we can instantiate the generic optionOf in the context of (x: T2) => T3. That causes T2 to become {} (because we have no candidates for it). We then finally end up with an error because string | undefined isn't assignable to {} in --strictNullChecks mode.

We might explore doing better in situations where instantiation of a generic function argument in the context of a function type parameter causes fixing of type parameters and attempt to delay inference from such arguments further. That would be what the "Suggestion" label refers to.

@RyanCavanaugh RyanCavanaugh added the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label Aug 17, 2018
@RyanCavanaugh RyanCavanaugh added Design Limitation Constraints of the existing architecture prevent this from being fixed and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Aug 17, 2018
@RyanCavanaugh
Copy link
Member

After a fairly long discussion of options here, we don't have any ideas for how to fix this without breaking other scenarios. Full unification is of course a "solution" but that's basically a complete ground-up rewrite, so not really in the cards for the time being.

@OliverJAsh
Copy link
Contributor Author

Thanks for the explanations. I keep running into issues with pipe and I'm not sure if they are the same or different to this one. See also #25637 and #25791.

Is there a good rule of of thumb we can use with pipe to avoid running into issues like this?

@OliverJAsh
Copy link
Contributor Author

Simpler test case for this:

declare const pipe: {
    // Workaround 1: enable this overload
    // <A, B, C>(ab: (a: A) => B, bc: (b: B) => C): (a: A) => C;

    <A, B, C, D>(ab: (a: A) => B, bc: (b: B) => C, cd: (c: C) => D): (a: A) => D;
};

declare const getString: () => string;
declare const orUndefined: (name: string) => string | undefined;
declare const identity: <T>(value: T) => T;

const fn = pipe(
    getString,

    /*
    Unexpected type error:
    Type 'string | undefined' is not assignable to type '{}'.
        Type 'undefined' is not assignable to type '{}'.
    */
    string => orUndefined(string),

    // Workaround 2: pass the function directly, instead of wrapping:
    // get,

    identity,
);

As mentioned in #29904.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants