-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Add elaboration when call fails all overloads but succeeds against the implementation signature #41001
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
Conversation
…e implementation signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions about the technique you used.
I also wish there was some way to estimate how many times people will see this new error. Any ideas there?
@@ -274,7 +274,7 @@ namespace ts { | |||
result.relatedInformation = relatedInformation ? | |||
relatedInformation.length ? | |||
relatedInformation.map(r => convertToDiagnosticRelatedInformation(r, newProgram, toPath)) : | |||
emptyArray : | |||
[] : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we ever figure out what the performance impact of []
vs emptyArray
is? Last I heard is that []
is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...eh? In this case, it's just because we mutate relatedInformation
in some places, so using a global singleton is bad.
const candidate = getSignatureFromDeclaration(implDecl as FunctionLikeDeclaration); | ||
const isSingleNonGenericCandidate = !candidate.typeParameters; | ||
if (!!chooseOverload([candidate], assignableRelation, isSingleNonGenericCandidate)) { | ||
addRelatedInfo(d, createDiagnosticForNode(implDecl, Diagnostics.The_call_would_have_succeeded_against_this_implementation_but_implementation_signatures_of_overloads_are_not_externally_visible)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mutate the related list? (and is that why you clone into related
above?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addRelatedInfo
, as the name implies, has always appended to the existing list.
Pretty often - almost any overload error where the overload being compiled against is actually in your source code (and thus has a body) rather than a library. |
Yeah, people on the VS Code team hit this, and it's a beginner mistake. If you don't hear about it much, it's because you bang your head on it for a bit and then figure it out. |
!!! error TS2769: Argument of type 'boolean' is not assignable to parameter of type 'number'. | ||
!!! related TS2793 tests/cases/compiler/functionOverloads2.ts:3:10: The call would have succeeded against this implementation, but implementation signatures of overloads are not externally visible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
Causes crashes in Definitely Typed -- see types/agent-base for example. I'll file a bug. |
Fixes #4797