Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement WeakSet object #2009

Closed
wants to merge 1 commit into from
Closed

Implement WeakSet object #2009

wants to merge 1 commit into from

Conversation

lupd
Copy link
Contributor

@lupd lupd commented Apr 5, 2022

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.

@codecov
Copy link

codecov bot commented Apr 5, 2022

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%) ⬇️
boa_engine/src/object/mod.rs 21.43% <18.75%> (+0.08%) ⬆️
boa_engine/src/context/intrinsics.rs 58.46% <50.00%> (+0.98%) ⬆️
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%) ⬇️
boa_engine/src/syntax/ast/node/template/mod.rs 51.72% <0.00%> (-3.45%) ⬇️
boa_engine/src/builtins/map/map_iterator.rs 91.17% <0.00%> (-2.95%) ⬇️
boa_engine/src/builtins/object/for_in_iterator.rs 91.42% <0.00%> (-2.86%) ⬇️
...a_engine/src/syntax/parser/statement/switch/mod.rs 46.55% <0.00%> (-1.73%) ⬇️
boa_engine/src/builtins/math/mod.rs 93.93% <0.00%> (-1.02%) ⬇️
... 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.

@lupd lupd changed the title Implement WeakSet object Implement WeakSet object Apr 5, 2022
@raskad
Copy link
Member

raskad commented Apr 6, 2022

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: Manishearth/rust-gc#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.

@Razican
Copy link
Member

Razican commented Oct 21, 2022

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

@lupd
Copy link
Contributor Author

lupd commented Oct 27, 2022

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

@Razican Razican added blocked Waiting for another code change builtins PRs and Issues related to builtins/intrinsics labels Oct 28, 2022
@Razican
Copy link
Member

Razican commented Oct 28, 2022

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

True :) There seems to be a draft PR for this: Manishearth/rust-gc#148

@lupd lupd marked this pull request as draft October 31, 2022 13:56
@nekevss
Copy link
Member

nekevss commented Nov 1, 2022

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

@nekevss
Copy link
Member

nekevss commented Nov 18, 2022

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

bors bot pushed a commit that referenced this pull request Feb 11, 2023
This Pull Request changes the following:

- Implement `WeakSet` buildin object.
- Supersedes #2009



Co-authored-by: raskad <[email protected]>
@raskad raskad closed this Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Waiting for another code change builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants