fix(runtime): honor defineProperty prototype index setters (from #9339) - #9392
Merged
Conversation
12 tasks
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe runtime now observes indexed descriptors defined on ChangesArray index descriptor fix
Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ Finishing Touches📝 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands #9339, whose branch conflicts against current
mainon GitHub's three-way merge (it cherry-picks cleanly). Author's commits preserved.It was blocked until now for a real reason: it depends on the
array_spec_setcall-site wiring andprototype_already_checkedthat the #9326 revert removed. #9370 reapplied that, so this applies cleanly again.Validation. All lint gates pass.
perry-runtime8 suites green underRUST_TEST_THREADS=1. Both relevant gap fixtures match the pinned Node 26.5.1 oracle byte-for-byte:test_gap_9220_9221_array_proto_pathsandtest_gap_typedarray_buffer_aliasing_7219(the latter matters because #9360 regresses it, and this confirms #9339 does not).I also wrote an independent probe for the behaviour it claims: an index setter installed on a prototype via
definePropertynow runs ona[3] = …, the read returns the getter's value, and no own property is created —setter saw: written | read: G | own: false, matching node exactly.One observation, not a blocker. In that probe I hit a module-goal difference rather than a defect in this PR. A
.tsfile with no import/export is CommonJS tonode --experimental-strip-types(sloppy mode), so a store blocked by a non-writable inherited index fails silently; Perry compiles it as a module, so strict mode throws. Addexport {}and both throw, agreeing on semantics and differing only in how node formats the error. Worth a separate look at whether Perry should follow node's module-goal inference for extensionless-import-free files, but it is independent of this change and this PR is what makes the strict-mode throw reachable at all.Summary by CodeRabbit
Bug Fixes
Array.prototypeorObject.prototype.TypeErrorhandling.Tests
defineProperty,defineProperties, andReflect.defineProperty.