Skip to content

Improve error when function overload set incorrectly authored #4797

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
danquirk opened this issue Sep 15, 2015 · 13 comments · Fixed by #41001
Closed

Improve error when function overload set incorrectly authored #4797

danquirk opened this issue Sep 15, 2015 · 13 comments · Fixed by #41001
Assignees
Labels
Bug A bug in TypeScript Domain: Error Messages The issue relates to error messaging Effort: Difficult Good luck. Fix Available A PR has been opened for this issue Help Wanted You can do this

Comments

@danquirk
Copy link
Member

Users do not realize that implementation signatures of overload sets do not count as an actual signature at call sites. Attempting to call their overloaded function then results in a surprising error and off they go to StackOverflow to ask a question or here to file a bug. This comes up fairly often (most recently with #4786).

Can we improve the error by checking the type at the callsite against the implementation signature if no other signatures are applicable? Ex:

class EventAggregator
{
    publish(event: string, data?: any): void;
    publish<T>(event: T): void {}
}

var ea: EventAggregator;
ea.publish([1,2,3]); 

Today the last line is an error: "number[] is not assignable to string." To many this just looks like the compiler for some reason chose the wrong overload, now what?

In this case after seeing array is not assignable to any overload signatures we could check whether it is assignable to the implementation signature. If it is, we could issue a potentially more helpful error mentioning this rule (implementation signatures are not considered overload signatures) and a potential fix.

@danquirk danquirk added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Sep 15, 2015
@omidkrad
Copy link

Not suggesting that I want it now, but I especially like the kind of errors that include a link to an online page with more details on the error code, examples of when this error may happen and possible solutions. Makes everybody's lives easier. Also not suggesting that GitHub wikis are perfect for that! 😉

@mhegazy
Copy link
Contributor

mhegazy commented Sep 15, 2015

@danquirk what is the proposed new error message?

@DanielRosenwasser
Copy link
Member

Maybe:

An applicable signature exists on the implementation. Consider declaring that signature as an overload.

@danquirk
Copy link
Member Author

That's a pretty good start. Really depends on how verbose/casual we want to be. Even the concept of implementation signature vs overload signature is not necessarily obvious to people coming from certain other languages. A squiggle/line number reference to the implementation signature could help. This would also be a good case for a Lightbulb/QuickFix type thing that C# currently does.

@omidkrad
Copy link

For me, @mhegazy's two point hint was very useful:

method definition is not counted as part of the overload set. you need to 1. define an overload signature of each overload, and 2. have an implementation signature that is compatible with all the overloads.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 15, 2015

The error we get today is:

Overload signature is not compatible with function implementation.

so we want to say something about the implementation signature is not part of the overloads, in additional to it being compatible.

so may be something like:

Overload signature is not compatible with function implementation. Implementation signatures are not part of the overloads, but has to be compatible with all other overloads.

@danquirk
Copy link
Member Author

I think the distinction of implementation signature vs overload signature still isn't necessarily obvious to people. Perhaps if we mention the type (ex 'with implementation signature of type T -> void') that would help clue people in to precisely what we mean.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 15, 2015

not sure i understand what you mean, which type?

@DanielRosenwasser
Copy link
Member

@mhegazy the type of the call signature implied by the implementation.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 15, 2015

so something like

{ <T>(event: T): void; } not assignable to {(event: string, data?: any): void; }

@danquirk
Copy link
Member Author

Right, because the distinction between implementation signature and overload signature is not a concept that's familiar coming from many other languages (which is why people keep messing this up). So it would be useful to include some other clue as to what we mean when we say implementation signature whether it's the type of the implementation signature or the line number.

So combining some of the conversation, imagine:

number[] is not assignable to string. An applicable call signature does exist on the implementation signature with type { (event: T): void; }. However, implementation signatures are not part of the available function overloads. Consider declaring an additional function overload signature with that type.

Note this is far more verbose than we usually are and somewhat more colloquial but I think that style is for the best (relevant: http://elm-lang.org/blog/compiler-errors-for-humans)

@mhegazy mhegazy added Bug A bug in TypeScript Help Wanted You can do this and removed In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Sep 15, 2015
@omidkrad
Copy link

I think the implementation signature should not be called function overload at all if it does not end up in the definition file. I think Mohamed's wording was more clear here "method definition is not counted as part of the overload set" so I guess the implementation signature or function definition isn't an overload itself.

@mhegazy mhegazy added this to the Community milestone Sep 18, 2015
@DanielRosenwasser DanielRosenwasser added the Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". label Sep 29, 2015
@sandersn sandersn modified the milestones: TypeScript 3.4.0, Backlog Mar 12, 2019
@sandersn sandersn removed their assignment Jan 7, 2020
@DanielRosenwasser DanielRosenwasser added the Domain: Error Messages The issue relates to error messaging label Apr 27, 2020
@DanielRosenwasser DanielRosenwasser added the PursuitFellowship Help wanted from Pursuit fellowship; others please avoid until Dec 19 label Oct 1, 2020
@DanielRosenwasser DanielRosenwasser added Effort: Difficult Good luck. and removed Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". labels Oct 1, 2020
@DanielRosenwasser
Copy link
Member

Changed the effort tag because you really do need to understand where to plug things in in overload resolution.

@DanielRosenwasser DanielRosenwasser removed the PursuitFellowship Help wanted from Pursuit fellowship; others please avoid until Dec 19 label Oct 1, 2020
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Error Messages The issue relates to error messaging Effort: Difficult Good luck. Fix Available A PR has been opened for this issue Help Wanted You can do this
Projects
None yet
7 participants