boa
boa copied to clipboard
Implement `WeakSet` object
This Pull Request implements the WeakSet object.
It changes the following:
- Add
WeakSetas a newObjectKind. - Add casts to
WeakSet. - Implement the
WeakSetobject (constructor, properties, instance methods, etc.). - Add tests for
WeakSetobject.
With these changes, the entire test suite for WeakSet passes. As well, additional tests using WeakSet (e.g. 14 tests in the Set test suite) also pass.
Codecov Report
Merging #2009 (22d9f5d) into main (ffc7a3e) will increase coverage by
0.09%. The diff coverage is64.61%.
@@ Coverage Diff @@
## main #2009 +/- ##
==========================================
+ Coverage 45.89% 45.98% +0.09%
==========================================
Files 206 207 +1
Lines 17150 17213 +63
==========================================
+ Hits 7871 7916 +45
- Misses 9279 9297 +18
| Impacted Files | Coverage Δ | |
|---|---|---|
| boa_engine/src/builtins/mod.rs | 7.93% <0.00%> (-0.13%) |
:arrow_down: |
| boa_engine/src/object/mod.rs | 21.43% <18.75%> (+0.08%) |
:arrow_up: |
| boa_engine/src/context/intrinsics.rs | 58.46% <50.00%> (+0.98%) |
:arrow_up: |
| boa_engine/src/builtins/weak_set/mod.rs | 84.44% <84.44%> (ø) |
|
| boa_engine/src/syntax/ast/position.rs | 20.00% <0.00%> (-12.00%) |
:arrow_down: |
| boa_engine/src/syntax/ast/node/template/mod.rs | 51.72% <0.00%> (-3.45%) |
:arrow_down: |
| boa_engine/src/builtins/map/map_iterator.rs | 91.17% <0.00%> (-2.95%) |
:arrow_down: |
| boa_engine/src/builtins/object/for_in_iterator.rs | 91.42% <0.00%> (-2.86%) |
:arrow_down: |
| ...a_engine/src/syntax/parser/statement/switch/mod.rs | 46.55% <0.00%> (-1.73%) |
:arrow_down: |
| boa_engine/src/builtins/math/mod.rs | 93.93% <0.00%> (-1.02%) |
:arrow_down: |
| ... and 19 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update ffc7a3e...22d9f5d. Read the comment docs.
Hi @lupd,
Thanks for the contribution!
I did a quick review of the code and the implementation looks mostly very good, but I think there is one critical part missing; the Weak in WeakSet ;)
Objects that are added to a WeakSet should only be added as a weak reference. That means that the garbage collector can in theory collect the object references, if they are not strongly referenced elsewhere.
Because it is tricky (impossible?) to demonstrate this with WeakSet, here is some code that shows this behaviour with a WeakRef. The principle is the same:
let ref;
{
let obj = {};
ref = new WeakRef(obj);
// This will always be `{}`
ref.deref()
}
// `obj` is now out of scope.
// This means that the object that was assigned to `obj` is now only weakly referenced by `ref`.
// The gc can now collect the object.
// This may be `{}` if the object was not yet collected.
// But at some point it will be `undefined` because the weak reference has been collected.
ref.deref()
If you want to try that code in node, I recommend starting node with --expose-gc and using global.gc() a bunch of times before the last ref.deref(). It does not seem completely deterministic, so you may also want to try running node a bunch of times. But you will definitely see the behavior sometimes.
I think this behavior is not possible with our current gc in boa. We are using rust-gc as our garbage collector and that crate does not implement weak gc references as far as I know.
We definitely weed weak gc references in boa for WeakRef, WeakSet and WeakMap. If you want to take a stab at this @lupd, the best place to start would probably be this issue in rust-gc, where weak references are being discussed: https://github.com/Manishearth/rust-gc/issues/65. If it seems reasonable it would be nice to get weak references contributed to rust-gc, otherwise we would probably have to implement our own gc.
@lupd did you have the chance to look into the comment by @raskad ? Can we help you with this?
I think this is blocked on having weak references in the garbage collector.
I think this is blocked on having weak references in the garbage collector.
True :) There seems to be a draft PR for this: https://github.com/Manishearth/rust-gc/pull/148
It's a bit of a work in progress...on a couple fronts 😅
@lupd Some weak types for the Gc were added with #2394 merged, so I think this should be workable 😄