Skip to content

[API] Enhance Trace trait with trace_non_roots, run_finalizer and standard types - #96

Merged
nekevss merged 1 commit into
boa-dev:mainfrom
shruti2522:api-2
Aug 10, 2026
Merged

[API] Enhance Trace trait with trace_non_roots, run_finalizer and standard types#96
nekevss merged 1 commit into
boa-dev:mainfrom
shruti2522:api-2

Conversation

@shruti2522

Copy link
Copy Markdown
Contributor

follow up to PR #95

  • added trace_non_roots and run_finalizer to the Trace trait across mark_sweep_branded and null_collector_branded
  • Implemented Trace and Finalize for standard library collections (HashMap, HashSet, BinaryHeap) and utility types (Instant, Path)
  • implemented Trace and Finalize for boa specific types (boa_string::JsString, icu_locale_core, either::Either, arrayvec::ArrayVec)

@shruti2522
shruti2522 marked this pull request as ready for review August 5, 2026 20:57
@shruti2522
shruti2522 marked this pull request as draft August 5, 2026 20:59
@shruti2522
shruti2522 marked this pull request as ready for review August 5, 2026 20:59
Copilot AI lite review requested due to automatic review settings August 5, 2026 20:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the GC Trace trait API (adding trace_non_roots and run_finalizer) and introduces optional Trace/Finalize implementations for additional standard-library and third-party types to broaden out-of-the-box GC compatibility.

Changes:

  • Added trace_non_roots and run_finalizer to Trace across collectors (mark_sweep, mark_sweep_branded, null_collector_branded).
  • Added optional Trace/Finalize impls (gated by features) for several external crates and additional std types.
  • Updated dependencies/features and lockfile to support the new optional integrations.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
oscars/src/collectors/null_collector_branded/trace.rs Expands Trace API and adds many new Trace/Finalize impls (std + external crates) under feature gates.
oscars/src/collectors/mark_sweep/trace.rs Extends Trace trait with trace_non_roots default method.
oscars/src/collectors/mark_sweep_branded/trace.rs Extends Trace trait with trace_non_roots and a default run_finalizer.
oscars/Cargo.toml Adds optional dependencies and feature flags for external type support.
Cargo.lock Updates dependency resolution to include newly added optional crates and transitive updates.
Suppressed comments (3)

oscars/src/collectors/null_collector_branded/trace.rs:351

  • Trace for hashbrown::HashSet is currently a no-op. Elements can contain Gc pointers and must be visited; add a T: Trace bound and trace each element.
unsafe impl<T, S> Trace for hashbrown::hash_set::HashSet<T, S> {
    #[inline]
    unsafe fn trace(&self, _tracer: &mut Tracer) {}
}

oscars/src/collectors/null_collector_branded/trace.rs:360

  • Trace for BinaryHeap is implemented as a no-op due to the lack of iter_mut(), but tracing only requires &self. This should iterate the heap and trace each element so reachable Gc pointers are not skipped.
unsafe impl<T: Trace> Trace for rust_alloc::collections::BinaryHeap<T> {
    #[inline]
    unsafe fn trace(&self, _tracer: &mut Tracer) {
        // BinaryHeap has no iter_mut(); the null collector's trace is a no-op
        // so no values need to be visited here.
    }
}

oscars/src/collectors/null_collector_branded/trace.rs:409

  • Trace for std::collections::HashSet is currently a no-op. Elements can contain Gc pointers and must be traced; add a T: Trace bound and trace each element.
unsafe impl<T, S> Trace for std::collections::HashSet<T, S> {
    #[inline]
    unsafe fn trace(&self, _tracer: &mut Tracer) {}
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread oscars/src/collectors/null_collector_branded/trace.rs Outdated
Comment thread oscars/src/collectors/null_collector_branded/trace.rs Outdated
Comment thread oscars/Cargo.toml
@shruti2522
shruti2522 force-pushed the api-2 branch 2 times, most recently from 2d3c80b to 2a3b0bf Compare August 5, 2026 21:25
@shruti2522 shruti2522 changed the title feat: enhance Trace trait with trace_non_roots, run_finalizer and standard types [API-expansion] Enhance Trace trait with trace_non_roots, run_finalizer and standard types Aug 10, 2026
@shruti2522 shruti2522 changed the title [API-expansion] Enhance Trace trait with trace_non_roots, run_finalizer and standard types [API] Enhance Trace trait with trace_non_roots, run_finalizer and standard types Aug 10, 2026

@nekevss nekevss left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for now. I left one comment, and we will need to address it in the future. @shruti2522 could you file an issue to track it.

Comment thread oscars/Cargo.toml
rustc-hash = "2.1.1"
thin-vec = { version = "0.2", optional = true }
# Optional Trace/Finalize impls for external types.
boa_string = { git = "https://github.com/boa-dev/boa.git", branch = "main", optional = true }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm, we will probably need to look into handling this differently in the future for a release ... I don't think this will resolve correctly for what we need.

But since this is still experimental we can leave it for now. But we will need to address it. It probably means that in the long run, oscars would need to be moved into boa OR the boa_string definitions will need to be added to boa_string

@nekevss
nekevss merged commit 92ae917 into boa-dev:main Aug 10, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants