feat(runtime): #2656 — make WeakMap/WeakSet actually weak

[proggeramlug]

Fixes #2656 (the WeakMap/WeakSet portion; WeakRef + FinalizationRegistry were already weak).

Stacked on #5461 (the array-push UAF fix). This branch includes that commit; review/merge #5461 first. The weakness work depends on it — WeakMap keys are commonly held in module-global arrays, which crashed before #5461.

Problem

WeakMap/WeakSet stored entries as plain [key, value] pair arrays that the GC traced strongly, so keys reachable only through the collection were never collected — weak in name/API only. Repro: a key held only by a WeakMap stays alive across gc().

Fix

Store each entry as a CLASS_ID_WEAK_ENTRY object:

• field 0 = key — a weak GC slot, skipped by the strong-edge scanners exactly like a WeakRef target / finalization record target (is_weak_target_trace_slot).
• field 1 = value — strong (traced while the key is live); undefined for a WeakSet (storing the member as the value too would pin it and defeat weakness).

The existing post-mark weak pass tombstones entries whose key was collected (nulling both slots, so the value is released next cycle); lookups skip tombstoned (undefined-key) entries. set reuses tombstone slots and delete compacts them, bounding growth. This is a direct reuse of the proven WeakRef/FinalizationRegistry machinery — no new GC phase.

Behavior (verified)

Under default GC and the auto-evacuation policy (PERRY_GEN_GC_EVACUATE=1):

• A WeakMap key / WeakSet member reachable only through the collection is collected (observed via a WeakRef + churn + gc()).
• Its value is released.
• Live entries are retained; has/get keep working; instanceof, iteration-placeholder inspect, and brand checks unaffected.

Regression test crates/perry/tests/issue_2656_weak_collections_actually_weak.rs (dead key collected, dead value released, dead WeakSet member collected, live entry + value retained, live member retained) is green. perry-runtime weakref unit test + 15 weak/collection test_gap_* files pass; the existing issue_2656 WeakRef/FinalizationRegistry GC test is unaffected.

Scope note (FORCE_EVACUATE)

The PERRY_GC_FORCE_EVACUATE full-evacuation debug stress mode (gc-stress CI; tag-gated, skips on PRs) over-collects weak targets in general — FinalizationRegistry has the identical limitation (its records are likewise non-root entry objects with a weak field-0). Root cause: valid_ptrs is built pre-copy, and a weak wrapper that is itself evacuated isn't resolvable in the post-mark liveness pass; non-moved WeakRef wrappers sidestep it. Fixing it needs GC copy-machinery changes (repairing weak slots of evacuated objects) and is left as a follow-up. The crashes seen under that mode come from the separate, pre-existing strong-array-in-closure bug #5467, not this change. No production GC mode (default, auto-evacuation) is affected.

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Updated WeakMap and WeakSet so entries are truly weak: collected keys are cleared correctly, and empty/tombstoned slots are handled so live entries remain available while dead ones are released.

• Tests

  • Added a regression test covering WeakMap/WeakSet behavior across garbage-collection churn, ensuring correctness for both keys and members.

• Documentation / Release

  • Updated the changelog and bumped the runtime version to v0.5.1197.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4adf9c7f-b650-459e-bc4d-1050b1c110b5

📥 Commits

Reviewing files that changed from the base of the PR and between 685c831 and 2b8b278.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (5)

• CHANGELOG.md
• CLAUDE.md
• Cargo.toml
• crates/perry-runtime/src/weakref.rs
• crates/perry/tests/issue_2656_weak_collections_actually_weak.rs

✅ Files skipped from review due to trivial changes (2)

• CLAUDE.md
• CHANGELOG.md

🚧 Files skipped from review as they are similar to previous changes (2)

• crates/perry/tests/issue_2656_weak_collections_actually_weak.rs
• crates/perry-runtime/src/weakref.rs

---

📝 Walkthrough

Walkthrough

Rewrites WeakMap/WeakSet runtime storage in weakref.rs from raw [key,value] pair arrays to dedicated CLASS_ID_WEAK_ENTRY objects with a GC-weak key field and tombstoning semantics. GC tracing, post-mark dispatch, and all WeakMap/WeakSet operations are updated accordingly. A regression test verifies dead keys are collected while live entries persist.

Changes

WeakMap/WeakSet Weak-Entry Objects (#2656)

Layer / File(s)	Summary
CLASS_ID_WEAK_ENTRY constant and entry object helpers crates/perry-runtime/src/weakref.rs	Removes unused js_array_alloc_with_length import; exports CLASS_ID_WEAK_ENTRY constant with weak-key/strong-value field layout documentation; defines WEAK_ENTRY_SHAPE_ID and adds weak_entry_new allocator and weak_entry_at accessor functions.
GC tracing and post-mark tombstoning crates/perry-runtime/src/weakref.rs	Updates is_weak_target_trace_slot to exclude CLASS_ID_WEAK_ENTRY field 0 from strong scanning; adds CLASS_ID_WEAK_ENTRY dispatch in process_weak_targets_after_mark; implements process_weak_entry_after_mark to set both key and value fields to TAG_UNDEFINED when the weak-key target is collected.
WeakMap/WeakSet operation rewrites crates/perry-runtime/src/weakref.rs	Refactors weak_collection_entries to iterate WeakEntry objects and skip tombstoned entries; rewrites js_weakmap_set to match existing keys, reuse tombstoned slots, allocate new entries, and rebind the entries array field; refactors js_weakmap_get and js_weakmap_has to scan entry objects; rewrites js_weakmap_delete to compact the entries array by dropping tombstones and the deleted entry; updates js_weakset_add to store members as WeakEntry keys with undefined value.
Regression test for #2656 crates/perry/tests/issue_2656_weak_collections_actually_weak.rs	Adds perry_bin() locator and compile_and_run() harness to compile TypeScript snippets into temporary binaries; implements weakmap_weakset_keys_are_collectible test that constructs live and dead WeakMap keys/values and WeakSet members, drives GC churn via repeated allocations, and asserts stdout contains expected markers confirming collected dead targets and retained live entries.
Version and release documentation Cargo.toml, CLAUDE.md, CHANGELOG.md	Bumps workspace package version from 0.5.1196 to 0.5.1197; adds v0.5.1197 CHANGELOG.md entry describing the WeakMap/WeakSet weak-entry storage change, GC integration, tombstoning semantics, and regression test reference.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related issues

• globals: make WeakRef and FinalizationRegistry actually weak #2656 — The runtime rewrite directly implements correct WeakMap/WeakSet weak-key semantics and the regression test explicitly validates the issue's requirement that collected keys no longer retain entries.

Poem

🐇 A map so weak, it lets things go,
When keys are gone, the entries flow—
Tombstoned swift by GC's hand,
No lingering ghosts in memory-land.
Hop hop hooray, the weak map's right,
Dead keys released into the night! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	PR description is comprehensive but violates three repository requirements: it bumps workspace version in Cargo.toml, edits CLAUDE.md Current Version, and adds CHANGELOG.md entry—all explicitly forbidden by template instructions.	Revert changes to Cargo.toml, CLAUDE.md, and CHANGELOG.md as per template instructions; maintainers handle version/metadata updates at merge time.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title accurately describes the main change: implementing true weak semantics for WeakMap/WeakSet, which is the primary objective of the PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/2656-weak-collections

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/perry-runtime/src/weakref.rs`:
- Around line 751-758: The weak_entry_at function implicitly assumes that the
entries array only contains POINTER_TAG-tagged values (which have zeros in their
low 48 bits), but this assumption is not documented or validated. If future code
changes allow non-pointer tags like TAG_UNDEFINED or TAG_HOLE to be stored in
entries, the masked extraction would produce invalid non-null garbage pointers
that bypass null checks. Add explicit documentation to the weak_entry_at
function explaining why entries can only contain POINTER_TAG-tagged values, and
consider adding a defensive check that verifies the high bits of the extracted
value match POINTER_TAG before casting to ensure the extraction is safe against
future modifications.

In `@crates/perry/tests/issue_2656_weak_collections_actually_weak.rs`:
- Line 37: The run command using Command::new(&output) does not set the working
directory to the temporary directory like the compile command does, which can
cause the binary to fail if it expects to run from that specific directory. Add
`.current_dir(dir)` to the Command::new(&output) chain before calling .output(),
matching the pattern used in the compile command to ensure consistent working
directory context.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 65d1964a-37be-4d04-a7cb-701971c23247

📥 Commits

Reviewing files that changed from the base of the PR and between 17a0e23 and 685c831.

📒 Files selected for processing (2)

• crates/perry-runtime/src/weakref.rs
• crates/perry/tests/issue_2656_weak_collections_actually_weak.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Set .current_dir(dir) when running the compiled binary.

The compile command correctly sets .current_dir(dir) (Line 24), but the run command does not. The upstream pattern in issue_4903_listen_callback_deferred.rs shows both compile and run commands should set the working directory to the temp directory. If the compiled binary expects to run from the temp directory (e.g., reads relative paths or creates temporary files), omitting this will cause incorrect behavior.

🔧 Proposed fix

-    let run = Command::new(&output).output().expect("run compiled binary");
+    let run = Command::new(&output)
+        .current_dir(dir)
+        .output()
+        .expect("run compiled binary");

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let run = Command::new(&output).output().expect("run compiled binary");
	let run = Command::new(&output)
	.current_dir(dir)
	.output()
	.expect("run compiled binary");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/perry/tests/issue_2656_weak_collections_actually_weak.rs` at line 37,
The run command using Command::new(&output) does not set the working directory
to the temporary directory like the compile command does, which can cause the
binary to fail if it expects to run from that specific directory. Add
`.current_dir(dir)` to the Command::new(&output) chain before calling .output(),
matching the pattern used in the compile command to ensure consistent working
directory context.