stage 2.7 tracking - Githubissues

michaelficarra commented 8 months ago

[X] complete spec text
[X] updated polyfill
[ ] assigned reviewer sign-off
- [x] @nicolo-ribaudo
- [x] @ljharb
[ ] editor sign-off
- [ ] @syg
- [ ] @bakkot
- [ ] @michaelficarra

API name choices (#9) can be finalised at plenary during advancement IMO.

nicolo-ribaudo commented 8 months ago

Do you plan to ask for 2.7 at this meeting?

michaelficarra commented 8 months ago

@nicolo-ribaudo I was not planning on it, but if I get reviews really soon and the stars align, I could. The deadline for advancement is only a week away.

ljharb commented 8 months ago

As a reviewer:

in both zipToArrays and zipToObjects, step 8b: b. Set paddingOption to ? [Get](https://tc39.es/ecma262/#sec-get-o-p)(options, "padding"). this should probably use GetOption, for consistency.
IteratorZipCore : why does this return a Generator? shouldn't it return the same kind of thing iterator helpers do?
in IteratorCloseAll, only the 0th completion is unpacked, meaning that if the others are abrupt, their exceptions are swallowed. if the desire is to close them all, then i would assume that if any are abrupt, an AggregateError is created that contains all of the abrupt completion values, so information is not lost.

In #9 I prefer zip instead of zipToArrays, and my strong need for #1 has been expressed in plenary as well as in the repo, but those are outside the scope of being a reviewer.

michaelficarra commented 8 months ago

this should probably use GetOption, for consistency

GetOption handles coercing to either Boolean or String. Padding can be anything, so we can't use it. I guess we could add a non-coercing option or somehow otherwise refactor it, but then we'd have to coordinate with ecma402 since it's just ripped straight from there.

why does this return a Generator? shouldn't it return the same kind of thing iterator helpers do?

The iterator helpers do return a Generator through CreateIteratorFromClosure.

in IteratorCloseAll, only the 0th completion is unpacked, meaning that if the others are abrupt

There's only one completion record passed to this AO. I'm confused by this feedback.

I prefer zip instead of zipToArrays

Yeah I have no opinion on it and I'm just gonna leave it up to committee.

my strong need for #1 has been expressed in plenary as well as in the repo

I had (possibly mistakenly) thought we settled that question at the last plenary. I will ask for confirmation that we do not tie that request to this proposal at the next plenary, regardless of whether I go for advancement. I think it's fine to pursue it as a separate proposal if you want, but I think it's too burdensome to ask for it to be included with this one.

ljharb commented 8 months ago

There's only one completion record passed to this AO. I'm confused by this feedback.

i'm referring to step 2.a of https://tc39.es/proposal-joint-iteration/#sec-closeall - you are creating N (list length) completion records from each IteratorClose call, but only the 0th is preserved.

bakkot commented 8 months ago

Ah, yeah, exceptions which arise when closing iterators get swallowed when there's already an exception in play - that's how IteratorClose works, anyway. Compare

let throwyClose = {
  [Symbol.iterator]: () => ({
    next: () => ({ done: false }),
    return: () => {
      console.log('this does get triggered');
      throw 0;
    },
  }),
};

for (let item of throwyClose) throw 1;

which prints "this does get triggered" but throws 1.

bakkot commented 8 months ago

The using proposal introduces SuppressedError for these cases, which... I guess we could use? But it would be a break from precedent.

michaelficarra commented 8 months ago

I guess we could use? But it would be a break from precedent.

I'd prefer iterator stuff to at least be internally consistent without a pretty good reason to deviate.

ljharb commented 8 months ago

alrighty, if there's precedent then it's clearly fine as-is.

I think it'd be nice to refactor GetOption so it can work for all options, but obv that's editorial and doesn't need to be part of, nor block, this proposal. Either way once GetOption lands in 262 it'll surely be removed from 402, so they'd presumably just use our version.

In that case, consider me signed off.

michaelficarra commented 8 months ago

Yeah I definitely would not have written GetOption that way had I written it from scratch. I'm sure we'll try to refactor it when we pull it in.

michaelficarra commented 5 months ago

@nicolo-ribaudo Still waiting on your review here. I'd like to propose this for advancement at the upcoming meeting.

nicolo-ribaudo commented 5 months ago

I'll work on it this weekend 👍

ljharb commented 5 months ago

@michaelficarra where are we at about array zipping? should i make another proposal and also ask for 2 or 2.7, since the semantics are basically locked in by this one, or should I make a PR to add Array.zip to this proposal?

michaelficarra commented 5 months ago

@ljharb https://docs.google.com/presentation/d/1Qj5h6MajJnji1obZsXea_cUgfwxur-yT6v-8rBTLqtg/edit#slide=id.g272a2848ddd_0_61. @syg does not seem to have been convinced in #1, and his arguments are starting to move me from neutral to slightly negative as well.

nicolo-ribaudo commented 5 months ago

LGTM ✅ I only one comment on making it easier to read the algorithm, everything else looks good.

[ ] If we pass longest: true and strict: true, all the iteratables must have the same length but we still read the padding option, even if we don't actually need it. Is this intentional? Maybe we should avoid reading it, or throw an error when defining both longest: true and strict: true. EDIT: I should have just re-read the algorithm.
[x] When mode is STRICT padding is an empty list. Can step 3.b.iii.1.a of IteratorZipCore be reached? I think no because we only ever put null in iters in the LONGEST case, but an assertion "Assert: mode is LONGEST" would make it easier to understand.

syg commented 5 months ago

Review

High-level bikeshedding

I find both the ToArrays and ToObjects name confusing. ToArrays iterates its iterables argument, then zips into a result Array. So it's like "gather input iterables by iterating argument, zip those iterables into an array per-step". ToObjects gets enumerable own properties from its iterables argument then zips into an object mapping the own keys. So it's like "gather input iterables by own for-in, zip those iterables into an object with the same keys". It'd be nice to reflect that the "gather input iterables" step is pretty different for both, since the "to" suffix suggests the difference is only in the output.

Options bag handling

It's not true that this is the first API that takes an options bag. Resizable buffers added an options bag to the ArrayBuffer and SharedArrayBuffer constructors which takes a maxByteLength option. The difference between that and the 402-style options bag is that non-Object values passed for the options bag cause ~empty~ to be returned for the option instead of throwing a TypeError.
Why have two mutually exclusive boolean options shortest and strict? Seems better to have a mode that can be "shortest", "longest", or "strict".
In the spirit of not coercing things, it'd be nice if string options threw on non-strings instead of ToString'ing them.

`zipToArrays`

We don't have the form "iv. Perform the following steps iterCount times:", I don't think?

`zipToObjects`

Step 17.b, missing "in ascending order".

IteratorZipCore

Core is an unusual suffix. Maybe just IteratorZip.

`%IteratorHelperPrototype%.return`

Needs a mutadis mutandis for the other iterator helpers?

michaelficarra commented 5 months ago

The difference between that and the 402-style options bag is that non-Object values passed for the options bag cause ~empty~ to be returned for the option instead of throwing a TypeError.

I would think our newly agreed normative conventions would want us to TypeError here, right?

Why have two mutually exclusive boolean options shortest and strict? Seems better to have a mode that can be "shortest", "longest", or "strict".

I went back and forth on this a lot. I also do not like the mutual exclusivity. Using 4 states to represent 3 puts a bad taste in my mouth. But using a string expands that to an infinite space to represent 3 states. We could make them constant fields on Iterator or something, but then that just adds a layer of indirection from a string. So unfortunately I think this is the best interface we can do.

In the spirit of not coercing things, it'd be nice if string options threw on non-strings instead of ToString'ing them.

We don't have any string options, so this is moot. I think we can defer the editorial decision for what we do about GetOption for now.

We don't have the form "iv. Perform the following steps iterCount times:", I don't think?

We don't, but we do have "Perform the following steps:", so it seems like a natural extension. IIRC @bakkot also wanted to use this form in one of his proposals?

Step 17.b, missing "in ascending order".

Done.

Core is an unusual suffix. Maybe just IteratorZip.

Done.

Needs a mutadis mutandis for the other iterator helpers?

Yes, but I didn't think that kind of elaboration about another proposal was necessary in this proposal. It's just a straightforward integration thing.

bakkot commented 5 months ago

We don't, but we do have "Perform the following steps:", so it seems like a natural extension. IIRC @bakkot also wanted to use this form in one of his proposals?

Not that I recall, but possibly?

We did discuss the option of "Repeat n times:".

michaelficarra commented 5 months ago

We did discuss the option of "Repeat n times:".

Yeah that's almost certainly what I am remembering.

syg commented 5 months ago

I would think our newly agreed normative conventions would want us to TypeError here, right?

Yeah, that's fair.

I went back and forth on this a lot. I also do not like the mutual exclusivity. Using 4 states to represent 3

I think I don't understand the 4-to-3.

michaelficarra commented 5 months ago

2 Booleans can represent 4 states (and only ever 4 states). We're trying to use them to represent something that has 3 states. So we need to check for that 4th invalid state and throw. That's not a great interface.

syg commented 5 months ago

Ah I thuoght the 4-to-3 thing was talking about the enum. I don't see what the problem with using strings is. 402 has it all over the place.

michaelficarra commented 5 months ago

I don't think we should be taking API inspiration from 402, sorry.

syg commented 5 months ago

It's just a very normal thing to have string constants for enum values...? I really don't understand the objection.

michaelficarra commented 5 months ago

I will add a slide so we can discuss it in plenary. I don't object, I'm just not convinced it's any better.

michaelficarra commented 5 months ago

This proposal is now at Stage 2.7 https://github.com/tc39/proposal-joint-iteration/commit/41031bf232e011237be22d09aa6a9808433cd107

tc39 / proposal-joint-iteration

stage 2.7 tracking #18

High-level bikeshedding

Options bag handling

`zipToArrays`

`zipToObjects`

IteratorZipCore

`%IteratorHelperPrototype%.return`