proposal-change-array-by-copy

proposal-change-array-by-copy copied to clipboard

Issue to track Stage 3 reviewers feedback

Spec: https://tc39.es/proposal-change-array-by-copy/

Reviewers:

• [x] @ljharb
• [x] @jridgewell
• [x] @chicoxyzzy
• [x] @nicolo-ribaudo

Editors:

• [ ] @bakkot
• [ ] @syg
• [ ] @michaelficarra

Once https://github.com/tc39/ecma262/pull/2606 lands, and then #69 closes #51, my review of this proposal is complete 👍

(altho an editorial note in the spec might be nice to explain why "with" isn't in "unscopables")

Recommendations:

• I think the toSpliced are more complicated than necessary because k is being used to index into both the source and output objects.
  • We could split it into insertion and read indices:

    1. Let _i_ be 0.
    1. Let _r_ be _actualStart__ + _actualDeleteCount_.
    1. Repeat, while _i_ < _actualStart_,
        1. Let _Pi_ be ! ToString(𝔽(_i_)).
        1. Let _iValue_ be ? Get(_O_, _Pi_).
        1. Perform ! CreateDataPropertyOrThrow(_A_, _Pi_, _iValue_).
        1. Set _i_ to _i_ + 1.
    1. For each element _E_ of _items_, do
        1. Let _Pi_ be ! ToString(𝔽(_i_)).
        1. Perform ! CreateDataPropertyOrThrow(_A_, _Pi_, _E_).
        1. Set _i_ to _i_ + 1.
    1. Repeat, while _r_ < _len_,
        1. Let _Pi_ be ! ToString(𝔽(_i_)).
        1. Let _from_ be ! ToString(𝔽(_r_)).
        1. Let _fromValue_ be ? Get(_O_, _from_).
        1. Perform ! CreateDataPropertyOrThrow(_A_, _Pi_, _fromValue_).
        1. Set _i_ to _i_ + 1.
        1. Set _r_ to _r_ + 1.
    

• ~~Add with to @@unscopeables.~~ See https://github.com/tc39/proposal-change-array-by-copy/issues/74#issuecomment-1011548355

Nits:

• Why are the with methods throwing a TypeError instead of clamping?
• I don't think you need to include TypedArraySortCompare, I'm just gonna assume it's the same as the one in ecma262.
• In %TypedArray%.p.toSorted, the Get in step 8.b should be made infallible.
• In %TypedArray%.p.toSpliced
  • move insertCount out of the if conditions, like it's done in Array.p.toSpliced.
  • _insert_Count_ in step 11 has a rouge _ in the middle.
  • The Set in step 15.b should be made infallible.
• In %TypedArray%.p.with, the Set in step 10.d should be made infallible.

@jridgewell with is a keyword and can't be used as a variable name, so makes no sense to add it to @@unscopeables.

Ahhhh.

We could still add it for consistency but it'd serve no purpose; which is why i want an editorial note instead.

Thanks for the initial review comments @jridgewell. Much appreciated. I'll get to work on those.

I split out the with out-of-bounds comment to here: https://github.com/tc39/proposal-change-array-by-copy/issues/75

Hi @ljharb, @jridgewell, @chicoxyzzy

I think #78 is the last known issue from the reviews so far, so hoping we might be able to get sign off once that merges.

The deadline for adding a stage advancement to the March agenda is this Friday 18th, would be great if we could make that.

@acutmore If you're looking to go for stage advancement, it's best to add it ASAP. You can always remove the agenda item before the meeting if you don't get the necessary reviews.

Indeed; please add it to the agenda now; the advancement criteria can be met at any time before the plenary item.

Thanks @michaelficarra, @ljharb . I had misunderstood the process.

Raised PR to attentively add this proposal to the agenda https://github.com/tc39/agendas/pull/1130

Editorially lgtm.

Editorial review:

• Array.prototype.toSpliced step 4 and %TypedArray%.prototype.toSpliced step 5 aren't really needed. The max and min functions are defined over the extended reals (i.e. reals + positive and negative infinities) and behave in the usual way.
• The toSorted methods should be updated to reflect the recent refactoring we did to use Abstract Closures. Sorry for the churn.

Non-editorial questions:

• Same as https://github.com/tc39/proposal-change-array-by-copy/issues/15. Have there been more thoughts on whether this is a problem or not a problem?

LGTM

Thanks @jridgewell !

---

Array.prototype.toSpliced step 4 and %TypedArray%.prototype.toSpliced step 5 aren't really needed. The max and min functions are defined over the extended reals (i.e. reals + positive and negative infinities) and behave in the usual way.

Nice spot @syg, yes that makes sense. Same tidy up applies to existing Array.prototype.splice upstream too. I can raise PRs.

The toSorted methods should be updated to reflect the recent refactoring we did to use Abstract Closures. Sorry for the churn.

This has been done in #78 and is now merged. Do let me know if needs further changes.

Same as https://github.com/tc39/proposal-change-array-by-copy/issues/15. Have there been more thoughts on whether this is a problem or not a problem?

The two potential issues as I see it are:

A) someone wanting to also get the deleted values B) the return type of the function is the same as splice. They both return T[] - but for splice this is a new array of the deleted items, but for toSpliced it returns a new array that is a copy of the original but with the deletions and insertions applied.

For A, I don't think this is an issue in that nothing has actually been deleted (unlike splice). The removed values are still accessible by calling originalArray.slice(start, start + deleteCount).

For B, this is harder to know if it will be an issue. I would like to think that the community would see the pattern from toSorted and toReversed and follow that the toXYZ methods do not mutate the receiver but return a copy of it with the requested modifications, and expect toSpliced to follow that. Rather than expect toSpliced to only return the removed values, effectively making the varargs of inserted values redundant.

Another alternative is for toSpliced to return something like { result: T[], skipped: T[] } or [T[], T[]] - which sounds like a more difficult API to teach as it does not follow an existing pattern set by existing array methods. Or we could drop toSpliced altogether to avoid this issue, though one reason for its inclusion is that it can be used to implement some of the other mutating array methods which are not included in this proposal e.g. toUnshifted. Personally I like the inclusion of toSpliced because it may help people learn the difference between splice and slice. In editors with auto-complete, typing splice would likely also show toSpliced as an option, which should be a hint that splice is mutating.

The removed values are still accessible by calling originalArray.splice(start, start + deleteCount).

.slice, not .splice :-p

Once tc39/ecma262#2606 lands, and then #69 closes #51, my review of this proposal is complete 👍

Hey @ljharb, now that #51 has closed do you consider your review of the latest spec complete?

@acutmore yep! i checked my box.

@acutmore yep! i checked my box.

Ta very much! 😀

LGTM

LGTM

Thanks @chicoxyzzy! Much appreciated