Check overloaded function implementation for every overload signature

[Amareis]

Suggestion

🔍 Search Terms

overload, overloaded function, overloaded implementation

✅ Viability Checklist

My suggestion meets these guidelines:

• This wouldn't be a breaking change in existing TypeScript/JavaScript code
• This wouldn't change the runtime behavior of existing JavaScript code
• This could be implemented without emitting different JS based on the types of the expressions
• This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
• This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

Initially discussed in #33912.

For overloaded function implementation its body could be checked with every overload signature. Effectively, it's just normalized overload signature (so arguments count is same for every overload, but some of them is just undefined) + implementation source code. If type checking for any of this enriched overloads is failing, implementation is incorrect.

📃 Motivating Example

Before:

function fun(a: string): number
function fun(a: number): string
function fun(a: string | number): number | string {
    return a //totally incorrect
}

After:

function fun(a: string): number
function fun(a: number): string
function fun(a /*:infer*/)/*:infer*/ {
    //TSXXXX: string is not assignable to number for overload (a: string) => number
    //TSXXXX: number is not assignable to string for overload (a: number) => string
    return a 

💻 Use Cases

Obviously, this will be good for every overloaded functions/methods, but checking will be somewhat slower.
For backward compatibility additional syntax (e.g. infer in function argument/return type position) or compiler flag may be required.
Handles many cases for #33912

[Amareis]

Additionally, it can be improved more with better overloads detection (with examples in #33912 (comment)), or when merging/implementing interface which contains overloads with class which implements them. But it's different issue, which I can fill later.

[MartinJohns]

Duplicate of #44262 / #13235. Not sure if the circumstances have changed.

[Amareis]

I think it's not a "duplicate", but "related to", since I suggest a some way to resolve all this issues.

Also, after some thinking, I see clear way to implement this: implementation must have no argument/return types, and noImplicitAny should be turned on. In such way it will be fully backward compatible and no need for any syntax changes or additional flags. Type of argument inside of a function will be inferred (for developer, e.g. in a IDE hints) just as union of overloads argument types - basically, same as now with explicit arguments types.

And complexity will be just O(n) for number of overloads, which is reasonably well. Also, typechecking of every overloading body will be slightly less complex than current typechecking of implementation body, where all arguments and return types is unions (which I think is most common way to write implementations). And no "unsafe checking" for implementation/overloads signature compatibility will be needed, as written in #44262 (comment)

[MartinJohns]

I think it's not a "duplicate", but "related to", since I suggest a some way to resolve all this issues.

Usually the team mentions that another approaches to solve a problem does not warrant a new issue when the problem is essentially the same.

Also, I don't really see how your suggestion would solve the problem. What is the type of a within the function? When it's "double checked with every overload signature" it's one time string, and a second time number. This would lead to issues, because a string can't be compared to a number, resulting in the "no overlap" error. That's just a simple case. So essentially some of the type checks would need to be disabled within, leading to other issues.

[Amareis]

When it's "double checked with every overload signature" it's one time string, and a second time number. This would lead to issues, because a string can't be compared to a number, resulting in the "no overlap" error.

Good point. But it still will be new behavior, so no existing and compiling code will be broken, and new code can properly checks for types of argument - DX is slightly worse, but here is additional type safety. And it can be handled by compiler for such cases, if there is only few possible errors like that - which i can't say for sure. How much correct code will be broken, when instead of union type one of its member is substituted? In ideal world, it should be no one. Even in your example, only equality checks could be meaningful.

Also, additional trouble can be caused by default argument values in implementation - how it should works with "enriched" overloads signature? For simple cases it should be compatible with all overloads argument type (which is flows naturally).

[RyanCavanaugh]

"Re-check the body using each overload signature" turns this into a huge problem when you're actually writing the code:

• What members appear in completion lists?
• What happens during a rename?
• What happens to quick fixes that depend on type information?
• How does find all references work?
  etc.

You also run into problems when you try to write the body because you're going to get incorrect never values:

// When check mode is "a is string" --
function fun(a: string) {
    if (typeof a === "string") {
        
    } else {
        // Error, 'valueOf' does not exist on 'never'
        return a.valueOf();
    }
}

You'll also see problems because, for good reason, TS doesn't consider code that only runs under type violations to be truly unreachable. A reasonable implementation of this overload set, for example, fails twice:

function fun(a: number, b: number): string;
function fun(a: string, b: string): number;

function fun_str(a: string, b: string): number {
    if (typeof a === "string") {
        return parseInt(b);
    } else {
        // Fails, can't legally return a string here
        return b.toString();
    }
}

function fun_num(a: number, b: number): string {
    if (typeof a === "string") {
        // Fails, can't legally return a number here
        return parseInt(b);
    } else {
        return b.toString();
    }
}

So it's just really not as simple as is claimed here; we'd need to make big advances in control flow, somewhat fundamental changes to how the language service thinks about the type of a node, and possibly invent new sorts of types altogether.

[sdrsdr]

Today I stumbled on a unchecked method override:

class STP {
	constructor (public f1:string){};
}
class STC extends STP {
	constructor (f1:string, public f2:string){
		super(f1);
	};
}


class A {
	constructor(public st:STP){

	};
	triiger_problem () {
		this.problem(this.st);
	}
	problem (params:STP) {
		console.log("pr@A: f1:",params.f1);
	}
}


class B extends A {
	override problem(params: STC): void {
		console.log("pr@B: f1:",params.f1,"f2:", params.f2);
		
	}
}


const b=new B(new STP("a"));
b.triiger_problem();

Is this a related issue or should I open a separate issue?

My problem is that parameters of the overridden method can change type to broader class that should not be allowed as it is possible to trigger unchecked access to probably missing fields

[Amareis]

@RyanCavanaugh good points, I think it can be split in two parts:

Type of arguments in checked implementation should be just inferred, just like it inferred now here (example from #33912 (comment), actually, that's pretty strange that arguments are not inferred in overloaded implementation now, since it's semantically identical to this example):

type DoStuff = {
    (arg: string): string
    (arg: number): number
}

//assignment is error currently, because return type also inferred as 'string | number'
//which is not assignable to string or number in overloads type
export const doStuff: DoStuff = function(
    // correctly inferred as string | number
    arg,
) {
    return arg
}

Implementation is still checked with inferred arguments types - so no any additional troubles here, and no any changes to tooling or types is needed. Also, for full backward compatibility arguments types can be inferred only under noImplicitAny flag, but i can't say for sure - maybe this even isn't needed, since it is just better inference...

Additionally, it can be expanded to infer types of class methods, which implements/merged some interface with defined overloads:

interface DoStuff {
    doStuff: {
        (arg: string): string
        (arg: number): number
    }
}

//or implements it, both don't infer args type
class DoStuff {
    doStuff = function(
        // still any
        arg,
    ) {
        return arg
    }
}

Second part about actually checking overloads is going to next comment.

[Amareis]

@RyanCavanaugh, if implementation is correct (not sure if it even possible to determine in current checker code), then, under strictFunctionTypes flag, overloads compatibility can be checked.

Overloads signatures is checked only on arity compatibility with implementation. Then "enriched" overloads signatures is created to match implementation shape, where extra args becames undefined, and ...rest pattern is handled.

Then function body checked against each of enriched signatures... And yes, here unreachable code and no-overlap errors should be ignored, I guess, since true errors already handled by implementation checking. Not sure how much additional burden it will cause on checker code, but don't seems as too much, when compared with something like handling return conditional types, which is can be used as alternative to properly checked overloads.

[Amareis]

(Bonus part)
With good-enough flow control, something like this can be made even in user-land code:

type JustSignatures<T> =
    T extends { (...args: infer A1): infer R1; (...args: infer A2): infer R2 } ?
        [...A1, R1] | [...A2, R2] :
        T extends { (...args: infer A1): infer R1; } ?
            [...A1, R1] :
            never;

function fun(a: number, b: number): string
function fun(a: string, b: string): number
function fun(...args: any) {
    // [number, number, string] | [string, string, number]
    type Overloads = JustSignatures<typeof fun>
    const [a, b, ret] = args as Overloads

    if (typeof a === 'string') {
        //currently, compiler cannot infer what b is always string and ret is number
        //since only truly discriminated unions is working for narrowing "linked" types
        return parseInt(b) as typeof ret;
    } else {
        return b.toString() as typeof ret;
    }
}

This can be alternate approach to correctly typed overloaded implementation, but it definitely needs some heavy compiler improvents - begins with, arguments binding currently being let-like, and such bindings isn't leverage const bindings benefits in "linked" types narrowing.

[Amareis]

How much correct code will be broken, when instead of union type one of its member is substituted?

Oh, and answering my own question, it's, of course, when you assign to value of such type something else. So it seems like issue with default arguments values, so in checked implementation it will be possible to assign to argument only intersection of all overloads' argument types... Which is somewhat confusing without good explanation.

[RyanCavanaugh]

Type of arguments in checked implementation should be just inferred, just like it inferred now here ...

What you're describing is #28172 or #25352, then

[RyanCavanaugh]

Also #22609. So overall I think the original "check it N ways" suggestion is declined on complexity grounds, and the remaining alternative proposals are duplicates of open issues. Agreed?

[typescript-bot]

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.