GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix(lualib): support Map/Set entry deletion during iteration by pilaoda · Pull Request #1728 · TypeScriptToLua/TypeScriptToLua

Conversation

pilaoda commented Jun 15, 2026

Copy link

Copy Markdown

Contributor

Problem

Deleting Map/Set entries during iteration breaks the iterator — delete() clears nextKey[key], the pointer the iterator relies on to advance. This violates the ES spec.

Solution

Rewrite Map/Set internals from linked list to flat arrays with tombstone deletion and V8-style version chain compaction:

orderedKeys/orderedValues: 1-based integer arrays (leverages Lua's contiguous array part for fast iteration)
keyIndex: hash table for O(1) lookup
Delete marks tombstone (nil), iterator skips via idx++
Compaction triggers when deletedCount > size, creates new arrays linked via version chain
Iterators transition lazily through the chain by adjusting index (same algorithm as V8's OrderedHashTable)

Also fixes has() returning false for keys with null/undefined values (now uses keyIndex instead of nextKey).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Copilot AI review requested due to automatic review settings

June 15, 2026 22:20

pilaoda changed the title ~~Title: fix(lualib): support Map/Set entry deletion during iteration~~ fix(lualib): support Map/Set entry deletion during iteration

Jun 15, 2026

Copilot AI reviewed

Jun 15, 2026

View reviewed changes

Copilot AI left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR refactors the Lua-lib Map/Set implementations to use insertion-ordered “flat array + tombstones” storage with compaction, and adds extensive iterator-mutation and memory-retention tests to validate behavior against JS/V8 semantics.

Changes:

Replaced linked-list style key ordering with array-backed ordered storage for Map and Set, including tombstone deletion and lazy iterator transition on compaction.
Added many new mutation tests for for-of and forEach iteration when deleting/re-adding entries.
Added “memory leak” regression tests that force Lua table rehashing and compare GC memory deltas.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
test/unit/builtins/set.spec.ts	Adds iterator-mutation cases, V8-style iterator stress tests, and a Set GC-retention regression test.
test/unit/builtins/map.spec.ts	Adds iterator-mutation cases, V8-style iterator stress tests, and a Map GC-retention regression test with detailed Lua rehash rationale.
src/lualib/Set.ts	Reimplements Set ordering via array-like tables with tombstones + compaction and iterator lazy-transition logic.
src/lualib/Map.ts	Reimplements Map ordering via array-like tables with tombstones + compaction and iterator lazy-transition logic; adds design doc comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/lualib/Map.ts

		public forEach(callback: (value: V, key: K, map: Map<K, V>) => any): void {
		for (const key of this.keys()) {
		callback(this.items.get(key), key, this);
		callback(this.orderedValues.get(this.keyIndex.get(key)), key, this);

Comment thread

src/lualib/Set.ts

Comment on lines +137 to +139

		const val = keys.get(idx);
		idx++;
		return { done: false, value: [val, val] as [T, T] };

Comment thread

src/lualib/Map.ts

Comment on lines +107 to +108

		holeCount++;
		oldKeys.set(-holeCount, i as any);

Comment thread

src/lualib/Map.ts

Comment on lines +112 to +113

		oldKeys.set(0, newKeys as any);
		oldValues.set(0, newValues as any);

Comment thread

test/unit/builtins/set.spec.ts

		};
		`.getLuaExecutionResult();
		expect(result.size).toBe(0);
		expect(result.retained).toBe(0);

Comment thread

test/unit/builtins/map.spec.ts

		};
		`.getLuaExecutionResult();
		expect(result.size).toBe(0);
		expect(result.retained).toBe(0);

pilaoda force-pushed the fix/map-set-delete-during-iteration branch from dfb31e5 to 908b802 Compare

June 15, 2026 23:12


          fix(lualib): rewrite Map/Set with flat array storage for safe iterati…

66c2d6d

…on during deletion

Replace linked list (nextKey/previousKey) with flat arrays + V8-style
version chain compaction. Deleted entries become tombstones (nil slots);
iterators skip them via idx++. Compaction creates new arrays and links
old → new; iterators transition lazily by adjusting their index.

Changes:
- Map/Set use keyIndex (hash), orderedKeys/orderedValues (arrays)
- delete() marks tombstone, triggers compact when deletedCount > size
- compact() records hole positions, links old arrays to new via index 0
- Iterator transitions through version chain (V8 Transition algorithm)
- has() uses keyIndex (works correctly with null/undefined values)

Based on V8's OrderedHashTable:
  https://chromium.googlesource.com/v8/v8/+/main/src/objects/ordered-hash-table.cc

Tests: 166 Map/Set tests including V8 stress tests, memory leak tests,
delete-during-iteration, re-add, and consecutive delete scenarios.
Adapted from V8's collection-iterator.js:
  https://chromium.googlesource.com/v8/v8/+/main/test/mjsunit/es6/collection-iterator.js

pilaoda force-pushed the fix/map-set-delete-during-iteration branch from 908b802 to 66c2d6d Compare

June 15, 2026 23:16

Reviewers

Copilot code review Copilot

Copilot review effort

Applies to this pull request for everyone.Learn more about Copilot code review.

Copilot left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.