Skip to content

add support for traceable collections to Rust - #6384

Merged
anonrig merged 1 commit into
mainfrom
yagiz/add-collections-support-to-rust
Apr 1, 2026
Merged

add support for traceable collections to Rust#6384
anonrig merged 1 commit into
mainfrom
yagiz/add-collections-support-to-rust

Conversation

@anonrig

@anonrig anonrig commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

@anonrig
anonrig requested review from guybedford and jasnell March 23, 2026 14:28
@anonrig
anonrig requested review from a team as code owners March 23, 2026 14:28
@ask-bonk

ask-bonk Bot commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

LGTM

github run

@anonrig
anonrig force-pushed the yagiz/add-collections-support-to-rust branch from ff76bff to 05b4eea Compare March 23, 2026 14:43
Comment thread src/rust/jsg-macros/lib.rs Outdated
Comment thread src/rust/jsg-macros/lib.rs Outdated

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm sorry I just don't think this is the right approach - matching name strings is clearly in violation of scoping rules, as I've mentioned before.

Please explain to me why we aren't using a trait to handle this?

@anonrig
anonrig force-pushed the yagiz/add-collections-support-to-rust branch 3 times, most recently from 5580e83 to 974acac Compare March 31, 2026 21:19
@anonrig
anonrig requested review from guybedford and jasnell March 31, 2026 21:20
@anonrig

anonrig commented Mar 31, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review feedback:

  • Reworked GC tracing to be trait-driven via Traced instead of field-type string matching.
  • GarbageCollected now extends Traced; trace() lives on Traced.
  • Removed #[jsg_traceable] / #[jsg_trace] and updated resource tracing to call Traced::trace() on every field.
  • Added no-op Traced impls for non-traceable/value types and delegating impls for wrappers/collections.
  • Updated docs/comments (including jsg_struct wording around class_name() usage and macro table formatting).
  • just clippy passes on this branch.

Could you please take another look?

Comment thread src/rust/jsg-macros/lib.rs Outdated

@jasnell jasnell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Findings from review:

  • [MEDIUM] name + custom_trace combination silently drops the name override — extract_name_attribute parses the full attr stream as a single MetaNameValue, which fails when custom_trace is also present
  • [MEDIUM] has_custom_trace_flag uses string-based parsing rather than syn meta parsing, which is inconsistent with the rest of the crate
  • [LOW] RefCell<T> tracing uses self.borrow() which will panic if a mutable borrow is active during GC

This review was generated by an AI assistant and may contain inaccuracies.

Comment thread src/rust/jsg-macros/resource.rs
Comment thread src/rust/jsg-macros/utils.rs
Comment thread src/rust/jsg/wrappable.rs Outdated

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please can we just add one more thing to this PR - a derive macro for Traced on structs and enums?

Example:

#[derive(jsg::Traced)]
struct MyPlainData { ... }

#[derive(jsg::Traced)]
enum MyPlainData { ... }

basically works just like the current trace.rs does anyway.

Comment thread src/rust/jsg-macros/trace.rs
Comment thread src/rust/jsg/wrappable.rs Outdated

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Marking this as conditional approval on the comments and derive macro being added.

@anonrig
anonrig force-pushed the yagiz/add-collections-support-to-rust branch from 974acac to 282b449 Compare April 1, 2026 12:59
@anonrig
anonrig force-pushed the yagiz/add-collections-support-to-rust branch from 282b449 to 2b8ac69 Compare April 1, 2026 12:59
@anonrig
anonrig enabled auto-merge (squash) April 1, 2026 13:38
@anonrig
anonrig merged commit 6a9488a into main Apr 1, 2026
22 of 23 checks passed
@anonrig
anonrig deleted the yagiz/add-collections-support-to-rust branch April 1, 2026 14:06
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