boa icon indicating copy to clipboard operation
boa copied to clipboard

Implement `WeakSet` object

Open lupd opened this issue 3 years ago • 2 comments

This Pull Request implements the WeakSet object.

It changes the following:

  • Add WeakSet as a new ObjectKind.
  • Add casts to WeakSet.
  • Implement the WeakSet object (constructor, properties, instance methods, etc.).
  • Add tests for WeakSet object.

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.

lupd avatar Apr 05 '22 17:04 lupd

Codecov Report

Merging #2009 (22d9f5d) into main (ffc7a3e) will increase coverage by 0.09%. The diff coverage is 64.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 data Powered by Codecov. Last update ffc7a3e...22d9f5d. Read the comment docs.

codecov[bot] avatar Apr 05 '22 17:04 codecov[bot]

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.

raskad avatar Apr 06 '22 23:04 raskad

@lupd did you have the chance to look into the comment by @raskad ? Can we help you with this?

Razican avatar Oct 21 '22 14:10 Razican

I think this is blocked on having weak references in the garbage collector.

lupd avatar Oct 27 '22 23:10 lupd

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

Razican avatar Oct 28 '22 08:10 Razican

It's a bit of a work in progress...on a couple fronts 😅

nekevss avatar Nov 01 '22 01:11 nekevss

@lupd Some weak types for the Gc were added with #2394 merged, so I think this should be workable 😄

nekevss avatar Nov 18 '22 19:11 nekevss