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

Overloads shouldn't differ by the return type of a function type. #571

Closed
vilicvane opened this issue Aug 30, 2014 · 9 comments
Closed

Overloads shouldn't differ by the return type of a function type. #571

vilicvane opened this issue Aug 30, 2014 · 9 comments
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead

Comments

@vilicvane
Copy link

If we define an interface:

interface IHandler {
    (): number;
    (): string;
}

There would be error "overloads cannot differ only by return type", which is reasonable.

However, when it comes to some situation like this:

interface IHandler<T> {
    (processer: (str: string) => string): void;
    (processer: (str: string) => number): void;
}

The compiler would treat (str: string) => string and (str: string) => number as two incompatible types, which is also reasonable but not useful enough.

I would suggest to use parameter types to defer and use return type to do type checking only.

The real situation could be Promise<T>.prototype.then, because it accepts null as parameter, if one of the two arguments is null, it will skip the signature we would want it to match and adopts to something wrong. We may add an overload with any as the return type of its function type, but this would mute type checking.

@ivogabe
Copy link
Contributor

ivogabe commented Aug 30, 2014

Your first example gives an error because a function cannot return a number AND a string. But your second example has a definition for a function that takes a function as argument that returns a string OR a number. That's possible, a function can return one of them.

I could understand that someone would like to write something like this:

function foo(getValue: () => string);
function foo(getValue: () => Promise<string>);
function foo(getValue: () => any) {
    var value = getValue();
    if (value instanceof Promise) {
        // ...
    } else {
        // ...
    }
}

Also changing the current behavior would be a breaking change, which should be avoided because it can break existing code.

@vilicvane
Copy link
Author

@ivogabe Thanks for replying. Though I have mentioned that I thought both of the behaviors are reasonable, but may not be good enough.

Consider this simplified promise interface:

interface IPromise<T> {
    then<R>(onfulfilled: void, onrejected: (reason) => T): IPromise<T>;
    then<R>(onfulfilled: (value: T) => R, onrejected?: (reason) => R): IPromise<R>;
}

var promise: IPromise<void>;

promise
    .then(value => {
        return '';
    })
    .then(null, reason => {
        return 0;
    })
    .then(value => {
        // value here gets type "number"
    });

As what I've commented, value gets type number, but based on IPromise definition, semantically, we are hoping the second invoking of then matches the first overload.

The logic of compiler is simple (as how TypeScript compiler should be), the return value of the second then make it no longer match the first overload, so it tries the second one.

As null is accepted by any type (another story), it adopts the second overload. This is logical, but not convenient and intuitive.

As for the question whether this would break current code, my opinion is that the impact could be considered minor.

@RyanCavanaugh
Copy link
Member

What exactly is the proposal here? We're not going to break the IHandler<T> pattern as it's useful and models real-world problems.

@vilicvane
Copy link
Author

Hi @RyanCavanaugh , I do think it's useful. What I mean is that, we may separate overload matching into two phases. During phase one, match the parameter types. And phase two, check the return type. It might be several return types in an interface, it would be okay. However, if a function argument, such as a callback, matches the parameter types, but not the return type, the compiler should give an error instead of just skipping this match.

@RyanCavanaugh
Copy link
Member

Think about this from the invoker of the callback's perspective -- they are going to invoke the function with some predefined set of arguments, then do something with the return value. It's possible for the invoker to determine what they got back from the callback, but it's impossible for them to know what type of parameters the function was expecting before calling it. Having the compiler match on the parameter values first is backwards from how any implementation could work in reality.

@vilicvane
Copy link
Author

However by allowing multiple return types would mostly solve the backwards issue imo (And this is exactly what people need to do now if they want to allow multiple return types, too).

@RyanCavanaugh
Copy link
Member

Can you post a minimal example of what you're describing and how it would work?

@vilicvane
Copy link
Author

example 1

interface IHandler {
    // allow multiple return types for the same parameter types.
    // as it's an interface, it doesn't need to concern the distinguishing
    // problem for developers.
    // and so far this won't have any backwards issue I think.
    (): string;
    (): number;
}

var handler: IHandler = () => {
    // either returning a string or number should be okay.
    return '';
    return 0;

    // however if trying to return something else, the compiler should give an error.
    return true;
    return undefined;
    // ...
};

example 2
which may have minor backwards issue.

interface IPromise<T> {
    then<R>(onfulfilled: void, onrejected: (reason) => T): IPromise<T>;
    then<R>(onfulfilled: (value: T) => R, onrejected?: (reason) => R): IPromise<R>;
}

var promise1: IPromise<string>;

var promise2 = promise1.then(null, reason => {
    return 0;
});

// following current spec, it would:
// 1. try to match the first overload, then fail because onrejected handler
//    asks type T (i.e. string here) for return value.
// 2. try to match the second overload, then succeed because it should.
// so there will not be an error, and promise2: IPromise<number>.

// for the proposed spec:
// I hope it would firstly match an overload by parameter types except function types,
// and parameter types of these function types if any.
// if the return types also matched that overload, everybody is happy.
// if not, try to find another overload that has exactly the same parameter types
// except function types, and parameter types of these function types if any, as the
// overload found before.
// if another matched overload is found, do return type checking. if it succeed,
// happy. if not, try process above again.
// if none completely matching overload, gives an error:
//   if any matching overload could be found before checking return type, it gives
//   an error of unmatched return type.
//   if no matching overload could be found even before checking return type, it gives
//   an error of no overload found.

var promise1: IPromise<string>;

var promise2 = promise1.then(null, reason => {
    // so here it should give an error because the return type is not T (i.e. string) or
    // anything else declared possible (though here only T).
    return 0;
});

// promise2: IPromise<string>;

@RyanCavanaugh
Copy link
Member

Again, the proposed behavior doesn't correspond to any runtime semantics that could exist.

We've tried many, many, many overload resolution rules and are satisfied with the ones we have unless there is some very good reason to change them (in a way that is not a breaking change). Some of the rules proposed in #360 might help with your scenario, depending.

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead
Projects
None yet
Development

No branches or pull requests

3 participants