-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(codegen): forward implicit-ctor args to a native base super() reached via require() #10636
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,16 @@ | ||
| Fixed a constructor-less subclass of a native base (`AsyncResource`, | ||
| `AsyncLocalStorage`, `EventEmitter`, `EventEmitterAsyncResource`, `LRUCache`, | ||
| `WebSocketServer`, the genuine `node:stream` classes) losing its `super()` | ||
| argument forwarding and native-surface install inside a CommonJS-wrapped | ||
| module — the shape real npm packages use. `const { AsyncResource } = | ||
| require("node:async_hooks")` is a genuine local there (the whole module body | ||
| runs inside the CJS wrap's IIFE), which class-heritage resolution could not | ||
| tell apart from a real user shadow of the same name, so it fell back to a | ||
| generic dynamic-value dispatch. For a base whose runtime value is a real ES | ||
| `class` (`AsyncResource`, `AsyncLocalStorage`), that dispatch called the value | ||
| without `new` and threw; for an old-style-function base (`EventEmitter`, the | ||
| stream classes) it happened to work, through a much slower indirect path | ||
| (measured ~5.5x more instructions per construction than the direct native | ||
| path). Class-heritage resolution now tracks a `require()`-destructured | ||
| binding's provenance and only treats it as shadowing when it did NOT come | ||
| from the real native module. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -218,6 +218,46 @@ pub(super) fn register_destructured_stream_ctors( | |
| return Vec::new(); | ||
| }; | ||
|
|
||
| // #10623: record the destructuring's PROVENANCE (local binding -> the | ||
| // export key it was destructured from) whenever the RHS resolves to a | ||
| // real native/Node-builtin module — regardless of the #8342 CJS-wrapper | ||
| // gate immediately below. Inside a CJS-wrapped module that gate skips the | ||
| // FULL native-module-alias registration (member reads/calls must fall | ||
| // through to the wrapper's real runtime `require(...)` there), but the | ||
| // destructured identifier is still genuinely bound FROM that native | ||
| // module at runtime. Class-heritage resolution (`class_decl.rs`) needs | ||
| // exactly that narrower fact to avoid treating `class X extends | ||
| // AsyncResource {}` as user-shadowed just because the CJS wrapper makes | ||
| // every top-level `const` a real local — without it, `super()` (explicit | ||
| // or the implicit default derived ctor) fell back to a generic | ||
| // call-the-value dispatch that neither installs the native base's surface | ||
| // nor tolerates bases whose runtime value enforces real ES `class` | ||
| // `[[Call]]` semantics (`AsyncResource` throws "cannot be invoked without | ||
| // 'new'"). | ||
| if require_resolvable_native_specifier(init).is_some() { | ||
| for prop in &obj_pat.props { | ||
| let (key, binding) = match prop { | ||
| ast::ObjectPatProp::Assign(assign) => { | ||
| let name = assign.key.sym.to_string(); | ||
| (name.clone(), name) | ||
| } | ||
| ast::ObjectPatProp::KeyValue(kv) => { | ||
| let key = match &kv.key { | ||
| ast::PropName::Ident(i) => i.sym.to_string(), | ||
| ast::PropName::Str(s) => s.value.as_str().unwrap_or("").to_string(), | ||
| _ => continue, | ||
| }; | ||
| let ast::Pat::Ident(binding) = kv.value.as_ref() else { | ||
| continue; | ||
| }; | ||
| (key, binding.id.sym.to_string()) | ||
| } | ||
| ast::ObjectPatProp::Rest(_) => continue, | ||
| }; | ||
| ctx.require_destructured_native_locals.insert(binding, key); | ||
|
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 | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '210,275p' crates/perry-hir/src/destructuring/var_decl_sources.rs
sed -n '245,290p' crates/perry-hir/src/lower_decl/class_decl.rs
sed -n '1330,1370p' crates/perry-hir/src/lower_decl/class_decl.rs
rg -n -C 3 'require_resolvable_native_specifier|native_parent|native_extends|require_destructured_native_locals' crates/perry-hir/srcRepository: PerryTS/perry Length of output: 50369 🏁 Script executed: sed -n '1,180p' crates/perry-hir/src/destructuring/var_decl_sources.rs
sed -n '180,225p' crates/perry-hir/src/lower_decl/class_decl.rs
sed -n '225,305p' crates/perry-hir/src/lower_decl/class_decl.rs
sed -n '1290,1365p' crates/perry-hir/src/lower_decl/class_decl.rs
sed -n '1,180p' crates/perry-hir/src/lower/tests/issue_10623_require_destructured_native_super.rsRepository: PerryTS/perry Length of output: 27211 Match native provenance by module and export. This map stores only Store the normalized module with the export key. Require both values to match the selected 🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
|
|
||
| // #8342: inside a CJS-wrapped module the wrap's synthetic | ||
| // `function require(...)` shadows the bare global `require`, and its | ||
| // built-in arm resolves `require("process")` etc. via `createRequire` at | ||
|
|
||
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 | 🟠 Major | ⚡ 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: 50370
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 25284
Invalidate native provenance after local reassignment. In the CJS-wrapper path, the var/let/const destructuring helper records
AsyncResource -> AsyncResourceinctx.require_destructured_native_locals.lower_ident_assignmentthen resolvesAsyncResource = UserBaseto the same local and emitsExpr::LocalSetwithout invalidating that provenance. Both class heritage paths use the stale entry to setlocally_shadowedto false, so the example can lowerXwith the nativeasync_hooks.AsyncResourceparent instead of the currentUserBasevalue. Invalidate the provenance for the resolved local inlower_ident_assignment; updating only the class checks does not own the stale state.🤖 Prompt for AI Agents
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 34972
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 6280
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50369
Guard provenance on the resolved
requirebinding.require_resolvable_native_specifierchecks only the callee name and literal specifier. It records provenance beforerequire_is_shadowed_by_localcan return. Therefore, a user-definedrequirecan makeconst { AsyncResource } = require("node:async_hooks")record native provenance even when it returns a user value.class_decl.rsthen suppresseslocally_shadowedand lowers the class againstasync_hooks::AsyncResource, ignoring that value.The existing
require_is_perry_cjs_wrapperhelper pair identifies the intentional CJS-wrapper exception. Allow provenance for an unshadowedrequireor that recognized wrapper only; do not allow it for arbitrary local, function, or importedrequirebindings.🤖 Prompt for AI Agents