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

…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

src/rust/jsg-macros/lib.rs

jasnell · 2026-03-27T15:43:38Z

src/rust/jsg/ffi.c++

+    switch (prop.kind) {
+      case PropertyKind::Prototype: {
+        // Mirrors registerPrototypeProperty / registerReadonlyPrototypeProperty in resource.h.
+        auto getterFn = makePropFn(prop.getter_callback, true);


If we're going to have bool args, let's at least label their callsites.

Suggested change

auto getterFn = makePropFn(prop.getter_callback, true);

auto getterFn = makePropFn(prop.getter_callback, true /* is getter */);

jasnell · 2026-03-27T15:43:54Z

src/rust/jsg/ffi.c++

+        // Mirrors registerPrototypeProperty / registerReadonlyPrototypeProperty in resource.h.
+        auto getterFn = makePropFn(prop.getter_callback, true);
+        KJ_IF_SOME(setterCb, prop.setter_callback) {
+          auto setterFn = makePropFn(setterCb, false);


Suggested change

auto setterFn = makePropFn(setterCb, false);

auto setterFn = makePropFn(setterCb, false /* is getter */);

jasnell · 2026-03-27T15:44:40Z

src/rust/jsg/ffi.c++

+    KJ_REQUIRE(constant.name.size() <= static_cast<size_t>(v8::String::kMaxLength),
+        "Rust constant name exceeds V8 string length limit", constant.name);
    auto name = ::workerd::jsg::check(v8::String::NewFromUtf8(
        isolate, constant.name.data(), v8::NewStringType::kInternalized, constant.name.size()));


This is duplicated a couple of times, likely worth putting into a utility method.

jasnell · 2026-03-27T15:45:30Z

src/rust/jsg/lib.rs

+    /// Registered under a unique symbol on the prototype. Invisible to normal
+    /// enumeration and string-key lookup; surfaced by `node:util` `inspect()`.
+    Inspect,
+}


The fact this enum is duplicated here and in jsg-macros/lib.rs is a bit annoying. Any chance to condense?

Unfortunately not. There is a comment before jsg-macros/lib.rs enum declaration.

A shared "types" crate could be used to define these without duplication, could it not? A bit more work on build but easier to maintain in one location. Also, that explains why this and the one in jsg-macros are duplicated, but this one and the third copy in v8.rs can be deduplicated. And, if nothing else, all three should cross reference each other with a comment reminding folks coming into this code in the future that the three need to be kept in sync.

marking this unresolved because the two definitions in lib.rs and v8.rs ought to be able to be deduplicated.

Keep the CXX bridge definition in v8.rs

Replace the standalone pub enum PropertyKind in lib.rs with a re-export:
pub use v8::ffi::PropertyKind;

Eliminate the manual conversion in resource.rs:936–940 that currently maps between the two:
// This entire match goes away: let ffi_kind = match kind { PropertyKind::Prototype => v8::ffi::PropertyKind::Prototype, PropertyKind::Instance => v8::ffi::PropertyKind::Instance, PropertyKind::Inspect => v8::ffi::PropertyKind::Inspect, };

jasnell · 2026-03-27T15:46:43Z

src/rust/jsg/README.md

+struct Counter { value: Cell<f64> }
+
+#[jsg_resource]
+impl Counter {


This appears to be duplicating the content in jsg-macros/README.md ... or that is duplicating this content. Either way, we should condense this.

jasnell · 2026-03-27T15:47:21Z

src/rust/jsg/v8.rs

+        Instance = 1,
+        /// Registered under a unique symbol; invisible to normal enumeration.
+        Inspect = 2,
+    }


Now a third definition of the same enum?

guybedford · 2026-03-27T22:21:29Z

src/rust/jsg-macros/lib.rs

+        };
+        let rust_method_name = method.sig.ident.clone();
+        let rust_name_str = rust_method_name.to_string();
+        let is_setter = rust_name_str.starts_with("set_");


We aren't validating here that a property declaration must either start with get_ or set_.

If someone defines #[jsg_property] foo () surely that isn't supported? Or do we want to support that for a getter only variant?

I think it's good to be stricter though and validate invalid patterns to ensure we have a well-defined model.

guybedford · 2026-03-27T22:23:17Z

src/rust/jsg-macros/lib.rs

+/// Parses the argument list of `#[jsg_property(placement [, name = "..."] [, readonly])]`.
+///
+/// `placement` must be either `instance` or `prototype` (required).
+/// Returns `(PropertyKind, Option<js_name_override>, is_readonly)`.
+fn parse_jsg_property_args(
+    tokens: TokenStream,
+) -> Result<(PropertyKind, Option<String>, bool), quote::__private::TokenStream> {


Can we make the placement macro argument optional? I think the default for JS of considering properties as instance properties is a fairly safe one to assume.

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++ 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

add instance/prototype properties to rust JSG

f4a70ad

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 added 3 commits March 27, 2026 11:26

fix clippy: extract find_or_insert as module-level fn, drop needless …

b69811f

…lifetime

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

jasnell reviewed Mar 27, 2026

View reviewed changes

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

jasnell reviewed Mar 27, 2026

View reviewed changes

guybedford reviewed Mar 27, 2026

View reviewed changes

	auto getterFn = makePropFn(prop.getter_callback, true);
	auto getterFn = makePropFn(prop.getter_callback, true /* is getter */);

	auto setterFn = makePropFn(setterCb, false);
	auto setterFn = makePropFn(setterCb, false /* is getter */);

Conversation

anonrig commented Mar 24, 2026

Uh oh!

ask-bonk bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ask-bonk bot commented Mar 24, 2026

Uh oh!

Uh oh!

Uh oh!

jasnell commented Mar 26, 2026

Uh oh!

ask-bonk bot left a comment

Choose a reason for hiding this comment

Uh oh!

ask-bonk bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ask-bonk bot commented Mar 26, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jasnell Mar 27, 2026 •

edited

Loading