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

flatMap should act like it does a yield * on each iterable #114

Closed
jorendorff opened this issue Jul 16, 2020 · 3 comments · Fixed by #194
Closed

flatMap should act like it does a yield * on each iterable #114

jorendorff opened this issue Jul 16, 2020 · 3 comments · Fixed by #194

Comments

@jorendorff
Copy link
Collaborator

jorendorff commented Jul 16, 2020

Currently Iterator.prototype.flatMap is specified roughly like this:

*flatMap(mapper) {
  for (let value of {[@@Iterator]: () => this}) {
    for (let innerValue of mapper(value)) {
      yield innerValue;
    }
  }
}

Arguably it should be more like this:

*flatMap(mapper) {
  for (let value of {[@@Iterator]: () => this}) {
    yield* mapper(value);
  }
}

This changes two details of behavior:

  • It causes the flatMap iterator helper's .throw(exc) method to forward the exception along to the inner iterator (consistent with other iterator helpers). Currently the inner iterator is instead closed, and exc is then thrown again.
  • It causes the flatMap iterator helper's .next(value) method to forward the value along to the inner iterator. Currently the inner iterator's .next() method is always called with no arguments.
@Jack-Works
Copy link
Member

Jack-Works commented Jul 17, 2020

*flatMap(mapper) {
  for (let value of {[@@Iterator]: () => this}) {
    yield* mapper(value);
  }
}

Let's assume this is an Iterable<T> while T is Iterable<Q>.

The current behavior is to call the mapper with the value Q but your suggested version is calling mapper with T which I think is not correct.

The correct semantics should be

*flatMap(mapper) {
  for (let value of {[@@Iterator]: () => this}) {
    yield* Iterator.from(value).map(mapper);
  }
}

@conartist6
Copy link

conartist6 commented Dec 22, 2020

@Jack-Works I think the current version is indeed calling mapper<T> not mapper<Q>. That would be in line with what Array.prototype.flatMap does at least.

@bakkot
Copy link
Collaborator

bakkot commented Jul 7, 2022

In #194 we went the other direction: now no iterator helpers act like they do yield *, because that's a generator thing rather than an iterator thing.

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

Successfully merging a pull request may close this issue.

4 participants