test(parity): pin key order across every way an object reaches a layout (#10868 stage 0) - #10919
proggeramlug wants to merge 1 commit into
Conversation
…ut (#10868 stage 0) Step 2.5 of #10868 makes shape identity content-canonical, which changes identity for every object in every program. Key ORDER is observable in JS, so a canonicalisation that merged two layouts with the same key SET but different order -- or a deleted-then-re-added key with a never-deleted one -- is a silent wrong answer everywhere, not a slow one. These tests land BEFORE any identity change so that every later slice is checked against them rather than written alongside them. Eleven scenarios, 75 output rows, each printed through Object.keys, for-in, Reflect.ownKeys and JSON.stringify: born vs grown; same set in two orders; tombstone then re-add; mid-list delete; born-wide vs grown-into-spill (the pair the design's birth-sizing precondition exists for); integer-like keys before string keys; defineProperty with enumerable:false; accessor position; symbols after strings; a null-prototype object; and two literal sites with equal key lists. Proven able to fail. The four views reach the key list by three different paths, so one sabotage was not enough; each was applied to the runtime and reverted, and the file's header records which proves which: keys= / forin= reverse js_object_keys' result 37 lines red own= reverse js_object_get_own_property_names 19 rows red, own= only json= reverse the keys array at publication 9 json= rows red (json/stringify_* read object_keys_array directly and bypass both functions above) The row that matters most is ab/ba: under the first sabotage both come back in the same order, i.e. it merges the two layouts this file exists to keep apart.
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with 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 (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughAdds a parity test file for canonical object shape identity and property order. The tests compare literal, grown, deleted, wide, and property-definition scenarios across multiple observable object views. ChangesShape Identity Order
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Other 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
|
Landed on A changelog fragment was written for it in the train ( The train was validated as one tree: all ratchets, |
1 participant
Stage 0 of #10868 step 2.5 (canonical shape identity). One new parity file, no runtime change.
Why identity tests land before any identity change: step 2.5 changes shape identity for every object in every program, and key order is observable in JS — so a canonicalisation that merged two layouts with the same key set but different order is a silent wrong answer everywhere, not a slow one. These tests exist first so every later slice is checked against them rather than written alongside them.
What the eleven scenarios pin
75 output rows, each scenario printed through
Object.keys,for-in,Reflect.ownKeysandJSON.stringify:INLINE_SLOT_FLOOR; same layout, different physical placement (the pair L8.3.7's birth-sizing precondition exists for)definePropertywithenumerable:falseis inownKeys, not inkeysownKeysProven able to fail
A row that cannot redden is documentation. The four views reach the key list by three different paths, so one sabotage was not enough; each was applied to the runtime, run, and reverted, and the file's header records which proves which so a reviewer can re-run it:
keys=/forin=js_object_keysown=js_object_get_own_property_names(viaReflect.ownKeys)own=onlyjson=object_keys_arrayread directly by thejson/stringify_*serialisersjson=rowsThe row that matters most is
ab/ba: under the first sabotage both come back in the same order — i.e. the sabotage merges the exact two layouts this file exists to keep apart. Under the third,holeprints{"d":1,"b":3,"a":4}— keys reversed, values left in their slots — which is the silent-wrong-value failure a wrong canonicalisation would produce.The
json=rows needed their own sabotage becauseJSON.stringifybypasses both enumeration functions; the first two sabotages left everyjson=row green. Reported rather than left for review: until the third sabotage existed those rows were documentation, not tests.Verification
Byte-identical to node on pristine
main@ v0.5.1632 (75 rows), built from its own tree — not only on the stacked branches where step 2.5 is being developed.Summary by CodeRabbit