IteratorResult<T> definition conflicts with the JS spec

[ldantonov]

TypeScript Version: 3.8.3

Search Terms: iterator return type

Code

const list = ['v1', 'v2', 'v3']

const f: () => Iterator<string> = () => {
  let index = 0
  return {
    next: (): IteratorResult<string> => {
      console.log(list)
      if (index < list.length) {
        return { value: list[index++]}
      } else {
        return { done: true }
      }
    }
  }
}

Expected behavior:
The code should compile without errors.
Actual behavior:
The code compiles with the error: "Type '{ done: true; }' is not assignable to type 'IteratorResult<string, any>'.
Property 'value' is missing in type '{ done: true; }' but required in type 'IteratorReturnResult'.(2322)"

The error can be bypassed by changing { done: true } to { value: undefined, done: true }. However, the specification allows omitting the value property. See Iteration Protocols

A suggested fix would be to change the definition of the type IteratorReturnResult to:

interface IteratorReturnResult<TReturn> {
    done: true;
    value?: TReturn;
}

Playground Link: https://www.typescriptlang.org/play?#code/MYewdgzgLgBANgS2jAvDA2gcgG4EZMA0MOATIcdgMyYC6AUHaJLAGYBcMAFAJSoB8MAJJQApgCcAhlBBiAPNDEIwAcwFoe-GAG86MeCNhKAJiIAeqGAAZdMMQYCuYsNpt6wZqBx4dh4qTIAlEQh7OCh5KEUVNQEdPXiYJggQOBEAOjgQZU5EaG5XeIQWLmMzGFl4JCgMkRUoAAteOIT4uyhHZy0YbAk4exEOXKh0UtMAajGaAF8CvSmYETgIERcW1ocnbRgjcAGYSP6YGbXj+NOZqaA

Related Issues:
#29982

[IllusionMH]

Suggested fix won't work as described here https://github.com/microsoft/TypeScript/pull/30790/files#r282595851

[ldantonov]

I confess I don't fully understand what the feared failure is, based on reading issue 30790. Is there a specific example of a type deduction failure that can illustrate that?

In any case, an alternative fix could be as follows, although without fully understanding the objection, I don't know if this is any better. This would see IteratorReturnResult tun into a type alias, and default TResult to match T to improve type inference:

type IteratorReturnResult<TResult> = { done: true, value?: TResult } | { done?: false, value: TResult }
type IteratorResult<T, TReturn = T> = IteratorYieldResult<T> | IteratorReturnResult<TReturn>

[IllusionMH]

I think there 2 kind of propblems will arise with value?: TReturn:

1. won't allow to use return value without checking for undefined first
2. won't catch cases when I forget to return a value.

const list = ['v1', 'v2', 'v3']

const f: () => Iterator<string, number> = () => {
  let index = 0
  return {
    next: (): IteratorResult<string, number> => {
      console.log(list)
      if (index < list.length) {
        return { value: list[index++]}
      } else {
        return { done: true, value: index } // Ooops, I can forget to return value here
      }
    }
  }
}

const iter = f();
const res = iter.next();

if (res.done) {
  console.log(`I wish I had ${res.value * 10} values instead`);
  //                          ~~~~~~~~~ Object is possibly 'undefined'. ts(2532)
}

[ldantonov]

Both points are inconveniences, but this is what the spec requires - value is optional when done is true, and therefore it can be undefined. Forcing a different protocol here could create an issue when interfacing with an iterator obtained from a JS library that follows the spec - TS asserts that value is always defined, yet the JS iterator could return { done: true }.

[fatcerberus]

One potential solution would be to define IteratorResult as a discriminated union (with done as the discriminant):

type IteratorResult<T> =
    | { done: false, value: T }
    | { done: true, value?: T };

Although it’s not clear what to do in the case where done is dynamically determined and therefore not statically known to be true or false (I’ve actually written code where this is the case). Should value be optional or mandatory in that case?

[rbuckton]

One potential solution would be to define IteratorResult as a discriminated union (with done as the discriminant):

type IteratorResult<T> =
    | { done: false, value: T }
    | { done: true, value?: T };

Although it’s not clear what to do in the case where done is dynamically determined and therefore not statically known to be true or false (I’ve actually written code where this is the case). Should value be optional or mandatory in that case?

IteratorResult is currently defined as a discriminated union. The problem with letting value be optional is as @IllusionMH describes:

won't allow to use return value without checking for undefined first

The only time it would be type-safe to allow value to be optional would be when TReturn is either undefined or void. We can't change the definition of IteratorReturnResult to make value optional only to serve that specific case. It might be worth considering whether we should consider a void property to be optional, just as we currently consider a void parameter "optional": Playground example.

@DanielRosenwasser - Any thoughts on extending the same treatment of a void parameter to a void property and treating it as an optional property?

[fatcerberus]

@rbuckton Maybe I'm missing something but the discriminated union I described would have value be mandatory if done is false - which means you only need to check for nullish values if done is true, and it's been my experience that in most cases code doesn't tend to return a meaningful value for done: true anyway (I'm open to being proven wrong on this, though). A typical loop would likely be:

const iter = obj[Symbol.iterator]();
while (!iter.done) {
    iter.value;  // never undefined, no need to check
}

No need to check for undefined inside the loop since the discriminated union in combination with CFA takes care of that. The only time you'd need to check is if done is true, which you probably should do anyway, or else use a non-nullish assertion ! if you're absolutely sure.

edit: I screwed up the loop, but it gets the point across and I'm too lazy to fix it. 😛

[ldantonov]

To reiterate, the problem is that, as written, IteratorReturnResult conflicts with the Iterator spec. @fatcerberus and I have suggested fixes. The argument against fixing it, so far, seems to be that it would make it less convenient to use, by requiring an undefined check. Two points:

1. The definition of the Iterator spec is what's driving the need for an 'undefined' check. I understand that one may wish it didn't, but that's a complaint about the spec itself. It doesn't work to have a type that reflects how we wish the spec to be and not how it actually is.
2. The inconvenience of checking for undefined seems extremely minor, especially considering the optional chaining and non-null assertion operators. Moreover, and this bears repeating, the Iterator spec is defined in a way that makes the 'undefined' check unavoidable.