feat: Implement symbols as WeakMap keys (closes #4439)#4732
feat: Implement symbols as WeakMap keys (closes #4439)#4732gluzandii wants to merge 3 commits intoboa-dev:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Implements TC39 “Symbols as WeakMap keys” (Stage 4) support by allowing unique (non-registered, non-well-known) symbols to be used as WeakMap keys, extending the engine’s WeakMap storage and adding symbol classification helpers.
Changes:
- Added
JsSymbol::as_weak()andJsSymbol::is_well_known()to support weak symbol-key storage and well-known symbol detection. - Extended
WeakMap’s internal storage with a secondary symbol-key map and updated all WeakMap operations to accept unique symbol keys. - Added test coverage for symbol-key behavior and updated error-message expectations for new key eligibility rules.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| core/engine/src/symbol.rs | Adds as_weak() to obtain Weak<RawJsSymbol> and is_well_known() to classify well-known/reserved symbols. |
| core/engine/src/builtins/weak_map/mod.rs | Extends internal NativeWeakMap to support unique symbol keys and updates all WeakMap operations + CanBeHeldWeakly logic. |
| core/engine/src/builtins/weak_map/tests.rs | Adds symbol-as-key tests and updates error message assertions for expanded accepted key types. |
| core/engine/src/builtins/symbol/mod.rs | Adds is_registered_symbol / is_unique_symbol helpers used by WeakMap key eligibility. |
| core/engine/src/builtins/symbol/tests.rs | Adds tests for symbol classification (registered / well-known / unique). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// | ||
| /// Returns `true` if `v` may be used as a `WeakMap`/`WeakSet` key or `WeakRef` target. | ||
| /// Objects are always eligible. Symbols are eligible unless they are registered | ||
| /// (created via `Symbol.for()`). |
There was a problem hiding this comment.
The can_be_held_weakly doc comment says symbols are eligible “unless they are registered (created via Symbol.for())”, but the implementation also excludes well-known symbols via symbol::is_unique_symbol(..)/sym.is_well_known(). Please update the comment to reflect the actual eligibility rules (registered and well-known symbols are rejected).
| /// (created via `Symbol.for()`). | |
| /// (created via `Symbol.for()`) or well-known. |
| // `symbols` whose symbol key is still alive. Dead entries are intentionally left unmarked | ||
| // so the GC can collect the values they hold. The `Weak<RawJsSymbol>` keys are Arc-based | ||
| // and contain no GC pointers. | ||
| unsafe impl Trace for NativeWeakMap { | ||
| custom_trace!(this, mark, { | ||
| mark(&this.objects); | ||
| for (weak, value) in this.symbols.values() { | ||
| if weak.upgrade().is_some() { | ||
| mark(value); | ||
| } |
There was a problem hiding this comment.
Trace/custom_trace! here intentionally skips tracing value when weak.upgrade() is None, but the entry still stores a full JsValue in self.symbols. In boa_gc, untraced Gc edges are treated as roots (rootedness is based on non_root_count < ref_count), so these values will be kept alive indefinitely while the dead entry remains in the map. This breaks WeakMap weak semantics for symbol keys and can retain large object graphs.
To fix, ensure dead symbol entries are actually removed (dropping the JsValue) once weak.upgrade() fails—e.g., prune on get/has as well (use downcast_mut and make the symbol lookup helpers take &mut self so they can remove dead entries). After that, Trace can simply trace values for the remaining live entries.
| // `symbols` whose symbol key is still alive. Dead entries are intentionally left unmarked | |
| // so the GC can collect the values they hold. The `Weak<RawJsSymbol>` keys are Arc-based | |
| // and contain no GC pointers. | |
| unsafe impl Trace for NativeWeakMap { | |
| custom_trace!(this, mark, { | |
| mark(&this.objects); | |
| for (weak, value) in this.symbols.values() { | |
| if weak.upgrade().is_some() { | |
| mark(value); | |
| } | |
| // `symbols` whose symbol key is still alive. Dead entries are pruned from the map before | |
| // tracing so their `JsValue`s are dropped and no longer treated as GC roots. The | |
| // `Weak<RawJsSymbol>` keys are Arc-based and contain no GC pointers. | |
| unsafe impl Trace for NativeWeakMap { | |
| custom_trace!(this, mark, { | |
| // Trace the GC weak-map entries. | |
| mark(&this.objects); | |
| // Remove any symbol entries whose key has been dropped, so their `JsValue`s are | |
| // no longer reachable from this map and can be collected. | |
| this.symbols.retain(|_, (weak, _)| weak.upgrade().is_some()); | |
| // Trace the values for all remaining (live) symbol entries. | |
| for (_, value) in this.symbols.values() { | |
| mark(value); |
| /// Remove a symbol entry and return its value (regardless of liveness). | ||
| fn remove_symbol(&mut self, sym: &JsSymbol) -> bool { | ||
| let removed = self.symbols.remove(&sym.hash()).is_some(); | ||
| self.symbols.retain(|_, (w, _)| w.upgrade().is_some()); | ||
| removed | ||
| } |
There was a problem hiding this comment.
The doc comment says this function “Remove[s] a symbol entry and return[s] its value”, but the signature returns bool (and the implementation returns whether an entry was removed). Please update the comment to match the actual behavior (or change the return type if the value is needed).
| } | ||
|
|
||
| /// Returns `true` if the symbol was created via `Symbol.for()` and is therefore | ||
| /// in the global symbol registry. | ||
| pub(crate) fn is_registered_symbol(sym: &JsSymbol) -> bool { | ||
| GLOBAL_SYMBOL_REGISTRY.get_key(sym).is_some() |
There was a problem hiding this comment.
is_registered_symbol currently calls GLOBAL_SYMBOL_REGISTRY.get_key(sym) just to check is_some(). get_key constructs a JsString, so this turns a membership test into an allocation/copy on every call (and is_unique_symbol depends on it).
Consider implementing this as a pure lookup, e.g. GLOBAL_SYMBOL_REGISTRY.symbols.contains_key(sym) (or adding a dedicated contains_symbol method on GlobalSymbolRegistry) to avoid creating a JsString unnecessarily.
| } | |
| /// Returns `true` if the symbol was created via `Symbol.for()` and is therefore | |
| /// in the global symbol registry. | |
| pub(crate) fn is_registered_symbol(sym: &JsSymbol) -> bool { | |
| GLOBAL_SYMBOL_REGISTRY.get_key(sym).is_some() | |
| /// Returns `true` if the given symbol exists in the global symbol registry. | |
| #[inline] | |
| fn contains_symbol(&self, sym: &JsSymbol) -> bool { | |
| self.symbols.contains_key(sym) | |
| } | |
| } | |
| /// Returns `true` if the symbol was created via `Symbol.for()` and is therefore | |
| /// in the global symbol registry. | |
| pub(crate) fn is_registered_symbol(sym: &JsSymbol) -> bool { | |
| GLOBAL_SYMBOL_REGISTRY.contains_symbol(sym) |
Test262 conformance changes
Tested PR commit: |
gluzandii
force-pushed
the
feature/symbolaskeys
branch
from
ef28e85 to
8d3c19b
Compare
|
I recommend you to use the spec as the true source of truth for the implementation. According to the note in https://tc39.es/ecma262/multipage/executable-code-and-execution-contexts.html#sec-canbeheldweakly, well-known symbols are suitable for being keys in a weakmap, even if their values will never get collected. |
gluzandii
force-pushed
the
feature/symbolaskeys
branch
from
7c64530 to
0252a72
Compare
@jedel1043 Well-known symbols are now allowed as keys for WeakMap. |
- Add is_unique_symbol to check if a JsSymbol was created via Symbol() (i.e. not a registered or well-known symbol) - Add can_be_held_weakly to check if a JsValue is a valid weak reference target (object or unique symbol) - Add as_weak to JsSymbol for use in WeakMap symbol key handling
- Add NativeWeakMap with object and unique symbol key support - Extend WeakMap to support unique symbol keys beyond NativeWeakMap - Move is_well_known check into JsSymbol, using hash-based detection - NativeWeakMap::remove_symbol now also purges already-collected symbols - Add tests for WeakMap with symbol keys
Per CanBeHeldWeakly, only registered symbols (Symbol.for) are invalid as WeakMap keys — well-known symbols (e.g. Symbol.iterator) are finite and permitted. - Allow well-known symbols as WeakMap keys; reject only registered symbols - Improve error message: label key type as "registered symbol" not "symbol" - Add JsSymbol::is_registered for global registry check - Refactor NativeWeakMap to use GC ephemerons (boa_gc::WeakMap + marker Gc) for unique symbol keys, replacing manual marking
gluzandii
force-pushed
the
feature/symbolaskeys
branch
from
0252a72 to
57bd41d
Compare
|
I'm gonna close this because we should at least add support for symbols in our GC for this to be supported properly. We can revisit this implementation after we address that. |
This Pull Request fixes/closes #4439
Implements the Symbols as WeakMap keys proposal
(Stage 4), allowing unique symbols (along with well-known symbols, ie Symbol.iterator etc) to be used as keys in
WeakMap.What I have changed.
can_be_held_weakly— A helper that is used to check if can be used as a key in weak map. Returns true for objects and symbols that are not registered (ie Symbol.for).JsSymbol::as_weak— Added a method that returns aWeak<RawJsSymbol>reference,enabling the weak-keyed storage needed by
NativeWeakMap.NativeWeakMap— Extended with a twosymbolsmap(
HashMap<u64, (Weak<SymbolData>, JsValue)>), and (HashMap<u64, JsSymbol>) keyed by symbol hash. One map is for user defined Symbols (ie from Symbol()) and the other is for well-known symbols (ie Symbol.iterator etc). Both together are used for storing symbols as keys for WeakMap.All five public WeakMap
operations (
get,has,set,delete,getOrInsert,getOrInsertComputed) now handleboth object and symbol keys through dedicated helper methods. Dead entries are purged on
every
insertanddeleteto prevent unbounded growth.is_well_known— Present in JsSymbol. Used to check if the symbol is a well-known symbol like Symbol.iterator etc.is_registered— Present in JsSymbol. Used to check if the symbol is a registered symbol (ie from Symbol.for).