add instance/prototype properties to rust JSG by anonrig · Pull Request #6401 · cloudflare/workerd

anonrig · 2026-03-24T17:55:47Z

No description provided.

ask-bonk

I'm Bonk, and I've done a quick review of your PR.

This PR adds #[jsg_prototype_property], #[jsg_instance_property], and #[jsg_inspect_property] macro support to the Rust JSG layer, mirroring the corresponding C++ JSG_PROTOTYPE_PROPERTY, JSG_INSTANCE_PROPERTY, JSG_LAZY_INSTANCE_PROPERTY, and JSG_INSPECT_PROPERTY macros.

Issues found (ranked by severity):

[HIGH] extract_name_attribute silently drops name override when lazy flag is present. #[jsg_instance_property(lazy, name = "cachedLabel")] — the documented combined form — does not work.
[MEDIUM] Lazy instance properties don't actually cache the getter result — the Rust side uses SetAccessorProperty instead of C++'s SetLazyDataProperty, so the getter fires on every access. The user-facing docs claim caching behavior.
[LOW] Inspect properties don't register in the inspectProperties dictionary — node:util inspect() may not discover Rust-defined inspect properties.

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

src/rust/jsg-macros/lib.rs

src/rust/jsg/ffi.c++

ask-bonk · 2026-03-24T18:03:57Z

Review posted successfully. Here's a summary of what I found:

3 issues filed on PR #6401 (all posted as inline comments in a single batch review):

[HIGH] extract_name_attribute drops name when combined with lazy — #[jsg_instance_property(lazy, name = "cachedLabel")] silently drops the name override because extract_name_attribute tries to parse the entire token stream as a single syn::MetaNameValue, which fails when lazy is present. Suggestion provided with a fix that scans comma-separated meta items.
[MEDIUM] Lazy instance properties don't cache — The Rust implementation uses SetAccessorProperty instead of V8's SetLazyDataProperty (due to callback ABI mismatch), so the getter fires on every access rather than once-and-cache. The user-facing docs claim caching behavior.
[LOW] Missing inspectProperties dictionary registration — The C++ side registers inspect property symbols in a lookup dictionary so node:util inspect() can discover them. The Rust side skips this step.

github run

src/rust/jsg/ffi.c++

jasnell · 2026-03-26T19:59:57Z

/bigbonk review this PR again

ask-bonk

Follow-up review after lazy support removal and inspect properties fix.

Previously flagged issues — status:

[HIGH] extract_name_attribute + lazy — No longer applicable. lazy has been removed; #[jsg_property] now uses parse_jsg_property_args which properly handles comma-separated meta items. extract_name_attribute is only called where name = "..." is the sole argument.
[MEDIUM] Lazy properties don't cache — Resolved. Lazy support removed per jasnell's feedback.
[LOW] Missing inspectProperties dictionary — Fixed. Lines 768-773 create the ObjectTemplate and line 845 registers each inspect symbol.
jasnell's NewFromUtf8 length check — Addressed with KJ_REQUIRE at line 779.

New findings below (posted as inline comments).

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

ask-bonk

Inline findings from follow-up review.

src/rust/jsg/ffi.c++

src/rust/jsg-test/tests/resource_properties.rs

ask-bonk · 2026-03-26T20:06:30Z

Review posted on PR #6401 with 4 inline comments. Here's a summary:

Previously flagged issues (all resolved):

extract_name_attribute + lazy — no longer applicable, lazy removed
Lazy properties don't cache — resolved, lazy support removed per jasnell's feedback
Missing inspectProperties dictionary — fixed (lines 768-773 + 845)
jasnell's NewFromUtf8 length check — addressed with KJ_REQUIRE at line 779

New findings:

[MEDIUM] Prototype properties use DontEnum unconditionally (ffi.c++:810, ffi.c++:814) — C++ registerPrototypeProperty uses v8::PropertyAttribute::None for normal (enumerable) properties, meaning they appear in for...in loops and getOwnPropertyDescriptor(...).enumerable === true. The Rust side always sets DontEnum, which is a behavioral difference. Suggested fix: use None for read-write and ReadOnly for read-only (without DontEnum).
[LOW] Spec-compliant makePropFn passes signature (ffi.c++:793) — C++ passes an empty signature for spec-compliant property getters/setters, while Rust passes the constructor's signature. Minor correctness difference unlikely to matter in practice.
[LOW] Missing enumeration test (resource_properties.rs:342) — Test verifies Object.keys() is empty (which is true for any prototype property regardless of DontEnum) but doesn't test for...in or getOwnPropertyDescriptor().enumerable which would reveal the attribute mismatch.

github run

src/rust/jsg-macros/lib.rs

src/rust/jsg/ffi.c++

src/rust/jsg/lib.rs

src/rust/jsg/README.md

src/rust/jsg/v8.rs

src/rust/jsg-macros/lib.rs

…Rust JSG - Prototype read-write properties were registered with DontEnum; they should use None (enumerable) to match C++ registerPrototypeProperty which uses Gcb::enumerable = true (resource.h:1454-1455). - Prototype read-only properties were registered with ReadOnly|DontEnum; they should use ReadOnly only, matching registerReadonlyPrototypeProperty (resource.h:1498-1501) with Gcb::enumerable = true. - The spec-compliant path of makePropFn was passing the constructor signature to FunctionTemplate::New; C++ passes an empty v8::Local<v8::Signature>() for spec-compliant prototype properties (resource.h:1438-1441). - Add three new tests that assert prototype properties are enumerable and appear in for...in, providing regression coverage for the DontEnum fix.

- PropertyKind::Prototype doc said 'Not directly enumerable' which is wrong after the DontEnum fix; update to say enumerable. - jsg_inspect_property docstring claimed no get_/set_ stripping occurs for the auto-derived symbol name, but derive_js_name() does strip those prefixes; correct the description to match the implementation. - scan_property_annotations used BTreeMap which sorted property groups alphabetically, causing registration order to differ from source order. Replace with a Vec-based find-or-insert to preserve source order. - methods and static_constants loops in create_resource_template were missing the KJ_REQUIRE length check added for properties; add it for consistency. - Remove redundant static_cast<v8::PropertyAttribute> wrapping a single enum value in the read-only instance property branch. - Remove dead 'let _ = p' binding in the readonly match arm of parse_jsg_property_args; p is already consumed by the guard expression.

…lifetime

codspeed-hq · 2026-03-30T14:25:39Z

Merging this PR will improve performance by 25.38%

⚡ 1 improved benchmark
✅ 69 untouched benchmarks
⏩ 129 skipped benchmarks¹

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
⚡	`bodyWithHeaders[Response]`	38.7 µs	30.8 µs	+25.38%

_{Comparing yagiz/add-properties-rust-jsg (db3380b) with main (ecfaecd)}

129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

anonrig requested review from guybedford and jasnell March 24, 2026 17:55

anonrig requested review from a team as code owners March 24, 2026 17:55

ask-bonk bot reviewed Mar 24, 2026

View reviewed changes

src/rust/jsg-macros/lib.rs Show resolved Hide resolved

src/rust/jsg-macros/lib.rs Outdated Show resolved Hide resolved

src/rust/jsg/ffi.c++ Outdated Show resolved Hide resolved

src/rust/jsg/ffi.c++ Show resolved Hide resolved

anonrig force-pushed the yagiz/add-properties-rust-jsg branch from 036c771 to d9f1930 Compare March 24, 2026 18:50

jasnell reviewed Mar 25, 2026

View reviewed changes

src/rust/jsg/ffi.c++ Outdated Show resolved Hide resolved

jasnell reviewed Mar 25, 2026

View reviewed changes

src/rust/jsg/ffi.c++ Outdated Show resolved Hide resolved

anonrig force-pushed the yagiz/add-properties-rust-jsg branch 4 times, most recently from 127a643 to 9f7bd00 Compare March 26, 2026 18:59

anonrig requested a review from jasnell March 26, 2026 18:59

anonrig enabled auto-merge (squash) March 26, 2026 18:59

anonrig force-pushed the yagiz/add-properties-rust-jsg branch from 9f7bd00 to f4a70ad Compare March 26, 2026 19:12

ask-bonk bot reviewed Mar 26, 2026

View reviewed changes

src/rust/jsg/ffi.c++ Outdated Show resolved Hide resolved

src/rust/jsg/ffi.c++ Outdated Show resolved Hide resolved

src/rust/jsg/ffi.c++ Outdated Show resolved Hide resolved

src/rust/jsg-test/tests/resource_properties.rs Show resolved Hide resolved

jasnell disabled auto-merge March 26, 2026 20:36

anonrig enabled auto-merge (rebase) March 27, 2026 15:37