fix(hir): classify Function.apply/.call with an unfoldable body as an eval surface#6399
Conversation
… eval surface
`Function.apply(thisArg, args)` and `Function.call(thisArg, …, body)` reach
CreateDynamicFunction indirectly — the same surface as a direct `Function(body)`,
just spelled through the constructor's own `apply`/`call`. The eval classifier
only recognized an *identifier* callee, so those sites were never classified: an
unfoldable body fell through to the generic call lowering and evaluated to
`undefined`. The caller then invoked `.apply` on that `undefined` and failed
several frames away with
TypeError: Function.prototype.apply was called on a value that is not a function
which names neither eval nor the real cause.
Perry cannot compile a body assembled from runtime data — a real ahead-of-time
limitation — but it must say so at the offending site. These callees are now
recognized, so an unfoldable body lowers to the deferred, located "cannot run in
an ahead-of-time compiled binary" error: compilation still succeeds, the notice
is printed, and it throws only if the site is actually reached.
Function.apply(null, argNames.concat(body))
// before: undefined → "apply was called on a value that is not a function"
// after: new Function() cannot run in an ahead-of-time compiled binary (app.ts:11)
Const-foldable spellings are unaffected — `try_eval_function_member_call_fold`
runs first, so `Function("a","b","return a+b")`, `Function.apply(null, [<literal
array>])` and `Function.call(null, …)` still compile natively and evaluate
exactly as in node. For `apply`, the body is the array's last element when the
array is a literal; when the list is assembled at runtime the array expression
itself stands in, which is non-constant by construction, so the classifier
buckets the site runtime-unknown.
mysql2's row-parser codegen is precisely this shape —
`Function.apply(null, argNames.concat(body)).apply(null, argValues)` — so a real
MySQL query died pointing at `apply` instead of at runtime code generation.
(mysql2 ships `disableEval: true` for no-codegen environments, which is the
correct setting under an AOT compiler; this change makes the diagnosis obvious
rather than the failure mysterious.)
Covered by `function_apply_dynamic_args_eval_surface`: the literal forms still
evaluate, the dynamic site throws a catchable Error naming the AOT limitation,
and it never degrades into the bogus `.apply` failure.
📝 WalkthroughWalkthroughChangesFunction.apply eval classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant IntegrationTest
participant PerryCompiler
participant GeneratedBinary
participant RuntimeArchives
IntegrationTest->>RuntimeArchives: Build static runtime archives
IntegrationTest->>PerryCompiler: Compile generated main.ts
PerryCompiler->>GeneratedBinary: Produce main_bin
IntegrationTest->>GeneratedBinary: Run literal and dynamic modes
GeneratedBinary->>IntegrationTest: Return outputs and caught AOT error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-hir/src/lower/expr_call/intrinsics/eval_strict.rs (1)
74-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant guard for
"eval".The guard
if !member_apply && !matches!(callee, ast::Expr::Member(_))is logically redundant. TheExpr::Memberextraction block explicitly enforcesid.sym.as_ref() == "Function", meaningnamecan never be"eval"ifcalleeis aMemberExpr. Therefore, the"eval"match arm is only reachable whencalleeis a directExpr::Identnamed"eval".Removing it simplifies the match expression.
♻️ Proposed refactor
let surface = match name { - "eval" if !member_apply && !matches!(callee, ast::Expr::Member(_)) => { - crate::eval_classifier::EvalSurface::Eval - } + "eval" => crate::eval_classifier::EvalSurface::Eval, "Function" => crate::eval_classifier::EvalSurface::FunctionCall, _ => return Ok(None), };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-hir/src/lower/expr_call/intrinsics/eval_strict.rs` around lines 74 - 80, In the `surface` match within the strict intrinsic evaluation logic, remove the redundant `!member_apply && !matches!(callee, ast::Expr::Member(_))` guard from the `"eval"` arm so every direct `"eval"` match maps to `EvalSurface::Eval`; leave the `"Function"` arm and fallback unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry/tests/function_apply_dynamic_args_eval_surface.rs`:
- Around line 120-121: Move the test function
function_apply_with_runtime_args_defers_to_a_located_aot_error out of the
integration-tests directory into an appropriate #[cfg(test)] unit-test module
under the perry source tree, preserving its assertions and setup so cargo test
executes it in standard CI.
---
Nitpick comments:
In `@crates/perry-hir/src/lower/expr_call/intrinsics/eval_strict.rs`:
- Around line 74-80: In the `surface` match within the strict intrinsic
evaluation logic, remove the redundant `!member_apply && !matches!(callee,
ast::Expr::Member(_))` guard from the `"eval"` arm so every direct `"eval"`
match maps to `EvalSurface::Eval`; leave the `"Function"` arm and fallback
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b0c3cec-3094-4393-8d3c-4f022fb9fb98
📒 Files selected for processing (2)
crates/perry-hir/src/lower/expr_call/intrinsics/eval_strict.rscrates/perry/tests/function_apply_dynamic_args_eval_surface.rs
| #[test] | ||
| fn function_apply_with_runtime_args_defers_to_a_located_aot_error() { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move acceptance test to a unit test module.
This integration test is located under crates/perry/tests/. As per coding guidelines, acceptance coverage must be placed in unit tests visible to cargo-test (e.g., inside a #[cfg(test)] module in crates/perry/src/), because integration suites under crates/*/tests/*.rs do not run on every PR.
Please relocate this test to ensure it reliably runs in the standard CI pipeline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry/tests/function_apply_dynamic_args_eval_surface.rs` around lines
120 - 121, Move the test function
function_apply_with_runtime_args_defers_to_a_located_aot_error out of the
integration-tests directory into an appropriate #[cfg(test)] unit-test module
under the perry source tree, preserving its assertions and setup so cargo test
executes it in standard CI.
Source: Coding guidelines
|
The It races the libuv threadpool: the three callback-form fd mutators are dispatched concurrently, so their callbacks fire in completion order. The gap harness compares perry against a freshly run node oracle, and the oracle itself is nondeterministic — across 40 runs, Fix is up as #6400 (buffer the three callback results, flush in a fixed order; assertions and expected output unchanged). Once that lands, rebase or rerun and this should go green. Note |
…ol (#6400) The three callback-form fd mutators are dispatched to the libuv threadpool concurrently, so their callbacks fire in completion order, which is not deterministic. The gap harness compares perry against a freshly run node oracle -- and the *oracle itself* is unstable: across 40 runs of `node --experimental-strip-types` this test produced 4 distinct outputs, emitting `fchown closed cb` before `futimes closed cb` on roughly one run in six. Perry is deterministic here (always submission order), so the test red-lit at random on unrelated PRs -- it took out #6397, #6398 and #6399 today, and cost a full bisect before the oracle was suspected. The test is about the error surface (err.code / err.syscall) of each fd mutator, not about libuv's scheduling. Buffer the three callback results and flush them in a fixed order once all three have landed. Every assertion is preserved and the emitted 10 lines are byte-for-byte what they were; only the print order is pinned. After: node emits 1 distinct output across 60 runs, perry 1 across 40, and the two are byte-identical. Co-authored-by: Ralph Küpper <ralph3@skelpo.com>
1 participant
The bug
Function.apply(thisArg, args)andFunction.call(thisArg, …, body)reach CreateDynamicFunction indirectly — the same surface as a directFunction(body), just spelled through the constructor's ownapply/call. The eval classifier only recognized an identifier callee, so those sites were never classified: an unfoldable body fell through to the generic call lowering and evaluated toundefined. The caller then invoked.applyon thatundefinedand failed several frames away withwhich names neither eval nor the real cause.
The fix
Perry cannot compile a body assembled from runtime data — that is a real ahead-of-time limitation, not a bug — but it must say so, at the offending site. These callees are now recognized, so an unfoldable body lowers to the deferred, located error:
Compilation still succeeds, the notice is printed, and it throws only if the site is actually reached.
Const-foldable spellings are unaffected —
try_eval_function_member_call_foldruns first, soFunction("a","b","return a+b"),Function.apply(null, [<literal array>])andFunction.call(null, …)still compile natively and evaluate exactly as in node. Forapply, the body is the array's last element when the array is a literal; when the list is assembled at runtime the array expression itself stands in, which is non-constant by construction, so the classifier buckets the site runtime-unknown.How it was found
Compiling a real Next.js + MySQL app. mysql2's row-parser codegen is precisely this shape —
Function.apply(null, argNames.concat(body)).apply(null, argValues)— so a real MySQL query died pointing atapplyinstead of at runtime code generation. (mysql2 shipsdisableEval: truefor no-codegen environments, which is the correct setting under an AOT compiler; this change makes the diagnosis obvious rather than the failure mysterious.)Test
function_apply_dynamic_args_eval_surface— the literal forms still evaluate (3/5/12), the dynamic site throws a catchableErrornaming the AOT limitation, and it never degrades into the bogus.applyfailure.Note: its
ensure_runtime_archivebuilds-p perry-runtime-static -p perry-stdlib-static.perry-runtime/perry-stdlibare rlib-only since #5422, so building the plain crates leaves no.aintarget/debugand the link silently falls back to a stale archive. (The olderissue_52*deferred-eval tests still build-p perry-runtimeand may be relying on an archive left behind by another target.)Summary by CodeRabbit
Function.apply(...)andFunction.call(...)invocations.Function,Function.apply, andFunction.callusage.