Fix error message when type argument arity is wrong #28366
Conversation
ghost
requested a review
from 
|
Looks good; can you resolve the merge conflict, and also make it |
|
@DanielRosenwasser Merge conflict resolved. |
ghost
assigned
There was a problem hiding this comment.
I still think fixing this bug does more harm than good. The current implementation is not confusing when it's wrong, and is not commonly wrong. Opinions @DanielRosenwasser ?
If we decide to take this, the change is technically solid but the wording is prolix. I'd like to keep the current terse style.
src/compiler/diagnosticMessages.json
Outdated
| @@ -2012,7 +2012,7 @@ | |||
| "category": "Error", | |||
| "code": 2557 | |||
| }, | |||
| "Expected {0} type arguments, but got {1}.": { | |||
| "Expected {0} type argument(s), but got {1}.": { | |||
There was a problem hiding this comment.
This just adds visual noise. Please revert it.
| } | ||
| const paramCount = min === max ? min : min + "-" + max; | ||
| return createDiagnosticForNodeArray(getSourceFileOfNode(node), typeArguments, Diagnostics.Expected_0_type_arguments_but_got_1, paramCount, typeArguments.length); | ||
| return createDiagnosticForNodeArray(getSourceFileOfNode(node), typeArguments, Diagnostics.No_overload_expects_0_type_argument_s_but_an_overload_does_exist_that_expects_1_type_argument_s, argCount, belowArgCount === -Infinity ? aboveArgCount : belowArgCount); |
There was a problem hiding this comment.
Can you revert this to the previous message? The only reason I see for the new message is that before, the expected was either min or min–max. Now expected is either above or below, but above is the common case anyway, and means basically the same as before.
There was a problem hiding this comment.
This message is consistent with the other new message when there are overloads, but I suppose the old message is fine as well.
|
Follow up: I discovered that this is a follow-up to #24031, which does the same thing for parameters instead of type parameters. I don't think that was a good change either, but a lot of people upvoted the bug. I think the reason fewer people upvoted this one is that it's much more rare with type parameters. Anyway, consistency is one good argument for merging this since we already have this error for parameters. That means my last comment is moot because that error message is the same as the parameter one. |
|
Requested changes made. |
@sandersn As a developer I would argue that this:
is plenty confusing. Imagine if instead of “0-2” it was “0-4” but the only legal arities were exactly 0 and exactly 4. You’d have to see the complete list of overloads to know what went wrong. On a related note, I had no idea generics could be overloaded until I saw this issue. Learn something new every day... |
|
@fatcerberus yup, the error messages have been corrected and now give a better idea of what went wrong. |
Fixes #28053
Diagnostics.jsonwith all the different permutations of the error messages. I believe that should ideally be handled in the "diagnostic emitter".