-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(runtime): the three gap tests keeping main red #10387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| ### Fixed | ||
|
|
||
| - `JSON.stringify` of an array built by a runtime helper that writes its element | ||
| slots directly no longer emits `null` for every non-number element. | ||
| `JSON.stringify([...Map.groupBy("aba", ch => ch).entries()])` answered | ||
| `[["a",[null,null]],["b",[null]]]` instead of `[["a",["a","a"]],["b",["b"]]]`, | ||
| while `arr[0]` and `String(arr)` on the same array stayed correct — so only a | ||
| JSON round-trip could see it. | ||
|
|
||
| `js_array_alloc` stamps `GC_ARRAY_RAW_F64_LAYOUT` ("every live slot is an | ||
| unboxed double") on the fresh, empty array, where it is vacuously true. A | ||
| producer such as `groupby.rs`'s `group_by_make_array` then sets `length` and | ||
| `std::ptr::write`s the element words itself, bypassing every noting store | ||
| helper that would have cleared the flag. That mislabelling was harmless until | ||
| `json::stringify_primitive_array` started taking the flag as proof and | ||
| emitting each slot through `write_number`: a NaN-boxed string read as a double | ||
| is non-finite, and JSON renders non-finite as `null`. | ||
| `test_gap_2899_2779_2777_static_helpers` went red on `main` in the window that | ||
| introduced that reader. | ||
|
|
||
| `object::gc_slots::rebuild_array_layout_from_slots` — the choke point every | ||
| such producer already calls to repair the GC pointer bitmap — now re-derives | ||
| the numeric-layout flag from the same slots it just walked. The reclassify is | ||
| clear-only, so an array that really is all-numbers keeps its fast path, and no | ||
| receiver that used to decline now passes. `test_gap_2899_2779_2777_static_helpers` | ||
| gained JSON coverage for string, boolean, object, mixed and numeric group | ||
| values, plus the non-JSON readback beside it so a future failure says which | ||
| half broke. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| ### Fixed | ||
|
|
||
| - A replaced `%ArrayIteratorPrototype%.next` (and its Map / Set / String | ||
| siblings) now drives spread, `Array.from` and call-spread, not just `for…of` | ||
| and a manual `.next()`. `[...[4, 5]]` under a patch that doubles each value | ||
| printed `4,5`; `[...new Set([1, 2])]` printed `1,2`; `[..."ab"]` printed | ||
| `a,b`; `f(...[13, 14])` passed the raw elements. | ||
|
|
||
| Each of these ends in a runtime element-COPY arm — `dense_spread_source`'s | ||
| memcpy (#7533), `js_set_to_array` / `js_map_entries`, | ||
| `js_string_to_char_array`, `js_array_like_to_array`'s array fast path — that | ||
| materializes the result without ever calling `.next()`, so the per-call | ||
| "is `next` still the builtin?" proof in `object/iterator_prototypes.rs` cannot | ||
| be reached from inside them. This is the same hole #10086 closed for the | ||
| `for…of` index loop and the array-destructuring fast arm, and it is closed the | ||
| same way: the family prototype object can only be patched after it has escaped | ||
| to user code through `Object.getPrototypeOf` / `Reflect.getPrototypeOf`, so | ||
| that escape is the choke point. `note_iterator_prototype_exposed` (was | ||
| `note_array_iterator_prototype_exposed`) now recognises the Map, Set and String | ||
| family prototypes and `%IteratorPrototype%` itself as well as the array one, | ||
| and the copy arms decline on their family's signal and run the real protocol. | ||
|
|
||
| The array arms move from the narrow `array_proto_iterator_modified` (the | ||
| `Symbol.iterator` slot was written) to the broader | ||
| `array_iteration_not_pristine`. The narrow fact implies the broad one, so | ||
| every receiver that declined before still declines. Nothing changes for a | ||
| program that never introspects a built-in iterator: all four signals stay | ||
| false until `Object.getPrototypeOf` hands the prototype out. | ||
|
|
||
| `test_gap_iterator_prototype_next_patch` has been red on `main` since it | ||
| landed on 2026-09-06 (gap-suite shard log for `87dc334920` — the first run that | ||
| contained it — already reported `pass -> parity_fail`), so this is a | ||
| first-time fix of a fixture that over-specified the implementation, not a | ||
| regression repair. The fixture and its | ||
| `crates/perry/tests/issue_9846_iterator_prototype_next_patch.rs` companion | ||
| gained `Array.from(array)`, call-spread, multi-operand spread, | ||
| `Array.from(set)`, `Array.from(map)`, `[...map]` and `Array.from(string)` | ||
| cases under the same patches. | ||
|
|
||
| The fixture also could not pass for a reason that was not Perry's: it called | ||
| `console.log` while a built-in iterator prototype was patched. Node builds | ||
| `SafeMap` out of `internal/per_context/primordials` lazily, and the parity | ||
| harness runs the oracle under `FORCE_COLOR=0`, which is the path that defers | ||
| that construction into the patched window — so the ORACLE died with | ||
|
|
||
| node:internal/per_context/primordials:449 | ||
| class SafeMap extends Map {}, | ||
|
|
||
| leaving `Node exit: 1, Perry exit: 0` however the runtime behaved. Output is now | ||
| buffered inside each patched window and flushed after the prototype is restored; | ||
| every value is still computed inside the window, which is the subject, and the | ||
| emitted text is byte-identical to the unbuffered run. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| ### Fixed | ||
|
|
||
| - `new SuppressedError(...) instanceof Error` is `true` again, and | ||
| `Object.getPrototypeOf(SuppressedError.prototype) === Error.prototype` / | ||
| `Object.getPrototypeOf(SuppressedError) === Error` now hold as ECMA-262 | ||
| requires. | ||
|
|
||
| `SuppressedError` was missing from `is_native_error_subclass_constructor`, so | ||
| the `globalThis` population loop never linked its prototype pair into the | ||
| Error family — `SuppressedError.prototype`'s `[[Prototype]]` stayed | ||
| `Object.prototype`. That was latent while `instanceof Error` answered from the | ||
| class registry (`extends_builtin_error(CLASS_ID_SUPPRESSED_ERROR)`, which | ||
| `js_suppressed_error_new` registers). It stopped being latent when | ||
| `js_instanceof` began consulting the instance's RECORDED prototype chain FIRST | ||
| for `class_id == CLASS_ID_ERROR`: the walk reached a chain with no | ||
| `Error.prototype` in it, returned `Some(false)`, and short-circuited the | ||
| registry fact that used to carry the answer. `test_gap_disposablestack_2875` | ||
| went red on `main` between `87dc334920` and `f207cf6618` — the window | ||
| containing that change — and stayed red. | ||
|
|
||
| The fix is the missing link itself, not a special case in `instanceof`: the | ||
| recorded chain is now correct, so the walk answers `true` on its own and | ||
| `Error.prototype.toString` is inherited (`String(err)` is | ||
| `"SuppressedError: both failed"`). `test_gap_disposablestack_2875` gained | ||
| direct assertions for both prototype links, for the `TypeError` control, and | ||
| for the inherited `toString`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -966,6 +966,14 @@ pub(crate) fn array_from_spread_value(value: f64) -> *mut ArrayHeader { | |
| throw_not_iterable(value()); | ||
| } | ||
| if jsv.is_any_string() { | ||
| // #9846: `[..."ab"]` is `GetIterator(str)` + drain per spec, and | ||
| // `js_string_to_char_array` is an element cut that never calls | ||
| // `%StringIteratorPrototype%.next`. That is unobservable — until the | ||
| // prototype object escapes to user code, after which the cut would | ||
| // silently ignore a patched `next`. Decline then, and run the protocol. | ||
| if crate::object::iterator_prototypes::string_iteration_not_pristine() { | ||
| return js_iterator_to_array(crate::symbol::js_get_iterator(value())); | ||
| } | ||
| let str_ptr = crate::value::js_get_string_pointer_unified(value()); | ||
| let str_bits = crate::value::STRING_TAG | (str_ptr as u64 & POINTER_MASK); | ||
| return crate::string::js_string_to_char_array(str_bits as i64) as *mut ArrayHeader; | ||
|
|
@@ -1038,7 +1046,7 @@ pub(crate) fn array_from_spread_value(value: f64) -> *mut ArrayHeader { | |
| // not preempt the walk below for that case: `js_get_iterator`'s patched | ||
| // branch reads the PROTOTYPE only, and would throw "not iterable" for an | ||
| // array carrying its own method once the prototype slot has been deleted. | ||
| if crate::array::array_proto_iterator_modified() | ||
| if crate::array::array_iteration_not_pristine() | ||
| && crate::array::js_array_is_array(value()).to_bits() == crate::value::TAG_TRUE | ||
| && !array_has_own_iterator(value()) | ||
| { | ||
|
|
@@ -1051,10 +1059,21 @@ pub(crate) fn array_from_spread_value(value: f64) -> *mut ArrayHeader { | |
| if crate::buffer::is_registered_buffer(raw_ptr()) { | ||
| return crate::buffer::buffer_to_array(raw_ptr() as *const crate::buffer::BufferHeader); | ||
| } | ||
| // #9846: the Set / Map arms below copy the backing store instead of | ||
| // driving `%SetIteratorPrototype%.next` / `%MapIteratorPrototype%.next`. | ||
| // Same trade as the array dense arm and the string cut above: free while | ||
| // the family prototype has never escaped to user code, wrong the moment it | ||
| // has, so decline on the escape signal and run the real protocol. | ||
| if crate::set::is_registered_set(raw_ptr()) { | ||
| if crate::object::iterator_prototypes::set_iteration_not_pristine() { | ||
| return js_iterator_to_array(crate::symbol::js_get_iterator(value())); | ||
|
Comment on lines
+1068
to
+1069
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '1010,1120p' crates/perry-runtime/src/array/iterator.rs
sed -n '270,330p' crates/perry-runtime/src/object/map_set_subclass.rs
sed -n '285,450p' crates/perry-runtime/src/symbol/iterator.rs
rg -n 'subclass_backing_for_default_iteration|subclass_has_iterator_override|map_iteration_not_pristine|set_iteration_not_pristine|js_map_entries|js_set_to_array|array_from_spread_value' crates/perry-runtime/srcRepository: PerryTS/perry Length of output: 29039 🏁 Script executed: sed -n '1,135p' crates/perry-runtime/src/object/iterator_prototypes.rs
sed -n '920,1120p' crates/perry-runtime/src/array/iterator.rs
sed -n '130,190p' crates/perry-runtime/src/symbol/iterator.rs
sed -n '285,385p' crates/perry-runtime/src/symbol/iterator.rs
sed -n '60,155p' crates/perry-runtime/src/array/from_concat.rsRepository: PerryTS/perry Length of output: 32400 Route dirty Map/Set subclasses through the iterator protocol. When the matching Proposed change Some(crate::object::map_set_subclass::CollectionBacking::Map(m)) => {
+ if crate::object::iterator_prototypes::map_iteration_not_pristine() {
+ return js_iterator_to_array(crate::symbol::js_get_iterator(value()));
+ }
return crate::map::js_map_entries(m as *const crate::map::MapHeader);
}
Some(crate::object::map_set_subclass::CollectionBacking::Set(s)) => {
+ if crate::object::iterator_prototypes::set_iteration_not_pristine() {
+ return js_iterator_to_array(crate::symbol::js_get_iterator(value()));
+ }
return crate::set::js_set_to_array(s as *const crate::set::SetHeader);
}
🤖 Prompt for AI Agents |
||
| } | ||
| return crate::set::js_set_to_array(raw_ptr() as *const crate::set::SetHeader); | ||
| } | ||
| if crate::map::is_registered_map(raw_ptr()) { | ||
| if crate::object::iterator_prototypes::map_iteration_not_pristine() { | ||
| return js_iterator_to_array(crate::symbol::js_get_iterator(value())); | ||
| } | ||
| return crate::map::js_map_entries(raw_ptr() as *const crate::map::MapHeader); | ||
| } | ||
| // `class X extends Map | Set` instance — spread (`[...container]`, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the development-history paragraph.
Lines 30-38 describe when a fixture first failed and classify the fix. This is not shipped behavior. Replace it with a concise statement of the added regression coverage.
Proposed change
Based on learnings: changelog fragments must describe final shipped behavior as one coherent release-note entry.
📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Learnings