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

Inferred type depends on physical order of method overloads in code #24275

Closed
Le0Michine opened this issue May 20, 2018 · 4 comments
Closed

Inferred type depends on physical order of method overloads in code #24275

Le0Michine opened this issue May 20, 2018 · 4 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@Le0Michine
Copy link

Le0Michine commented May 20, 2018

TypeScript Version: 2.9.0-dev.20180519

Search Terms:
infer, extract function parameter types, function wrapper

Code
gist https://gist.github.com/Le0Michine/395417fbf050ec8d8aca0721e32b575a

interface Param {
    name: string;
    id?: number;
}

interface Result {
    status: number;
}

type CallBack<T> = (result: T) => void;

interface OriginalApi1 {
    fn(cb?: CallBack<Result>): string;
    fn(p: Param, cb?: CallBack<Result>): string;
}

// Same as OriginalApi1 but with diffrent order of methods
interface OriginalApi2 {
    fn(p: Param, cb?: CallBack<Result>): string;
    fn(cb?: CallBack<Result>): string;
}

// Creates wrapper type which has methods with the same names as T returning Promise instead of using callbacks
type Wrapper<T> = { [K in keyof T]: T[K] extends (p: infer P1, cb?: (res: infer P2) => void) => infer R1 ? (p: P1) => Promise<P2> : never };
// Same as Wrapper, supposed to return never if P1 is a Function
type WrapperIgnoringOverloads<T> = { [K in keyof T]: T[K] extends (p: infer P1, cb?: (res: infer P2) => void) => infer R1 ? P1 extends Function ? never : (p: P1) => Promise<P2> : never };

let a: Wrapper<OriginalApi1>;
let b: Wrapper<OriginalApi2>;

a.fn({name: 'name'}); // OK,    (property) fn: (p: Param) => Promise<Result>
b.fn({name: 'name'}); // Error, (property) fn: (p: CallBack<Result>) => Promise<Result>

let c: WrapperIgnoringOverloads<OriginalApi1>;
let d: WrapperIgnoringOverloads<OriginalApi2>;

c.fn({name: 'name'}); // OK,    (property) fn: (p: Param) => Promise<Result>
d.fn({name: 'name'}); // Error, (property) fn: never

interfaces created specifically for this example, in my use case there is actually class declaration in a *.d.ts file, this is the reason why it doesn't look like this:

interface OriginalApi {
    fn(p?: Param, cb?: CallBack<Result>): string;
}

Expected behavior:
resulting type for Wrapper<OriginalApi1> and Wrapper<OriginalApi2> should be the same, as OriginalApi1 and OriginalApi2 are identical types

Wrapper<OriginalApi1> {
    fn: (p: Param) => Promise<string>
}
Wrapper<OriginalApi2> {
    fn: (p: Param) => Promise<string>
}

Actual behavior:

types Wrapper<OriginalApi1> and Wrapper<OriginalApi2> are different despite the fact that OriginalApi1 and OriginalApi2 are the same

Wrapper<OriginalApi1> {
    fn: (p: Param) => Promise<string>
}
Wrapper<OriginalApi2> {
    fn: (p: CallBack<Result>) => Promise<string>
}

Playground Link:
https://www.typescriptlang.org/play/#src=...

Related Issues:
no

@mhegazy
Copy link
Contributor

mhegazy commented May 21, 2018

ReturnType or the use of infer in a function parameter or return position does handle overloads. the last overload is used for inference as it is assumed to be the most general.

See more in https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#type-inference-in-conditional-types

@mhegazy mhegazy added the Design Limitation Constraints of the existing architecture prevent this from being fixed label May 21, 2018
@Le0Michine Le0Michine reopened this May 21, 2018
@Le0Michine
Copy link
Author

Le0Michine commented May 21, 2018

Is there any way to workaround this limitation? Haven't seen this information about function overloading in other places and there is no any tsc Warning/Error so I'm not sure everybody using TypeScript considers this approach as a best practice.
Why TS can't choose proper overload taking into account parameters amount and types? Is it something what isn't going to be changed?

I see that it seems to be by design for infer keyword (still looks weird as there is no any Warning about that), so I reduced my example to the following snippet, it still looks confusing:

interface OriginalApi1 {
    fn(p: string): string;
    fn(p: number): string;
}

// Same as OriginalApi1 but with diffrent order of methods
interface OriginalApi2 {
    fn(p: number): string;
    fn(p: string): string;
}

// Creates wrapper type which has methods accepting only string parameters
type Wrapper<T> = { [K in keyof T]: T[K] extends (p: infer P1) => any ? P1 extends string ? (p: P1) => P1 : never : never };

let a: Wrapper<OriginalApi1>;
let b: Wrapper<OriginalApi2>;

a.fn(); // Error,    (property) fn: never
b.fn('str'); // OK, (property) fn: (p: string) => string

playground

I was trying to do something similar without infer, it would solve my issue, however, it doesn't work as well. In the next example I'm trying to filter functions by parameter type, it works well if there is no overloading and doesn't work at all with it

interface OriginalApi1 {
    fn1(p: string): string;
    fn1(p: number, p2: number): string;
    fn2(p: number, p2: number): string;
}

// Filters methods by parameter type
type Filter<T> = { [K in keyof T]: T[K] extends (p: string) => any ? T[K] : never }

let c: Filter<OriginalApi1>;

c.fn1('str'); // OK
c.fn1(23, 42); // OK, why? error expected
c.fn2(23, 42); // Error, (property) fn2: never, as expected

playground

@mhegazy
Copy link
Contributor

mhegazy commented May 21, 2018

Overload resolution is currently a zero-order process, and is not deferred nor does it work on higher order types. see related discussion in #6606 (comment)

@typescript-bot
Copy link
Collaborator

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

FrankHassanabad added a commit to elastic/kibana that referenced this issue Feb 9, 2021
…on and adds deleteUserRole utility (#90533)

## Summary

Unskips tests after a ES promotion and adds a delete user role utility.

Ref:
#90229
#88302

Removes one `any` from the utils by switching to using `ProvidedType`

Before:
<img width="558" alt="Screen Shot 2021-02-05 at 2 45 37 PM" src="https://user-images.githubusercontent.com/1151048/107098890-8dce5b80-67cd-11eb-8f6e-51f83eef4647.png">

After:
<img width="513" alt="Screen Shot 2021-02-05 at 4 13 23 PM" src="https://user-images.githubusercontent.com/1151048/107098898-9161e280-67cd-11eb-8085-a5220938834e.png">

Turns out that return types on overloaded functions aren't easy fwiw and will fall on the bottom one which in this case looked to be `any` which we don't want:
microsoft/TypeScript#24275 (comment)


### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
FrankHassanabad added a commit to elastic/kibana that referenced this issue Feb 10, 2021
…romotion and adds deleteUserRole utility (#90533) (#90847)

* [Security Solutions][Detection Engine] Unskips tests after ES promotion and adds deleteUserRole utility (#90533)

## Summary

Unskips tests after a ES promotion and adds a delete user role utility.

Ref:
#90229
#88302

Removes one `any` from the utils by switching to using `ProvidedType`

Before:
<img width="558" alt="Screen Shot 2021-02-05 at 2 45 37 PM" src="https://user-images.githubusercontent.com/1151048/107098890-8dce5b80-67cd-11eb-8f6e-51f83eef4647.png">

After:
<img width="513" alt="Screen Shot 2021-02-05 at 4 13 23 PM" src="https://user-images.githubusercontent.com/1151048/107098898-9161e280-67cd-11eb-8085-a5220938834e.png">

Turns out that return types on overloaded functions aren't easy fwiw and will fall on the bottom one which in this case looked to be `any` which we don't want:
microsoft/TypeScript#24275 (comment)

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

# Conflicts:
#	x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_index.ts
#	x-pack/test/detection_engine_api_integration/security_and_spaces/tests/delete_signals_migrations.ts

* Fixes test failure as the permissions are true for 7.x -> 7.12 compared to false for 7.11
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

No branches or pull requests

3 participants