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

Improve generic type signature of Iterator.from() #59927

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

nikolaybotev
Copy link

@nikolaybotev nikolaybotev commented Sep 10, 2024

Fixes #59926

Background

Iterator.from returns an iterator that passes along the return value of its wrapped iterator, but does NOT pass along the argument to its next method to the wrapped iterator. This behavior is observed in Node.js 22 (and other implementations, see below) using the following scripts and I assume is the standard behavior:

Conformance

core-js

❯ node -p 'console.log(process.version, typeof Iterator); require("core-js/stage/3"); Iterator.from(function* () { return 42; }()).next();'
v20.17.0 undefined
{ value: 42, done: true }

node.js 22

❯ node -p 'console.log(process.version, typeof Iterator); Iterator.from(function* () { return 42; }()).next();'         
v22.0.0 function
{ value: 42, done: true }

Firefox

navigator.userAgent
"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:132.0) Gecko/20100101 Firefox/132.0"
Iterator.from(function* () { return 42; }()).next()
Object { value: 42, done: true }

Opera

navigator.userAgent
'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/127.0.0.0 Safari/537.36 OPR/113.0.0.0'
Iterator.from(function* () { return 42; }()).next()
{value: 42, done: true}

Problem

The current type signature for Iterator.from basically ignores the TReturn type argument of the underlying Iterator protocol, which restrict certain valid code from passing typescript compilation, such as this example:

Change

  • Add the TReturn type argument to Iterator.from and pass it through from the argument to the return type so as to reflect the observed behavior of the Iterator.from implementation.
  • Update the baselines for tests affected by this change.
  • For consistency, rename the Iterator class' TResult type argument to TReturn.

Tests

  • Tested manually using the script linked above under the Problem section.
  • Added test code in tests/cases/compiler/builtinIterator.ts to cover the scenario of the iterator returned by Iterator.from passing along the return value of the wrapped iterator.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Sep 10, 2024
@nikolaybotev
Copy link
Author

nikolaybotev commented Sep 10, 2024

I am looking into the 4 tests errors.

The first two errors relate to expected errors and types/symbols in tests/cases/compiler/builtinIterator.ts. With the changes here, the following code from that file now compiles without errors:

declare const g1: Generator<string, number, boolean>;
const iter1 = Iterator.from(g1);

I believe this is the correct behavior!

I do not know how to update the test baseline to reflect this new behavior introduced here.

I am going to look into the other two errors and report here in a subsequent comment.

@nikolaybotev
Copy link
Author

nikolaybotev commented Sep 10, 2024

Regarding the first 2 errors, here is a script that demonstrates that the code that now compiles (and fails the tests), should indeed compile, and runs as expected in Node.js 22:

const g1: Generator<string, number, boolean> = function* g1gen() {
    yield "hello";

    return 42;
}();
const iter1 = Iterator.from(g1);

console.log(iter1.next()); // { value: 'hello', done: false }
console.log(iter1.next()); // { value: 42, done: true }

https://github.com/nikolaybotev/iteratorhelpersdemo/blob/main/problems/ts/tests-cases-compiler-builtinIterator.ts

@nikolaybotev
Copy link
Author

nikolaybotev commented Sep 10, 2024

The other two errors are about Correct type/symbol baselines for

  • tests/cases/conformance/statements/VariableStatements/usingDeclarations/awaitUsingDeclarationsWithIteratorObject.ts
  • tests/cases/conformance/statements/VariableStatements/usingDeclarations/usingDeclarationsWithIteratorObject.ts

Those two files contain calls to Iterator.from and I believe the baseline should be updated to reflect the new return type of Iterator.from.

Again, I do not (yet) know how to go about doing this.

I would be glad to take it on give some pointers in the right direction.

@nikolaybotev
Copy link
Author

@microsoft-github-policy-service agree

@nikolaybotev
Copy link
Author

Done. I have updated the baselines.

@nikolaybotev
Copy link
Author

Now going to see if I can add the two test scenarios linked in the description of this PR as new tests.

@nikolaybotev
Copy link
Author

@microsoft-github-policy-service agree

@nikolaybotev
Copy link
Author

Tests added.

@nikolaybotev
Copy link
Author

nikolaybotev commented Sep 10, 2024

I just realized my mistake in expecting Iterator.from to accept iterators that expect a next argument. This is not type-safe. I have reverted that part of the change and am updating the PR and issue descriptions.

@nikolaybotev
Copy link
Author

This PR is now ready for review.

@nikolaybotev
Copy link
Author

nikolaybotev commented Sep 11, 2024

I am now realizing that the passing along of the wrapped iterator's return value by the object returned by Iterator.from is very likely an implementation optimization side-effect and not intended behavior by design.

NOT passing along the value argument to next() is achieved by removing one argument to one function call.

NOT passing along the return value of return() requires checking and unwrapping the IteratorResult object if done is true... adding unnecessary overhead.

Iterator Helpers really operate on one-way streams and the one-way stream aspect of Iterators and Generators is really best captured by their Iterator<T, undefined, undefined> specialization, and as such this is what the baseline Iterator.from signature appears to reflect -- the design intent, not any underlying implementation side effects, which can (and will be 99.99% of the time) be ignored by users.

The Iterator<T, unknown, undefined> and Iterator<T, undefined, unknown> variations make the protocol more permissive for the user without sacrificing correctness: the first one is used in the argument position to tell the client - give me an Iterator that takes no arguments in its next method (because I am not going to send any), but can return whatever IteratorReturnResult it wants because I am going to ignore it anyway. Conversely, the other variant is used in the return position to say - I am giving you an Iterator object, that you can pass anything you want to its next method as I am going to ignore it, and you are never going to get any value in IteratorReturnResult back (well you might, but that's an implementation optimization side-effect and you can ignore it and should not expect it or rely on it).

Can someone confirm my reading is correct?

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
Status: Not started
Development

Successfully merging this pull request may close these issues.

Improve Iterator Helper Type Signatures
2 participants