Skip to content

add jsg_static_constant to rust jsg#6284

Merged
anonrig merged 1 commit intomainfrom
yagiz/add-static-constant-rust-jsg
Mar 10, 2026
Merged

add jsg_static_constant to rust jsg#6284
anonrig merged 1 commit intomainfrom
yagiz/add-static-constant-rust-jsg

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Mar 9, 2026

Adds static constant support to Rust JSG. Example:

#[jsg_resource]
impl WebSocket {
    #[jsg_static_constant]
    pub const CONNECTING: i32 = 0;

    #[jsg_static_constant]
    pub const OPEN: i32 = 1;

    #[jsg_static_constant]
    pub const CLOSING: i32 = 2;

    #[jsg_static_constant]
    pub const CLOSED: i32 = 3;
}
// In JavaScript:
//   WebSocket.CONNECTING === 0
//   WebSocket.OPEN === 1
//   new WebSocket(...).CLOSING === 2  (also on instances via prototype)

@anonrig anonrig requested review from a team as code owners March 9, 2026 20:15
@anonrig anonrig added the rust Pull requests that update rust code label Mar 9, 2026
@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 9, 2026

Good, confirmed this is PR #6284 by anonrig. I'm reviewing as ask-bonk[bot].

Now let me do my final analysis. Let me review the whole change holistically:

Summary of changes:

  1. New #[jsg_static_constant] proc macro attribute (marker only)
  2. generate_resource_impl in jsg-macros scans for #[jsg_static_constant] on const items
  3. New ConstantValue enum and From impls for numeric types in jsg/lib.rs
  4. New Member::StaticConstant variant
  5. New StaticConstantDescriptor CXX shared struct in v8.rs
  6. C++ FFI code in ffi.c++ that sets the constants on constructor and prototype
  7. Comprehensive tests
  8. Documentation updates

Analysis:

The implementation is well-structured and faithfully mirrors the C++ JSG_STATIC_CONSTANT behavior:

  • Property attributes match (ReadOnly + DontDelete when spec-compliant, ReadOnly otherwise)
  • Sets on both constructor and prototype template — matches C++ exactly
  • Name is used as-is (no camelCase) — correct for constants
  • Only numeric types — matches Web IDL constant semantics

Potential issues I've identified:

  1. In ffi.c++, the constants are set on constructor (FunctionTemplate) and prototype (also from constructor->PrototypeTemplate()). The C++ JSG does constructor->Set(...) and constructor->PrototypeTemplate()->Set(...) — which is exactly the same thing. This is correct.

  2. The let ConstantValue::Number(number_value) = value; destructuring in lib.rs:87 is an irrefutable pattern since there's only one variant. This is fine and will produce a compile error if a new variant is added, forcing updates.

  3. Tests cover: constructor access, instance/prototype access, read-only enforcement, coexistence with methods. Good coverage.

  4. The ConstantValue enum could potentially benefit from #[non_exhaustive] if it's intended to be extended with string or boolean constants later, but since this is all within the same crate, it doesn't matter. The enum is public though — worth noting but not actionable.

  5. Documentation is comprehensive and consistent across all modified docs.

I don't see any actionable issues — no bugs, no safety concerns, no backward compat problems, no missing compat flags. The implementation correctly matches the C++ JSG behavior.

LGTM

github run

@anonrig anonrig enabled auto-merge March 9, 2026 20:18
@anonrig anonrig requested a review from mikea March 9, 2026 20:22
@anonrig anonrig force-pushed the yagiz/add-static-constant-rust-jsg branch from 05c558d to fadfb26 Compare March 9, 2026 23:01
@anonrig anonrig merged commit ccca821 into main Mar 10, 2026
47 of 49 checks passed
@anonrig anonrig deleted the yagiz/add-static-constant-rust-jsg branch March 10, 2026 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants