fix(hir): bind named function expression's own name in its body (#5126)#5156
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesNamed function expression self-binding fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry/tests/issue_5126_named_fn_expr_self_ref.rs (1)
83-100: 💤 Low valueConsider adding an explicit outer-binding shadowing test case.
The comment mentions "self-shadowing" but the current
stepcase doesn't have a separate outer binding with the same name to shadow. While the implementation handles this correctly, an explicit test would strengthen confidence:const f = "outer"; const fn = function f() { return typeof f; }; console.log(fn()); // should print "function", not "string"This is optional since the core self-reference behavior is well-tested.
🤖 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/issue_5126_named_fn_expr_self_ref.rs` around lines 83 - 100, The test function named_fn_expr_self_ref_edge_cases has a comment mentioning that "the self-binding must shadow an outer binding of the same name," but the current test cases do not explicitly demonstrate this shadowing behavior. Add a test case to the code string that creates an outer binding with a given name (like a string or number) and then defines a named function expression with the same name, verifying that references to that name inside the function resolve to the function itself rather than the outer binding. This strengthens the test coverage by explicitly validating the shadowing behavior mentioned in the comment.
🤖 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.
Nitpick comments:
In `@crates/perry/tests/issue_5126_named_fn_expr_self_ref.rs`:
- Around line 83-100: The test function named_fn_expr_self_ref_edge_cases has a
comment mentioning that "the self-binding must shadow an outer binding of the
same name," but the current test cases do not explicitly demonstrate this
shadowing behavior. Add a test case to the code string that creates an outer
binding with a given name (like a string or number) and then defines a named
function expression with the same name, verifying that references to that name
inside the function resolve to the function itself rather than the outer
binding. This strengthens the test coverage by explicitly validating the
shadowing behavior mentioned in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a2e21be-2de9-49d4-8269-40f1fc2caa65
📒 Files selected for processing (2)
crates/perry-hir/src/lower/expr_function.rscrates/perry/tests/issue_5126_named_fn_expr_self_ref.rs
A named function expression could not reference its own name from inside
its body, so a recursive self-call threw `ReferenceError: f is not
defined`. Per spec, a NamedFunctionExpression's identifier is bound
(read-only) within the function's own body, independent of the binding it
is later assigned to.
Fix: in `lower_fn_expr`, lower a named function expression with the name
in scope and check whether the lowered closure actually captured it. When
it did (a genuine recursive self-reference), wrap the function in an
immediately-invoked arrow that binds the name to the function value:
(() => { let f = <function expr>; return f; })()
The `let f = <closure referencing f>` shape is exactly the
self-recursive-const pattern `collect_boxed_vars` already boxes, so the
name resolves to the function through its heap box. When the body does not
reference its own name (the common case, including the synthetic
`Function(...)` body), the scaffolding is discarded and the bare closure
is returned unchanged so `.name` and HIR shape are preserved.
f13a978 to
1a33193
Compare
Fixes #5126.
Problem
A named function expression could not reference its own name from inside its body, so a recursive self-call threw
ReferenceError:Per spec, a
NamedFunctionExpression's identifier is bound (read-only) within the function's own body, independent of the binding it is later assigned to. Perry dropped that binding — recursion only worked when the outer binding name happened to match the inner name (so the outer capture resolved it).Fix
In
lower_fn_expr(crates/perry-hir/src/lower/expr_function.rs), a named function expression is now lowered with its own name in scope, then we inspect whether the lowered closure actually captured that name:let f = <closure referencing f>shape is exactly the self-recursive-constpattern thatcollect_boxed_varsalready boxes (aStmt::LetwhoseClosureinit references the Let's own id), sofresolves to the function through its heap box — reusing the proven recursion machinery with no new HIR node.Function(...)constructor body) → the scaffolding is discarded and the bareClosureis returned unchanged, preserving.nameand HIR shape.Detection is precise (based on the real capture analysis, not a text/AST heuristic), and the self-name correctly shadows any outer binding of the same name per spec.
Verification
120,55 (fib), generator self-yield*, IIFE self-call720, outer-variable capture, and.namepreserved for non-self-referential named expressions.crates/perry/tests/issue_5126_named_fn_expr_self_ref.rs(2 tests, both green).cargo test -p perry-hirfully green (172 tests), including theFunction(...)-constructor HIR-shape test that guards against over-wrapping.No changelog/version bump per maintainer's release-at-merge workflow.
Summary by CodeRabbit
Release Notes
Bug Fixes
ReferenceError. This also improves consistency of behavior when the name is shadowed or when the function interacts with captured variables.Tests