Skip to content

Comments

Fix panic in Function constructor with nested lexical bindings (#4531)#4645

Merged
hansl merged 3 commits intoboa-dev:mainfrom
Deepak-negi11:fix/4531-function-constructor-scope
Feb 24, 2026
Merged

Fix panic in Function constructor with nested lexical bindings (#4531)#4645
hansl merged 3 commits intoboa-dev:mainfrom
Deepak-negi11:fix/4531-function-constructor-scope

Conversation

@Deepak-negi11
Copy link
Contributor

Fixes #4531

Problem

Function("function f() { const a = 42; return () => a; } return f()()")() panics with "index out of bounds" / "must be declarative environment".

Root Cause

The Function constructor compiles with force_function_scope=true, but FunctionExpression::analyze_scope was missing the scope index optimization step. The generic optimize_scope_indices also didn't account for the forced function scope, causing compile-time scope indices to mismatch the runtime environment stack.

Fix

Added optimize_scope_indices_function_constructor() in scope_analyzer.rs that calls visit_function_like directly with force_function_scope=true and call it from FunctionExpression::analyze_scope.

Test

Added a regression test function_constructor_nested_lexical_binding.

…ev#4531)

Added optimize_scope_indices_function_constructor() to account for
force_function_scope=true in the Function constructor's scope analysis.

Closes boa-dev#4531
@github-actions
Copy link

github-actions bot commented Feb 21, 2026

Test262 conformance changes

Test result main count PR count difference
Total 52,862 52,862 0
Passed 49,497 49,497 0
Ignored 2,261 2,261 0
Failed 1,104 1,104 0
Panics 0 0 0
Conformance 93.63% 93.63% 0.00%

@nekevss nekevss requested a review from a team February 24, 2026 02:31
@nekevss nekevss added the bug Something isn't working label Feb 24, 2026
@hansl
Copy link
Contributor

hansl commented Feb 24, 2026

@Deepak-negi11 Could you fix the formatting issue? Everything else looks fine, so once that passes I'll approve the merge.

@hansl hansl self-requested a review February 24, 2026 20:25
Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just cargo fmt should do.

@jedel1043 jedel1043 added the ast Issue surrounding the abstract syntax tree label Feb 24, 2026
@jedel1043 jedel1043 added this to the v1.0.0 milestone Feb 24, 2026
@hansl hansl enabled auto-merge February 24, 2026 21:15
@hansl hansl added this pull request to the merge queue Feb 24, 2026
Merged via the queue into boa-dev:main with commit 957e6f8 Feb 24, 2026
17 checks passed
@codecov
Copy link

codecov bot commented Feb 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.02%. Comparing base (6ddc2b4) to head (241b9aa).
⚠️ Report is 673 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4645      +/-   ##
==========================================
+ Coverage   47.24%   57.02%   +9.78%     
==========================================
  Files         476      549      +73     
  Lines       46892    60138   +13246     
==========================================
+ Hits        22154    34295   +12141     
- Misses      24738    25843    +1105     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines +286 to +288
analyze_binding_escapes(self, false, scope.clone(), interner)?;
optimize_scope_indices_function_constructor(self, scope);
Ok(())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got late to review this, but I'm wondering how this affects ordinary FunctionExpressions, since this not only touches all Function() calls, but also all (function() {}) style expressions.

Copy link
Member

@jedel1043 jedel1043 Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would just optimize scope indices two times instead of only one for (function(){}), so I think it would've been better to inline optimize_scope_indices_function_constructor into the specific place where it's used

if let Err(reason) =
function.analyze_scope(strict, context.realm().scope(), context.interner())
{
return Err(js_error!(SyntaxError: "failed to analyze function scope: {}", reason));
}

which avoids affecting all non-dynamic functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ast Issue surrounding the abstract syntax tree bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Panic in YT JS challenge code: "must be declarative environment"

4 participants