Defer processing generic functions returning intersected functions #58875
Conversation
src/compiler/checker.ts
Outdated
| return false; | ||
| } | ||
| const returnType = getReturnTypeOfSignature(signature); | ||
| return returnType.flags & TypeFlags.Intersection ? some((returnType as IntersectionType).types, isFunctionType) : isFunctionType(returnType); |
There was a problem hiding this comment.
(or whatever that helper is called that walks intersections and maybe unions)
There was a problem hiding this comment.
someType handles "single" types and unions, not intersections. There is everyContainedType that works for unions and intersections but I'd need someContainedType here. It can be done here, I just didn't bother.
There was a problem hiding this comment.
Maybe not in this PR, I guess, but isUnitLikeType, isTypeDerivedFrom, isGenericTypeWithUnionConstraint, isGenericTypeWithoutNullableConstraint all do this same thing. (isEmptyObjectType copies/pastes the every variant` too.)
There was a problem hiding this comment.
Just pushed out a change introducing someContainedType
| return false; | ||
| } | ||
| const returnType = getReturnTypeOfSignature(signature); | ||
| return returnType.flags & TypeFlags.Intersection ? someContainedType(returnType, isFunctionType) : isFunctionType(returnType); |
There was a problem hiding this comment.
Isn't the point of the someContainedType helper that this can just be
| return returnType.flags & TypeFlags.Intersection ? someContainedType(returnType, isFunctionType) : isFunctionType(returnType); | |
| return someContainedType(returnType, isFunctionType); | |
?
There was a problem hiding this comment.
I intentionally dont want to run this for unions here. Not because I know we shouldnt but because I dont know if we should. Changing the behavior for unions would increase the likelihood of this breaking something for some project
|
Guess that means we should |
|
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
|
@weswigham Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
…urning-intersected-functions
|
The above is just a single break (a single type-level test case from The break is related to this comment. There is no way to know if deferring will lead to better or worse results. It turns out that in this specific case, it leads to worse. This test case lucked out heavily though. Similarly, it just happens to use an intersection to add extra properties. And that (without this PR) prevents this deferred processing. It similarly wouldn't work if it would be declared as a single type. I think this break is acceptable. It's not great that it breaks something in the wild but it doesn't make sense to have functions with extra properties to behave differently based on how those extra properties are declared. There is a simple workaround for this case, one can move the nested |
fixes #58833