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

IterableIterator<T>.next().value is now any instead of T #33353

Closed
blixt opened this issue Sep 10, 2019 · 27 comments · May be fixed by #58243
Closed

IterableIterator<T>.next().value is now any instead of T #33353

blixt opened this issue Sep 10, 2019 · 27 comments · May be fixed by #58243
Labels
Question An issue which isn't directly actionable in code

Comments

@blixt
Copy link

blixt commented Sep 10, 2019

TypeScript Version: 3.6.2

Search Terms: iterableiterator iterator

Code

declare function getIterator(): IterableIterator<number>;
const iter = getIterator()
const { value } = iter.next()

Expected behavior:
value: number (TS 3.5)

Actual behavior:
value: any

Playground Link:

NOTE: Playground is running TS 3.5 and exhibits "Expected behavior"
http://www.typescriptlang.org/play/#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXwHMQMBJDEGKDHGACgEoAueMiqAIwhFcupgB5UyALbsKAPgDcAKDB4AzhnhZyMeAF5CxHlRoNZCpQG94ANygRkCAL4blqgHSoQADwz6gA

Related Issues:

@MattiasBuelens
Copy link
Contributor

The root cause is that IterableIterator<T> uses Iterator<T>, only specifying the type of yielded values. The type of returned values (TReturn) defaults to any. As a result, iter.next() is of type IteratorResult<number, any> which equals IteratorYieldResult<number> | IteratorReturnResult<any>. The former has { value: number } and the latter has { value: any }, so the union becomes { value: any }.

That said: IteratorResult is a discriminated union type, so you can use its done property to find out whether you're dealing with an IteratorYieldResult or an IteratorReturnResult. This would also make your code sample more correct: if iter is empty, value will probably not be a number. Unfortunately, you can't use a destructuring assignment then, since that loses the "connection" between done and value.

const iter = getIterator();
const result = iter.next();
if (!result.done) {
    const value = result.value;
    // value is a number here
}

@blixt
Copy link
Author

blixt commented Sep 10, 2019

Okay, fair enough. I had been using done but I didn't realize the type was discriminated.

I specifically ran into this with calling numberArray[Symbol.iterator]() – could that type be narrowed since we know the behavior of array iterators? If done is true, then value will be undefined.

@MattiasBuelens
Copy link
Contributor

Yeah, it's unfortunate that TReturn defaults to any, since most iterators return { done: true, value: undefined } once they're done. I guess it may be needed for backwards compatibility with older code? 🤷‍♂

@brainkim
Copy link

Related issue:
#33241

Turns out you can’t rely on the checking the done property to narrow the type of value so in your example above value is still of type any.

@MattiasBuelens
Copy link
Contributor

Apparently this only happens when strictNullChecks is off (see comment). If possible, you can try turning on strictNullChecks (or even beter: strict).

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Sep 16, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.7.0 milestone Sep 16, 2019
@rbuckton
Copy link
Member

The problem was that we previously conflated the yielded type and the return type in generators. Unfortunately, defaulting to any was necessary for backwards compatibility. Ideally I would default TReturn to void, but that caused significant breaks in real-world code.

@blixt
Copy link
Author

blixt commented Sep 18, 2019

What about interface Array<T> { [Symbol.iterator]: … }? Can that iterator be typed better than any?

@papb
Copy link

papb commented Dec 30, 2020

I have been using the following workaround so far:

export interface StrictIterableIterator<T, TReturn> extends Iterator<T, TReturn> {
	[Symbol.iterator](): StrictIterableIterator<T, TReturn>;
}

export interface StrictAsyncIterableIterator<T, TReturn> extends AsyncIterator<T, TReturn> {
	[Symbol.asyncIterator](): StrictAsyncIterableIterator<T, TReturn>;
}

@RyanCavanaugh RyanCavanaugh added Question An issue which isn't directly actionable in code and removed Needs Investigation This issue needs a team member to investigate its status. 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
Copy link
Member

Closing per above comment #33353 (comment)

Requests for change in this behavior can be made in a new suggestion issue. Thanks!

@mindplay-dk
Copy link

To others struggling with this:

Iterator<T, void> is not the answer - this will just give you another problematic type union of T | void.

If you're not using the return type (most iterators do not) the answer is Iterator<T, never> - the type union will be T | never, which resolves to T, and there you go.

This was quite frustrating and very unintuitive. I'd suggest you reopen and resolve this with a breaking change in the next major release. This is exactly the kind of drag that makes some people think TS is just a troublemaker that gets in the way of productivity. Iterators are common enough, and using return types is rare enough, that this ought to just work by default.

The number of people who would be affected by changing the default return type to never is probably almost none - maybe a few people might even discover and fix a bug.

@SergeyDvornikovSetronica

I welcome everyone.

Yes, unfortunately, I also ran into a problem when I specify Iterator<T, void>. Thanks, @mindplay-dk , for problem solving

@SergeyDvornikovSetronica

Unfortunately, Iterator<T, never> also not a fully functional option. Something like this

const iterator: Iterator<number, never> = loadNumbers();
const next = iterator.next();
const str = next.value.toFixed()

will not catch compilation error. Even in strict mode.

Although this code:

const iterator: Iterator<number, never> = loadNumbers();
const next = iterator.next();
if (next.done) {
  const str = next.value.toFixed() // Error
}

will result in a compilation error

@papb
Copy link

papb commented Sep 20, 2022

@mindplay-dk @SergeyDvornikovSetronica why not just use my solution above?

@SergeyDvornikovSetronica

@papb, unfortunately, this does not work, for exactly the same reason that is discussed everywhere.
Simple code:

export interface StrictIterableIterator<T, TReturn> extends Iterator<T, TReturn> {
  [Symbol.iterator](): StrictIterableIterator<T, TReturn>;
}

function* get(): StrictIterableIterator<number, void> {
  yield 999;
}

const iterator = get();
const next = iterator.next();
if (!next.done) {
  next.value.toFixed();
}

leads to compilation error in ts 3.8.3 with error:

test.ts:262:14 - error TS2339: Property 'toFixed' does not exist on type 'number | void'.
  Property 'toFixed' does not exist on type 'void'.

262   next.value.toFixed();
                 ~~~~~~~

even with strict mode disabled, although IDEA says everything is fine. Those. exactly what they say a lot where

@mindplay-dk
Copy link

Although this code:

const iterator: Iterator<number, never> = loadNumbers();
const next = iterator.next();
if (next.done) {
  const str = next.value.toFixed() // Error
}

will result in a compilation error

@SergeyDvornikovSetronica that's working as intended, as far as I can tell?

If next.done is true, that means there is no value, iteration has finished.

Here's an example to explain more clearly:

function* loadNumbers(): Iterator<number, void> {
    yield* [1,2,3];
}

const iterator = loadNumbers();

const next = iterator.next();

if (next.done) {
  const str = next.value.toFixed() // 👍 Correctly errors (you're done - there is no value)
} else {
  const str = next.value.toFixed() // 👍 Works
}

@mindplay-dk @SergeyDvornikovSetronica why not just use my solution above?

It was more verbose, and I don't need the result type, which I would never use. (I don't know why it exist in JS in the first place - a function should either return or yield, doing both has really confusing ergonomics.)

Simply type-hinting like I did in the example here has been working fine for me.

The remaining question is why does this default to any, which doesn't actually work - instead of never, which works fine?

I think this issue should be reopened.

@SergeyDvornikovSetronica

@mindplay-dk ,

that's working as intended, as far as I can tell?

Yes,

const iterator: Iterator<number, never> = loadNumbers();
const next = iterator.next();
if (next.done) {
  const str = next.value.toFixed() // Error
}

works as intended, but your example:

function* loadNumbers(): Iterator<number, void> {
    yield* [1,2,3];
}

const iterator = loadNumbers();

const next = iterator.next();

if (next.done) {
  const str = next.value.toFixed() // 👍 Correctly errors (you're done - there is no value)
} else {
  const str = next.value.toFixed() // 👍 Works <-- Error
}

not working (for me, on ts 3.8.3) with error

error TS2339: Property 'toFixed' does not exist on type 'number | void'.
  Property 'toFixed' does not exist on type 'void'.

491   const str = next.value.toFixed() // 👍 Works

although IDEA doesn't highlight any errors

@SergeyDvornikovSetronica

@mindplay-dk ,

The remaining question is why does this default to any, which doesn't actually work - instead of never, which works fine?

As I said before, never also has a problem:

function* loadNumbers(): Iterator<number, never> {
  yield* [1, 2, 3];
  return undefined as never;
}

const iterator = loadNumbers();

const next = iterator.next();
const str = next.value.toFixed() // 👍 Works

compiles without problems even though it is an error

@mindplay-dk
Copy link

@SergeyDvornikovSetronica I think the assumption with these type definitions is you're going to check done - apparently, if you don't check done, the assumption is that's because you know you're not done, but there's no scenario (that I can think of) where you would manually read one result from an iterator, and also no scenario where you would iterate without checking done.

So this doesn't force you to correctly write manual iteration code - that may not have been the primary goal with these types, or it could be that this was just written before TS 3.5 added smarter union type checking.

Arguably, maybe the underlying type union should have been the "reverse" somehow, forcing you to "prove" that there is a value by checking done first, and it would resolve to never if you didn't check.

Here's an example of forcing a caller to check a type union:

type TResultDone<T> = { done: true }; // { done: true, value: void } would also work (but value: never would not)

type TResultNotDone<T> = { done: false, value: T };

type TResult<T> = TResultDone<T> | TResultNotDone<T>;

function maybeNumber(): TResult<number> {
  return { done: false, value: 1 };
}

const result = maybeNumber();

const a = result.value.toFixed(); // 👍 correctly fails: you must check done first

if (result.done) {
  const b = result.value.toFixed(); // 👍 correctly fails: we're done, there is no value
} else {
  const c = result.value.toFixed(); // 👍 succeeds: we're not done, so there is a value
}

As noted in the comment, a union with { done: true, value: never } would not work in terms of narrowing the type of value - apparently, a union between void and T is void, whereas a union between never and T is T, so maybe that's the issue with the underlying Iterator types...

I haven't looked closely, but this should be applicable to solving this Iterator problem.

I'm sure more improvements could be made here, which is why I'm calling for this issue to be reopened.

You did mention you're using TS 3.8.3, which is very old - I don't know which other problems it might have, but I did check my last example here with that version, and it worked fine. Maybe consider upgrading to TS 4. You also mentioned IDEA doesn't highlight errors - that's probably because you're using the built-in TS 4 version - to get correct error messages in IDEA (or VS Code or most other IDEs) you need to configure the IDE to use your local TS version instead.

@shicks
Copy link
Contributor

shicks commented Oct 12, 2022

but there's no scenario (that I can think of) where you would manually read one result from an iterator, and also no scenario where you would iterate without checking done.

This is pretty common when you want to get the first element from an iterable that doesn't otherwise provide direct access (e.g. a Set). If the set happens to be empty, then it's going to be undefined. Using never glosses over that easy-to-forget-about case.

As noted in the comment, a union with { done: true, value: never } would not work in terms of narrowing the type of value - apparently, a union between void and T is void, whereas a union between never and T is T, so maybe that's the issue with the underlying Iterator types...

I don't think this is true for any version of TypeScript. The union between void and T is T|void. The problem (to my mind) is that void is strictly less useful than undefined (and will complain if you try to pass it somewhere that accepts T|undefined), and it's perfectly reasonably to use map.values().next().value to get a V|undefined, just like you might write map.get(someKey) to get the same type if you don't know that the key is in the map. I think the precedent of potentially-missing collection elements being T|undefined is pretty strong here, so Iterator<T, undefined> seems like the clearly correct thing to do. I'd be interested to see how much actually breaks if this is changed for specific individual collections.

@GabenGar
Copy link

Shouldn't this be reopened? Now that Iterator Helpers is going to be a thing in browsers, generators are going to be used a lot more outside of for...of loops and therefore this annoying typing problem (which already expresses when trying to get a first value from Set) is only going to get worse.

@Johnrobmiller
Copy link

Johnrobmiller commented Apr 18, 2024

This issue should be reopened.

The type gymnastics required here are fairly extreme, and needlessly so. Also, the solution to achieve the expected behavior is too obstruse -- users should not be expected to know in advance any special knowledge required to achieve the expected result here.

The way this works should be redesigned with these points in mind.

@rbuckton
Copy link
Member

No matter what changes we might make here, the only safe way to access .value is by checking .done. Any operation that merely performs .next().value is going to get the union of the yielded value (T) and the return value (TReturn).

In #58243 we are looking into adding the TReturn and TNext type parameters to Iterable/IterableIterator/AsyncIterable/AsyncIterableIterator just as we do for Iterator/Generator/AsyncIterator/AsyncGenerator, though the default for TReturn will still be any as anything else is a significant breaking change.

We're also looking to explicitly type built-ins to use void for TReturn instead of any to more accurately reflect the underlying behavior. You will still need to discriminate on done to get the correct type for value, but at least the union of both sides will be T | void instead of any.

@GabenGar
Copy link

So is this typescript-approved way to fetch the first value of a set?

const inputs = ["typescript"] as const;
const setExample: ReadonlySet<(typeof inputs)[number]> = new Set(inputs);
const step = setExample.keys().next();

if (!step.done) {
  const value = step.value;
} else {
  const value = step.value;
}

Can't even use const value = !step.done ? step.value : step.value because it widens up to any.

@rbuckton
Copy link
Member

The .value in the done: true branch isn't relevant. You can use const value = !step.done ? step.value : undefined if you'd like. Alternatively, this helper function can be useful:

function getValue<T, TReturn>(res: IteratorResult<T, TReturn>): T {
  if (res.done) {
    throw new Error("Iterator was closed");
  }
  return res.value;
}

and then you can use getValue(setExample.keys().next()).

It's important to remember that the compiler can't make as many assumptions about iterators as it can about a tuple, so inputs[0] in your example is guaranteed to have the type "typescript" because the index is known. The compiler doesn't know about the size of setExample, since it's not fixed, and also doesn't know whether a given call to .next() is the first or last call to that function, or anywhere in between.

@rbuckton
Copy link
Member

You could also use this code to retrieve the first value:

const [value] = setExample.keys()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

Successfully merging a pull request may close this issue.