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

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

Open
BurtHarris opened this Issue Oct 5, 2016 · 21 comments

Comments

Projects
None yet
@BurtHarris
Copy link

BurtHarris commented Oct 5, 2016

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>;`
@yortus

This comment has been minimized.

Copy link
Contributor

yortus commented Oct 5, 2016

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.

@aluanhaddad

This comment has been minimized.

Copy link
Contributor

aluanhaddad commented Oct 5, 2016

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

@yortus

This comment has been minimized.

Copy link
Contributor

yortus commented Oct 5, 2016

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.

@BurtHarris

This comment has been minimized.

Copy link
Author

BurtHarris commented Oct 5, 2016

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.

@yortus

This comment has been minimized.

Copy link
Contributor

yortus commented Oct 5, 2016

@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 #8357, so it would be win-lose.
@Igorbek

This comment has been minimized.

Copy link
Contributor

Igorbek commented Oct 5, 2016

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.

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

@BurtHarris

This comment has been minimized.

Copy link
Author

BurtHarris commented Oct 5, 2016

Thanks @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.

@yortus

This comment has been minimized.

Copy link
Contributor

yortus commented Oct 5, 2016

@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.

@BurtHarris

This comment has been minimized.

Copy link
Author

BurtHarris commented Oct 5, 2016

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.

@winterbe

This comment has been minimized.

Copy link

winterbe commented Aug 11, 2017

I agree with @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).

@riggs

This comment has been minimized.

Copy link

riggs commented Dec 2, 2017

I have code creating and using an iterator directly and just had to fix a bug that would've been caught if a the type definition for value was correct for an exhausted iterator.

@BurtHarris

This comment has been minimized.

Copy link
Author

BurtHarris commented Dec 14, 2017

@mhegazy can you comment on what's happening re this issue? There was past comment that a solution depended on generic type variable (#2175). As I understand it that got resolved, is there anything still blocking an updated definition of IteratorResult?

@ogwh

This comment has been minimized.

Copy link

ogwh commented Feb 3, 2018

I was about to report this as a bug myself, does anyone know what's happening?

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Feb 5, 2018

@rbuckton can you try changing this to a union return type and validate that it improves RWC / the examples listed here?

@mhegazy mhegazy added this to the TypeScript 2.8 milestone Feb 6, 2018

@mhegazy mhegazy removed this from the TypeScript 2.8 milestone Mar 9, 2018

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Apr 25, 2018

The proposal in #2983 is still valid today, we should use that.

@DHager

This comment has been minimized.

Copy link

DHager commented Jun 21, 2018

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

@BurtHarris Thanks for this workaround, it was exactly what I was struggling to find. (Other keywords for future searchers: exception, exemption, bypass, cast.)

@felixfbecker

This comment has been minimized.

Copy link

felixfbecker commented Jul 14, 2018

Maybe this could be considered for 3.0 if it is a slight breaking change? With the AsyncIterables used in NodeJS streams this is becoming a bigger pain point in TS

@rbuckton

This comment has been minimized.

Copy link
Member

rbuckton commented Aug 14, 2018

@DHager: another solution to your issue is return { done: true, value: undefined! };.

@RyanCavanaugh: Changing IteratorResult to a union is incompatible with the current NodeJS definitions on DefinitelyTyped which create a forward declaration for IteratorResult as an interface. This is something that /// <reference lib="es2015.iterable" /> is intended to solve, but that would force a dependency on TypeScript 3.0 in NodeJS's declarations.

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Oct 30, 2018

Currently waiting on DefinitelyTyped infrastructure changes.

@Igorbek

This comment has been minimized.

Copy link
Contributor

Igorbek commented Oct 30, 2018

@DanielRosenwasser What exact change is being waited? Could you please give the link to DT issue for tracking?

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Nov 26, 2018

Sorry; the change is for DefinitelyTyped to support version redirects. Not sure if @rbuckton or @sandersn has an open issue for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment