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

built-in iterators should be disposable #59633

Merged
merged 2 commits into from
Aug 16, 2024
Merged

Conversation

rbuckton
Copy link
Member

This updates the definitions for IteratorObject and AsyncIteratorObject to be disposable, per the spec:

It should be noted that %IteratorPrototype% is synonymous with Iterator.prototype per the Iterator Helpers proposal and is represented by IteratorObject in TypeScript, and that %AsyncIteratorPrototype% is synonymous with AsyncIterator.prototype per the Async Iterator Helpers proposal and is represented by AsyncIteratorObject in TypeScript.

Fixes #59263

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Aug 14, 2024
@rbuckton rbuckton requested review from weswigham and sandersn August 14, 2024 18:14
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I like the code change, but I had a couple of questions about the tests.

Comment on lines +19 to +22
using it3 = new MyIterator();
using it4 = [].values();
using it5 = new Map<string, string>().entries();
using it6 = new Set<string>().keys();
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why these succeed when using it7 = i fails. They're all Iterators too. Where do they get a [Symbol.dispose] method?

Copy link
Member Author

Choose a reason for hiding this comment

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

it7 is illustrating that the Iterator interface (which only describes a minimal Iterator compatible with for..of) does not implement Symbol.dispose. Only iterators that inherit from the IteratorObject interface (which represents the global Iterator.prototype value) will have a Symbol.dispose by default.

Iterators returned from values()/keys()/entries() on Map/Set/Array/etc. will inherit from IteratorObject and thus are disposable.

@@ -1874,4 +1874,140 @@ describe("unittests:: evaluation:: awaitUsingDeclarations", () => {
"catch",
]);
});

it("deterministic collapse of Await", async () => {
const { main, output } = evaluator.evaluateTypeScript(
Copy link
Member

Choose a reason for hiding this comment

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

Are these tests of existing functionality? I don't see how changing the type of [Async]IteratorObject would change its runtime behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

This test was in the wrong test suite. I merely moved while adding other tests.

@rbuckton rbuckton merged commit f025a5b into main Aug 16, 2024
32 checks passed
@rbuckton rbuckton deleted the disposable-iterator-objects branch August 16, 2024 15:57
@trevorade
Copy link

I'm seeing some build errors that are related to this:

image

Rather than,

interface IteratorObject<T, TReturn, TNext> extends Disposable {
}

Should it have been:

interface Iterator<T, TReturn, TNext> extends Disposable {
}

IteratorObject extends Iterator so the mismatch of IteratorObject being disposable but Iterator not being disposable causes compiler errors in other locations.

@jakebailey
Copy link
Member

jakebailey commented Aug 26, 2024

No, it being on the interface isn't correct, I think you just need to update your @types/node to pick up fixes we made (specifically, removing the redeclaration of entries).

@trevorade
Copy link

trevorade commented Aug 26, 2024

Thx for the guidance!

For anyone else looking into this, the referenced PR is: DefinitelyTyped/DefinitelyTyped@8c36e53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Iterator and AsyncIterator missing Symbol.dispose method in esnext.disposable
5 participants