fix(runtime): a class static setter receives the assigned value, not the class (#11669) - #11672
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughStatic accessor closures now use dedicated getter and setter thunks. They carry the declaring class ID and establish static accessor context when called. Static accessor installation uses the new constructor, and a regression test covers assignment and reflection-related setter paths. ChangesStatic Accessor Invocation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change restores passing the assigned value to class static setters. No concrete merge-blocking defect was identified in the supplied context. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Static setters now receive the assigned value while retaining the declaring class’s context. Review found no demonstrated expansion of authority, but the change touches security-relevant runtime state and exception behavior. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e95e17c to
035befc
Compare
Keep the static accessor thunks of this branch; main fixture #11669 retained.
1 participant
Closes #11669
Root cause
#11651 made class static accessors real accessor properties of the class function object.
install_declared_static_accessor(object/class_value.rs) fills each pair's reflected closure halves withclass_accessor_function_value. That builds the instance thunks, which call the compiled entry asraw(this)/raw(this, value). A compiled static accessor has nothisparameter: it isfn() -> f64/fn(value), and it readsthisfrom the static-this override. So a static setter invoked through its closure half received the receiver, meaning the class itself, as its value, and never hadthisarmed.Before #11651 those closures existed only for descriptor reflection, which is why nothing noticed. Now the class function object's own accessor property is what the generic
[[Set]]paths invoke. An instrumented build confirmed thatStatics.sv = 2(js_put_value_set_packed_miss) reachesclass_accessor_setter_thunk. The same thing happens through a dynamic receiver (o.tag = v), an inherited setter on a subclass,Reflect.set,Object.assign, andObject.getOwnPropertyDescriptor(C, k).set.call(C, v).Fix
The static accessor halves now get static thunks (
class_static_accessor_{getter,setter}_thunk, object/class_registry/registration.rs). They followclass_value::class_static_accessor_call_{get,set}'s protocol: arm staticthiswith the receiver, push the declaring class as the private-static owner (a class id kept as a Number capture), and callfn()/fn(value).class_accessor_source_func_ptrrecognises them too, sotoStringof a reflected static accessor is unchanged. #11651's design is untouched: static accessors are still accessor properties of the class function object.Tests
New
test-files/test_gap_11669_class_static_setter_value.tscovers direct, dynamic-receiver, computed-key and inherited (subclass) writes. It also covers a private static reached from a setter invoked through a subclass, a setter with areturn, the reflected.set.call(C, v)/.get.call(Sub),Reflect.set,Object.assign, and a write after a genericdefineProperty. On pristine main it fails (NaN,set Base class Base {…},this.namewrong forSub). With the fix it is byte-identical to Node 26.5.1.Validation
Linux x86_64 (perrymaster), release with
CODEGEN_UNITS=16,PERRY_NO_AUTO_OPTIMIZE=1, Node 26.5.1 (/opt/node-v26.5.1-linux-x64). The pristine arm isorigin/mainb0bf0ae, built the same way.test_gap_*whose name contains class, static, define_property or accessor, including feat(runtime): class static accessors are accessor properties of the class function object #11651's three tests), compiled, run, and byte-compared to Node:test_gap_10480_define_property_generic_descriptor_accessors,test_gap_11499_class_object_static_writeand the new test, which fail on main and pass with the fix;test_gap_2159_defineproperty_class_prototypefails on both arms (known_failures.json).RUST_TEST_THREADS=1 cargo test --release -p perry-runtime: 4798 passed, 0 failed.cargo fmt --checkandcheck_file_size.shpass.run_lint_gates.sh(SKIP_COMPILE_GATES=1): 103 of 105 pass. The two failures are the known ones:cargo xwin check, because cargo-xwin is not installed on the host, and "Public benchmark evidence freshness", which is red on main.Not run:
cargo test -p perry-codegen, because codegen is untouched;Summary by CodeRabbit
Reflect.set,Object.assign, and property definitions.