Skip to content

add support for traceable collections to Rust#6384

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

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

Conversation

@anonrig
Copy link
Copy Markdown
Member

@anonrig anonrig commented Mar 23, 2026

@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
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 23, 2026

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
Copy link
Copy Markdown
Contributor

@guybedford guybedford left a comment

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
Copy link
Copy Markdown
Member Author

anonrig commented Mar 31, 2026

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?

Copy link
Copy Markdown
Collaborator

@jasnell jasnell left a comment

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.

Copy link
Copy Markdown
Contributor

@guybedford guybedford left a comment

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.

Copy link
Copy Markdown
Contributor

@guybedford guybedford left a comment

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