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

add types for iterator helpers proposal #58222

Merged
merged 11 commits into from
Jul 19, 2024
Merged

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Apr 17, 2024

Fixes #54481.

I have only minimal tests because almost all the types are copied directly from Array with very minimal changes (mostly dropping the third "array" parameter to callbacks and the "thisArg" parameter to callback-taking methods, and returning NativeIterator<T> instead of T[]). But I'm happy to make the tests more extensive if you'd like.

For reviewing:

  • ignore bd51b00 through 30d9bc2; the last one reverts the earlier ones so the next commits can be clearer
  • f5393db and ebc4e71 add the new BuiltinIterator and AsyncBuiltinIterator types and adopt them everywhere except the .generated files. AsyncBuiltinIterator isn't necessary yet but I figured we might as well do this plumbing work now.
  • c4a8a08 adds the new types following the strategy @rbuckton suggested below

The "rebaseline" commits are just running npm run test -- --no-lint; npx hereby baseline-accept and committing the result.


Current status (as of 2024-04-25):

I'll address the outstanding todos one #58243 lands.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Apr 17, 2024
@fatcerberus
Copy link

I will need someone to tell me how to handle the generated DOM typings

The DOM typings are maintained at https://github.com/microsoft/TypeScript-DOM-lib-generator

@bakkot
Copy link
Contributor Author

bakkot commented Apr 17, 2024

More concretely, I'll need someone to tell me how to handle the change: changing the DOM typings to use NativeIterator would break anyone not importing esnext.iterator, but there's not (currently) an equivalent of esnext for DOM typings, so there's not anywhere else to do the update.

@Renegade334
Copy link
Contributor

Renegade334 commented Apr 17, 2024

More concretely, I'll need someone to tell me how to handle the change: changing the DOM typings to use NativeIterator would break anyone not importing esnext.iterator, but there's not (currently) an equivalent of esnext for DOM typings, so there's not anywhere else to do the update.

Would the following work?

  1. Introduce a hollow NativeIterator interface in es2015.iterable.d.ts
  2. Re-type all intrinsic appearances of IterableIterator in es2015.iterable.d.ts etc. to NativeIterator
  3. Extend the NativeIterator interface in esnext.iterator.d.ts, add the constructor definition, etc.

This would allow what you're looking for: keeps compatibility for pre-esnext targets, while allowing the helper methods to be merged into NativeIterator in esnext. The DOM library could then be retyped without causing BC issues.

Sparse illustration:

// @target: esnext
// @lib: esnext

// @filename: lib.es2015.iterable.d.ts

interface Iterator<T, TReturn = any, TNext = undefined> {
    next(...args: [] | [TNext]): IteratorResult<T, TReturn>;
    return?(value?: TReturn): IteratorResult<T, TReturn>;
    throw?(e?: any): IteratorResult<T, TReturn>;
}

interface Iterable<T> {
    [Symbol.iterator](): Iterator<T>;
}

interface IterableIterator<T> extends Iterator<T> {
    [Symbol.iterator](): IterableIterator<T>;
}

interface NativeIterator<T, TReturn = void, TNext = undefined> extends Iterator<T, TReturn, TNext> {
    [Symbol.iterator](): NativeIterator<T, TReturn, TNext>;
}

interface Generator<T = unknown, TReturn = any, TNext = unknown> extends NativeIterator<T, TReturn, TNext> {
    // etc.
}

interface Array<T> /* etc. */ {
    // was: [Symbol.iterator](): IterableIterator<T>;
    [Symbol.iterator](): NativeIterator<T>;

    // was: entries(): IterableIterator<[number, T]>;
    entries(): NativeIterator<[number, T]>;

    // etc.
}


// @filename: lib.esnext.iterator.d.ts

interface NativeIterator<T, TReturn, TNext> {
    map<U>(callbackfn: (value: T, index: number) => U): NativeIterator<U>;

    // etc.
}

// @filename: test.ts

const mappedArrayIterator = ['a', 'b', 'c'].entries().map(([k, s]) => k * s.charCodeAt(0)); // NativeIterator<number>

const castToIterableIterator: IterableIterator<number> = mappedArrayIterator; // NativeIterator<T> remains assignable to IterableIterator<T>, for what it's worth

As for the question of extending IterableIterator versus creating a new interface for native Iterator instances, my own $0.02 is in favour of the latter. IterableIterator is a mature interface, and it isn't just used internally: a cursory glance through GH shows quite a few examples of manually-implemented iterable iterator objects in the ecosystem, and these remain valid iterables/iterators in esnext even though they are not descended from Iterator.prototype and don't implement helper methods. Maintaining separation between the "implements iterator protocols" interface and the "descends from Iterator.prototype" interface makes sense logically, and has the added benefit of not breaking third-party implementations of the existing interface.

@bakkot
Copy link
Contributor Author

bakkot commented Apr 17, 2024

Would the following not work?

Nice, that sounds like a good approach to me.

@rbuckton
Copy link
Member

rbuckton commented Apr 17, 2024

@bakkot the approach I am experimenting with to support abstract on NativeIterator looks something like this:

// lib.esnext.iterator.d.ts
/// <reference lib="es2015.iterable" />
export {};

// Abstract type that allows us to mark `next` as `abstract`
declare abstract class Iterator<T> {
  abstract next(value?: undefined): IteratorResult<T, void>;
}

// Merge all members of `NativeIterator<T>` into `Iterator<T>`
interface Iterator<T> extends globalThis.NativeIterator<T, void, undefined> {}

// Capture the `Iterator` constructor in a type we can use in the `extends` clause of `IteratorConstructor`.
type NativeIteratorConstructor = typeof Iterator;

declare global {
  // Global `NativeIterator<T>` interface that can be augmented by polyfills
  interface NativeIterator<T, TReturn, TNext> {
    // prototype elements
  }

  // Global `IteratorConstructor` interface that can be augmented by polyfills
  interface IteratorConstructor extends NativeIteratorConstructor {
    // static elements
  }

  var Iterator: IteratorConstructor;
}

// lib.es2015.iterable.d.ts

...

interface NativeIterator<T, TReturn = void, TNext = undefined> extends Iterator<T, TReturn, TNext> {
  [Symbol.iterator](): NativeIterator<T>;
}

...

And in use:

new Iterator<number>(); // ts(2511): Cannot create an instance of an abstract class.

class C extends Iterator<number> {} // ts(2515): Non-abstract class 'C' does not implement inherited
                                    //           abstract member next from class 'Iterator<number>'.

Lib references can't really be referenced as modules, and even if you could this provides no exports. However, we will still augment the global scope with the types defined in declare global.

@rbuckton
Copy link
Member

There need to be a few other changes so that it can also be used with generators, though.

find<S extends T>(predicate: (value: T, index: number) => value is S): S | undefined;
find(predicate: (value: T, index: number) => unknown): T | undefined;

readonly [Symbol.toStringTag]: "Iterator";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest defining this as

Suggested change
readonly [Symbol.toStringTag]: "Iterator";
readonly [Symbol.toStringTag]: string;

otherwise subclasses of Iterator won't be able to redefine it.


declare var Iterator: (abstract new <T>() => NativeIterator<T>) & IteratorConstructor;

// TODO BEFORE MERGING: update all existing IterableIterator-return methods to return NativeIterator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested elsewhere, I would suggest you define NativeIterator as follows in the es2015 libs and then just update all of the IterableIterator references to NativeIterator:

interface NativeIterator<T, TReturn = void, TNext = undefined> extends Iterator<T, TReturn, TNext> {
  [Symbol.iterator](): NativeIterator<T, TReturn, TNext>;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I split that into a separate PR, or do it here?

@@ -0,0 +1,133 @@
interface NativeIterator<T, TReturn = void, TNext = undefined> extends Iterator<T, TReturn, TNext> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The export {} trick mentioned in my earlier comment can address the abstract next() method definition.

@@ -0,0 +1,133 @@
interface NativeIterator<T, TReturn = void, TNext = undefined> extends Iterator<T, TReturn, TNext> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The word choose "Native" is not so good. Do we have other options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rbuckton had previously suggested Builtin, which I'm also fine with.

@Renegade334
Copy link
Contributor

Renegade334 commented Apr 17, 2024

@rbuckton does the interface merging make any existing "custom" Iterator objects fail type validation in target:esnext, since they won't implement the new Iterator.prototype methods?

Edit: I overlooked the module hacking. Very nice.

@Jack-Works
Copy link
Contributor

@rbuckton does the interface merging make any existing "custom" Iterator objects fail type validation in target:esnext, since they won't implement the new Iterator.prototype methods?

It does not merging the current Iterator interface, but create a new one called NativeIterator

@rbuckton
Copy link
Member

@rbuckton does the interface merging make any existing "custom" Iterator objects fail type validation in target:esnext, since they won't implement the new Iterator.prototype methods?

@bakkot is correct. This should not affect anyone implementing a custom Iterator, or even IterableIterator.

After this is in we may want to add a shortcuts for it to getIterationTypesOfIterableFast and getIterationTypesOfIteratorFast in the checker.

@bakkot
Copy link
Contributor Author

bakkot commented Apr 18, 2024

Updated almost exactly following this comment, except that I had BuiltinIterator's Symbol.iterator method pass all the type arguments to the return type; this is necessary so Generator can extend BuiltinIterator (otherwise the Symbol.iterator methods' types conflict).


A consequence of this change is that the type of yield* [] is now void instead of any, because the default for TReturn on BuiltinIterator is void instead of any (as it is for `Iterator).


Also, passing void for the second argument in the return type in next in

declare abstract class Iterator<T> {
    abstract next(value?: undefined): IteratorResult<T, void>;
}

means that classes attempted to implement Iterator have some pain: you can't just return { done: false, value: 0 } from your next because the done value there will be inferred as boolean, which means that the value field has to conform to void so that it can extend the { done: true, value: TReturn } half of IteratorResult<T, void>, which it does not. Also, you can't make an iterator which actually does have a non-void value for the done: true case, which seems like an unnecessary constraint.

I have test cases illustrating the difficulties:

    class BadIterator1 extends Iterator<number> {
      next() {
      ~~~~
!!! error TS2416: Property 'next' in type 'BadIterator1' is not assignable to the same property in base type 'Iterator<number>'.
!!! error TS2416:   Type '() => { readonly done: false; readonly value: 0; } | { readonly done: true; readonly value: "a string"; }' is not assignable to type '(value?: undefined) => IteratorResult<number, void>'.
!!! error TS2416:     Type '{ readonly done: false; readonly value: 0; } | { readonly done: true; readonly value: "a string"; }' is not assignable to type 'IteratorResult<number, void>'.
!!! error TS2416:       Type '{ readonly done: true; readonly value: "a string"; }' is not assignable to type 'IteratorResult<number, void>'.
!!! error TS2416:         Type '{ readonly done: true; readonly value: "a string"; }' is not assignable to type 'IteratorReturnResult<void>'.
!!! error TS2416:           Types of property 'value' are incompatible.
!!! error TS2416:             Type 'string' is not assignable to type 'void'.
        if (Math.random() < .5) {
          return { done: false, value: 0 } as const;
        } else {
          return { done: true, value: "a string" } as const;
        }
      }
    }
    
    class BadIterator2 extends Iterator<number> {
      next() {
      ~~~~
!!! error TS2416: Property 'next' in type 'BadIterator2' is not assignable to the same property in base type 'Iterator<number>'.
!!! error TS2416:   Type '() => { done: boolean; value: number; }' is not assignable to type '(value?: undefined) => IteratorResult<number, void>'.
!!! error TS2416:     Type '{ done: boolean; value: number; }' is not assignable to type 'IteratorResult<number, void>'.
!!! error TS2416:       Type '{ done: boolean; value: number; }' is not assignable to type 'IteratorReturnResult<void>'.
!!! error TS2416:         Types of property 'done' are incompatible.
!!! error TS2416:           Type 'boolean' is not assignable to type 'true'.
        return { done: false, value: 0 };
      }
    }
    
    class BadIterator3 extends Iterator<number> {
      next() {
      ~~~~
!!! error TS2416: Property 'next' in type 'BadIterator3' is not assignable to the same property in base type 'Iterator<number>'.
!!! error TS2416:   Type '() => { done: boolean; value: number; } | { done: boolean; value: string; }' is not assignable to type '(value?: undefined) => IteratorResult<number, void>'.
!!! error TS2416:     Type '{ done: boolean; value: number; } | { done: boolean; value: string; }' is not assignable to type 'IteratorResult<number, void>'.
!!! error TS2416:       Type '{ done: boolean; value: number; }' is not assignable to type 'IteratorResult<number, void>'.
!!! error TS2416:         Type '{ done: boolean; value: number; }' is not assignable to type 'IteratorReturnResult<void>'.
!!! error TS2416:           Types of property 'done' are incompatible.
!!! error TS2416:             Type 'boolean' is not assignable to type 'true'.
        if (Math.random() < .5) {
          return { done: false, value: 0 };
        } else {
          return { done: true, value: "a string" };
        }
      }
    }

I see a few approaches to improving this last issue:

  • pass any as the second parameter for IteratorResult in the abstract class Iterator (this fixes the BadIterator1 and BadIterator2 cases)
  • pass unknown as the second parameter for IteratorResult in the above (this fixes the BadIterator1 and BadIterator2 cases)
  • make abstract class Iterator take a second parameter for TReturn, defaulting to void (this fixes the BadIterator1 and BadIterator2 cases if the user explicitly specifies the second parameter in the classes)

@bakkot
Copy link
Contributor Author

bakkot commented Apr 18, 2024

One detail not captured by the types here is that the iterator returned by helpers like map unconditionally has a return method. I guess that's probably worth expressing?

@rbuckton
Copy link
Member

means that classes attempted to implement Iterator have some pain: you can't just return { done: false, value: 0 } from your next because the done value there will be inferred as boolean, which means that the value field has to conform to void so that it can extend the { done: true, value: TReturn } half of IteratorResult<T, void>, which it does not.

We could probably add TReturn = void, TNext = undefined type parameters to the built in Iterator class without issue, since it's up to the user to implement next anyways.

@rbuckton
Copy link
Member

One detail not captured by the types here is that the iterator returned by helpers like map unconditionally has a return method. I guess that's probably worth expressing?

Do helpers like map also have a throw method? If so, you could just use Generator for those. If not, we'd have to add another NativeIterator subtype for those.

@rbuckton
Copy link
Member

Regarding the typing issues, I'm hoping #58243 might help with that.

@bakkot
Copy link
Contributor Author

bakkot commented Apr 19, 2024

Do helpers like map also have a throw method? If so, you could just use Generator for those. If not, we'd have to add another NativeIterator subtype for those.

Nope, just return. throw is really a generator-specific thing.

Suggestions for the name? CloseableBuiltinIterator, maybe, since presence of return indicates that you can close the iterator?

@rbuckton
Copy link
Member

Suggestions for the name? CloseableBuiltinIterator, maybe, since presence of return indicates that you can close the iterator?

I suppose that's fine. @DanielRosenwasser, do you have any thoughts on the name for this type?

@rbuckton
Copy link
Member

Assuming we land #58243 today or tomorrow, I'd like to try to get this PR updated and ready to merge tomorrow as well. In addition to updating from main, there are a few changes we will need to make to align with #58243 and --strictBuiltinIteratorReturn. I'll provide additional feedback once #58243 has merged, or if needed I can directly update this PR with any necessary changes.

@bakkot
Copy link
Contributor Author

bakkot commented Jul 18, 2024

@rbuckton I'm happy either way; if it's easier for you to just push the changes yourself rather than leaving feedback that's fine by me. Otherwise I'll try to get to your feedback asap, though I can't commit to having it done tomorrow.

(Or if it's easier for you to just make a new PR, that's also fine.)

I did the initial update on main, though of course it will need another after #58243.

@rbuckton
Copy link
Member

#58243 has merged. I'm happy to align this PR to get it merged today.

@rbuckton
Copy link
Member

I've made updates to align with the changes in #58243, though it required a few small compromises. If necessary, we can refine this somewhat after its in the beta.

@rbuckton rbuckton marked this pull request as ready for review July 19, 2024 20:29
@typescript-bot
Copy link
Collaborator

The TypeScript team hasn't accepted the linked issue #54481. If you can get it accepted, this PR will have a better chance of being reviewed.

@rbuckton rbuckton merged commit 307ff6c into microsoft:main Jul 19, 2024
32 checks passed
@bakkot bakkot deleted the iterator-helpers branch July 19, 2024 20:52
@rbuckton
Copy link
Member

I realize I neglected to run the user test suite before merging, though I expect it will only call out projects that were already called out by #58243. Running that suite now in case that assumption turns out to be false.

@typescript-bot run dt
@typescript-bot test top400
@typescript-bot test tsserver top100
@typescript-bot user test this
@typescript-bot user test tsserver

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 19, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
run dt ✅ Started
test top400 ✅ Started
test tsserver top100 ✅ Started
user test this ✅ Started
user test tsserver ✅ Started

@rbuckton
Copy link
Member

Also running benchmarks as I expect I need to put up a follow-up PR that adds a BuiltinIterator optimization to the checker like we do for Iterator

@typescript-bot perf test

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 19, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
perf test ✅ Started

rbuckton added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jul 19, 2024
@petamoriken
Copy link
Contributor

Why AsyncBuiltinIterator? I think BuiltinAsyncIterator is a more consistent name.

@petamoriken
Copy link
Contributor

ref: #59388

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PSA: potential lib breaking change after iterator-helpers proposal