-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix: Error subclasses get .stack and [object Error]; a CommonJS entry keeps Node's ticks-first ordering (#9410, #9412) #9432
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
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| ### Fixed | ||
|
|
||
| - **An `Error` subclass now has a `.stack` and reports `[object Error]`.** | ||
| `class A extends Error {}` produced instances whose `.stack` was `undefined` | ||
| and whose `Object.prototype.toString` tag was `"[object Object]"`. The base | ||
| class was fine — `new Error("x").stack` has always been a string — so only | ||
| subclasses were affected, and the claude-code bundle has **93** of them and | ||
| **106** `.stack` reads. `claude doctor` printed ~10 real frames and 14,573 | ||
| bytes of stderr under node; under perry it printed ` - at <anonymous>` | ||
| and 120 bytes. Silent: no error, just a missing trace. | ||
|
|
||
| One root cause behind both symptoms. `class A extends Error {}` deliberately | ||
| produces an ordinary `GC_TYPE_OBJECT` class instance rather than a | ||
| `GC_TYPE_ERROR` `ErrorHeader`, so that the subclass's own fields have | ||
| somewhere to live. `alloc_error` — the only place that fills | ||
| `ErrorHeader.stack` — is therefore never reached, and neither is any | ||
| `stack` on `Error.prototype`, which carries only `name` and `message`. The | ||
| `[object Error]` branch of `js_object_to_string` is keyed on that same GC | ||
| header byte, so a subclass fell through to the `class_id` block and out the | ||
| `"[object Object]"` default. | ||
|
|
||
| The class-id registry that answers this question already existed and was | ||
| wired at four other sites — `instanceof Error`, `util.types.isNativeError`, | ||
| `Error.prototype.toString`'s subclass arm, and prototype-chain resolution | ||
| all consult `extends_builtin_error(class_id)`. Neither the tag nor the stack | ||
| did. | ||
|
|
||
| - `crates/perry-runtime/src/object/to_string_tag.rs` — tag a | ||
| `extends_builtin_error` class instance `"Error"`, set *before* the | ||
| `Symbol.toStringTag` hook so a subclass's own tag still wins (§20.1.3.6 | ||
| consults the tag property last). | ||
| - `crates/perry-runtime/src/error_subclass_stack.rs` (new; `error.rs` was | ||
| within 90 lines of the 2,000-line CI cap) — `js_error_subclass_capture_stack` | ||
| installs the own, non-enumerable, configurable `stack` accessor node | ||
| installs, capturing the FRAME at the construction site. The head | ||
| (`"name: message"`) is formatted on read, not at capture, because that is | ||
| what V8 does and what the ubiquitous | ||
| `constructor(m) { super(m); this.name = "X" }` shape needs: node reports | ||
| `"X: m"`, and the assignment happens after `super()` returns. A user | ||
| `Error.prepareStackTrace` still wins, as it does for | ||
| `Error.captureStackTrace`. The setter redefines `stack` as a plain data | ||
| property, so `err.stack = ""` keeps working. | ||
| - `crates/perry-runtime/src/object/class_constructors.rs` — install it from | ||
| `js_error_subclass_default_init` (the synthesized standalone ctor, which | ||
| also serves the dynamic-parent `super` path) and from | ||
| `default_error_init_for_implicit_chain` (the dynamic `new` replay), the | ||
| two runtime sites that already stamped `message`/`name` and stopped there. | ||
| In the replay the install is moved above the message guard, which returns | ||
| early for a no-argument `new X()` — exactly the instances that would | ||
| otherwise still have no trace. | ||
| - `crates/perry-codegen/src/expr/this_super_call.rs`, | ||
| `crates/perry-codegen/src/lower_call/new_error_init.rs` (new; the | ||
| static-`new` Error arm moved out of `new.rs`, which was 5 lines from the | ||
| 2,000-line CI gate) — the same call from the two codegen sites that stamp | ||
| `message`/`name` inline: an explicit `super(message)` into a built-in | ||
| Error, and the static-`new` arm for a subclass with no own constructor. | ||
| `this` is reloaded from its slot first; the stamps above it can collect. | ||
|
|
||
| A unit test in the new module installs the accessor under forced evacuation, | ||
| which is the only condition that can expose an unrooted pointer — and which | ||
| caught the first cut of that rooting reading a NaN-box handle back with | ||
| `get_raw_const_ptr`, aborting every Error-subclass construction with | ||
| "runtime handle kind mismatch". Nothing in the unit suite constructed an | ||
| Error subclass before, so only a compiled probe saw it. | ||
|
|
||
| Validation: `test-files/test_gap_9410_error_subclass_stack.ts` | ||
| byte-compared against `node --experimental-strip-types` across a bare | ||
| subclass, a `this.name`-assigning subclass, one with an extra field, a | ||
| two-level subclass, a subclass that sets `message` after an argument-less | ||
| `super()`, `TypeError`/`RangeError` subclasses, a factory-constructed | ||
| instance, a caught throw, `Error.captureStackTrace` on a subclass, and | ||
| controls for the base `Error`, a non-Error class and a plain object. The | ||
| fixture asserts the portable parts of the contract — `typeof stack`, the | ||
| head line, the `toString` tag, `name`/`message`/`instanceof`, and that | ||
| `stack` is an own but non-enumerable property that stays out of | ||
| `Object.keys` — because stack CONTENTS are host-specific. Demonstrated | ||
| failing on a compiler built from unfixed `origin/main` (46 diverging lines). |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| ### Fixed | ||
|
|
||
| - **A `require()` of a builtin no longer demotes `process.nextTick` below | ||
| promise microtasks.** | ||
|
|
||
| ```js | ||
| require("path"); // delete this line and perry matched node | ||
| const o = []; | ||
| process.nextTick(() => o.push("nextTick")); | ||
| Promise.resolve().then(() => o.push("p1")); | ||
| (async () => { await null; o.push("await"); })(); | ||
| setTimeout(() => console.log(JSON.stringify(o)), 20); | ||
| // node: ["nextTick","p1","await"] | ||
| // perry: ["p1","await","nextTick"] (5/5 deterministic) | ||
| ``` | ||
|
|
||
| The deferral itself is correct, and measurement says so: the same file run | ||
| by node 26 as `.cjs` prints `["nextTick","p1","await"]`, as `.mjs` | ||
| `["p1","await","nextTick"]`. An ES module evaluates inside its module job's | ||
| promise chain, so its first tick drain lands after the promise queue — which | ||
| is exactly what `js_mark_entry_module_esm` (#788) models. It was being | ||
| applied to the wrong module kind. | ||
|
|
||
| Entry codegen decided "is this an ES module?" with | ||
| `!hir.imports.is_empty() || !hir.exports.is_empty() || has_top_level_await`. | ||
| A bare `require(` with no top-level `import` classifies the entry as | ||
| CommonJS, and `cjs_wrap` then rewrites it to ESM — injecting | ||
| `import { createRequire as __perry_cjs_create_require } from 'node:module'` | ||
| and `export default _cjs`. Both halves of that predicate became true for | ||
| every CommonJS program. The `require("path")` call itself contributes no | ||
| import at all; it folds to a native-module reference. Every real bundle | ||
| requires a builtin and every minimal fixture does not, so the ordering was | ||
| right in exactly the programs a test suite contains and wrong in exactly the | ||
| programs users run. | ||
|
|
||
| - `crates/perry-codegen/src/collectors/cjs_scaffolding.rs` — | ||
| `is_cjs_wrapped_module`, keyed on the local name the wrap's synthetic | ||
| `createRequire` import binds. Recognised from the HIR, not from an | ||
| expectation about the template: if the wrap stops emitting it the | ||
| predicate degrades to "not wrapped" (today's behaviour) rather than to a | ||
| wrong answer for hand-written ESM, and a user's own | ||
| `import { createRequire } from 'node:module'` is not mistaken for it | ||
| because the match is on the alias, not the specifier. | ||
| - `crates/perry-codegen/src/codegen/entry.rs` — gate only the | ||
| `js_mark_entry_module_esm` call on that. The `is_esm_entry` below it keeps | ||
| its meaning for GlobalDeclarationInstantiation: a CommonJS module's | ||
| top-level `function` declarations live inside the module wrapper and are | ||
| not global-object properties either, so "not a Script" stays the right | ||
| answer there — and that predicate is mirrored in `perry-hir`'s | ||
| `lower_module_fn`, which runs before the wrap flag is knowable in codegen. | ||
| - `crates/perry/src/commands/compile/cjs_wrap/preamble_canary_tests.rs` — | ||
| a template canary in the same family as #7139/#7152: rename the local in | ||
| `wrap.rs` and every CommonJS entry silently goes back to ES-module tick | ||
| ordering with nothing going red. Plus a negative control, so the fix | ||
| cannot drift the other way and give real ESM entries CommonJS ordering. | ||
|
|
||
| Validation: `test-files/test_gap_9412_require_builtin_tick_order.cts` | ||
| byte-compared against node — ticks first, a tick scheduled from inside a tick | ||
| joining the same drain, a tick scheduled from inside a microtask landing | ||
| after it, and a second event-loop turn where no evaluation checkpoint could | ||
| apply. It has to be a `.cts`: this repo is `"type": "module"`, so a plain | ||
| `.ts` is an ES module for node and perry alike and cannot carry the shape | ||
| (#9418 taught the runner to discover `.cts`). | ||
| `test-files/test_gap_9412_entry_tick_order.ts` pins the ESM side so the fix | ||
| cannot be "stop deferring, always". Demonstrated failing on a compiler built | ||
| from unfixed `origin/main`. |
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 12406
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 35330
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 38702
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 19431
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 9516
Use provenance, not the synthetic local name alone. HIR normalizes
node:moduletomoduleand preservesimportedandlocal, so a user import can still usecreateRequire as __perry_cjs_create_require. Matching source and imported name alone does not prevent the collision. Add a synthetic marker or reserve the local before HIR lowering, excludetype_onlyandruntime_erasedimports, and add a regression test.🤖 Prompt for AI Agents