Skip to content

fix: class constructor scope index mismatch and arguments binding slot allocation#5352

Open
acsses wants to merge 3 commits intoboa-dev:mainfrom
acsses:main
Open

fix: class constructor scope index mismatch and arguments binding slot allocation#5352
acsses wants to merge 3 commits intoboa-dev:mainfrom
acsses:main

Conversation

@acsses
Copy link
Copy Markdown

@acsses acsses commented Apr 28, 2026

Description

This PR fixes two related bugs in the bytecode compiler that caused a panic
(index out of bounds: the len is 0 but the index is 0) when executing
esbuild-bundled JavaScript code targeting ES2020.


Bug 1: arguments binding missing slot in FunctionEnvironment

Root cause:

scope_analyzer.rs always creates an arguments binding in the function
scope, but without the ESCAPES flag:

// scope_analyzer.rs
drop(env.create_mutable_binding(arguments.clone(), false));
// → BindingFlags: MUTABLE | LEX (no ESCAPES)

num_bindings_non_local() only counts bindings with ESCAPES=true, so the
arguments binding was excluded from the slot count. This caused
FunctionEnvironment to be created with 0 bindings, while the bytecode still
emitted PutLexicalValue(index=0) for it, resulting in a panic at runtime.

Fix (boa_ast/src/scope.rs):

Include the arguments binding in the slot count unconditionally:

pub fn num_bindings_non_local(&self) -> u32 {
    let arguments = JsString::from("arguments");
    self.inner
        .bindings
        .borrow()
        .iter()
        .filter(|binding| binding.escapes() || binding.name == arguments)
        .count() as u32
}

Bug 2: Class constructor scope index mismatch

Root cause:

scope_analyzer.rs assigns scope indices to class constructor scopes via
visit_function_like, which increments self.index before calling
function_scope.set_index(self.index). For example, when
function_scope.all_bindings_local() is false:

self.index += 1;  // e.g. index: 0 → 1
scopes.function_scope.set_index(self.index);  // scope_index = 1

However, compile_class() in class.rs always called push_scope() exactly
once for the function scope, placing it at constant_scope(0). At runtime,
function_construct uses code.constant_scope(last_env) to retrieve the
function scope. Since the function_scope had scope_index=1 but was stored
at constant_scope(0), the wrong (or nonexistent) scope was accessed, causing
a panic.

This is in contrast to function.rs (FunctionCompiler), which correctly
mirrors the scope index assignment by:

  1. Pushing name_scope first if it has non-local bindings
  2. Conditionally pushing function_scope based on all_bindings_local() and
    requires_function_scope()

Fix (boa_engine/src/bytecompiler/class.rs):

Apply the same scoping logic as FunctionCompiler to the class constructor:

if let Some(expr) = &class.constructor {
    // Mirror scope_analyzer: push name_scope first if non-local
    if let Some(name_scope) = class.name_scope {
        if !name_scope.all_bindings_local() {
            compiler.code_block_flags |= CodeBlockFlags::HAS_BINDING_IDENTIFIER;
            let _ = compiler.push_scope(name_scope);
        }
    }

    // Mirror FunctionCompiler logic for HAS_FUNCTION_SCOPE
    let contains_direct_eval = expr.contains_direct_eval();
    if contains_direct_eval || !expr.scopes().function_scope().all_bindings_local() {
        compiler.code_block_flags |= CodeBlockFlags::HAS_FUNCTION_SCOPE;
    } else {
        compiler.code_block_flags.set(
            CodeBlockFlags::HAS_FUNCTION_SCOPE,
            expr.scopes().requires_function_scope(),
        );
    }

    if compiler.code_block_flags.has_function_scope() {
        let _ = compiler.push_scope(expr.scopes().function_scope());
    } else {
        compiler.variable_scope = expr.scopes().function_scope().clone();
        compiler.lexical_scope = expr.scopes().function_scope().clone();
    }
    // ... rest of constructor compilation
}

Test cases

// Bug 1: arguments binding slot
class A {
  constructor() {
    const x = 1;
  }
}
new A();

// Bug 2: class constructor with non-local bindings (closure)
var B = class {
  constructor(components) {
    const { getHasher } = components;
    this.fn = () => getHasher("sha2-256");
  }
};
new B({ getHasher: (x) => x });

How it was found

The panic was triggered by esbuild-bundled code with --target=es2020, which
transforms class fields into __publicField() helper calls. The combination
of multiple __publicField calls followed by const declarations in a class
constructor exposed both bugs simultaneously.

acsses added 3 commits April 27, 2026 22:32
…cation

Two bugs caused a runtime panic in class constructors:

1. `num_bindings_non_local()` in `boa_ast/src/scope.rs` excluded the
   `arguments` binding because it lacks the `ESCAPES` flag, resulting in
   `FunctionEnvironment` being created with 0 slots while the bytecode
   emitted `PutLexicalValue(index=0)`.

2. `compile_class()` in `boa_engine/src/bytecompiler/class.rs` always
   pushed the constructor's function scope to `constant_scope(0)`, while
   `scope_analyzer` assigned it a higher index via `visit_function_like`,
   causing a mismatch at runtime.

Fix (1) by counting `arguments` bindings unconditionally in
`num_bindings_non_local()`.

Fix (2) by applying the same scope push logic as `FunctionCompiler`:
push `name_scope` first if non-local, then conditionally push
`function_scope` based on `all_bindings_local()` and
`requires_function_scope()`.

Fixes boa-dev#5351
@acsses acsses requested a review from a team as a code owner April 28, 2026 02:44
@github-actions github-actions Bot added the Waiting On Review Waiting on reviews from the maintainers label Apr 28, 2026
@github-actions github-actions Bot added this to the v1.0.0 milestone Apr 28, 2026
@github-actions github-actions Bot added C-VM Issues and PRs related to the Boa Virtual Machine. C-AST Issue surrounding the abstract syntax tree labels Apr 28, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 1, 2026

Test262 conformance changes

Test result main count PR count difference
Total 53,125 53,125 0
Passed 51,051 51,049 -2
Ignored 1,482 1,482 0
Failed 592 594 +2
Panics 0 7 +7
Conformance 96.10% 96.09% -0.00%
New panics (7):
test/staging/sm/class/superElemDelete.js (previously Passed)
test/staging/sm/expressions/destructuring-array-default-simple.js (previously Failed)
test/staging/sm/expressions/destructuring-array-default-function.js (previously Failed)
test/staging/sm/expressions/destructuring-array-default-function-nested.js (previously Failed)
test/staging/sm/expressions/destructuring-array-default-class.js (previously Failed)
test/staging/sm/expressions/destructuring-array-default-call.js (previously Failed)
test/language/statements/class/arguments/access.js (previously Passed)

Tested main commit: 2cc4791a912eecf64d21244bd8ecad25318fbc3f
Tested PR commit: bd812f286eae0e1e41ba71d2827cfa63654b3365
Compare commits: 2cc4791...bd812f2

@codecov
Copy link
Copy Markdown

codecov Bot commented May 1, 2026

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.95%. Comparing base (6ddc2b4) to head (bd812f2).
⚠️ Report is 961 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/bytecompiler/class.rs 78.57% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5352       +/-   ##
===========================================
+ Coverage   47.24%   59.95%   +12.71%     
===========================================
  Files         476      566       +90     
  Lines       46892    62829    +15937     
===========================================
+ Hits        22154    37671    +15517     
- Misses      24738    25158      +420     

☔ 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.

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

Labels

C-AST Issue surrounding the abstract syntax tree C-VM Issues and PRs related to the Boa Virtual Machine. Waiting On Review Waiting on reviews from the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant