Skip to content

Method overloads are often frustrating for me #40827

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

Closed
bpasero opened this issue Sep 29, 2020 · 5 comments
Closed

Method overloads are often frustrating for me #40827

bpasero opened this issue Sep 29, 2020 · 5 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@bpasero
Copy link
Member

bpasero commented Sep 29, 2020

TypeScript Version: 4.0.x
Search Terms: overload

Code

enum SideBySideEditor {
    PRIMARY,
    SECONDARY,
    BOTH
}

interface URI { }

interface IEditorInput { }

interface IResourceOptions {
    supportSideBySide?: SideBySideEditor;
}

function toResource(editor: IEditorInput | undefined | null): URI | undefined;
function toResource(editor: IEditorInput | undefined | null, options: IResourceOptions & { supportSideBySide?: SideBySideEditor.PRIMARY | SideBySideEditor.SECONDARY }): URI | undefined;
function toResource(editor: IEditorInput | undefined | null, options: IResourceOptions & { supportSideBySide: SideBySideEditor.BOTH }): URI | { primary?: URI, secondary?: URI } | undefined;
function toResource(editor: IEditorInput | undefined | null, options?: IResourceOptions): URI | { primary?: URI, secondary?: URI } | undefined {
    // Method implementation left out for brevity.
    return undefined;
}

export function getOriginalUri(editor: IEditorInput | undefined | null): URI | undefined;
export function getOriginalUri(editor: IEditorInput | undefined | null, options: IResourceOptions & { supportSideBySide?: SideBySideEditor.PRIMARY | SideBySideEditor.SECONDARY }): URI | undefined;
export function getOriginalUri(editor: IEditorInput | undefined | null, options: IResourceOptions & { supportSideBySide: SideBySideEditor.BOTH }): URI | { primary?: URI, secondary?: URI } | undefined;
export function getOriginalUri(editor: IEditorInput | undefined | null, options?: IResourceOptions): URI | { primary?: URI, secondary?: URI } | undefined {

    // What I would like to type:
    // return toResource(editor, options);

    // What I have to type
    if (!editor) {
        return undefined;
    }

    if (!options) {
        return toResource(editor);
    }

    if (options.supportSideBySide === SideBySideEditor.BOTH) {
        return toResource(editor, options);
    }

    if (options.supportSideBySide === SideBySideEditor.PRIMARY || options.supportSideBySide === SideBySideEditor.SECONDARY) {
        return toResource(editor, options);
    }
}

Expected behavior:
I can call the one method with overloads from the other without having to do a lot of checks.

Actual behavior:
I am often running into a problem when dealing with methods that define different overloads based on the types passed in. This works great if the method is called from the outside, but if internally I split the method up into 2 (as in the example above) I fail to call the one from the other somehow. I need to either duplicate all the code or do a lot of type checks in the second method that I will have to do again in the first method.

If there is guidelines how to do this any better, that would also help me.

Playground Link: Playground

@MartinJohns
Copy link
Contributor

Duplicate of #39885 / #1805.

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Sep 29, 2020
@RyanCavanaugh
Copy link
Member

I think you should really have a third overload on toResource:

function toResource(editor: IEditorInput | undefined | null): URI | undefined;
function toResource(editor: IEditorInput | undefined | null, options: IResourceOptions & { supportSideBySide?: SideBySideEditor.PRIMARY | SideBySideEditor.SECONDARY }): URI | undefined;
function toResource(editor: IEditorInput | undefined | null, options?: IResourceOptions & { supportSideBySide: SideBySideEditor.BOTH }): URI | { primary?: URI, secondary?: URI } | undefined;
function toResource(editor: IEditorInput | undefined | null, options?: IResourceOptions): URI | { primary?: URI, secondary?: URI } | URI | undefined;
function toResource(editor: IEditorInput | undefined | null, options?: IResourceOptions): URI | { primary?: URI, secondary?: URI } | undefined {
    return undefined;
}

export function getOriginalUri(editor: IEditorInput | undefined | null): URI | undefined;
export function getOriginalUri(editor: IEditorInput | undefined | null, options: IResourceOptions & { supportSideBySide?: SideBySideEditor.PRIMARY | SideBySideEditor.SECONDARY }): URI | undefined;
export function getOriginalUri(editor: IEditorInput | undefined | null, options: IResourceOptions & { supportSideBySide: SideBySideEditor.BOTH }): URI | { primary?: URI, secondary?: URI } | undefined;
export function getOriginalUri(editor: IEditorInput | undefined | null, options?: IResourceOptions): URI | { primary?: URI, secondary?: URI } | undefined {
    // OK
    return toResource(editor, options);
}

The thing to remember is that the implementation signature isn't visible (this is intentional) - in many cases (but not all, which is why this is intentional), you do need to write that signature "twice" (once as an overload, once as implementation). Without that last signature, this API design makes very little sense IMO, which is also why TS has a hard time reasoning about it.

@bpasero
Copy link
Member Author

bpasero commented Sep 29, 2020

@RyanCavanaugh thanks, I learned something. Was not aware that the implementation signature is hidden.

@DanielRosenwasser
Copy link
Member

Back to #4797. 😢

@opl-
Copy link

opl- commented Oct 5, 2020

Here's an especially frustrating and simple example of what I believe to be the same issue, which just happened to me:

findAll(path: string, createIfMissing: true): Node[];
findAll(path: string, createIfMissing: false): Node[] | null;
findAll(path: string, createIfMissing: boolean): Node[] | null {/* ... */}

find(path: string, createIfMissing: true): Node;
find(path: string, createIfMissing: false): Node | null;
find(path: string, createIfMissing: boolean): Node | null {
    const nodes = this.findAll(path, createIfMissing);
    //                               ^^^^^^^^^^^^^^^ Error (see below)
    if (!nodes) return null;
    return nodes[nodes.length - 1];
}

The error displayed on createIfMissing:

No overload matches this call.
  Overload 1 of 2, '(path: string, createIfNone: true): Node<D>[]', gave the following error.
    Argument of type 'boolean' is not assignable to parameter of type 'true'.
  Overload 2 of 2, '(path: string, createIfNone: false): Node<D>[] | null', gave the following error.
    Argument of type 'boolean' is not assignable to parameter of type 'false'. ts(2769)

What makes it ridiculous is that if the createIfMissing has to be either true or false, then why should I be unable to pass it into a function that can take in either of those values?


Thanks to @RyanCavanaugh's comment above mentioning that the implementation signature is hidden, I actually managed to come up with a workaround for this specific issue while explaining it above, so here it is in case anyone else has the same issue:

Instead of declaring two specific overloads, in this instance you can simply declare one specific and one more general overload, and TypeScript should use the more specific one when possible. In practice:

findAll(path: string, createIfMissing: true): Node[];
findAll(path: string, createIfMissing: boolean): Node[] | null;
findAll(path: string, createIfMissing: boolean): Node[] | null {/* ... */}

// Same for `find()`

Additionally, if you want your implementation to have a default value, you will need to explicitly mark the argument as optional in the overload, or TypeScript will require that all calls provide all arguments:

findAll(path: string, createIfMissing: true): Node[];
findAll(path: string, createIfMissing?: boolean): Node[] | null;
findAll(path: string, createIfMissing = false): Node[] | null {/* ... */}

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

5 participants