Repository navigation
Let #[jsg_struct] fields hold JavaScript values - #8160
Conversation
A #[jsg_struct] field can now hold a JavaScript value, as a C++
JSG_STRUCT field of type jsg::JsRef<T> does. Such a field is a
jsg::v8::TracedGlobal<M>, which now implements Type, ToJS and FromJS
and converts as a v8::Local<'s, M> does:
- TracedGlobal<Value> takes any value, and a missing property reads as
undefined, as with jsg::JsRef<jsg::JsValue>.
- TracedGlobal<Object> (and the other markers with a FromJS for their
Local) takes only a value of that type, with the Local's TypeError
("Expected Object but got number").
- Converting to JavaScript gives back the value itself.
The macro needed no change: it already converts each field through its
own type and traces every field. So a struct with such a field holds GC
references, and a resource that keeps it traces the value, so a cycle
through it is collected.
Porting MemoryCache (#8066) needs this. Its fallback produces a
CacheValueProduceResult, whose value is a jsg::JsRef<jsg::JsValue>.
The Rust port reads that object by hand today. With this change, and
with typed promises (#8153), it can be a struct carried by a
jsg::Promise<'s, T>. That needs a 'static T, so the field is a
TracedGlobal and not a Local with the struct's lifetime.
Tests cover keeping the value's identity (with and without
fast_jsg_struct), any value and missing properties, a rejected
non-object, a struct kept past the scope it was converted in, and the
collection of a cycle through such a field. No JS-visible change;
nothing outside tests uses this yet.
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
d77597c3.
This review, of aba0b045
✅ Approved — No issues found: the TracedGlobal conversions pass through to the existing Local conversions, the field is traced through the existing Traced impl, and the tests cover the behaviour.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No security concerns.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
📋 Code Quality: ✅ LGTM
The new Type, ToJS and FromJS impls for v8::TracedGlobal<M> in src/rust/jsg/wrappable.rs pass through to the matching Local<'s, M> impls, and their where bounds are correct. The tests cover identity, absent properties, rejection of a value of the wrong type, keeping a struct past its scope, and collecting a cycle through such a field.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
The README and doc-comment additions describe the new behaviour accurately.
Reviewed commit: aba0b045
Clippy's float_cmp lint rejects assert_eq! on an f64.
There was a problem hiding this comment.
Superseded by the review of
9f48815d.
This review, of d77597c3
✅ Approved — No issues found. The TracedGlobal conversions call straight through to the existing Local conversions, the field is traced, and the test change since the last review only swaps a float comparison for an integer one.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No security concerns.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
📋 Code Quality: ✅ LGTM
The TracedGlobal<M> impls in src/rust/jsg/wrappable.rs call straight through to the matching Local<'s, M> impls, and their where bounds match. The tests cover the behaviour described in the pull request.
⚡ Performance: ✅ LGTM
The conversions call straight through to the existing ones and add no allocation beyond the traced handle itself.
📝 Documentation: ✅ LGTM
The README and doc-comment additions describe the new behaviour accurately.
Reviewed commit: d77597c3
There was a problem hiding this comment.
Superseded by the review of
34875c68.
This review, of 9f48815d
✅ Approved — No issues found. The TracedGlobal conversions call straight through to the existing Local conversions, the field is traced, and the test change since the last review only swaps a float comparison for an integer one.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No security concerns.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
📋 Code Quality: ✅ LGTM
The TracedGlobal<M> impls in src/rust/jsg/wrappable.rs call straight through to the matching Local<'s, M> impls, and their where bounds match. The tests cover the behaviour described in the pull request.
⚡ Performance: ✅ LGTM
The conversions call straight through to the existing ones and add no allocation beyond the traced handle itself.
📝 Documentation: ✅ LGTM
The README and doc-comment additions describe the new behaviour accurately.
Reviewed commit: 9f48815d — same changes as d77597c3, so this is that commit's review (no new review was run; line numbers are as of d77597c3)
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Checked out and ran the new tests locally; they pass, and adding a GC between the two scopes in struct_js_value_field_outlives_its_scope still passes, which might be worth including since it's what actually exercises the 'static path.
One thing to be aware of: a struct moved out of a resource after GC has traced it keeps a weak, untraced handle. I reproduced this with a resource method that does self.link.borrow_mut().take() after a GC; the next GC collects the value and to_js panics with C++ passed an empty v8::Local. This isn't new to this PR (a bare TracedGlobal field has the same issue), and #7982's jsg::RefCell::take(lock) / make_strong addresses it, but structs being value types makes moving them out a more natural pattern, so it's worth keeping in mind for the MemoryCache port.
AI generated, under my review
…ruct-js-values # Conflicts: # src/rust/jsg-macros/README.md
#[jsg_struct] fields now need TypeInfo for RTTI. A TracedGlobal<T> field is described as T, as C++ describes a jsg::JsRef<T> field and as Rust describes Local<T> and Global<T>.
There was a problem hiding this comment.
Superseded by the review of
6872a33e.
This review, of 34875c68
💬 Approved with comments — The conversions and the RTTI impl are correct. Two INFO notes: the new #[jsg_struct] docs don't warn that a struct moved out of a resource holds an untraced handle, and the out-of-scope test never runs a GC between its two scopes.
Findings: 2 info (2 are inline comments on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 2 info
The conversions and the RTTI are correct. The docs leave out the move-out hazard, and one test never runs a GC between its scopes.
Recommendation: 💬 Optional Nits
- [INFO] The new
#[jsg_struct]README text doesn't warn that a struct moved out of a resource after a GC holds a weak handle that nothing traces. (src/rust/jsg-macros/README.md:38) - [INFO]
struct_js_value_field_outlives_its_scopenever runs a GC between its two scopes, so it doesn't exercise the'staticpath. (src/rust/jsg-test/tests/jsg_struct.rs:519)
View full review
TypeInfo for TracedGlobal<T> forwards to T, as the other handle types do. The approver's take-after-GC panic comes from existing TracedGlobal semantics, which src/rust/jsg/README.md already documents. The new #[jsg_struct] docs make that state easier to reach but don't point at that rule.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security concerns. The impls pass straight through to the existing Local conversions.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
⚡ Performance: ✅ LGTM
The conversions and the TypeInfo impl only forward to the existing ones.
📝 Documentation: ✅ LGTM
The README and doc-comment additions describe the conversion behaviour accurately.
Reviewed commit: 34875c68
Once a GC has visited a resource that keeps a #[jsg_struct] with a TracedGlobal field, the handle is weak and relies on the resource's trace. Taken out of the field with a plain move, nothing traces it and the next GC can collect the value, after which to_js panics with "C++ passed an empty v8::Local". With jsg::RefCell (#7982), borrow_mut detaches the struct before it can leave, so it is strong again. * A test takes such a struct out of a resource's jsg::RefCell after a GC, runs another GC and converts it back. Without the detach in borrow_mut, it fails with that panic. * struct_js_value_field_outlives_its_scope runs a GC between its two scopes, and holds objects rather than a string literal, so it checks that the struct's handles keep their values. * The #[jsg_struct] docs say that a resource changing such a struct after construction keeps it in a jsg:: cell.
|
@guybedford thanks. With #7982 merged, both of your points are addressed in 6872a33:
|
There was a problem hiding this comment.
Superseded by the review of
ea6976b0.
This review, of 6872a33e
✅ Approved — Both earlier INFO notes are fixed (the cell guidance in the docs and a GC between the test's two scopes), and the new code has no issues.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No security concerns.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
📋 Code Quality: ✅ LGTM
The README and the lib.rs doc comment now tell readers to keep such a struct in a jsg:: cell. The out-of-scope test runs Harness::request_gc between its two scopes. The take-after-GC case is covered in traceable_gc.rs.
⚡ Performance: ✅ LGTM
The conversions just call the existing Local implementations.
📝 Documentation: ✅ LGTM
The documentation describes the conversion behaviour and the cell requirement accurately.
Reviewed commit: 6872a33e
There was a problem hiding this comment.
✅ Approved — Both earlier INFO notes are fixed (the cell guidance in the docs and a GC between the test's two scopes), and the new code has no issues.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No security concerns.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
📋 Code Quality: ✅ LGTM
The README and the lib.rs doc comment now tell readers to keep such a struct in a jsg:: cell. The out-of-scope test runs Harness::request_gc between its two scopes. The take-after-GC case is covered in traceable_gc.rs.
⚡ Performance: ✅ LGTM
The conversions just call the existing Local implementations.
📝 Documentation: ✅ LGTM
The documentation describes the conversion behaviour and the cell requirement accurately.
Reviewed commit: ea6976b0 — same changes as 6872a33e, so this is that commit's review (no new review was run; line numbers are as of 6872a33e)
A #[jsg_struct] field can now hold a JavaScript value, as a C++
JSG_STRUCT field of type jsg::JsRef does. Such a field is a
jsg::v8::TracedGlobal, which now implements Type, ToJS and FromJS
and converts as a v8::Local<'s, M> does:
undefined, as with jsg::JsRefjsg::JsValue.
Local) takes only a value of that type, with the Local's TypeError
("Expected Object but got number").
The macro needed no change: it already converts each field through its
own type and traces every field. So a struct with such a field holds GC
references, and a resource that keeps it traces the value, so a cycle
through it is collected.
Porting MemoryCache (#8066) needs this. Its fallback produces a
CacheValueProduceResult, whose value is a jsg::JsRefjsg::JsValue.
The Rust port reads that object by hand today. With this change, and
with typed promises (#8153), it can be a struct carried by a
jsg::Promise<'s, T>. That needs a 'static T, so the field is a
TracedGlobal and not a Local with the struct's lifetime.
Tests cover keeping the value's identity (with and without
fast_jsg_struct), any value and missing properties, a rejected
non-object, a struct kept past the scope it was converted in, and the
collection of a cycle through such a field. No JS-visible change;
nothing outside tests uses this yet.