reviewing

[devsnek]

cc @michaelficarra @dtribble

this isn't quite ready for stage 3 yet (i need to review some error behaviour), but i wanted to let y'all know pretty much all the semantics are laid out at this point. if you have any insights/criticism/etc please feel free to share them and i will try to address them when i can (i'm starting university over the next few weeks so things will be busy).

[c69]

do you plan to implement this proposal as (proof of concept) npm package ?

[devsnek]

it's already been implemented in core-js so I don't think making a separate package is needed.

[ljharb]

core-js isn't the entirety of the shim ecosystem - separate packages are absolutely needed - but I'll be making those once the proposal's stage 3.

[michaelficarra]

@devsnek It seems that flatMap is currently flattening iterables instead of iterators. To comply with the philosophy that the committee agreed upon when approving Array.prototype.flatMap (X.prototype.flatMap flattens Xs), we only want to flatten iterators. I'm not sure what the proper way to distinguish those is, though. Another concern is the observability of the Get, which is a problem we did not have with the Array.prototype.flatMap implementation since it uses IsArray.

[devsnek]

the real issue is that we can't just flatten anything that happens to have a next method. I discussed this a bit in the other issue, but really Symbol.iterator is the correct interface to use here. Do you have more information on that philosophy? I feel like the stdlib is small enough that we don't necessarily have these patterns yet.

[bergus]

@michaelficarra Could you link the discussion of that philosophy, please? I can't find it in the tc39-notes.
And you might want to revisit that. Any other OOP language I am familiar with (not the functional ones that have a strictly typed monad) implements flatMap more pragmatically, so that X.prototype.flatMap can flatten any container into an X. If people return strings from flatMap, it's their fault that they returned a string container. Iterable is the right interface to use for arbitrary containers, and it makes testing for the interface easy (throwing an error in case it's not iterable).

[michaelficarra]

@devsnek @bergus Not really additional context, but here's the mentions in the notes:

• https://github.com/tc39/tc39-notes/blob/master/meetings/2017-09/sept-28.md#15i-flatmap-for-stage-3
• https://github.com/tc39/tc39-notes/blob/master/meetings/2017-11/nov-28.md#10ia-arrayprototypeflatten--arrayprototypeflatmap-for-stage-3

@domenic, @bterlson, @ljharb, and I were the major participants in that decision.

If people return strings from flatMap, it's their fault that they returned a string container

This is the argument that was used to support the iterables stance for Array.prototype.flatMap and was largely rejected as impractically strict by the committee. Personally, I prefer the API to be more strict, but that does not fit well with general JavaScript built-in API design.

so that X.prototype.flatMap can flatten any container into an X

Could you elaborate? That doesn't make sense to me. What's the API for enumerating the contents of arbitrary "container"s? Or are you just talking about values that support some iteration API?

[c69]

@michaelficarra can you maybe share a minimal strawperson usage code of such version of flatMap to illustrate what do you mean ?

[michaelficarra]

@c69 usage of what?

[c69]

A sample of how a user would call a flatMap on a iterator and what that will return.

edit:
because it seems that you are suggesting

[1,2,3].values().flatMap(v => [v, v*v].values())

i.e.:

Iterator.flatMap( fn: (v: any,i) => Iterator|any)

with a check where Symbol.Iterator for return value exists in prototype chain, and if not, then treat it as if it was 1-element long vanilla iterator.

[michaelficarra]

@c69 This proposal currently adds a flatMap to %Iterator.prototype%. The issue I was raising is how that version of flatMap handles the values returned by the mapper function given as its parameter. In an ideal world, the mapper would be required to return values of type X when given to X.prototype.flatMap. But since we can't guarantee that, we assume that the mapper can return any kind of value and we need to have a defined behaviour for handling non-Xs. Array.prototype.flatMap distinguishes flattenable things from non-flattenable things in a non-observable way (IsArray) and handles non-flattenable things as if they were implicitly wrapped in a singleton array (thus becoming flattenable). This is a pretty decent trade-off. The version of flatMap currently proposed in this repo distinguishes flattenable things from non-flattenable things in an observable way (thing[Symbol.iterator]), which is not great, and also may have a problematic definition of what is flattenable for people who are intentionally/accidentally taking advantage of the non-flattenable behaviour. So I'm bringing up 2 concerns:

1. Is there something we can do that's non-observable to determine whether we are in the flattenable/non-flattenable case? Are implementers going to be okay with an observable test here?
2. Are we sure that the borders we've drawn for the flattenable identification are the ones we want, particularly given the precedent of Array.prototype.flatMap?

I did not suggest I knew the answers to these questions. @devsnek suggests that we've done the best we can on the latter question.

[devsnek]

if it makes you feel better, the most common variant of Symbol.iterator is return this

[ljharb]

Iterators are always iterable by convention (although not by contract) - this feels like a simple case where we can maximize utility without forcing users to always add an Iterator.from wrapper.

[bergus]

@michaelficarra Thanks, I must have glanced over that. I always thought that supporting the heterogenous case and non-flattened string values was mostly useful for arbitrary-depth flatten, but from reading tc39/proposal-flatMap#8 it seems like it was also requested for flatMap. 😞
I'd rather see a better distinction between map and flatMap, it seems like a missed opportunity to me.

What's the API for enumerating the contents of arbitrary "container"s? Or are you just talking about values that support some iteration API?

Yes, I was referring to some enumeration interface on the value. For example, Scala's List and Iterator flatMaps only require IterableOnce, or Rust's Iterator flat_map only requires IntoIterator.

On the other hand, Java's Stream flatMap also requires a Stream as the callback return type. My personal experience with that: I was constantly annoyed by the verbosity of having to append .stream() every time I wanted to return a collection. (I'm trying to avoid Java where I can).
Without a type system or at least a runtime error, I would predict many forgotten-conversion mistakes in JavaScript.

[devsnek]

perhaps we should just push flatMap to another proposal? this seems like a rather complex interface issue.

[michaelficarra]

My biggest concern is on the optimisability of flatMap given the observability of the test. If engines say they're okay with implementing as-is, I am fine going forward with using iterable as the discriminator.

[devsnek]

I don't see any problems with the optimisability of that, just a standard IC at the callback site, although my knowledge of js optimisation is limited to V8.

[c69]

Scala (2.13) solves this quite elegantly, imho..

• Iterator.flatMap callback needs to return an Iterable
  • which is the same (or very similar) behavior that we have in current version of spec draft.

Yes, this means that our method becomes somewhat "less lazy", but in fact its only the processing of one element in the source iterator that creates an eager computation spike.

https://www.scala-lang.org/api/current/scala/collection/Iterator.html#flatMap[B](f:A=%3Escala.collection.IterableOnce[B]):Iterator[B]

p.s.: nice quote from the same page - "one should never use an iterator after calling a method on it" [except .next and .hasNext].

[michaelficarra]

Now that I'm co-championing this proposal, we are short on reviewers. @bakkot has volunteered to be a second reviewer.

@dtribble Ping. We require your sign-off to push this proposal to stage 3.

[devsnek]

I emailed Dean a while ago and haven't heard anything back. I think we might just need to get a different reviewer.

[c69]

Any update on the review ?

[codehag]

I believe we are getting close to stage 3 here, but I don't know if the reviewers are still active. Should we ask for new reviewers at the next meeting?

[michaelficarra]

@codehag I think we should.

[michaelficarra]

Tracking reviews (along with other stage 3 entrance criteria) in #117 now.