Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicit Symbol.iterator call in for..of loops / spread destructuring doesn't infer this generic type parameter #38388

Open
Validark opened this issue May 7, 2020 · 8 comments
Labels
Experience Enhancement Noncontroversial enhancements Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros Suggestion An idea for TypeScript
Milestone

Comments

@Validark
Copy link

Validark commented May 7, 2020

TypeScript Version: 4.0.0-dev.20200507

Search Terms: Symbol.iterator type parameter implicit calling for..of loops object spread destructuring this generic

Code

What follows is 3 different ways of iterating over an object using a custom-made iterator. However, using Symbol.iterator implicitly via the for..of loop fails to produce the same behavior:

const obj = {
	x: 1,
	y: 2,
	z: 3,
	*[Symbol.iterator]<T extends object>(
		this: T,
	): Generator<NonNullable<{ [K in keyof T]: [K, NonNullable<T[K]>] }[keyof T]>, void, unknown> {
		for (const entry of Object.entries(this)) yield entry as never;
	},
};

{
	const iter = obj[Symbol.iterator]();

	for (let result = iter.next(); !result.done; result = iter.next()) {
		const { value } = result;
		value; // ["x", number] | ["y", number] | ["z", number]
	}
}

for (const value of obj[Symbol.iterator]()) {
	value; // ["x", number] | ["y", number] | ["z", number]
}

for (const value of obj) {
	value; // NonNullable<{ [K in keyof T]: [K, NonNullable<T[K]>]; }[keyof T]>
	// bad! This should be ["x", number] | ["y", number] | ["z", number]
}

// also, spread destructuring suffers from the same problem!
const values = [...obj]; // Array<NonNullable<{ [K in keyof T]: [K, NonNullable<T[K]>]; }[keyof T]>>
// bad! This should be Array<["x", number] | ["y", number] | ["z", number]>

Expected behavior: for (const value of obj) should implicitly do the type equivalent of calling our iterator symbol, i.e. for (const value of obj[Symbol.iterator]()). That means our type parameter should be correctly inferred like so: obj[Symbol.iterator]<T>(this: T).

Actual behavior: T cannot be inferred as the local this type, so all derivative types cannot be evaluated and the type stays as its generic version.

Playground Link

Related Issues: n/a

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label May 8, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.0 milestone May 8, 2020
@Validark
Copy link
Author

Validark commented May 31, 2020

I did my best to track down this problem on my own, and I believe the problem stems from this bit of code. However, I don't know the codebase well enough to know where to go from here:

// file: src/compiler/checker.ts
/**
 * Gets the *yield*, *return*, and *next* types of an `Iterable`-like or `AsyncIterable`-like
 * type from its members.
 *
 * If we successfully found the *yield*, *return*, and *next* types, an `IterationTypes`
 * record is returned. Otherwise, `noIterationTypes` is returned.
 *
 * NOTE: You probably don't want to call this directly and should be calling
 * `getIterationTypesOfIterable` instead.
 */
function getIterationTypesOfIterableSlow(type: Type, resolver: IterationTypesResolver, errorNode: Node | undefined) {
	const method = getPropertyOfType(type, getPropertyNameForKnownSymbolName(resolver.iteratorSymbolName));
	const methodType = method && !(method.flags & SymbolFlags.Optional) ? getTypeOfSymbol(method) : undefined;
	if (isTypeAny(methodType)) {
		return setCachedIterationTypes(type, resolver.iterableCacheKey, anyIterationTypes);
	}

	const signatures = methodType ? getSignaturesOfType(methodType, SignatureKind.Call) : undefined;
	if (!some(signatures)) {
		return setCachedIterationTypes(type, resolver.iterableCacheKey, noIterationTypes);
	}

	const iteratorType = getUnionType(map(signatures, getReturnTypeOfSignature), UnionReduction.Subtype);
	const iterationTypes = getIterationTypesOfIterator(iteratorType, resolver, errorNode) ?? noIterationTypes;
	return setCachedIterationTypes(type, resolver.iterableCacheKey, iterationTypes);
}

I think the problem is that map(signatures, getReturnTypeOfSignature) is used to grab the (raw) return type(s) and that neglects to account for the possibility of there being an inferred this parameter.

Consider this (simpler) code:

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

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

for (const value of obj) {
    console.log(value); // value is `keyof P`, but should be `"x" | "y" | "z"`
}

Here are the relevant variables in getIterationTypesOfIterableSlow when inferring value from for (const value of obj)

type: { x: number; y: number; z: number; [Symbol.iterator]<P extends object>(this: P): Generator<keyof P, any, unknown>; }
methodType: <P extends object>(this: P) => Generator<keyof P, any, unknown>
signatures: [ <P extends object>(this: P): Generator<keyof P, any, unknown> ]
iteratorType: Generator<keyof P, any, unknown>

I am guessing that assignParameterType or assignContextualParameterTypes may have something to do with the solution, but again, I really have no idea. Please help!

@Validark Validark changed the title Implicit Symbol.iterator call in for..of loops and object spreading doesn't infer type parameter(s) Implicit Symbol.iterator call in for..of loops / spread destructuring doesn't infer this generic type parameter May 31, 2020
@Validark
Copy link
Author

I fixed it!! I'll submit a PR soon!
image

@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Aug 31, 2020
@Validark
Copy link
Author

Validark commented Oct 2, 2020

Sorry for dropping off the face of the earth. I'm back in the game! My next priority and current goal is to get this PR out this week.

@Validark
Copy link
Author

Update: I've been putting a bunch of hours into this, I hope I am almost finished with it now.

@RyanCavanaugh RyanCavanaugh removed Needs Investigation This issue needs a team member to investigate its status. Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone labels Feb 4, 2022
@RyanCavanaugh RyanCavanaugh removed this from the TypeScript 4.6.1 milestone Feb 4, 2022
@RyanCavanaugh RyanCavanaugh added Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript labels Feb 4, 2022
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Feb 4, 2022
@RyanCavanaugh
Copy link
Member

We'd take a PR on this so long as it's not invasive, but it doesn't seem like this is a high priority right now due to the low volume of feedback we've seen on it.

@greggman
Copy link

I think I ran into this issue recently. I had code like this

    [Symbol.iterator](): IterableIterator<CaptureState> {
        let i = 0;
        this.iterating = true;

        return {
            registry: this,
            next() {
                while (i < this.registry.objects.length) {
                    const obj = this.registry.objects[i++].deref();
                    if (obj === undefined) {
                        continue;
                    }
                    return { value: this.registry.get(obj), done: false };
                }
                this.registry.iterating = false;
                return { done: true };
            },
        };
    }

It complains about this.registry not existing but it does exist and the code actually runs fine. I worked around it by simplifying the code but was surprised TS lost track of this.

@rotu
Copy link

rotu commented Jan 26, 2024

This also causes a leak of type parameters:

type ArrayIteratorMethod = <This extends ArrayLike<any>>(this: This) => IterableIterator<This[number]>
type MyStringArray = {
    [k:number]: string,
    length: number,
    [Symbol.iterator]: ArrayIteratorMethod
}
const y = ['a','b','c'] as MyStringArray

for (const b of y[Symbol.iterator]()){ // const b:string
    //     ^?
    b satisfies string
    // @ts-expect-error
    b satisfies symbol 
}
for (const b of y){ // const b: This[number]  (should be inferred as string, but instead `This` escapes from scope)
    //     ^?
    b satisfies string
    // so this unexpectedly passes, since the type of 'b' behaves like `any`
    // @ts-expect-error
    b satisfies symbol
}

Workbench Repro

@typescript-bot typescript-bot added the Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros label Jan 26, 2024
@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 15, 2024

👋 Hi, I'm the Repro bot. I can help narrow down and track compiler bugs across releases! This comment reflects the current state of this repro running against the nightly TypeScript.


Comment by @rotu

❌ Failed: -

  • Unused '@ts-expect-error' directive.

Historical Information
Version Reproduction Outputs
4.9.3, 5.0.2, 5.1.3, 5.2.2, 5.3.2

❌ Failed: -

  • Unused '@ts-expect-error' directive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experience Enhancement Noncontroversial enhancements Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants