-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(runtime): honor AsyncResource.bind and other own function overrides #10046
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
52ee277
4e3a591
52941fa
57e2abd
ae054d1
8978f00
901ee7a
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,19 @@ | ||
| Fix dynamic calls to a closure's own `bind`, `call`, `apply`, and `toString` | ||
| properties. These overrides now take precedence over Function.prototype's | ||
| intrinsic dispatch, including native constructors such as AsyncResource and | ||
| AsyncLocalStorage with their own static `bind` methods. Own accessors are | ||
| invoked once, and non-callable own properties throw instead of falling through | ||
| to an intrinsic. Ordinary Function.prototype fast paths remain unchanged, | ||
| including `apply` with an arguments object. | ||
|
|
||
| Named builtin ESM imports now install their module's dispatch/attachment bucket | ||
| before initializing the export snapshot. This prevents an early named import | ||
| from caching an AsyncResource/AsyncLocalStorage constructor without its own | ||
| static methods or prototype. Installation remains per-module, so unrelated | ||
| native modules remain eligible for dead stripping. | ||
|
|
||
| Adds a runtime regression and a bounded, application-independent native matrix | ||
| covering builtin/import/require/alias forms, async context and receiver capture, | ||
| custom own methods/accessors, non-callable overrides, and intrinsic fallbacks. | ||
| The standalone CI suite prepares coherent native providers outside its fixture | ||
| timeouts, uses the pinned Node oracle, and publishes failed compiler output. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| //! Named ESM builtin cells must be initialized after their module's installer. | ||
| use crate::{compile_module, CompileOptions}; | ||
| use perry_hir::{types::Type, Expr, Function, Module, Stmt}; | ||
|
|
||
| fn compile_lookup(module_name: &str, property: &str) -> String { | ||
| let mut module = Module::new("named_import_install.ts"); | ||
| module.functions.push(Function { | ||
| id: 0, | ||
| name: "lookup".into(), | ||
| type_params: Vec::new(), | ||
| params: Vec::new(), | ||
| return_type: Type::Any, | ||
| body: vec![Stmt::Return(Some(Expr::Call { | ||
| callee: Box::new(Expr::ExternFuncRef { | ||
| name: "js_native_module_named_esm_export_value".into(), | ||
| param_types: vec![Type::String, Type::String], | ||
| return_type: Type::Any, | ||
| }), | ||
| args: vec![ | ||
| Expr::String(module_name.into()), | ||
| Expr::String(property.into()), | ||
| ], | ||
| type_args: Vec::new(), | ||
| byte_offset: 0, | ||
| }))], | ||
| is_async: false, | ||
| is_generator: false, | ||
| is_strict: true, | ||
| is_exported: false, | ||
| captures: Vec::new(), | ||
| decorators: Vec::new(), | ||
| was_plain_async: false, | ||
| was_unrolled: false, | ||
| }); | ||
| let bytes = compile_module( | ||
| &module, | ||
| CompileOptions { | ||
| emit_ir_only: true, | ||
| ..Default::default() | ||
| }, | ||
| ) | ||
| .expect("compile named import cell lookup"); | ||
| let ir = String::from_utf8(bytes).expect("LLVM IR"); | ||
| let start = ir | ||
| .find("define double @perry_fn_named_import_install_ts__lookup(") | ||
| .expect("lookup function emitted"); | ||
| let tail = &ir[start..]; | ||
| let end = tail.find("\n}\n").expect("lookup function terminated"); | ||
| tail[..end + 3].to_string() | ||
| } | ||
|
|
||
| #[test] | ||
| fn named_builtin_cell_installs_only_its_module_before_lookup() { | ||
| for (module, property, installer) in [ | ||
| ("async_hooks", "AsyncResource", "js_nm_install_async_hooks"), | ||
| ( | ||
| "node:async_hooks", | ||
| "AsyncLocalStorage", | ||
| "js_nm_install_async_hooks", | ||
| ), | ||
| ("util", "inherits", "js_nm_install_util"), | ||
| ] { | ||
| let ir = compile_lookup(module, property); | ||
| let install = ir | ||
| .find(&format!("call void @{installer}(")) | ||
| .expect("module installer emitted"); | ||
| let lookup = ir | ||
| .find("call double @js_native_module_named_esm_export_value(") | ||
| .expect("export cell lookup emitted"); | ||
| assert!( | ||
| install < lookup, | ||
| "installer must precede cache population: {ir}" | ||
| ); | ||
| assert!( | ||
| !ir.contains("@js_nm_install_all("), | ||
| "no blanket install: {ir}" | ||
| ); | ||
| assert!( | ||
| !ir.contains("call void @js_nm_install_fs("), | ||
| "no unrelated module: {ir}" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn unknown_named_module_does_not_install_unrelated_buckets() { | ||
| let ir = compile_lookup("unknown-module", "value"); | ||
| assert!(ir.contains("call double @js_native_module_named_esm_export_value(")); | ||
| assert!(!ir.contains("call void @js_nm_install_")); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,8 @@ mod primitive_methods; | |
| mod proto_dispatch; | ||
| mod string_methods; | ||
|
|
||
| #[cfg(test)] | ||
| mod closure_override_tests; | ||
| #[cfg(test)] | ||
| mod code_point_at_dispatch_tests; | ||
| #[cfg(test)] | ||
|
|
@@ -1716,22 +1718,45 @@ pub unsafe extern "C-unwind" fn js_native_call_method( | |
| if crate::value::addr_class::is_above_handle_band(raw_addr) | ||
| && crate::closure::is_closure_ptr(raw_addr) | ||
| && !crate::closure::closure_is_key_deleted(raw_addr, method_name) | ||
| // apply/call/bind/toString on a closure receiver have dedicated | ||
| // spec-accurate arms below; the dynamic-prop read would resolve | ||
| // them through the Function.prototype expando fallback to the | ||
| // GENERIC thunks, which lose arguments-object argArrays | ||
| // (`G.apply(this, arguments)`). | ||
| && !matches!(method_name, "apply" | "call" | "bind" | "toString") | ||
| { | ||
| let dyn_val = crate::closure::closure_get_dynamic_prop(raw_addr, method_name); | ||
| if dyn_val.to_bits() != crate::value::TAG_UNDEFINED { | ||
| // #10045: own overrides (including AsyncResource.bind) beat the | ||
| // Function.prototype fast paths. Keep those fast paths on a miss: | ||
| // the generic prototype thunks lose arguments-object argArrays | ||
| // (`G.apply(this, arguments)`). An own undefined/non-callable slot | ||
| // is not a miss: invoking it must throw instead of using a builtin. | ||
| let intrinsic_name = matches!(method_name, "apply" | "call" | "bind" | "toString"); | ||
| let own_override = intrinsic_name | ||
| && (crate::closure::closure_has_own_dynamic_prop(raw_addr, method_name) | ||
| || crate::object::get_accessor_descriptor(raw_addr, method_name).is_some()); | ||
| let dyn_val = if !intrinsic_name || own_override { | ||
| crate::closure::closure_get_dynamic_prop(raw_addr, method_name) | ||
| } else { | ||
| f64::from_bits(crate::value::TAG_UNDEFINED) | ||
| }; | ||
| if dyn_val.to_bits() != crate::value::TAG_UNDEFINED || own_override { | ||
| let dyn_val = root_scope.root_nanbox_f64(dyn_val); | ||
| // The permissive value-call bridge returns undefined for | ||
| // nullish callees. A member invocation must instead reject a | ||
| // non-callable own value. The prototype-object probe may | ||
| // allocate, so keep the resolved method in a mutable root. | ||
| if own_override | ||
| && !crate::proxy::proxy_wraps_callable(dyn_val.get_nanbox_f64()) | ||
| && !crate::object::is_function_prototype_object_value(dyn_val.get_nanbox_f64()) | ||
| { | ||
|
Comment on lines
+1742
to
+1745
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: #!/bin/bash
set -euo pipefail
rg -n -C 5 \
'is_function_prototype_object_value|proxy_wraps_callable|is_callable|closure_override' \
crates/perry-runtime/src/object/native_call_method.rs \
crates/perry-runtime/src/object/native_call_method/closure_override_tests.rs \
crates/perry-runtime/srcRepository: PerryTS/perry Length of output: 50370 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- changed path ---'
sed -n '1690,1805p' crates/perry-runtime/src/object/native_call_method.rs
printf '%s\n' '--- callable-related definitions ---'
rg -n -C 8 \
'fn (is_callable|value_is_callable)|pub(crate)? fn (is_callable|value_is_callable)|is_function_prototype_object_value|proxy_wraps_callable' \
crates/perry-runtime/src/object crates/perry-runtime/src/closure.rs crates/perry-runtime/src/proxy.rs
printf '%s\n' '--- closure override tests ---'
fd -i 'closure_override_tests.rs' crates/perry-runtime
test -f crates/perry-runtime/src/object/native_call_method/closure_override_tests.rs &&
cat -n crates/perry-runtime/src/object/native_call_method/closure_override_tests.rsRepository: PerryTS/perry Length of output: 39283 🏁 Script executed: #!/bin/bash
set -u
printf '%s\n' '--- closure-related files ---'
fd -i 'closure' crates/perry-runtime/src | head -80
printf '%s\n' '--- override test/module references ---'
rg -n -C 4 'closure_override|mod closure|f\.bind|own override' \
crates/perry-runtime/src/object/native_call_method.rs \
crates/perry-runtime/src/object \
crates/perry-runtime/src/test* 2>/dev/null || true
printf '%s\n' '--- general callable predicate ---'
sed -n '1,75p' crates/perry-runtime/src/object/instanceof.rs
printf '%s\n' '--- closure pointer implementation ---'
rg -n -C 12 'fn is_closure_ptr|pub.*is_closure_ptr|CLOSURE_MAGIC' crates/perry-runtime/srcRepository: PerryTS/perry Length of output: 50372 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- override tests ---'
wc -l crates/perry-runtime/src/object/native_call_method/closure_override_tests.rs
cat -n crates/perry-runtime/src/object/native_call_method/closure_override_tests.rs
printf '%s\n' '--- closure module map ---'
ast-grep outline crates/perry-runtime/src/closure | head -120
printf '%s\n' '--- closure pointer definition ---'
rg -n -C 10 'is_closure_ptr' crates/perry-runtime/src/closureRepository: PerryTS/perry Length of output: 39351 Use the general callable predicate for own overrides.
🤖 Prompt for AI Agents |
||
| crate::error::js_throw_type_error_not_a_function( | ||
| std::ptr::null(), | ||
| 0, | ||
| method_name.as_ptr(), | ||
| method_name.len(), | ||
| ); | ||
| } | ||
| // #6438: same rebind as the GC_TYPE_CLOSURE arm below — | ||
| // `closure_get_dynamic_prop` may return a method read off the | ||
| // closure's `Object.setPrototypeOf` proto, whose bound `this` | ||
| // (an object-literal method binds the literal) would otherwise | ||
| // win over IMPLICIT_THIS and leave `this` as the PROTO. | ||
| let bound = crate::closure::clone_closure_rebind_this( | ||
| dyn_val.to_bits(), | ||
| dyn_val.get_nanbox_u64(), | ||
| f64::from_bits(object().to_bits()), | ||
| ); | ||
| // #8495: root the displaced receiver across the call below — the | ||
|
|
@@ -1747,11 +1772,19 @@ pub unsafe extern "C-unwind" fn js_native_call_method( | |
| // introduced `refreshed_args` and reached ten sites but not | ||
| // this one. | ||
| let call_args = refreshed_args(); | ||
| let result = crate::closure::js_native_call_value( | ||
| f64::from_bits(bound), | ||
| call_args.as_ptr(), | ||
| call_args.len(), | ||
| ); | ||
| let result = if crate::proxy::js_proxy_is_proxy(f64::from_bits(bound)) == 1 { | ||
| crate::proxy::call_proxy_value_with_this( | ||
| f64::from_bits(bound), | ||
| object(), | ||
| &call_args, | ||
| ) | ||
| } else { | ||
| crate::closure::js_native_call_value( | ||
| f64::from_bits(bound), | ||
| call_args.as_ptr(), | ||
| call_args.len(), | ||
| ) | ||
| }; | ||
| IMPLICIT_THIS.with(|c| c.set(prev_this_h.get_nanbox_u64())); | ||
| return result; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| //! #10045: own callable properties must beat Function.prototype fast paths. | ||
| use crate::{closure, gc::RuntimeHandleScope, value}; | ||
|
|
||
| extern "C" fn original(_closure: *const closure::ClosureHeader, _arg: f64) -> f64 { | ||
| -1.0 | ||
| } | ||
|
|
||
| extern "C" fn own_method(_closure: *const closure::ClosureHeader, _arg: f64) -> f64 { | ||
| crate::object::js_implicit_this_get() | ||
| } | ||
|
|
||
| fn check_own_method_dispatch(proxy: bool) { | ||
| let _lock = crate::gc::global_side_table_test_lock(); | ||
| let scope = RuntimeHandleScope::new(); | ||
| closure::js_register_closure_arity(original as *const u8, 1); | ||
| closure::js_register_closure_arity(own_method as *const u8, 1); | ||
| for method in ["bind", "call", "apply", "toString"] { | ||
| let receiver = scope.root_nanbox_f64(value::js_nanbox_pointer(closure::js_closure_alloc( | ||
| original as *const u8, | ||
| 0, | ||
| ) as i64)); | ||
| let implementation = scope.root_nanbox_f64(value::js_nanbox_pointer( | ||
| closure::js_closure_alloc(own_method as *const u8, 0) as i64, | ||
| )); | ||
| if proxy { | ||
| let handler = scope.root_nanbox_f64(value::js_nanbox_pointer( | ||
| crate::object::js_object_alloc(0, 0) as i64, | ||
| )); | ||
| implementation.set_nanbox_f64(crate::proxy::js_proxy_new( | ||
| implementation.get_nanbox_f64(), | ||
| handler.get_nanbox_f64(), | ||
| )); | ||
| } | ||
| closure::closure_set_dynamic_prop( | ||
| value::js_nanbox_get_pointer(receiver.get_nanbox_f64()) as usize, | ||
| method, | ||
| implementation.get_nanbox_f64(), | ||
| ); | ||
| let args = [42.0]; | ||
| let result = unsafe { | ||
| super::js_native_call_method( | ||
| receiver.get_nanbox_f64(), | ||
| method.as_ptr().cast(), | ||
| method.len(), | ||
| args.as_ptr(), | ||
| args.len(), | ||
| ) | ||
| }; | ||
| assert_eq!( | ||
| result.to_bits(), | ||
| receiver.get_nanbox_u64(), | ||
| "own {method} receiver" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn closure_own_function_methods_override_intrinsic_dispatch() { | ||
| check_own_method_dispatch(false); | ||
| } | ||
|
|
||
| #[test] | ||
| fn closure_own_function_methods_preserve_proxy_receiver() { | ||
| check_own_method_dispatch(true); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| //! Application-independent native coverage of dynamic own function overrides. | ||
| use std::{path::Path, process::Command}; | ||
|
|
||
| #[test] | ||
| fn standalone_regression() { | ||
| let root = Path::new(env!("CARGO_MANIFEST_DIR")).join("../.."); | ||
| let output = Command::new("node") | ||
| .arg(root.join("scripts/test-async-resource-own-bind.mjs")) | ||
| .env("PERRY_BIN", env!("CARGO_BIN_EXE_perry")) | ||
| .env("PERRY_WORKSPACE_ROOT", &root) | ||
| .env("PERRY_TEST_BUILD_RUNTIME", "1") | ||
| .current_dir(&root) | ||
| .output() | ||
| .expect("run bounded Node regression driver"); | ||
| assert!( | ||
| output.status.success(), | ||
| "{}\n{}", | ||
| String::from_utf8_lossy(&output.stdout), | ||
| String::from_utf8_lossy(&output.stderr) | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| // #10045: application-independent dispatch, context, and fallback regression. | ||
| import fs from 'node:fs'; | ||
| import os from 'node:os'; | ||
| import path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { spawnSync } from 'node:child_process'; | ||
| import { prepareRequireRuntime } from './test-require-runtime.mjs'; | ||
|
|
||
| const root = path.dirname(path.dirname(fileURLToPath(import.meta.url))); | ||
| prepareRequireRuntime(root); | ||
| const compiler = process.env.PERRY_BIN ?? path.join(root, 'target/perry-dev/perry'); | ||
| const work = fs.mkdtempSync(path.join(os.tmpdir(), 'perry-async-own-bind-')); | ||
| const env = { ...process.env }; | ||
| if (env.PERRY_TEST_WASM === '1' && env.PERRY_RUNTIME_DIR) delete env.PERRY_WORKSPACE_ROOT; | ||
| let passed = false; | ||
| function run(name, executable, args, timeout, extraEnv = {}) { | ||
| const result = spawnSync(executable, args, { cwd: work, env: { ...env, ...extraEnv }, | ||
| encoding: 'utf8', timeout, maxBuffer: 8 * 1024 * 1024 }); | ||
| fs.writeFileSync(path.join(work, `${name}.log`), `${result.stdout ?? ''}${result.stderr ?? ''}`); | ||
| if (result.error || result.status !== 0) { | ||
| throw new Error(`${name}: ${result.error ?? result.status} (signal ${result.signal ?? 'none'})\n` + | ||
| `${result.stdout ?? ''}${result.stderr ?? ''}`); | ||
| } | ||
| return result.stdout; | ||
| } | ||
| try { | ||
| for (const [fixture, expected] of [ | ||
| ['main.ts', 'PASS: own function methods and async context binding\n'], | ||
| ['require.cjs', 'PASS: require AsyncResource.bind\n'], | ||
| ]) { | ||
| const source = path.join(root, 'tests/modules/async_resource_own_bind', fixture); | ||
| if (run(`node-${fixture}`, process.execPath, [source], 15000) !== expected) throw new Error('Node witness missing'); | ||
| for (const opt of ['0', 's', 'z']) { | ||
| const label = `${fixture}-O${opt}`; | ||
| const output = path.join(work, `native-${label}${process.platform === 'win32' ? '.exe' : ''}`); | ||
| run(`compile-${label}`, compiler, ['compile', source, '-o', output, | ||
| '--cache-dir', path.join(work, `cache-${label}`), '--platform', 'bun', | ||
| '--no-auto-optimize', '--no-color', ...(env.PERRY_TEST_WASM === '1' ? ['--enable-wasm-runtime'] : [])], | ||
| 120000, { PERRY_LL_OPT_LEVEL: opt }); | ||
| if (run(`native-${label}`, output, [], 15000) !== expected) throw new Error(`${label} witness mismatch`); | ||
| console.log(`PASS async-resource-own-bind ${label}`); | ||
| } | ||
| } | ||
| passed = true; | ||
| } finally { | ||
| if (passed) fs.rmSync(work, { recursive: true }); | ||
| else console.error(`Retained regression diagnostics: ${work}`); | ||
| } |
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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 7676
🤖 get_repo_knowledge executed:
get_repo_knowledge PerryTS/perry /tmp/coderabbit-repo-knowledge/perryts-perry-d4a878bc/learnings /tmp/coderabbit-repo-knowledge/perryts-perry-d4a878bc/conventionsLength of output: 35376
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 19850
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 16279
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 4658
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 9433
Install the pinned Node runtime in
cargo-test-perry.cargo-test-perryrunscrates/perry/tests/async_resource_own_bind.rs, which invokesnodethroughCommand::new("node"). This job does not configure Node from.node-version, so the test can use the runner-image Node version instead of the pinned oracle. Add an equivalentactions/setup-nodestep withnode-version-file: .node-versionbefore the integration shard runs.🤖 Prompt for AI Agents