Skip to content

Commit c3dc829

Browse files
RealColdFryRealColdFry
authored
fix Set iterator not reflecting mutations after creation (#1691)
* fix Set iterator not reflecting mutations after creation (#1671) Set.values(), keys(), and entries() eagerly captured firstKey at iterator creation time. If the Set was mutated (e.g. via delete) before the iterator was consumed, the iterator still saw the stale firstKey. Read firstKey lazily on the first next() call instead. * Add tests for Set iterator mutation and exhaustion behavior * Fix Map iterator mutation and add tests for mutation and exhaustion behavior
1 parent cfb6e2b commit c3dc829

File tree

4 files changed

+140
-24
lines changed

4 files changed

+140
-24
lines changed

‎src/lualib/Map.ts‎

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -113,46 +113,64 @@ export class Map<K extends AnyNotNil, V> {
113113
}
114114

115115
public entries(): IterableIterator<[K, V]> {
116+
const getFirstKey = () => this.firstKey;
116117
const { items, nextKey } = this;
117-
let key = this.firstKey;
118+
let key: K | undefined;
119+
let started = false;
118120
return {
119121
[Symbol.iterator](): IterableIterator<[K, V]> {
120122
return this;
121123
},
122124
next(): IteratorResult<[K, V]> {
123-
const result = { done: !key, value: [key, items.get(key!)] as [K, V] };
124-
key = nextKey.get(key!);
125-
return result;
125+
if (!started) {
126+
started = true;
127+
key = getFirstKey();
128+
} else {
129+
key = nextKey.get(key!);
130+
}
131+
return { done: !key, value: [key!, items.get(key!)] as [K, V] };
126132
},
127133
};
128134
}
129135

130136
public keys(): IterableIterator<K> {
137+
const getFirstKey = () => this.firstKey;
131138
const nextKey = this.nextKey;
132-
let key = this.firstKey;
139+
let key: K | undefined;
140+
let started = false;
133141
return {
134142
[Symbol.iterator](): IterableIterator<K> {
135143
return this;
136144
},
137145
next(): IteratorResult<K> {
138-
const result = { done: !key, value: key };
139-
key = nextKey.get(key!);
140-
return result as IteratorResult<K>;
146+
if (!started) {
147+
started = true;
148+
key = getFirstKey();
149+
} else {
150+
key = nextKey.get(key!);
151+
}
152+
return { done: !key, value: key! };
141153
},
142154
};
143155
}
144156

145157
public values(): IterableIterator<V> {
158+
const getFirstKey = () => this.firstKey;
146159
const { items, nextKey } = this;
147-
let key = this.firstKey;
160+
let key: K | undefined;
161+
let started = false;
148162
return {
149163
[Symbol.iterator](): IterableIterator<V> {
150164
return this;
151165
},
152166
next(): IteratorResult<V> {
153-
const result = { done: !key, value: items.get(key!) };
154-
key = nextKey.get(key!);
155-
return result;
167+
if (!started) {
168+
started = true;
169+
key = getFirstKey();
170+
} else {
171+
key = nextKey.get(key!);
172+
}
173+
return { done: !key, value: items.get(key!) };
156174
},
157175
};
158176
}

‎src/lualib/Set.ts‎

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -102,46 +102,64 @@ export class Set<T extends AnyNotNil> {
102102
}
103103

104104
public entries(): IterableIterator<[T, T]> {
105+
const getFirstKey = () => this.firstKey;
105106
const nextKey = this.nextKey;
106-
let key: T = this.firstKey!;
107+
let key: T | undefined;
108+
let started = false;
107109
return {
108110
[Symbol.iterator](): IterableIterator<[T, T]> {
109111
return this;
110112
},
111113
next(): IteratorResult<[T, T]> {
112-
const result = { done: !key, value: [key, key] as [T, T] };
113-
key = nextKey.get(key);
114-
return result;
114+
if (!started) {
115+
started = true;
116+
key = getFirstKey();
117+
} else {
118+
key = nextKey.get(key!);
119+
}
120+
return { done: !key, value: [key!, key!] as [T, T] };
115121
},
116122
};
117123
}
118124

119125
public keys(): IterableIterator<T> {
126+
const getFirstKey = () => this.firstKey;
120127
const nextKey = this.nextKey;
121-
let key: T = this.firstKey!;
128+
let key: T | undefined;
129+
let started = false;
122130
return {
123131
[Symbol.iterator](): IterableIterator<T> {
124132
return this;
125133
},
126134
next(): IteratorResult<T> {
127-
const result = { done: !key, value: key };
128-
key = nextKey.get(key);
129-
return result;
135+
if (!started) {
136+
started = true;
137+
key = getFirstKey();
138+
} else {
139+
key = nextKey.get(key!);
140+
}
141+
return { done: !key, value: key! };
130142
},
131143
};
132144
}
133145

134146
public values(): IterableIterator<T> {
147+
const getFirstKey = () => this.firstKey;
135148
const nextKey = this.nextKey;
136-
let key: T = this.firstKey!;
149+
let key: T | undefined;
150+
let started = false;
137151
return {
138152
[Symbol.iterator](): IterableIterator<T> {
139153
return this;
140154
},
141155
next(): IteratorResult<T> {
142-
const result = { done: !key, value: key };
143-
key = nextKey.get(key);
144-
return result;
156+
if (!started) {
157+
started = true;
158+
key = getFirstKey();
159+
} else {
160+
key = nextKey.get(key!);
161+
}
162+
return { done: !key, value: key! };
145163
},
146164
};
147165
}

‎test/unit/builtins/map.spec.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,44 @@ describe.each(iterationMethods)("map.%s() preserves insertion order", iterationM
210210
});
211211
});
212212

213+
describe.each(iterationMethods)("map.%s() handles mutation", iterationMethod => {
214+
test("iterator persists after delete", () => {
215+
util.testFunction`
216+
const map = new Map<number, string>([[1, "a"], [2, "b"]]);
217+
const iter = map.${iterationMethod}();
218+
map.delete(1);
219+
return iter.next().value;
220+
`.expectToMatchJsResult();
221+
});
222+
223+
test("iterator with delete and add between iterations", () => {
224+
util.testFunction`
225+
const map = new Map<number, string>([[1, "a"], [2, "b"], [3, "c"]]);
226+
const iter = map.${iterationMethod}();
227+
iter.next(); // 1
228+
map.delete(2);
229+
map.set(4, "d");
230+
const results: IteratorResult<any>[] = [];
231+
let r = iter.next();
232+
while (!r.done) { results.push({ done: r.done, value: r.value }); r = iter.next(); }
233+
return results;
234+
`.expectToMatchJsResult();
235+
});
236+
237+
test("iterator does not restart after exhaustion", () => {
238+
util.testFunction`
239+
const map = new Map<number, string>([[1, "a"], [2, "b"]]);
240+
const iter = map.${iterationMethod}();
241+
const results: boolean[] = [];
242+
results.push(iter.next().done!);
243+
results.push(iter.next().done!);
244+
results.push(iter.next().done!); // should be done
245+
results.push(iter.next().done!); // should still be done, not restart
246+
return results;
247+
`.expectToMatchJsResult();
248+
});
249+
});
250+
213251
describe("Map.groupBy", () => {
214252
test("empty", () => {
215253
util.testFunction`

‎test/unit/builtins/set.spec.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,48 @@ describe.each(iterationMethods)("set.%s() preserves insertion order", iterationM
195195
});
196196
});
197197

198+
describe.each(iterationMethods)("set.%s() handles mutation", iterationMethod => {
199+
test("iterator persists after delete", () => {
200+
util.testFunction`
201+
const set1 = new Set<string | number>();
202+
set1.add(42);
203+
set1.add("forty two");
204+
205+
const iterator1 = set1.${iterationMethod}();
206+
set1.delete(42);
207+
208+
return iterator1.next().value;
209+
`.expectToMatchJsResult();
210+
});
211+
212+
test("iterator with delete and add between iterations", () => {
213+
util.testFunction`
214+
const set = new Set([1, 2, 3]);
215+
const iter = set.${iterationMethod}();
216+
iter.next(); // 1
217+
set.delete(2);
218+
set.add(4);
219+
const results: IteratorResult<any>[] = [];
220+
let r = iter.next();
221+
while (!r.done) { results.push({ done: r.done, value: r.value }); r = iter.next(); }
222+
return results;
223+
`.expectToMatchJsResult();
224+
});
225+
226+
test("iterator does not restart after exhaustion", () => {
227+
util.testFunction`
228+
const set = new Set([1, 2]);
229+
const iter = set.${iterationMethod}();
230+
const results: boolean[] = [];
231+
results.push(iter.next().done!);
232+
results.push(iter.next().done!);
233+
results.push(iter.next().done!); // should be done
234+
results.push(iter.next().done!); // should still be done, not restart
235+
return results;
236+
`.expectToMatchJsResult();
237+
});
238+
});
239+
198240
test("instanceof Set without creating set", () => {
199241
util.testFunction`
200242
const myset = 3 as any;

0 commit comments

Comments
 (0)