-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Elaborate type of invoked expression in error message #5784
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
Elaborate type of invoked expression in error message #5784
Conversation
Hi @erictsangx, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
@erictsangx, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
unionOfDifferentParameterTypes();// error - no call signatures | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
!!! error TS2349: Cannot invoke an expression whose type lacks a call signature. | ||
!!! error TS2658: Cannot invoke an expression whose type is a union with incompatible call signatures. | ||
|
||
var unionOfDifferentNumberOfSignatures: { (a: number): number; } | { (a: number): Date; (a: string): boolean; }; | ||
unionOfDifferentNumberOfSignatures(); // error - no call signatures |
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.
Shouldn't this call get the new error too?
This is a nice improvement, although I'd rather see an elaboration of the original error, something like
|
I don't think this is a sufficient check; for instance, you shouldn't report a different error in the following case: Basically, this is for people who are dumbfounded by the fact that each of their constituent types is callable but the result of the union is not callable. This new error message is even less clear because it throws more jargon into the mix for no reason. I think if you also checked whether at least one of the constituent types had a call signature, that would be a sufficient condition to issue the new message. |
Also, can you add the test I showed above if it turns out that my suggested change doesn't affect any tests? Thanks! |
a63f800
to
19f9379
Compare
Thank you for the clarification, guys. It seems I was doing it wrong. |
let unionCallSignatures: Signature[] = []; | ||
let hasError = false; | ||
forEach(unionApparentType.types, type => { | ||
const eachCallSignature = getSignaturesOfType(type, SignatureKind.Call); |
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.
Extract the logic here out to a function called typeHasCallSignatures
and place it next to typeHasConstructSignatures
. Don't base the new function on typeHasConstructSignatures
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.
Then use (apparentType.flags & TypeFlags.Union && forEach((<UnionType>apparentType).types, typeHasCallSignatures)
19f9379
to
8b3f46c
Compare
Updated. I will create another pr for |
8b3f46c
to
4737d12
Compare
@@ -23,7 +23,7 @@ tests/cases/compiler/constructorOverloads4.ts(11,1): error TS2348: Value of type | |||
|
|||
(new M.Function("return 5"))(); | |||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | |||
!!! error TS2349: Cannot invoke an expression whose type lacks a call signature. | |||
!!! error TS2349: Cannot invoke an expression whose type lacks a call signature. Type 'Function' has no compatible call signatures. |
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.
This is a regression
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.
Specifically, we're not looking for call signatures here. We're looking for a construct signature, so we're not appropriately reporting an error.
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.
Can you explain the semantics here? I don't think that class Function
and function Function
will merge (hence the duplicate identifier errors), so the error that changes should be based on only one of the definitions. The first definition has no call signatures, so if the error is based on the first definition, then it looks correct to me.
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.
I must have misread the error span.
Actually, never mind my last comment. We can work on this change in this PR. |
@@ -9438,7 +9443,12 @@ namespace ts { | |||
error(node, Diagnostics.Value_of_type_0_is_not_callable_Did_you_mean_to_include_new, typeToString(funcType)); | |||
} | |||
else { | |||
error(node, Diagnostics.Cannot_invoke_an_expression_whose_type_lacks_a_call_signature); | |||
if (apparentType.flags & TypeFlags.Union && forEach((<UnionType>apparentType).types, typeHasCallSignatures)) { | |||
// TODO check Not all types in '{0}' have compatible call signatures. |
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.
Just get rid of this portion entirely and use it in the separate PR. I'd rather not temporarily disable error messages until another PR comes in.
… union of types with call signatures add diagnostic message 2658: Cannot invoke an expression whose type lacks a call signature. Type '{0}' has no compatible call signatures. Closes microsoft#5640
4737d12
to
557433e
Compare
@DanielRosenwasser updated |
👍 |
manually merged. thanks |
No description provided.