Fixed generic inference when the contextual type is a signature with properties #52495
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Andarist
commented
Jan 30, 2023
src/compiler/checker.ts
Outdated
| @@ -36975,7 +36975,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (signature && signature.typeParameters) { | |||
| const contextualType = getApparentTypeOfContextualType(node as Expression, ContextFlags.NoConstraints); | |||
| if (contextualType) { | |||
| const contextualSignature = getSingleSignature(getNonNullableType(contextualType), callSignature ? SignatureKind.Call : SignatureKind.Construct, /*allowMembers*/ false); | |||
| const contextualSignature = getSingleSignature(getNonNullableType(contextualType), callSignature ? SignatureKind.Call : SignatureKind.Construct, /*allowMembers*/ true); | |||
There was a problem hiding this comment.
I'm a little bit hesitant to say that I'm 100% confident in this fix. I'm pretty sure that it's on the right track but there is a chance that it needs some further polish to handle cases in which the generics are used both in the signature and in those extra properties on the type.
This change satisfies the whole existing test suite but mixing signatures with extra properties isn't the most popular technique. So I'm a little bit worried that the existing test suite might not capture some requirements. Gonna investigate this sometime later.
There was a problem hiding this comment.
History found, explanation for current behavior here. If you want to toggle this boolean (and I can see why), you need to validate the other members don't reference the type parameters we want to promote to the signature. That's pretty hard, so a reasonable proxy is checking that the other members of the type don't reference any type variables at all.
There was a problem hiding this comment.
I implemented the suggested change - could you take another look at this?
weswigham
requested changes
Mar 15, 2023
src/compiler/checker.ts
Outdated
| @@ -36975,7 +36975,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (signature && signature.typeParameters) { | |||
| const contextualType = getApparentTypeOfContextualType(node as Expression, ContextFlags.NoConstraints); | |||
| if (contextualType) { | |||
| const contextualSignature = getSingleSignature(getNonNullableType(contextualType), callSignature ? SignatureKind.Call : SignatureKind.Construct, /*allowMembers*/ false); | |||
| const contextualSignature = getSingleSignature(getNonNullableType(contextualType), callSignature ? SignatureKind.Call : SignatureKind.Construct, /*allowMembers*/ true); | |||
There was a problem hiding this comment.
History found, explanation for current behavior here. If you want to toggle this boolean (and I can see why), you need to validate the other members don't reference the type parameters we want to promote to the signature. That's pretty hard, so a reasonable proxy is checking that the other members of the type don't reference any type variables at all.
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 57eb903. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 57eb903. You can monitor the build here. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 57eb903. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at 57eb903. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@weswigham Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
@Andarist looks like there's a fair bit going on in the top100 suite to look into for this one. Maybe related to eagerly producing more of the type structure to check for type variable presence? |
|
Hey @weswigham, it looks like the DT test run failed. Please check the log for more details. |
|
@weswigham Here they are:
CompilerComparison Report - main..52495
System
Hosts
Scenarios
TSServerComparison Report - main..52495
System
Hosts
Scenarios
StartupComparison Report - main..52495
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
So far I managed to analyze 2 of the reported cases. When it comes to When it comes to There is a notable difference between the inferred props // Omit2<any, BsPrefixProps & DropdownItemProps> & BsPrefixProps & DropdownItemPropsand with my change they are: props // Omit2<React.ComponentPropsWithRef<As>, BsPrefixProps & DropdownItemProps> & BsPrefixProps & DropdownItemPropsWhich honestly... looks like an improvement? The signature contained in |
|
Ooof, definitely going to have to take a peek at the perf cost too (is |
|
@typescript-bot perf test this faster |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 57eb903. You can monitor the build here. Update: The results are in! |
|
Just rechecking; could be that the diff is due to some bad baseline behavior I'm working on fixing as part of the perf overhaul. |
|
@jakebailey Here they are:Comparison Report - main..52495
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Nope, it's still a perf hit. Bummer. Though, xstate is a funky test as it is; somehow only 3 seconds of checking. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixed #52262