Fix Callback argument type not inferred for union of interfaces #59309 #59672
Conversation
| // This signature will contribute to contextual union signature | ||
| signatureList = [signature]; | ||
| } | ||
| else if (!compareSignaturesIdentical(signatureList[0], signature, /*partialMatch*/ true, /*ignoreThisTypes*/ true, /*ignoreReturnTypes*/ true, compareTypesSubtypeOf)) { |
There was a problem hiding this comment.
If you are loosening here how parameter types are meant to be compared I'd probably expect here:
| else if (!compareSignaturesIdentical(signatureList[0], signature, /*partialMatch*/ true, /*ignoreThisTypes*/ true, /*ignoreReturnTypes*/ true, compareTypesSubtypeOf)) { | |
| else if (!compareSignaturesIdentical(signatureList[0], signature, /*partialMatch*/ true, /*ignoreThisTypes*/ true, /*ignoreReturnTypes*/ true, compareTypesAssignable)) { |
Unless there is already a reason why the subtype relationship was used here.
There was a problem hiding this comment.
I'm not really sure when each relation should be used. I used compareTypesSubtypeOf because that's what is used in a call to compareSignaturesIdentical with partialMatch === true in the function findMatchingSignatures.
There was a problem hiding this comment.
findMatchingSignatures gets only called by getUnionSignatures so I agree that this probably makes sense
| const types = (type as UnionType).types | ||
| .map(type => ({ type, signature: getContextualCallSignature(type, node) })) | ||
| .filter(type => type.signature) | ||
| .sort((t1, t2) => t1.signature!.parameters.length - t2.signature!.parameters.length); |
There was a problem hiding this comment.
You need to account for rest parameters here so it would be more appropriate to use getParameterCount here over directly checking the .parameters.length. Please also consider cases like
((a: string, b?: string) => void) | ((a: string, ...rest: string[]) => void)Both of those signatures have the same parameter count but perhaps you'd like to specifically sort one of them as the first one.
There was a problem hiding this comment.
Thank you. I think that actually what I need is getMinArgumentCount.
For the case ((a: string, b?: string) => void) | ((a: string, ...rest: string[]) => void) I don't think it matters which one is first.
There was a problem hiding this comment.
I'm not sure if it will make any difference but to exercise even more funky test cases you could add test cases with signatures of this shape:
((a: string, ...rest: [string, number?] | [number, boolean?]) => void) | ((a: string, ...rest: [string, number] | [number, boolean]) => void)Note that I didn't think through what types you should use in those rest parameters, just that they can be unions of tuples and that one of the signatures might have some of the elements there required and some might have them optional etc.
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The reported error in That said, maybe you could also filter out here signatures that can't be applicable in the first place. Like, if the function is |
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
That's smart, I added that. |
Please verify that:
Backlogmilestone (required)mainbranchhereby runtestslocallyFixes #59309