Checking for unsafe function overloads

[rotemdan]

(This attempts to provide a solution for a more general version of #13223. I wanted to separate the two issues as the title of #13223 doesn't successfully describe the more general case addressed here)

This is still somewhat of a work in progress, I still need to validate this correctly addresses all scenarios, including all cases where there are there multiple overloaded parameters.

Consider this function:

function example(s: "Hello"): number;
function example(s: string): boolean;
function example(s: string): number | boolean {
	if (s === "Hello")
		return 54;
	else if (typeof s === "string")
		return true;
	else
		throw new TypeError("The given argument is not a string");
}

Superficially, it may seem like this function signature successfully describes and enforces the relationship between the parameter types and return types of the function at runtime. However, maybe surprisingly, it turns out this isn't really the case in practice:

Consider this secondary function:

function example2(a: string) {
	return example(a);
}

Now let's call the secondary function:

let result = example2("Hello");

The inferred type of result is boolean, however its actual value at runtime is 54, which is not of type boolean.

Why did this happen? Why did the compiler get it wrong?

Because Typescript uses erasable types, it cannot positively determine if a should be bound to the literal type "Hello" during design time. Relying on type annotations alone the compiler cannot disambiguate between the particular string "Hello" and any other string.

Proposal

Add a compiler switch such as --noUnsafeFunctionOverloads that would disallow function overloads that satisfy the following three conditions:

1. One parameter overload type is a supertype of a second one.
2. At least one of the parameters or the return type of the more general overload is not a supertype of the corresponding parameter or return type of the more specific overload.
3. The more specific overload is positioned before the more general one (edit: it seems like currently the compiler doesn't care about ordering in this case, so maybe this requirement isn't really necessary).

For illustration here's a different example where enabling the option would not cause a compilation error:

function example(s: "Hello"): 54;
function example(s: string): number;
function example(s: string): number {
	if (s === "Hello")
		return 54;
	else if (typeof s === "string")
		return Math.random();
	else
		throw new TypeError("The given argument is not a string");
}

Since number is a supertype of 54, the function would still yield a valid return type (number) even if the argument passed to s is annotated as string but has the value "Hello" at runtime.

However, this example would fail type-checking, as the second parameter in the second overload is not a supertype of the second parameter in the first overload:

function example(a: "Hello", b: boolean): boolean;
function example(a: string, b: number): boolean;
function example(a: string, b: boolean | number): boolean{
	if (a === "Hello") {
		callSomeFunctionThatExpectsBoolean(b);
		return true;
	} else if (typeof a === "string") {
		callSomeFunctionThatExpectsNumber(b);
		return false;
	}
	else
		throw new TypeError("The first argument must be a string");
}

[rotemdan]

An alternative approach is to infer the return type as the union of the two conflicting overloads' return types, and do the same for all other parameters, however this would still compromise type safety for additional parameters as it would accept effectively illegal parameter type combinations that would successfully compile but fail at run-time.

function example(a: "Hello", b: boolean): number;
function example(a: string, b: number): boolean;
function example(a: string, b: boolean | number): number | boolean {
	if (a === "Hello") {
		callSomeFunctionThatExpectsBoolean(b);
		return 54;
	} else if (typeof a === "string") {
		callSomeFunctionThatExpectsNumber(b);
		return true;
	}
	else
		throw new TypeError("The first argument must be a string");
}

let x: string = someFunctionThatReturnsString();

// Both these calls would compile but possibly error at runtime:

// This would error at runtime if x !== "Hello":
let result1 = example(x, false); // `result1` gets type `number | boolean`

// This would error at runtime if x === "Hello":
let result2 = example(x, 1); // `result2` also gets type `number | boolean`

Because of this risk I primarily suggested to entirely disallow the pattern, instead of giving a false sense of safety to the programmer. I would also understand if this alternative would seem more attractive though (maybe due to practical considerations), but I wouldn't personally recommend it, especially when there is more than one parameter.

[rotemdan]

Another alternative is to implicitly convert the more general parameter type to a subtraction type that excludes the more specific parameter type(s), so the implicit signature would look like:

function example(a: "Hello", b: boolean): number;
function example(a: string - "Hello", b: number): boolean;
function example(a: string, b: boolean | number): number | boolean {
	if (a === "Hello") {
		callSomeFunctionThatExpectsBoolean(b);
		return 54;
	} else if (typeof a === "string") {
		callSomeFunctionThatExpectsNumber(b);
		return true;
	}
	else
		throw new TypeError("The first argument must be a string");
}

Now a caller must prove to the compiler that it has tested whether the argument match the literal type "Hello" before it is passed to the function. The resulting code may be a bit "strange" looking though:

function example2(a: string): number | boolean {
	if (a === "Hello")
		return example(a, false); // `a` is narrowed to "Hello"
        else // if (typeof a === "string")
		return example(a, 100); // `a` is narrowed to the 
                                        // subtraction type `string - "Hello"`
}

I don't believe this solution is practical, though, because some types may be very difficult or computationally expensive to guard on (think about arrays of complex objects etc.), and if the function is imported from an external module it doesn't seem reasonable to me to assume the programmer has a deep understanding of the type of object they are supposed to pass to it as an argument, or have access to a predefined guard function for it.

This solution is also possible to do "manually", by explicitly using subtraction types in the overloaded signature, even with the suggested compiler flag enabled, so maybe that could be one of the recommended methods to achieve the desired effect without risking type inference "disasters".

[RyanCavanaugh]

There doesn't seem to be much evidence that people are regularly making mistakes here in a way that we could reliably detect without introducing false positives. But the real problem is that any implementation here is going to be at least O(n^2) on the number of signatures, and there are many .d.ts files which have literally dozens of signatures per function. The cost and complexity of checking for "correctness" in overloads every time when the errors are so rare to begin with doesn't seem worth it.

[rotemdan]

@RyanCavanaugh

Hi, I think as a general advice I would suggest to the TypeScript team to never disregard issues that directly undermine the effectiveness of having a type checker at the first place. This is not even an issue about "soundness", it is a case where the compiler would "deliberately" infer an incorrect type that in practice only serves to confuse the user and there is nothing they can do about it (consider the typing may be received from a declaration file they don't feel qualified enough to change).

The proposal was to have this under an option. Whoever would enable that option should accept the performance implications of using it. It's a conscious choice.

If I were designing a language, I would personally never feel I need to "wait" until I have "evidence" until I need to address some case where my compiler was deliberately inferring a wrong type. I would immediately correct the problem. Compilers should be seen as "theorem provers", not "recommendation engines". I'm sorry but I don't find the way this was handled reasonable, considering the potential magnitude of the problem. I think you should leave these kinds of issues open, until, say, it might be considered for TypeScript 4.0 or something like that, along with other similar cases.

[RyanCavanaugh]

Thanks for the feedback on this point. We have an overall complexity "budget" and need to spend that budget wisely. Things which produce minimal gains do produce gains, but the question is whether or not the complexity invested to produce those gains pays off more than some other activity would. It's easy to think of things in terms of pure cost, but with a finite amount of complexity humans can handle, we have to think about them in terms of opportunity cost -- even under a flag, we have extra code to deal with, slightly less performance (it really does all add up), and yet one more question to ask ourselves ("How would X feature work under flags Y, Z, and Q?)" when considering new features.

Regarding leaving issues open, the "closed" bit is just a color on a webpage and should be treated as such. You can feel however you want about it, but we require the ability to make decisions and indicate the results of those decisions. The decision here is "We're not doing this anytime soon". We've communicated that decision by closing the issue. This doesn't mean you can't comment here, or point out areas where the feature would deliver more value than we thought it would, or engage in other ways. The decision has been made for now not to do this and it'd be a disservice to anyone coming across the issue to think that it was on the table for the near future.