Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Why does 'with' throw on out-of-bounds access #75

Closed
acutmore opened this issue Jan 14, 2022 · 15 comments
Closed

Why does 'with' throw on out-of-bounds access #75

acutmore opened this issue Jan 14, 2022 · 15 comments
Labels
question Further information is requested
Milestone

Comments

@acutmore
Copy link
Collaborator

acutmore commented Jan 14, 2022

Opening issue to discuss a point @jridgewell raised here.

Why are the with methods throwing [snip] instead of clamping?

[0].with(99, 1); // RangeError!

[].with(-1, 0); // RangeError!
@acutmore acutmore mentioned this issue Jan 14, 2022
7 tasks
@acutmore acutmore added this to the Stage 3 milestone Jan 14, 2022
@acutmore acutmore added the question Further information is requested label Jan 14, 2022
@ljharb
Copy link
Member

ljharb commented Jan 14, 2022

I'm not sure I understand. They do throw a RangeError, not a TypeError - in step 6 of https://tc39.es/proposal-change-array-by-copy/#sec-array.prototype.with.

Is the question why they don't cycle around, at/slice-style?

@acutmore
Copy link
Collaborator Author

acutmore commented Jan 14, 2022

Related data points:

Array.prototype.at doesn't throw on out-of-bounds access.

[0].at(99); // undefined

ImmutableJS List.prototype.set increases the size of the created list to incoporate an out-of-bounds access. Filling in with undefined.

[...Immutable.List([]).set(3, 'v')]; // [undefined, undefined, undefined, 'v']
[...Immutable.List([]).set(-1, 'v')]; // ['v']

@jridgewell
Copy link
Member

They do throw a RangeError, not a TypeError… Is the question why they don't cycle around, at/slice-style?

My mistake, but the intention is why do they throw an error at all? Arrays in particular are super sloppy, I can't think of another method that throws. Usually, they clamp the value to be between [0, array.length), and continue performing the operation with the new index. Eg, toSpliced clamps the start and deleteCount params without throwing.

@acutmore
Copy link
Collaborator Author

acutmore commented Jan 14, 2022

Some alternative options:

// A) Ignore
[0].with(5, 'v'); // [0]

// B) Clamp
[0].with(5, 'v'); // ['v']

// C) Expand
[0].with(5, 'v'); // [0, undefined, undefined, undefined, undefined, 'v']
[0].with(-5, 'v'); // ['v'] // still clamp Math.max(0, index)

@ljharb

This comment has been minimized.

@acutmore
Copy link
Collaborator Author

Thanks! yes. Corrected that now 😅

Another data point: TypedArray.prototype.set throws a RangeError

(new Uint8Array(1)).set([], 99); // RangeError!

@ljharb
Copy link
Member

ljharb commented Jan 14, 2022

There’s really not another method like with on arrays or typed arrays anywhere to compare with. I think it’s different enough from the other array methods that it would be fine for it to be slightly different.

Is there any non=consistency reason why the current behavior isn’t preferred?

@jridgewell
Copy link
Member

  • Consistency with regular assignment (Array expands if index is larger, TypedArray silently ignores)
  • Eases replace-or-append operation (eg index == arr.length ? arr.toSpliced(index, 0, item) : arr.with(index, item) can just be arr.with(index, item))

@ljharb
Copy link
Member

ljharb commented Jan 14, 2022

that’s == - what about if it’s larger? like the length is 2 and index is 5.

@jridgewell
Copy link
Member

Expanding behavior would make the most sense to me. Clamping to length - 1 seems strange, and also strange if it clamped to length.

I'm not saying we need to change the behavior, it was just something I noticed during my review.

@ljharb
Copy link
Member

ljharb commented Jan 14, 2022

So there's these options:

  • current behavior: throw a RangeError when an index isn't < length and >= 0.
  • do like at and slice does, accept all numbers, and "handle" an index from -length to length - 1 - so in this case, something length or higher would expand the array, filling in holes with undefined; something less than -length would what, unshift, filling holes with undefined?

I'm not sure what else there would be. silently clamping seems like it'd cause lots of bugs.

@jridgewell
Copy link
Member

something less than -length would what, unshift, filling holes with undefined?

Probably just be ignored, and return a new array slice.

@acutmore
Copy link
Collaborator Author

acutmore commented Jan 15, 2022

I'm not saying we need to change the behavior, it was just something I noticed during my review.

I appreciate you bringing it up, good to ensure these things are discussed rather than just silently slip into the spec.

The idea of .with was to be the immutable replacement of index assignment.

So it could make sense to match Array's behavior of expanding the length when assignment beyond the end, but filling with undefined not holes.

That said, it does feel like the existing behaviour of having a RangeError would help find more off-by-one bugs. If someone does want the expanding behaviour they can still use array index assignment.

const a = [0, 1];
const a2 = [...a];
a2[3] = 3;
a2; // [0, 1, empty, 3]
const a3 = [...a2]
a3; // [0, 1, undefined, 3]

@acutmore
Copy link
Collaborator Author

acutmore commented Jan 15, 2022

Eases replace-or-append operation (eg index == arr.length ? arr.toSpliced(index, 0, item) : arr.with(index, item) can just be arr.with(index, item))

I think that code would be the same as

arr.toSpliced(/* at: */ index, /* delete: */ 1, /* insert: */ item);
let a = ['a', 'b', 'c'];

a.toSpliced(0, 1, 'v'); // ['v', 'b', 'c']
a.toSpliced(1, 1, 'v'); // ['a', 'v', 'c']
a.toSpliced(2, 1, 'v'); // ['a', 'b', 'v']

a.toSpliced(a.length, 1, 'v'); // ['a', 'b', 'c', 'v']
a.toSpliced(99, 1, 'v');       // ['a', 'b', 'c', 'v']

a.toSpliced(-1, 1, 'v');  // ['a', 'b', 'v']
a.toSpliced(-99, 1, 'v'); // ['v', 'b', 'c']

@acutmore
Copy link
Collaborator Author

I'm going to close this issue, based on my assessment of this design in #75 (comment) and #75 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants