Skip to content

Commit 5b65bfb

Browse files
jdeniaujdeniau
committed
fix(List): preserve undefined values when grown past 32 elements
The trie root was stored as `null` in two places (small-List constructor and the "origin within tail" branch of setListBounds), so a List grown past 32 elements while all values are `undefined` read them back as `null`. Normalize both to `undefined`, matching the rest of the file.
1 parent 50bf39e commit 5b65bfb

4 files changed

Lines changed: 135 additions & 6 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,9 @@ Dates are formatted as YYYY-MM-DD.
88

99
## Unreleased
1010

11-
- fix(IndexedCollection): `has(index)` on a lazy `Seq` of unknown size now checks index existence instead of searching for a value equal to the index
12-
- [TypeScript]: `reduce`/`reduceRight` without an initial value now infer the result type from the collection's values when the reducer returns a value (e.g. `list.reduce((a, b) => a + b)` infers `number`), matching `Array#reduce`. Previously an explicit type argument was required.
11+
- fix(List): a `List` grown past 32 elements while all its values are `undefined` no longer reads those values back as `null` (affected `get`, iteration, `toArray`, `equals` and `hashCode`)
12+
13+
- [TypeScript]: `reduce`/`reduceRight` without an initial value now infer the result type from the collection's values when the reducer returns a value (e.g. `list.reduce((a, b) => a + b)` infers `number`), matching `Array#reduce`. Previously an explicit type argument was required. [#2205](https://github.com/immutable-js/immutable-js/pull/2205)
1314

1415
## 5.1.8
1516

@@ -21,10 +22,10 @@ Dates are formatted as YYYY-MM-DD.
2122
- fix(List): guard oversized bounds in setListBounds. Fixes CVE https://github.com/immutable-js/immutable-js/security/advisories/GHSA-v56q-mh7h-f735
2223
- perf(Map): index large hash-collision buckets for faster lookups. Fixes CVE https://github.com/immutable-js/immutable-js/security/advisories/GHSA-xvcm-6775-5m9r
2324

24-
2525
## 5.1.7
2626

2727
- fix(Repeat): lastIndexOf returned size instead of size - 1 by @chatman-media in https://github.com/immutable-js/immutable-js/pull/2227. Fixes CVE [CVE-2026-29063 ](https://github.com/immutable-js/immutable-js/security/advisories/GHSA-wf6x-7x77-mvgw)
28+
- fix(IndexedCollection): `has(index)` on a lazy `Seq` of unknown size now checks index existence instead of searching for a value equal to the index [#2203](https://github.com/immutable-js/immutable-js/pull/2203)
2829

2930
## 5.1.6
3031

‎__tests__/List.ts‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -935,6 +935,100 @@ describe('List', () => {
935935
);
936936
});
937937

938+
// A List grown past 32 elements while every value is `undefined` must read
939+
// back `undefined` at every index, never `null`.
940+
describe('preserves stored undefined (never reads back as null)', () => {
941+
// Asserts every in-bounds slot reads as `undefined` (not `null`) through
942+
// every read path: get, toArray, the values iterator (both directions),
943+
// forEach, and the structural equals / hashCode.
944+
function expectAllUndefined(list: List<number | undefined>, size: number) {
945+
const expected = List<number | undefined>(
946+
new Array(size).fill(undefined)
947+
);
948+
949+
expect(list.size).toBe(size);
950+
for (let i = 0; i < size; i++) {
951+
expect(list.get(i)).toBeUndefined();
952+
}
953+
expect(list.toArray()).toEqual(new Array(size).fill(undefined));
954+
expect([...list.values()]).toEqual(new Array(size).fill(undefined));
955+
expect([...list.toSeq().reverse().values()]).toEqual(
956+
new Array(size).fill(undefined)
957+
);
958+
list.forEach((value) => expect(value).toBeUndefined());
959+
// @ts-expect-error null should not be here
960+
expect(list.includes(null)).toBe(false);
961+
expect(list.equals(expected)).toBe(true);
962+
expect(list.hashCode()).toBe(expected.hashCode());
963+
}
964+
965+
it('setSize(32).setSize(33) — minimal repro', () => {
966+
expectAllUndefined(
967+
List<number | undefined>().setSize(32).setSize(33),
968+
33
969+
);
970+
});
971+
972+
it('setSize(1).setSize(33) — grows a tiny List across the boundary', () => {
973+
expectAllUndefined(List<number | undefined>().setSize(1).setSize(33), 33);
974+
});
975+
976+
it('setSize(31).setSize(33)', () => {
977+
expectAllUndefined(
978+
List<number | undefined>().setSize(31).setSize(33),
979+
33
980+
);
981+
});
982+
983+
it('setSize(32).setSize(65) — undefined region spanning two leaf nodes', () => {
984+
expectAllUndefined(
985+
List<number | undefined>().setSize(32).setSize(65),
986+
65
987+
);
988+
});
989+
990+
it('setSize(32).push(undefined) — grow via push, not setSize', () => {
991+
expectAllUndefined(
992+
List<number | undefined>().setSize(32).push(undefined),
993+
33
994+
);
995+
});
996+
997+
it('writing undefined past index 32 in a mutation', () => {
998+
const list = List<number | undefined>().withMutations((m) => {
999+
for (let i = 0; i < 33; i++) {
1000+
m.set(i, undefined);
1001+
}
1002+
});
1003+
expectAllUndefined(list, 33);
1004+
});
1005+
1006+
// The sequence reported in #2230, reaching the same all-undefined state
1007+
// through unshift/delete/shift rather than setSize.
1008+
it('unshift/delete/setSize/shift sequence (issue #2230)', () => {
1009+
const list = List<number | undefined>()
1010+
.unshift(86)
1011+
.unshift(87)
1012+
.unshift(54) // [54, 87, 86]
1013+
.delete(1) // [54, 86]
1014+
.setSize(6) // [54, 86, undefined, undefined, undefined, undefined]
1015+
.shift()
1016+
.shift() // [undefined, undefined, undefined, undefined]
1017+
.delete(1); // -> [undefined, undefined, undefined]
1018+
1019+
expectAllUndefined(list, 3);
1020+
});
1021+
1022+
// A genuine `null` value is a valid element and must round-trip as `null`,
1023+
// distinct from an absent / undefined slot.
1024+
it('keeps an explicitly stored null as null', () => {
1025+
const list = List<number | null>().setSize(32).setSize(33).set(0, null);
1026+
expect(list.get(0)).toBeNull();
1027+
expect(list.get(1)).toBeUndefined();
1028+
expect(list.toArray()[0]).toBeNull();
1029+
});
1030+
});
1031+
9381032
it('can be efficiently sliced', () => {
9391033
const v1 = Range(0, 2000).toList();
9401034
const v2 = v1.slice(100, -100).toList();

‎eslint.config.mjs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,12 @@ export default tseslintConfig(
144144
'jest/expect-expect': [
145145
'error',
146146
{
147-
assertFunctionNames: ['expect', 'expectIs', 'expectIsNot'],
147+
assertFunctionNames: [
148+
'expect',
149+
'expectIs',
150+
'expectIsNot',
151+
'expectAllUndefined',
152+
],
148153
additionalTestBlockFunctions: [],
149154
},
150155
],

‎src/List.js‎

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ export class List extends IndexedCollection {
4848
assertNotInfinite(size);
4949
if (size > 0 && size < SIZE) {
5050
// eslint-disable-next-line no-constructor-return
51-
return makeList(0, size, SHIFT, null, new VNode(iter.toArray()));
51+
return makeList(0, size, SHIFT, undefined, new VNode(iter.toArray()));
5252
}
5353
// eslint-disable-next-line no-constructor-return
5454
return empty.withMutations((list) => {
@@ -279,7 +279,18 @@ ListPrototype['@@transducer/result'] = function (obj) {
279279
return obj.asImmutable();
280280
};
281281

282+
/**
283+
* A node in the List's 32-wide trie. At inner levels `array` holds child
284+
* `VNode`s; at the leaf level it holds the List's values. Missing slots are
285+
* `undefined` array holes.
286+
*
287+
* @template T
288+
*/
282289
class VNode {
290+
/**
291+
* @param {Array<VNode<T> | T | undefined>} array
292+
* @param {OwnerID} [ownerID]
293+
*/
283294
constructor(array, ownerID) {
284295
this.array = array;
285296
this.ownerID = ownerID;
@@ -417,6 +428,16 @@ function iterateList(list, reverse) {
417428
}
418429
}
419430

431+
/**
432+
* @param {number} origin
433+
* @param {number} capacity
434+
* @param {number} level
435+
* @param {VNode | undefined} [root] The trie root, or `undefined` when every
436+
* in-range value lives in the tail (or is a virtual `undefined`).
437+
* @param {VNode | undefined} [tail]
438+
* @param {OwnerID} [ownerID]
439+
* @param {number} [hash]
440+
*/
420441
function makeList(origin, capacity, level, root, tail, ownerID, hash) {
421442
const list = Object.create(ListPrototype);
422443
list.size = capacity - origin;
@@ -534,6 +555,14 @@ function editableVNode(node, ownerID) {
534555
return new VNode(node ? node.array.slice() : [], ownerID);
535556
}
536557

558+
/**
559+
* Returns the leaf `VNode` holding `rawIndex`, or `undefined` when no node is
560+
* allocated for it (an all-`undefined` region).
561+
*
562+
* @param {List} list
563+
* @param {number} rawIndex
564+
* @returns {VNode | undefined}
565+
*/
537566
function listNodeFor(list, rawIndex) {
538567
if (rawIndex >= getTailOffset(list._capacity)) {
539568
return list._tail;
@@ -677,7 +706,7 @@ function setListBounds(list, begin, end) {
677706
newOrigin -= newTailOffset;
678707
newCapacity -= newTailOffset;
679708
newLevel = SHIFT;
680-
newRoot = null;
709+
newRoot = undefined;
681710
newTail = newTail && newTail.removeBefore(owner, 0, newOrigin);
682711

683712
// Otherwise, if the root has been trimmed, garbage collect.

0 commit comments

Comments
 (0)