fix(38388) Add support for `this` parameters in iterationTypes and fix caching issues #41451

Validark · 2020-11-09T06:40:56Z

This PR:

Adds support for this parameters in iterationTypes!

const obj = {
	x: 1,
	y: 2,
	z: 3,

	*[Symbol.iterator]<P>(this: P) {
		for (const k in this) yield k;
	},
};

for (const value of obj) {
    console.log(value); // value is `"x" | "y" | "z"`
}

Fixes the iterator caching logic. Looking at the code, I noticed a number of problems that could arise from the caching logic. Here is a reproduction of two of the issues I found. Basically, getIterationTypesOfIterable had 3 problems:
- Cached iterationTypes don't error like they should (see the first bug in the playground).
  - To fix this, we don't cache iterationTypes that produced errors, which makes getIterationTypesOfIterableSlow get called every time and produce the proper errors.
  - We can also take advantage of this design by detecting in getIterationTypesOfIterable whether something was cached to determine whether or not to cache or reportTypeNotIterableError (which we don't want to do if there was already an error from resolveCall, for example).
  - Keep in mind that caches should only be read from or written to when implicitThisType === type.
- When use permits either AsyncIterables or (Sync)Iterables converted via getAsyncFromSyncIterationTypes it would call these functions in this order: getIterationTypesOfIterableCached(type, asyncResolver) || getIterationTypesOfIterableFast(type, asyncResolver) || getIterationTypesOfIterableCached(type, syncResolver) || getIterationTypesOfIterableFast(type, syncResolver) || getIterationTypesOfIterableSlow(type, asyncResolver) || getIterationTypesOfIterableSlow(type, syncResolver). This is incorrect because an object could have an Symbol.asyncIterator that is neither cached nor an instance of a simple global type, and that logic could then fall back on the syncResolver section. Now it runs cached || fast || slow in order for both Symbol.asyncIterator and Symbol.iterator when applicable. This led to the 2nd and 3rd bugs demonstrated in the playground link above.
- iterationTypesOfAsyncIterable was shared between Symbol.iterator and Symbol.asyncIterator. Now iterationTypesOfAsyncedSyncIterable is used to store Symbol.iterator calls that got converted via getAsyncFromSyncIterationTypes.

To make implicit Symbol.iterator calls like actual Symbol.iterator calls, I hooked them up to the resolveCall function. This function and a few of the ones it calls had to be modified so that arbitrary Nodes could be passed into it without analyzing the Node as whatever it is. The implicit call case is differentiated by the implicitThisType being passed around. I.e. when implicitThisType is present, then the node should be treated solely as an errorNode, and the only NodeKind-specific behavior should be in getting the boundaries for error messages. typeArguments and parametric args should always be empty.

Implicit iterator calls do have different behavior from regular calls though. Intersected iterator functions will produce intersected results instead of just grabbing the first one. See #39693. Unioned iterator functions will evaluate each unioned constituent separately (passing the constituent, not the top-level type, as the implicitThisType) and the results will be unioned together. See here for examples. This means there will be inconsistency between how for (const x of obj) vs for (const x of obj[Symbol.iterator]()) is evaluated, although there was already inconsistency before and I think it is for the better.

ghost · 2020-11-09T06:41:09Z

All CLA requirements met.

typescript-bot · 2020-11-09T06:41:23Z

The TypeScript team hasn't accepted the linked issue #38388. If you can get it accepted, this PR will have a better chance of being reviewed.

Validark · 2020-11-09T07:05:28Z

I noticed on my machine that the errors in errorsInGenericTypeReference changed from Could not find symbol V to Cannot find name 'V'. Is that actually the product of one of my changes...? I don't think it is, but I could update the files if wanted. (that's all that's causing the checks to fail)

Validark · 2020-11-15T15:01:28Z

tests/baselines/reference/errorsInGenericTypeReference.errors.txt

@@ -1,5 +1,4 @@
 tests/cases/compiler/errorsInGenericTypeReference.ts(11,17): error TS2304: Cannot find name 'V'.
-tests/cases/compiler/errorsInGenericTypeReference.ts(17,31): error TS2304: Cannot find name 'V'.


This is probably not good. I am still pretty sure this has nothing to do with my changes, I just wanted to update the baseline references so my tests don't fail. Perhaps another issue should be opened for this case?

rbuckton

There's a lot going on here and it will take me some time to review. I'm not certain I agree with some of the changes here, as there are a large number of places where you've switched to use a less precise type so that you can treat node as only an "error node". A separate errorNode argument argument would likely be preferable in those cases.

It might be worth breaking out the changes to fix the caching issues you mentioned into a separate PR, as those changes seem less controversial to me.

rbuckton · 2021-01-05T01:23:42Z

src/compiler/checker.ts

@@ -26267,33 +26268,33 @@ namespace ts {
            return true;
        }

-        function callLikeExpressionMayHaveTypeArguments(node: CallLikeExpression): node is CallExpression | NewExpression | TaggedTemplateExpression | JsxOpeningElement {
+        function callLikeExpressionMayHaveTypeArguments(node: Node): node is CallExpression | NewExpression | TaggedTemplateExpression | JsxOpeningElement {


Why was the precision for this argument's type reduced?

rbuckton · 2021-01-05T01:24:16Z

src/compiler/checker.ts

-                // This gets us diagnostics for the type arguments and marks them as referenced.
-                forEach(node.typeArguments, checkSourceElement);
-            }
+        function resolveUntypedCall(node: Node): Signature {


Why was the precision for this argument's type reduced?

rbuckton · 2021-01-05T01:25:14Z

src/compiler/checker.ts

                    checkExpression(argument);
                });
            }
            return anySignature;
        }

-        function resolveErrorCall(node: CallLikeExpression): Signature {
-            resolveUntypedCall(node);
+        function resolveErrorCall(node?: Node): Signature {


I'm still not clear why you're using a less specific type here.

rbuckton · 2021-01-05T01:25:54Z

src/compiler/checker.ts

@@ -26367,58 +26368,68 @@ namespace ts {
            return !!(t.flags & (TypeFlags.Void | TypeFlags.Undefined | TypeFlags.Unknown | TypeFlags.Any));
        }

-        function hasCorrectArity(node: CallLikeExpression, args: readonly Expression[], signature: Signature, signatureHelpTrailingComma = false) {
+        function hasCorrectArity(node: Node | undefined, args: readonly Expression[], signature: Signature, signatureHelpTrailingComma: boolean, implicitThisType?: Type) {


Again, I don't agree with using a less precise type here.

rbuckton · 2021-01-05T01:29:34Z

src/compiler/checker.ts

-                    const returnSourceType = instantiateType(contextualType, outerContext && outerContext.returnMapper);
-                    inferTypes(returnContext.inferences, returnSourceType, inferenceTargetType);
-                    context.returnMapper = some(returnContext.inferences, hasInferenceCandidates) ? getMapperFromContext(cloneInferredPartOfContext(returnContext)) : undefined;
+        function inferTypeArguments(node: Node | undefined, signature: Signature, args: readonly Expression[], checkMode: CheckMode, context: InferenceContext, implicitThisType?: Type): Type[] {


Another case of using a less precise type, and I'm still not clear on why implicitThisType should cause most of this function to be ignored.

Validark · 2021-01-05T08:27:52Z

There's a lot going on here and it will take me some time to review. I'm not certain I agree with some of the changes here, as there are a large number of places where you've switched to use a less precise type so that you can treat node as only an "error node". A separate errorNode argument argument would likely be preferable in those cases.

It might be worth breaking out the changes to fix the caching issues you mentioned into a separate PR, as those changes seem less controversial to me.

Thanks for getting back to me! I really wasn't sure whether to prioritize making fewer changes and making it work with what's already there or to make changes like you suggested. I tried to think of how to solve the problem while making the smallest number of changes, then take one step towards what I would consider ideal. I wasn't sure what kind of philosophy there is about these kinds of things so I just guessed. Since my guess was wrong, I'll go back and make the changes I think would be ideal instead of trying to guess where my hands should be tied. I did kinda deviate from that anyway and got caught up a bit when I noticed a bunch of other issues while looking at the code. I will work on this more this week. I'll make this better.

(My first implementation was more along the lines of your suggestion but I thought it could be rejected for adding more than absolutely necessary)

RyanCavanaugh · 2022-02-04T21:19:02Z

🧹 Closing due to staleness. Please open a new PR if you'd like to continue. Thanks!

Validark added 7 commits October 18, 2020 11:32

Make implicit Symbol.iterator calls use resolveCall

7815ff0

Make resolveCall node argument possibly undefined

7e44daa

Retain errors for cached iterationTypes

a3a0016

Fix iterationTypes caching, add tests

75eb677

Add explanation to resolveCall

1ede677

Finishing touches, add arityError tests

201b55f

Finish up iterator redux

64b97af

typescript-bot assigned rbuckton Nov 9, 2020

typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Nov 9, 2020

Validark added 2 commits November 15, 2020 08:56

Optimize cache-ing a little more for iterationTypes

cb6de59

Update errorsInGenericTypeReference

ce784b8

Validark commented Nov 15, 2020

View reviewed changes

Update errorsInGenericTypeReference whitespace....?

af8596d

Fix typo

9eac547

sandersn requested a review from rbuckton November 23, 2020 23:27

rbuckton reviewed Jan 5, 2021

View reviewed changes

Merge 'origin/master' into iter-fix

7f54f67

RyanCavanaugh closed this Feb 4, 2022

rotu mentioned this pull request Jan 13, 2024

Iterator of array intersection incorrectly typed #57002

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(38388) Add support for `this` parameters in iterationTypes and fix caching issues #41451

fix(38388) Add support for `this` parameters in iterationTypes and fix caching issues #41451

Validark commented Nov 9, 2020

ghost commented Nov 9, 2020 •

edited by ghost

Loading

typescript-bot commented Nov 9, 2020

Validark commented Nov 9, 2020 •

edited

Loading

Validark Nov 15, 2020

rbuckton left a comment •

edited

Loading

rbuckton Jan 5, 2021

rbuckton Jan 5, 2021

rbuckton Jan 5, 2021

rbuckton Jan 5, 2021

rbuckton Jan 5, 2021

Validark commented Jan 5, 2021 •

edited

Loading

RyanCavanaugh commented Feb 4, 2022

		@@ -1,5 +1,4 @@
		tests/cases/compiler/errorsInGenericTypeReference.ts(11,17): error TS2304: Cannot find name 'V'.
		tests/cases/compiler/errorsInGenericTypeReference.ts(17,31): error TS2304: Cannot find name 'V'.

fix(38388) Add support for this parameters in iterationTypes and fix caching issues #41451

fix(38388) Add support for this parameters in iterationTypes and fix caching issues #41451

Conversation

Validark commented Nov 9, 2020

ghost commented Nov 9, 2020 • edited by ghost Loading

typescript-bot commented Nov 9, 2020

Validark commented Nov 9, 2020 • edited Loading

Validark Nov 15, 2020

Choose a reason for hiding this comment

rbuckton left a comment • edited Loading

Choose a reason for hiding this comment

rbuckton Jan 5, 2021

Choose a reason for hiding this comment

rbuckton Jan 5, 2021

Choose a reason for hiding this comment

rbuckton Jan 5, 2021

Choose a reason for hiding this comment

rbuckton Jan 5, 2021

Choose a reason for hiding this comment

rbuckton Jan 5, 2021

Choose a reason for hiding this comment

Validark commented Jan 5, 2021 • edited Loading

RyanCavanaugh commented Feb 4, 2022

fix(38388) Add support for `this` parameters in iterationTypes and fix caching issues #41451

fix(38388) Add support for `this` parameters in iterationTypes and fix caching issues #41451

ghost commented Nov 9, 2020 •

edited by ghost

Loading

Validark commented Nov 9, 2020 •

edited

Loading

rbuckton left a comment •

edited

Loading

Validark commented Jan 5, 2021 •

edited

Loading