Fix contextual typing on yield and return expressions in generator function #49736
Conversation
This file contains hidden or 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
typescript-bot
added
Author: Team
For Milestone Bug
Member
Author
|
@typescript-bot perf test this |
Collaborator
|
Heya @gabritto, I've started to run the perf test suite on this PR at e955108. You can monitor the build here. Update: The results are in! |
Collaborator
|
@gabritto Here they are:
CompilerComparison Report - main..49736
System
Hosts
Scenarios
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
andrewbranch
approved these changes
Jun 29, 2022
Member
andrewbranch
left a comment
andrewbranch
left a comment
There was a problem hiding this comment.
Makes sense to me. Definitely get @rbuckton to double check though.
src/compiler/checker.ts
Outdated
| if (!iterationTypes) { | ||
| if (contextualReturnType.flags & TypeFlags.Union) { | ||
| contextualReturnType = filterType(contextualReturnType, type => { | ||
| const generator = createGeneratorReturnType(anyType, anyType, anyType, (functionFlags & FunctionFlags.Async) !== 0); |
Member
There was a problem hiding this comment.
Maybe this should be hoisted out to a createTypeChecker singleton anyGenerator? (Maybe lazily initialized though)
Member
Author
|
@typescript-bot perf test this |
Collaborator
|
Heya @gabritto, I've started to run the perf test suite on this PR at e955108. You can monitor the build here. Update: The results are in! |
rbuckton
suggested changes
Jun 30, 2022
Contributor
rbuckton
left a comment
rbuckton
left a comment
There was a problem hiding this comment.
I think the approach is sound, but you're doing extra work by comparing the Generator type to each constituent.
src/compiler/checker.ts
Outdated
| if (contextualReturnType.flags & TypeFlags.Union) { | ||
| contextualReturnType = filterType(contextualReturnType, type => { | ||
| const generator = createGeneratorReturnType(anyType, anyType, anyType, (functionFlags & FunctionFlags.Async) !== 0); | ||
| return checkTypeAssignableTo(generator, type, /*errorNode*/ undefined); |
Contributor
There was a problem hiding this comment.
getIterationTypeOfGeneratorFunctionReturnType will do this work for you (returning undefined if its not generator-like), would this work instead?
const isAsyncGenerator = (functionFlags & functionFlags.Async) !== 0;
const iterationReturnType = mapType(contextualReturnType,
type => getIterationTypeOfGeneratorFunctionReturnType(IterationTypeKind.Return, type, isAsyncGenerator));
if (iterationReturnType) {
return undefined;
}mapType will remove undefined results, and will return undefined if all results were undefined, so it can also act as a filter.
Member
Author
There was a problem hiding this comment.
The problem is that getIterationTypeOfGeneratorFunctionReturnType returns, e.g., any for type number, which is why I went for removing the useless union components in the first place. Should I somehow change the behavior of getIterationTypeOfGeneratorFunctionReturnType instead? That seemed more tricky.
Contributor
There was a problem hiding this comment.
It returns any if the type passed is a number type? It's only supposed to return any if the type passed in is any. For anything else that isn't iterable it's supposed to return undefined.
Most of the relevant functions used by getIterationTypeOfX have something like the following at the top of the function:
if (isTypeAny(type)) {
return anyIterationTypes;
}
Member
Author
There was a problem hiding this comment.
Yes, it returns any. What happens is: we call getIterationTypeOfGeneratorFunctionReturnType with type number. This calls getIterationTypesOfGeneratorFunctionReturnType, which then returns getIterationTypesOfIterable(number, use, /*errorNode*/ undefined) || getIterationTypesOfIterator(number, resolver, /*errorNode*/ undefined).
getIterationTypesOfIterable(number, use, /*errorNode*/ undefined) returns undefined, so we return getIterationTypesOfIterator(number, resolver, /*errorNode*/ undefined).
getIterationTypesOfIterator(number, resolver, /*errorNode*/ undefined) eventually just tries to get the types of properties next, return, throw, which are all any for type number. So it returns something like IteratorType { next = any, throw = any, return = any }, from which we conclude that the generator return type of type number is any.
So yeah, in short, getIterationTypeOfGeneratorFunctionReturnType returns any for a non-iterable/iterator type like number, it doesn't return undefined.
I don't understand all the subtleties involved, so that's why I went with filtering the union by checking generator type assignability, but maybe I could be filtering using some of the other getIterationTypesX functions?
I think filtering by calling getIterationTypesOfIterable and seeing if it's undefined could maybe work at first, but the fact that when it returns undefined we also call getIterationTypesOfIterator seemed to suggest to me that getIterationTypesOfIterable didn't cover all of the cases of iterator/iterable types.
Let me know what you think.
Contributor
There was a problem hiding this comment.
It seems getIterationTypesOfIteratorSlow may be doing the wrong thing here. It should be returning noIterationTypes for number:
function getIterationTypesOfIteratorSlow(type: Type, resolver: IterationTypesResolver, errorNode: Node | undefined) {
const iterationTypes = combineIterationTypes([
getIterationTypesOfMethod(type, resolver, "next", errorNode),
getIterationTypesOfMethod(type, resolver, "return", errorNode),
getIterationTypesOfMethod(type, resolver, "throw", errorNode),
]);
return setCachedIterationTypes(type, resolver.iteratorCacheKey, iterationTypes);
}getIterationTypesOfMethod(number, resolver, "next", errorNode)- This is probably the culprit, as it should returnundefinedfor typenumber, but instead returnsanyIterationTypesdue to this logic:// Both async and non-async iterators *must* have a `next` method. const methodSignatures = methodType ? getSignaturesOfType(methodType, SignatureKind.Call) : emptyArray; if (methodSignatures.length === 0) { if (errorNode) { const diagnostic = methodName === "next" ? resolver.mustHaveANextMethodDiagnostic : resolver.mustBeAMethodDiagnostic; error(errorNode, diagnostic, methodName); } return methodName === "next" ? anyIterationTypes : undefined; // ^- should probably just be `return undefined;` }
getIterationTypesOfMethod(number, resolver, "return", errorNode)- This looks like it returnsundefinedfor typenumber, which is correct.getIterationTypesOfMethod(number, resolver, "throw", errorNode)- This looks like it returnsundefinedfor typenumber, which is correct.
The results of those three methods are passed to combineIterationTypes, which would then return noIterationTypes once the iteration types for the "next" method are corrected.
Contributor
There was a problem hiding this comment.
I need to look into this a bit further, as changing this results in some other additional diagnostics that we may not want.
Contributor
There was a problem hiding this comment.
The return value for getIterationTypesOfIteratorSlow should be fixed by #50146.
Member
Author
There was a problem hiding this comment.
I ended up not using mapType in the way you suggested, because in the previous version of the code, when we call getIterationTypeOfGeneratorFunctionReturnType on a union, although it maps the workers to each component type in the union, the way it combines the resulting iterationTypes is specific, so I don't think we'd get the same result if we just called mapType, which just unions the resulting mapped types.
Contributor
There was a problem hiding this comment.
I think it should have produced the same type for IterationTypeKind.Return, since the logic in getIterationTypesOfIterable related to union types just appends the resulting types and unions them for yield and return, with a bit more in the way of caching and error reporting. The only case that would differ would be for IterationTypeKind.Next, since the next types are intersected rather than unioned.
src/compiler/checker.ts
Outdated
| if (!node.asteriskToken && contextualReturnType.flags & TypeFlags.Union) { | ||
| contextualReturnType = filterType(contextualReturnType, type => { | ||
| const generator = createGeneratorReturnType(anyType, anyType, anyType, isAsyncGenerator); | ||
| return checkTypeAssignableTo(generator, type, /*errorNode*/ undefined); |
Contributor
There was a problem hiding this comment.
The same idea applies here as above.
Collaborator
|
@gabritto Here they are:
CompilerComparison Report - main..49736
System
Hosts
Scenarios
TSServerComparison Report - main..49736
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
Author
|
@typescript-bot perf test this |
Collaborator
|
Heya @gabritto, I've started to run the perf test suite on this PR at e955108. You can monitor the build here. Update: The results are in! |
Collaborator
|
@gabritto Here they are:
CompilerComparison Report - main..49736
System
Hosts
Scenarios
TSServerComparison Report - main..49736
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rbuckton
approved these changes
Aug 5, 2022
Member
Author
|
@typescript-bot perf test this |
Collaborator
|
Heya @gabritto, I've started to run the perf test suite on this PR at f6132ca. You can monitor the build here. Update: The results are in! |
Collaborator
|
@gabritto Here they are:
CompilerComparison Report - main..49736
System
Hosts
Scenarios
TSServerComparison Report - main..49736
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fixes #45596.
Also complemented the fix for #35995.