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

Exhaustive checking of overloaded function return types #13225

Closed
rotemdan opened this issue Dec 30, 2016 · 24 comments
Closed

Exhaustive checking of overloaded function return types #13225

rotemdan opened this issue Dec 30, 2016 · 24 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@rotemdan
Copy link

rotemdan commented Dec 30, 2016

Currently:

function func(a: number): number;
function func(a: string): string;
function func(a: number | string): number | string {
	return 54;
}

const result = func("hi"); // result gets type `string`, however the 
                           // actual return type would always be `number` 
                           // (or more specifically, the literal type `54`).

And:

function func(a: number): number;
function func(a: string): string;
function func(a: number | string): number | string {
	if (typeof a === "number")
		return "oops"; // this doesn't error
	else if (typeof a === "string")
		return 54; // nor this
}

There are several stages for gradually addressing this:

  1. Check there exists at least one code path that returns each possible return type (either a string or a number in this example) or a type that's a assignable to the union of all possible return types (returning a value with type string | number might work as well, although it wouldn't be really 'safe') such that the union of all actual return types at all return paths exactly matches the union of all expected return types (I might give an example to clarify what this means later).
  2. Check that code paths where a is narrowed to number always return a number and when narrowed to string, always return a string (note that this check would only be reliable if the parameter a is not itself reassigned with a value having an incompatible type before the runtime assertion. Unfortunately there is no current way to modify function parameters with the equivalent of const or readonly so it has to be determined based on flow analysis).
  3. In the body of the function, infer the return type as a guarded polymorphic type that describes the relationship between parameter and return types (and possibly infer similar types to describe relationships between the parameters themselves) and even allow to defer the resolution of the return type back to the caller when the parameter given to func is itself a union (I'm not sure if that's currently possible, but it could be useful, see more details about how this can be achieved in the linked comment).
@DanielRosenwasser DanielRosenwasser added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Dec 30, 2016
@rotemdan
Copy link
Author

rotemdan commented Dec 31, 2016

I'm questioning whether values with types that represents some union of the expected set of possible return types should be allowed to be returned from the function in the first place:

function func(a: number): number;
function func(a: string): string;
function func(a: number | string): number | string {
  let result: number | string = Math.random() > 0.5 ? 54 : "Hi";
  return result;
}

What benefit is provided by allowing the return value to have type number | string?

Maybe the programmer "trusts" that it got a result from somewhere, and that result is expected to behave according to the contract described by the overloaded function signature?

function trustedFunction(a: number | string): number | string {
  // ...
}

function func(a: number): number;
function func(a: string): string;
function func(a: number | string): number | string {
  let result = trustedFunction(a);
  return result;
}

Is it really worth allowing this?
If trustedFunction really followed the contract, why isn't it annotated with an overloaded signature as well?

@RyanCavanaugh RyanCavanaugh added Working as Intended The behavior described is the intended behavior; this is not a bug and removed In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Jan 10, 2017
@RyanCavanaugh
Copy link
Member

This basically comes down to this example:

function func(a: number): number;
function func(a: string): string;
function func(a: number | string): number | string {
  let result: number | string = ...;
  return result;
}

Should this be allowed? Yes. Overloads are divorced from implementation and we only do the barest of checking that the signatures are plausibly correct (most of which seems to backfire anyway).

Overall we're not looking to increase complexity of inference / etc here when the signature algorithm today is quite simple and explainable, even if it leaves some soundness to be desired.

@rotemdan
Copy link
Author

rotemdan commented Jan 10, 2017

@RyanCavanaugh

I'm assuming this is a decision that was agreed by the whole team. In that case I feel I can't reason out how they came into this strange conclusion.

It's one thing to feel disinterested or dis-motivated about a subject such that it doesn't "feel" like the right thing to do (which I suspect is the case here). It's another thing to scheme up a completely absurd line of reasoning and convince yourself and an entire group of people of it. I'm sorry but I feel like I'm wasting my time here. This doesn't sound very serious to me.

@RyanCavanaugh
Copy link
Member

I'm assuming this is a decision that was agreed by the whole team.

Correct

It's one thing to feel disinterested or dis-motivated about a subject such that it doesn't "feel" like the right thing to do (which I suspect is the case here). [...] This doesn't sound very serious to me.

I'm sorry you feel this way. We've had a lot of different challenges around how we check overload signatures vs the implementation signature and the current approach appears to be at least a local maximum in terms of allowing correct patterns, disallowing incorrect patterns, and providing sufficient freedom to implement the body in a way that isn't overly burdensome. Intersecting these rules with even more complexity in the checking of the function body isn't an area where we'll see good returns from spending our constrained complexity and performance resources.

Concrete data we're looking at here:

  • Very few people report accidently implementing overloads incorrectly
  • Contrast the above point with increased cognitive load to understand how to write function bodies "correctly" under a regime where the signatures have extra influence on the checking of functions
  • Pass-through overloads are extremely common and would require writing "artificial" code to satisfy the checker

@Shlomibo
Copy link

@rotemdan @RyanCavanaugh

I've posted a suggestion for function overloading that I think would have solve this issue, and let a programmer write such code

function func(a: number): number;
function func(a: string): string;
function func(a: number | string): number | string {
  let result: number | string = ...;
  return result;
}

And I hope is simple.
#12041

@rotemdan
Copy link
Author

rotemdan commented Jan 12, 2017

@RyanCavanaugh @Shlomibo

Here's my initial example again:

function func(a: number): number;
function func(a: string): string;
function func(a: number | string): number | string {
	return 54;
}

const result = func("hi"); // result gets type `string`, however the 
                           // actual return type would always be `number` 
                           // (or more specifically, the literal type `54`).

I can't understand why this was rejected? I have mentioned clearly It is possible to check return types even while still allowing return values to have union types:

By ensuring the union of the all actual return value types exactly matches the union of all overload return types

(For simplicity I'm excluding some cases with any)

I wonder if anyone has actually read what I wrote?

The only interpretation I have is that the TS team simply doesn't care. They like the status quo and they don't want to change anything. It seems like the only way to have them consider anything is to have multiple independent people nagging about it for years until they give up, and then what do you know? it may turn out it was worth it!

@RyanCavanaugh
Copy link
Member

I wonder if anyone has actually read what I wrote?

Yes

The only interpretation I have is that the TS team simply doesn't care.

Honestly this hurts. We're real people here and we're making decisions to the best of our ability.

It seems like the only way to have them consider anything is to have multiple independent people nagging about it

Getting feedback from more than one person is actually valuable, yes. If only one person in the world can be bothered to bring something up, it really is not that important.

until they give up

😢

@gcnew
Copy link
Contributor

gcnew commented Jan 13, 2017

@rotemdan We are not entitled to order the team what to do. We can give suggestions and sometimes our suggestion gets noticed, especially, as you said, if many people support it. This doesn't mean I don't like yours. I don't like the attitude you are currently exhibiting. Having said that you can do more than a mere suggestion - you can take the onus on you and try to implement it yourself. I do belive that a working prototype can get you a long way down the road both at understanding the complexities involved and also giving you credit in the team's eyes. They can use the prototype and weigh whether it's really beneficial and whether they want to support it for forever.

As an example, I believe that the current state of generic functions is quite miserable and actually gets in the way of many everyday tasks. That's why I've decided to create a prototype and hopefully convince the team it's worth it (see #9949).

@rotemdan
Copy link
Author

rotemdan commented Jan 13, 2017

@gcnew

I was never trying to tell them what to do. It is simply that the apparent reasoning they sometime display is so twisted I've mostly given up on trying to understand it, so I don't care anymore. I'm not sure even they are aware of all the nonsense they have been constantly trying to sell the community over the years (until they changed their mind). Most people buy it, to some degree. I don't.

(a classic case is static inheritance which they aggressively tried to reject for years until they finally fixed it. The amusing part is that that the issue is still labeled as Canonical and Won't Fix)

I agree that simply implementing the feature myself, or a simplified version of it, might be a more productive way to deal with the situation. This is actually a path I'm seriously considering nowadays. However, I don't feel forking the compiler would be the right way to go, for me at least.

I'll try to explain. I simply don't have time to go and deeply study the type-checker code base. I have looked at it, but it seems like working on it would simply move the problem from one place to another. Even if I get past the learning curve, then say I implement some feature, and it takes me a total of 7 unpayed work days to do so, and they reject it or drag it for months it so that I have to constantly rebase and modify the implementation. I'm not sure that's something I'm willing to take part of.

Instead I plan to start small by writing tslint rules and use them myself for a while. Over time try to incrementally make them more sophisticated and maybe less "heuristic" and more sound until they take some stable shape. The important aspect about this is that I don't have to continuously merge my changes and rebuild the compiler.

Hopefully, at some point, I hope that tslint-vscode would provide full-project integration in VSCode (i.e. it could incrementally check the entire codebase at every change) so that it could act effectively as a secondary type-checker. At that point, it might become very worthy to spend more time on developing custom rules (even for personal purposes), and maybe at that point the TS team might expose more sophisticated APIs so that one can effectively write a prototype auxiliary type-checker with it without astronomical effort.

@rotemdan
Copy link
Author

rotemdan commented Jan 13, 2017

@gcnew

I'll try to summarize how many TS issues go:

  1. I open a reasonable, non-esoteric subject like "Exhaustive checking of overloaded function return type".
  2. They consider it (at least they make it look like so).
  3. Without much explanation they close the issue as "Working as Intended" (although it is definitely not "working" and I doubt it is "intended" to always stay like this) which effectively proclaims "TypeScript will never have exhaustive checking over overloaded function return types" and cover it with an almost politician level argument that ignores the majority of the ideas I suggested and concentrates on only one they didn't like.
  4. Months/years pass. I've long given up on them. They actually implement it or some of it. The closed issue stays buried as only one of tens of thousands of other issues.

What am I going to learn from this? most likely that this is a complete waste of time.

Maybe the best solution would be to instead develop an auxiliary type checker, that would use the TS APIs, and would be a cross between a linter and a real, rigorous type-checker, to post-process over a checked program, or as a plugin to the existing compiler that wouldn't require continuous forking and merging of the TS codebase. It may be the future evolution of tslint, or something completely different.

@Shlomibo
Copy link

@rotemdan
I think that I actually understand the decision of the design team.
In TS where signatures decoupled from implementation - validating a correct output for any valid input (for non-trivial programs) would require you to solve the halting problem.

But I don't think signatures must be decoupled from implementation - coupling the two would have solve your issue too.

I wonder if anyone has actually read what I wrote?

Twice. Now it was a third.

@rotemdan
Copy link
Author

rotemdan commented Jan 13, 2017

@Shlomibo

The idea is that the compiler would simply go through the types of the values returned at all code paths, create a union of them, and check that the resulting union is structurally equivalent to the union of the expected return types.

It doesn't matter if some code paths are never executed at run-time. It would simply mean that the check isn't completely sound, which is not different than most other constructs of the language.

Similarly to a regular check of a return value type. the check would never have false positives, i.e. it is not possible that the check would incorrectly miss cases where a value of a specific type is returned.

If some code path returns any then the check would be satisfied immediately.

The check would provide more safety than not having it at all, again, like a lot of the checks already done in the language.

@Shlomibo
Copy link

The idea is that the compiler would simply go through the types of the values returned at all code paths, create a union of them, and check that the resulting union is structurally equivalent to the union of the expected return types.

How this would solve this?

function func(a: number): number;
function func(a: string): string;
function func(a: number | string): number | string {
	if (typeof a === "number")
		return "oops"; // this doesn't error
	else if (typeof a === "string")
		return 54; // nor this
}

@rotemdan
Copy link
Author

rotemdan commented Jan 13, 2017

@Shlomibo

That would be accepted, since the union of return value types cover all expected return types. The check is just a coverage check, in some analogy to the idea "code coverage" from automated testing.

Checking for matching of argument and return types was the "next stage" I described. It is a separate component.

@Shlomibo
Copy link

Well - that coverage check would be useful only in trivial functions (and its usefulness can be argued for such functions).
And codes like the one Ryan posted would make even your simplest check unseful.

And I really thins that the problem is that signatures are decoupled from implementation. That's why you don't have that problem in other languages.

IMHO that is.

@Shlomibo
Copy link

Anyway - I really don't think that the decision not to implement this was from not caring.
If you'll think about all the ways a type can be transformed across a function - you (or I) will get a brain salad ;)

@rotemdan
Copy link
Author

rotemdan commented Jan 13, 2017

@Shlomibo

I suggested a type-checker feature that seems reasonable, increases type safety and is not very difficult to implement. I have no idea why a group of reasonable people would tag it as Working as Expected.

The case where a union is returned from an overloaded function is mostly an edge case, my side comment about it was a display of personal reservation and questioning about it, based on common sense. They took that side comment and made it look like it was the whole suggestion. That seemed dishonest and got me somewhat upset.

The only reasonable conclusion I could make is that either they didn't carefully read what I wrote, or they simply don't care about it.

@Shlomibo
Copy link

Shlomibo commented Jan 13, 2017

Maybe they concluded that there are more edge cases, and that the edge cases are more common than seemed.
When I think about it - for most non-trivial overloaded functions, a code like the one Ryan posted would be very common. Making the suggested feature unusable when it mostly needed.

Also, as a note - what you posted as multiple features at stages, was understood by me as a single feature, implemented and running in stages.
Now, without saying that this was unclear (It is just a misunderstanding made by me) - people may know and understand different things from you do.

But I surely understand the disappointment.

@rotemdan
Copy link
Author

rotemdan commented Jan 13, 2017

@Shlomibo

Feature suggestions are meant to initiate discussions, not define final specifications. Since I am not the designer of the language and have not written the specification or the compiler, it is clear that I am not in a position to work out all the small details and edge cases.

Since they did not provide any meaningful feedback, and don't seem to display interest at participating at fruitful two-way discussions, it is not possible for me to improve or amend any proposal that is made.

Their closed-door way of handling things and controlling information creates an unfair advantage for them. It is very difficult to an outside individual to provide a reasonable proposal without any interactive feedback.

This is not only based on the way they responded here, but also on tens of previous suggestions I made (some of them were not under my real name) over a period of several years. Some of those tanked proposals have inspired new features that were later introduced into the language.

@Shlomibo
Copy link

Shlomibo commented Jan 13, 2017

We'll have to agree to disagree.

They did provide feedback, and I don't think they have to provide detailed reports of their meetings, nor make the "meetings" span into boards where people are answering at different times and days.

I understand the disappointment, especially after it seemed like the feature is going to make it into the language.
But I can't really think that someone whom actually implemented the feature to see if it would work, throw it away because he's not caring.

@rotemdan
Copy link
Author

rotemdan commented Jan 13, 2017

@Shlomibo

What you are implying is that this issue tracker is not meant for proposals that are less than trivial, because non-trivial proposals almost always need some sort of feedback loop to evolve to a reasonable form, and the TS team is not willing to provide it.

I agree that is most likely the condition, due to this I am not going to spend any more time on this particular issue, and not likely to spend time on similar ones.

On a side note, in Typescript many features don't even start as proposals (at least not publicly available ones). They are immediately implemented and posted as pull requests.

For comparison the Python PEP (Python Enhancement Proposal) is a multi-way, interactive process, with a well-defined protocol on which everyone is bound, including the original creator of the language (I don't really know if the protocol is operating exactly as described in practice, I'm just giving it as an example of a different way that things could be formulated, not necessarily because I think it is somehow the best).

@Shlomibo
Copy link

Shlomibo commented Jan 13, 2017

This is not what I'm implying.
I think the situation is that this issue seemed simple and trivial, and discovered to be otherwise, but too simple to cover the whole problem.

You could post a fixed version of it, which would handle the problems pointed by the team (AKA: feedback).
You still can.

@rotemdan
Copy link
Author

@Shlomibo

If you mean this:

function func(a: number): number;
function func(a: string): string;
function func(a: number | string): number | string {
  let result: number | string = ...;
  return result;
}

That was an example @RyanCavanaugh copied and pasted from my side remark about my personal reservations on the idea of returning a union, which is not related to the original proposal. The original proposal handles that case, including in the second stage (simply check if the expected return type is included in the returned union). The third stage doesn't really, but I mostly gave it as a conceptual idea of how the previous two could be extended further.

@Shlomibo
Copy link

Simply checking the returning union would serve nothing on this case.
The union type is declared upon the returned variable, which is checked against the returned type of the function, which once again is checked to be compatible with the overloaded signatures.

And I once again wonder if your suggestion is single feature, with multiple stages (some of them undecidable at all), or multiple features, making the first stage a feature of its own?
Which is it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants