TypeScript 2.0 change in IterableResult<T> makes implementing iterators harder

TypeScript Version: 2.0.3
The definition in lib.es6.d.ts of IteratorResult (removing the ? after value) seems to have broken existing Iterator implementations, and is particularly troublesome with strict null checking enabled. See http://stackoverflow.com/questions/39813087/why-did-typescript-2-0-change-iteratorresultk for more details.

Code
A short version:

export interface JavaIterator<E> {
    hasNext(): boolean;
    next(): E;
    remove(): void;
}
...
class IterableAdapter<T> implements Iterable<T>, IterableIterator<T> {
    private _iterator: JavaIterator<T>
    constructor(private collection: JavaCollection<T>){}

    [Symbol.iterator]() { this._iterator = this.collection.iterator(); return this;}

    next(): IteratorResult<T> {
        if (!this._iterator.hasNext()) return { done: true }; // Error on this line
        return {done: false, value: this._iterator.next()}
    }
}

this version works

class IterableAdapter<T> implements Iterable<T>, IterableIterator<T> {
    private _iterator: JavaIterator<T>
    constructor(private collection: JavaCollection<T>){}

    [Symbol.iterator]() { this._iterator = this.collection.iterator(); return this;}

    next(): IteratorResult<T> {
        if (!this._iterator.hasNext()) {
            // A bit of a hack needed here needed for strict null checking
            return { done: true, value: undefined } as any as IteratorResult<T>;
        }
        return {done: false, value: this._iterator.next()}
    }
}

original bug

class HashMapKeyIterable<K,V> implements Iterator<K>, IterableIterator<K> {
    private _bucket: HashMapEntry<K,V>[];
    private _index: number;

    constructor( private _buckets : Iterator<HashMapEntry<K,V>[]> ){
        this._bucket = undefined;
        this._index = undefined;
    }

    [Symbol.iterator]() { return this }

    next():  IteratorResult<K> {
        while (true) {
            if (this._bucket) {
                const i = this._index++;
                if (i < this._bucket.length) {
                    let item = this._bucket[i];
                    return {done: false, value: item.key}
                }
            }
            this._index = 0
            let x = this._buckets.next();
            if (x.done) return {done: true}; // Under TS 2.0 this needs to
            this._bucket = x.value;          // return {done: true: value: undefined};
            }
        }
    }

Expected behavior:
Clean compile under TypeScript 2.0

Actual behavior:
Errors reported: TS2322: Type '{ done: true; }' is not assignable to type 'IteratorResult'. Property 'value' is missing in type '{ done: true; }'

Note: with strict null checking on, I seem to need to go further, resorting to

return { done: true, value: undefined } as any as IteratorResult<T>;`

This does seem to be a bug and a divergence from IteratorResult in the ECMAScript spec. The spec states the following about the value property:

If done is true, this is the return value of the iterator, if it supplied one. If the iterator does not have a return value, value is undefined. In that case, the value property may be absent from the conforming object if it does not inherit an explicit value property.

There is a long chain of issues around this. It is a little confusing. See #8357 for some the motivation.

Both #8357 and #8938 mention that with boolean literal types, IteratorResult can be properly fixed.

We now have boolean literal types, but it seems it's still difficult to fix - see #2983 (comment).

TL;DR the fix for this is now held up on default generic type variables (#2175). Unless of course someone can come up with a better alternative.

To me the fix seems easy: switch the definition in lib.es6.d.ts back to what it was in 1.8:

interface IteratorResult<T> {
    done: boolean;
    value?: T;
}

Entangling this with an enhancement that tries to enforce complex conditions for the property's optional status seems overreaching. I don't see why it should be held up on default generic type variables.

Burt Harris (@BurtHarris)

Burt Harris (@BurtHarris):

The previous version of IteratorResult also isn't accurate, because it incorrectly conflates the yield and return types.
Simply switching back would break things that were made possible by the 'fix' to Variable is considered undefined in for-of loop when target is es6 #8357, so it would be win-lose.

Troy Gerwien (@yortus)

TL;DR the fix for this is now held up on default generic type variables (#2175). Unless of course someone can come up with a better alternative.

Troy Gerwien (@yortus) I've just wrote an alternative #2983 (comment)

Troy Gerwien (@yortus)

Thanks Troy Gerwien (@yortus) .

It seems to me like it's not the previous definition of IteratorResult that is conflating yield and return types, that seems like it has to be something in tsc handling of for..of.

If I understand the background on #8357, that would only only comes about when using strict null checking. The change in lib.es6.d.ts impacts any users who upgrade to TypeScript 2.0, even if they are not using the new features.

Burt Harris (@BurtHarris)

Burt Harris (@BurtHarris) the problem in both the current and the previous IteratorResult declarations is that value is typed as T, but really must support two different types. That's because value is the yielded value when done is false, and the returned value when done is true. These values aren't necessarily (or even usually) of the same type. For example:

function* foo() {
    yield 1;
    yield 2;
    yield 3;
    return 'done';
}

// Looping over the next() values for foo() gives:
// { done: false, value: 1 }                                                   
// { done: false, value: 2 }                                                   
// { done: false, value: 3 }                                                   
// { done: true, value: 'done' }

So the type of value is number when done==false, and string when done==true. The current/previous definition can't capture that because it only has one type argument T. A union of the two types wouldn't be helpful either.

Yes, but the latest issue you raise sounds like a problem with generators, not necessarily a problem with iterators. And it certainly makes sense to me that the type associated with a generator could perhaps need to account for the difference between yield and return. However, the code I'm working toward isn't (and probably won't be) dependent on generators.

Now I'll be the first to admit I'm not an expert on generators or iterators, and that if I were designing these features I would have specified the protocol differently. But based on my reading of the iterator protocol from Mozilla.org, I inferred that in the value returned from a next() method, both done and value are optional, suggesting only one or the other need be provided. Personally, I think that part of the protocol makes sense, though it might be hard to model completely in terms of TypeScript. Perhaps that's why you are saying its dependent default generic type variables, is that right?

But in terms of a practicing programmer (using a released tool) rather than a language designer, I would prefer that TypeScript 2 accept the old interface until the details of proper enforcement the evolved protocol can be worked out, or until I opt-in to enforcement of a more restrictive one.

Work around microsoft/TypeScript#11375

Burt Harris (@BurtHarris)

I agree with Burt Harris (@BurtHarris) that the interface IteratorResult is very confusing. The protocol clearly states that an undefined value is valid if done: true, but it's currently not covered by the interface. This scenario is no corner case. I'd say pretty much every custom iterator will run into this problem when strictNullChecks is enabled (which should be the case in every project if you ask me).

Not sure if this has been suggested but could we fix this by just defining IteratorResult as a discriminated union, i.e.

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

Bruce Pascoe (@fatcerberus)

~~Bruce Pascoe (@fatcerberus) I believe that when done is true, value should always be undefined:~~

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

edit: see my later comment

No, that’s not correct:

function* g() {
    yield 812;
    return 1208;
}

const iter = g();
console.log(iter.next());  // { done: false, value: 812 }
console.log(iter.next());  // { done: true, value: 1208 }

Ah, of course, hahaha ☺️

It kind of has to be two types though, the return value doesn't have to be the same as the yielded value:

function* g() {
  yield 1
  yield 2
  return 'hello'
}

const iter = g()

console.log(iter.next());  // { done: false, value: 1 }
console.log(iter.next());  // { done: false, value: 2 }
console.log(iter.next());  // { done: true, value: 'hello' }

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

relevant: #2983

It kind of has to be two types though, the return value doesn't have to be the same as the yielded value

For this particular use case, I think union type should suffice.

const a: IteratorResult<number | string> = iter.next()

For this particular use case, I think union type should suffice.

Using a type union wouldn't accurately let me work with the iterator though:

function* g() {
  yield 1
  yield 2
  return 'hello'
}

const iter: Iterator<number | string> = g()

for (let num of iter) {
  console.log(num * 2) // <- cannot multiply string with 2
}

This will incorrectly tell me that num can be a string...

This should be resolved by #30790

It turns out #30790 does pretty much exactly what I suggested (plus more!). Nice. 😃

Why close this? #30790 is a PR, not an issue, and it hasn't been merged yet.

Burt Harris (@BurtHarris)

Agreed.

Burt Harris (@BurtHarris) can you reopen this?

Should be merged now.

Thank you all.

Navigation Menu

Repository navigation

TypeScript 2.0 change in IterableResult<T> makes implementing iterators harder #11375

31 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

TypeScript 2.0 change in IterableResult<T> makes implementing iterators harder #11375

Description

Activity

yortus commented on Oct 5, 2016

aluanhaddad commented on Oct 5, 2016

yortus commented on Oct 5, 2016

BurtHarris commented on Oct 5, 2016

yortus commented on Oct 5, 2016

Igorbek commented on Oct 5, 2016

BurtHarris commented on Oct 5, 2016

yortus commented on Oct 5, 2016

BurtHarris commented on Oct 5, 2016

winterbe commented on Aug 11, 2017

31 remaining items

fatcerberus commented on May 31, 2019

LinusU commented on Jun 1, 2019

fatcerberus commented on Jun 1, 2019

LinusU commented on Jun 2, 2019

KSXGitHub commented on Jun 2, 2019

LinusU commented on Jun 2, 2019

rbuckton commented on Jun 2, 2019

fatcerberus commented on Jun 2, 2019

knocte commented on Jul 2, 2019

KSXGitHub commented on Jul 2, 2019

DanielRosenwasser commented on Jul 8, 2019

BurtHarris commented on Jul 14, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions