From c1bc0cdfa93901b9114ae6a13612162f4fb869fc Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 26 Apr 2026 20:47:25 +0200 Subject: [PATCH] LibJS: Allocate local variable indices in source order The scope collector stored identifier_groups and variables in HashMaps and then sorted them alphabetically before assigning local register indices. The sorts existed only because HashMap iteration order is non-deterministic; alphabetical was a stable choice for comparing bytecode against the now-removed C++ port. Switch both maps to indexmap::IndexMap so iteration follows the order of first reference (= source order), and drop the alphabetical sorts. Local indices now reflect declaration order, which matches what shows up in bytecode dumps and is easier to read alongside the source. Add a focused bytecode test using zebra/yak/aardvark to pin the new allocation order; existing tests using let/var declarations have their local indices renumbered to match. --- Cargo.lock | 1 + Libraries/LibJS/Rust/Cargo.toml | 1 + Libraries/LibJS/Rust/src/scope_collector.rs | 22 +++++++--------- .../LibJS/AST/expected/catch-param-scope.txt | 8 +++--- Tests/LibJS/AST/expected/class-scope.txt | 6 ++--- Tests/LibJS/AST/expected/closure-captures.txt | 6 ++--- .../AST/expected/default-param-scope.txt | 8 +++--- Tests/LibJS/AST/expected/eval-indirect.txt | 16 ++++++------ .../object-property-key-no-capture.txt | 4 +-- .../expected/delete-expression-register.txt | 14 +++++----- .../expected/for-of-cond-rhs-block-order.txt | 8 +++--- .../Bytecode/expected/local-source-order.txt | 26 +++++++++++++++++++ .../nested-function-decl-source-order.txt | 18 ++++++------- .../Bytecode/input/local-source-order.js | 12 +++++++++ 14 files changed, 94 insertions(+), 56 deletions(-) create mode 100644 Tests/LibJS/Bytecode/expected/local-source-order.txt create mode 100644 Tests/LibJS/Bytecode/input/local-source-order.js diff --git a/Cargo.lock b/Cargo.lock index 6826eb14b1..100375979b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -375,6 +375,7 @@ version = "0.1.0" dependencies = [ "bytecode_def", "cbindgen", + "indexmap", "libunicode_rust", "num-bigint", "num-integer", diff --git a/Libraries/LibJS/Rust/Cargo.toml b/Libraries/LibJS/Rust/Cargo.toml index fd27f24710..2cec274b2f 100644 --- a/Libraries/LibJS/Rust/Cargo.toml +++ b/Libraries/LibJS/Rust/Cargo.toml @@ -13,6 +13,7 @@ libunicode_rust = { path = "../../LibUnicode/Rust", default-features = false } num-bigint = "0.4" num-traits = "0.2" num-integer = "0.1" +indexmap = "2" [build-dependencies] bytecode_def = { path = "../BytecodeDef" } diff --git a/Libraries/LibJS/Rust/src/scope_collector.rs b/Libraries/LibJS/Rust/src/scope_collector.rs index 72708a8f70..9cc12407d8 100644 --- a/Libraries/LibJS/Rust/src/scope_collector.rs +++ b/Libraries/LibJS/Rust/src/scope_collector.rs @@ -49,6 +49,7 @@ //! - `IdentifierGroup` — a set of identifier references with the same //! name within one scope (multiple `foo` refs are grouped together) +use indexmap::IndexMap; use std::cell::RefCell; use std::collections::HashMap; use std::rc::Rc; @@ -189,8 +190,8 @@ struct ScopeRecord { scope_level: ScopeLevel, scope_data: Option>>, - variables: HashMap, - identifier_groups: HashMap, + variables: IndexMap, + identifier_groups: IndexMap, functions_to_hoist: Vec, // Parameter tracking @@ -221,8 +222,8 @@ impl ScopeRecord { scope_type, scope_level, scope_data, - variables: HashMap::new(), - identifier_groups: HashMap::new(), + variables: IndexMap::new(), + identifier_groups: IndexMap::new(), functions_to_hoist: Vec::new(), has_function_parameters: false, parameter_names: Vec::new(), @@ -931,13 +932,11 @@ impl ScopeCollector { /// - It's NOT used inside a `with` statement /// - The scope chain is NOT poisoned by `eval()` fn resolve_identifiers(records: &mut [ScopeRecord], index: usize, initiated_by_eval: bool, suppress_globals: bool) { + // identifier_groups is an IndexMap, so iteration is in source order + // of first reference. Local variable indices follow that order. let groups = std::mem::take(&mut records[index].identifier_groups); - // Sort groups by name for deterministic local variable indices - // (HashMap iteration order is arbitrary). - let mut sorted_groups: Vec<_> = groups.into_iter().collect(); - sorted_groups.sort_by(|a, b| a.0.cmp(&b.0)); let mut propagate_to_parent: Vec<(SharedUtf16String, IdentifierGroup)> = Vec::new(); - for (name, mut group) in sorted_groups { + for (name, mut group) in groups { // Annotate each Identifier AST node with its declaration kind, // so the bytecode generator knows how to handle TDZ checks, etc. if let Some(dk) = group.declaration_kind { @@ -1256,9 +1255,8 @@ impl ScopeCollector { }); } - // Sort by name for deterministic output (HashMap iteration order is arbitrary). - vars_to_initialize.sort_by(|a, b| a.name.cmp(&b.name)); - var_names.sort(); + // vars_to_initialize and var_names follow source order via the + // insertion order of `record.variables`, which is now an IndexMap. if last_position.contains_key(utf16!("arguments") as &[u16]) { has_function_named_arguments = true; diff --git a/Tests/LibJS/AST/expected/catch-param-scope.txt b/Tests/LibJS/AST/expected/catch-param-scope.txt index 2ad471ac39..0536a8df05 100644 --- a/Tests/LibJS/AST/expected/catch-param-scope.txt +++ b/Tests/LibJS/AST/expected/catch-param-scope.txt @@ -34,15 +34,15 @@ Program (script) @2:1 │ │ └─ BindingPattern (object) │ │ ├─ entry │ │ │ └─ name -│ │ │ └─ Identifier "msg" [variable:1] @14:16 +│ │ │ └─ Identifier "msg" [variable:0] @14:16 │ │ └─ entry │ │ └─ name -│ │ └─ Identifier "code" [variable:0] @14:21 +│ │ └─ Identifier "code" [variable:1] @14:21 │ └─ BlockStatement @14:29 │ └─ ReturnStatement @15:9 │ └─ BinaryExpression (+) @15:20 -│ ├─ Identifier "msg" [variable:1] @15:16 -│ └─ Identifier "code" [variable:0] @15:22 +│ ├─ Identifier "msg" [variable:0] @15:16 +│ └─ Identifier "code" [variable:1] @15:22 ├─ FunctionDeclaration "catch_shadow" @20:1 │ └─ body │ └─ FunctionBody @21:5 diff --git a/Tests/LibJS/AST/expected/class-scope.txt b/Tests/LibJS/AST/expected/class-scope.txt index e7cd4b2c9f..35ffbe0228 100644 --- a/Tests/LibJS/AST/expected/class-scope.txt +++ b/Tests/LibJS/AST/expected/class-scope.txt @@ -41,7 +41,7 @@ Program (script) @2:1 │ └─ FunctionBody @20:5 │ ├─ VariableDeclaration (let) @20:5 │ │ └─ VariableDeclarator @20:5 -│ │ ├─ Identifier "key" [variable:1] (let) @20:9 +│ │ ├─ Identifier "key" [variable:0] (let) @20:9 │ │ └─ StringLiteral "hello" @20:22 │ ├─ ClassDeclaration @21:5 │ │ └─ ClassExpression "Bar" @21:5 @@ -51,7 +51,7 @@ Program (script) @2:1 │ │ │ └─ BlockStatement @21:5 │ │ └─ elements │ │ └─ ClassMethod @21:5 -│ │ ├─ Identifier "key" [variable:1] (let) @22:10 +│ │ ├─ Identifier "key" [variable:0] (let) @22:10 │ │ └─ FunctionExpression "" [strict] @22:9 │ │ └─ body │ │ └─ FunctionBody @23:13 @@ -59,7 +59,7 @@ Program (script) @2:1 │ │ └─ NumericLiteral 1 @23:20 │ └─ ReturnStatement @26:5 │ └─ NewExpression @26:12 -│ └─ Identifier "Bar" [variable:0] @26:16 +│ └─ Identifier "Bar" [variable:1] @26:16 └─ FunctionDeclaration "static_members" @30:1 └─ body └─ FunctionBody @31:5 diff --git a/Tests/LibJS/AST/expected/closure-captures.txt b/Tests/LibJS/AST/expected/closure-captures.txt index 7cda06b338..839bd6e397 100644 --- a/Tests/LibJS/AST/expected/closure-captures.txt +++ b/Tests/LibJS/AST/expected/closure-captures.txt @@ -8,7 +8,7 @@ Program (script) @3:1 │ │ └─ NumericLiteral 1 @4:20 │ ├─ VariableDeclaration (let) @5:5 │ │ └─ VariableDeclarator @5:5 -│ │ ├─ Identifier "not_captured" [variable:1] (let) @5:9 +│ │ ├─ Identifier "not_captured" [variable:0] (let) @5:9 │ │ └─ NumericLiteral 2 @5:24 │ ├─ FunctionDeclaration "inner" @6:5 │ │ └─ body @@ -18,8 +18,8 @@ Program (script) @3:1 │ └─ ReturnStatement @9:5 │ └─ BinaryExpression (+) @9:20 │ ├─ CallExpression @9:17 -│ │ └─ Identifier "inner" [variable:0] @9:12 -│ └─ Identifier "not_captured" [variable:1] (let) @9:22 +│ │ └─ Identifier "inner" [variable:1] @9:12 +│ └─ Identifier "not_captured" [variable:0] (let) @9:22 ├─ FunctionDeclaration "level0" @13:1 │ └─ body │ └─ FunctionBody @14:5 diff --git a/Tests/LibJS/AST/expected/default-param-scope.txt b/Tests/LibJS/AST/expected/default-param-scope.txt index 4ea2dfe981..c367517b07 100644 --- a/Tests/LibJS/AST/expected/default-param-scope.txt +++ b/Tests/LibJS/AST/expected/default-param-scope.txt @@ -43,16 +43,16 @@ Program (script) @3:1 │ └─ FunctionBody @17:5 │ ├─ VariableDeclaration (var) @17:5 │ │ └─ VariableDeclarator @17:5 -│ │ ├─ Identifier "v" [variable:1] (var) @17:9 +│ │ ├─ Identifier "v" [variable:0] (var) @17:9 │ │ └─ Identifier "a" [argument:0] @17:13 │ ├─ VariableDeclaration (let) @18:5 │ │ └─ VariableDeclarator @18:5 -│ │ ├─ Identifier "l" [variable:0] (let) @18:9 +│ │ ├─ Identifier "l" [variable:1] (let) @18:9 │ │ └─ Identifier "a" [argument:0] @18:13 │ └─ ReturnStatement @19:5 │ └─ BinaryExpression (+) @19:14 -│ ├─ Identifier "v" [variable:1] (var) @19:12 -│ └─ Identifier "l" [variable:0] (let) @19:16 +│ ├─ Identifier "v" [variable:0] (var) @19:12 +│ └─ Identifier "l" [variable:1] (let) @19:16 └─ FunctionDeclaration "destruct_defaults" @23:1 ├─ parameters │ └─ BindingPattern (object) diff --git a/Tests/LibJS/AST/expected/eval-indirect.txt b/Tests/LibJS/AST/expected/eval-indirect.txt index cfe9c4a55b..dd2f90d9fe 100644 --- a/Tests/LibJS/AST/expected/eval-indirect.txt +++ b/Tests/LibJS/AST/expected/eval-indirect.txt @@ -19,28 +19,28 @@ Program (script) @2:1 │ └─ FunctionBody @10:5 │ ├─ VariableDeclaration (let) @10:5 │ │ └─ VariableDeclarator @10:5 -│ │ ├─ Identifier "x" [variable:1] (let) @10:9 +│ │ ├─ Identifier "x" [variable:0] (let) @10:9 │ │ └─ NumericLiteral 1 @10:13 │ ├─ VariableDeclaration (var) @11:5 │ │ └─ VariableDeclarator @11:5 -│ │ ├─ Identifier "e" [variable:0] (var) @11:9 +│ │ ├─ Identifier "e" [variable:1] (var) @11:9 │ │ └─ Identifier "eval" [global] @11:13 │ ├─ ExpressionStatement @12:5 │ │ └─ CallExpression @12:6 -│ │ ├─ Identifier "e" [variable:0] (var) @12:5 +│ │ ├─ Identifier "e" [variable:1] (var) @12:5 │ │ └─ StringLiteral "x" @12:10 │ └─ ReturnStatement @13:5 -│ └─ Identifier "x" [variable:1] (let) @13:12 +│ └─ Identifier "x" [variable:0] (let) @13:12 ├─ FunctionDeclaration "eval_method" @17:1 │ └─ body │ └─ FunctionBody @18:5 │ ├─ VariableDeclaration (let) @18:5 │ │ └─ VariableDeclarator @18:5 -│ │ ├─ Identifier "x" [variable:1] (let) @18:9 +│ │ ├─ Identifier "x" [variable:0] (let) @18:9 │ │ └─ NumericLiteral 1 @18:13 │ ├─ VariableDeclaration (var) @19:5 │ │ └─ VariableDeclarator @19:5 -│ │ ├─ Identifier "obj" [variable:0] (var) @19:9 +│ │ ├─ Identifier "obj" [variable:1] (var) @19:9 │ │ └─ ObjectExpression @19:15 │ │ └─ ObjectProperty @19:15 │ │ ├─ StringLiteral "eval" @19:17 @@ -48,11 +48,11 @@ Program (script) @2:1 │ ├─ ExpressionStatement @20:5 │ │ └─ CallExpression @20:13 │ │ ├─ MemberExpression @20:8 -│ │ │ ├─ Identifier "obj" [variable:0] (var) @20:5 +│ │ │ ├─ Identifier "obj" [variable:1] (var) @20:5 │ │ │ └─ Identifier "eval" @20:9 │ │ └─ StringLiteral "x" @20:17 │ └─ ReturnStatement @21:5 -│ └─ Identifier "x" [variable:1] (let) @21:12 +│ └─ Identifier "x" [variable:0] (let) @21:12 └─ FunctionDeclaration "local_eval_name" [direct-eval] [uses-this] [might-need-arguments] @25:1 └─ body └─ FunctionBody @26:5 diff --git a/Tests/LibJS/AST/expected/object-property-key-no-capture.txt b/Tests/LibJS/AST/expected/object-property-key-no-capture.txt index 1b0e4b80f6..2512491def 100644 --- a/Tests/LibJS/AST/expected/object-property-key-no-capture.txt +++ b/Tests/LibJS/AST/expected/object-property-key-no-capture.txt @@ -4,7 +4,7 @@ Program (script) @1:1 └─ FunctionBody @2:5 ├─ VariableDeclaration (var) @2:5 │ └─ VariableDeclarator @2:5 - │ ├─ Identifier "x" [variable:1] (var) @2:9 + │ ├─ Identifier "x" [variable:0] (var) @2:9 │ └─ NumericLiteral 1 @2:13 ├─ FunctionDeclaration "inner" @3:5 │ └─ body @@ -15,4 +15,4 @@ Program (script) @1:1 │ ├─ StringLiteral "x" @4:18 │ └─ NumericLiteral 42 @4:21 └─ ReturnStatement @6:5 - └─ Identifier "x" [variable:1] (var) @6:12 + └─ Identifier "x" [variable:0] (var) @6:12 diff --git a/Tests/LibJS/Bytecode/expected/delete-expression-register.txt b/Tests/LibJS/Bytecode/expected/delete-expression-register.txt index a569a8e059..40d8a5e23a 100644 --- a/Tests/LibJS/Bytecode/expected/delete-expression-register.txt +++ b/Tests/LibJS/Bytecode/expected/delete-expression-register.txt @@ -14,21 +14,21 @@ block0: f$2bc22ddd delete-expression-register.js:3:7 Registers: 7 Blocks: 3 - Locals: arguments~0, x~1 + Locals: x~0, arguments~1 Constants: [0] = Undefined [1] = Bool(false) [2] = Int32(0) block0: - [ 0] CreateArguments dst:arguments~0, is_immutable:false - [ 10] Mov3 dst1:x~1, src1:Undefined, dst2:x~1, src2:Bool(false), dst3:reg5, src3:x~1 - [ 30] JumpTrue condition:x~1, target:block2 + [ 0] CreateArguments dst:arguments~1, is_immutable:false + [ 10] Mov3 dst1:x~0, src1:Undefined, dst2:x~0, src2:Bool(false), dst3:reg5, src3:x~0 + [ 30] JumpTrue condition:x~0, target:block2 block1: - [ 40] DeleteByValue dst:reg6, base:arguments~0, property:Int32(0) + [ 40] DeleteByValue dst:reg6, base:arguments~1, property:Int32(0) [ 50] Mov dst:reg5, src:reg6 block2: - [ 60] Mov dst:x~1, src:reg5 - [ 70] Return value:x~1 + [ 60] Mov dst:x~0, src:reg5 + [ 70] Return value:x~0 diff --git a/Tests/LibJS/Bytecode/expected/for-of-cond-rhs-block-order.txt b/Tests/LibJS/Bytecode/expected/for-of-cond-rhs-block-order.txt index c418c27e9a..60404e676c 100644 --- a/Tests/LibJS/Bytecode/expected/for-of-cond-rhs-block-order.txt +++ b/Tests/LibJS/Bytecode/expected/for-of-cond-rhs-block-order.txt @@ -14,7 +14,7 @@ block0: f$0adf9089 for-of-cond-rhs-block-order.js:2:21 Registers: 17 Blocks: 20 - Locals: n~0, r~1 + Locals: r~0, n~1 Constants: [0] = Bool(false) [1] = Undefined @@ -68,7 +68,7 @@ block9: [ 1c8] Jump target:block10 block10: - [ 1d0] Mov dst:r~1, src:reg16 + [ 1d0] Mov dst:r~0, src:reg16 [ 1e0] JumpFalse condition:reg12, target:block12 block11: @@ -80,11 +80,11 @@ block12: [ 220] JumpTrue condition:reg12, target:block11 block13: - [ 230] Mov dst:n~0, src:reg16 + [ 230] Mov dst:n~1, src:reg16 [ 240] JumpFalse condition:reg12, target:block15 block14: - [ 250] ThrowIfTDZ src:r~1 + [ 250] ThrowIfTDZ src:r~0 [ 258] Jump target:block2 block15: diff --git a/Tests/LibJS/Bytecode/expected/local-source-order.txt b/Tests/LibJS/Bytecode/expected/local-source-order.txt new file mode 100644 index 0000000000..e1426d465d --- /dev/null +++ b/Tests/LibJS/Bytecode/expected/local-source-order.txt @@ -0,0 +1,26 @@ +$da832d31 local-source-order.js:12:1 + Registers: 7 + Blocks: 1 + Constants: + [0] = Undefined + +block0: + [ 0] GetGlobal dst:reg6, `source_order_locals` + [ 18] Call dst:reg5, callee:reg6, this_value:Undefined, source_order_locals + [ 38] End value:reg5 + + +source_order_locals$7988cb5f local-source-order.js:6:5 + Registers: 7 + Blocks: 1 + Locals: zebra~0, yak~1, aardvark~2 + Constants: + [0] = Int32(1) + [1] = Int32(2) + [2] = Int32(3) + +block0: + [ 0] Mov3 dst1:zebra~0, src1:Int32(1), dst2:yak~1, src2:Int32(2), dst3:aardvark~2, src3:Int32(3) + [ 20] Add dst:reg5, lhs:zebra~0, rhs:yak~1 + [ 30] Add dst:reg6, lhs:reg5, rhs:aardvark~2 + [ 40] Return value:reg6 diff --git a/Tests/LibJS/Bytecode/expected/nested-function-decl-source-order.txt b/Tests/LibJS/Bytecode/expected/nested-function-decl-source-order.txt index e290c5c845..f96161b991 100644 --- a/Tests/LibJS/Bytecode/expected/nested-function-decl-source-order.txt +++ b/Tests/LibJS/Bytecode/expected/nested-function-decl-source-order.txt @@ -19,23 +19,23 @@ block0: outer$541206d9 nested-function-decl-source-order.js:12:5 Registers: 11 Blocks: 1 - Locals: alpha~0, beta~1, delta~2, dup~3, gamma~4 + Locals: alpha~0, beta~1, dup~2, gamma~3, delta~4 Constants: [0] = Undefined block0: - [ 0] Mov3 dst1:alpha~0, src1:Undefined, dst2:beta~1, src2:Undefined, dst3:delta~2, src3:Undefined - [ 20] Mov2 dst1:dup~3, src1:Undefined, dst2:gamma~4, src2:Undefined + [ 0] Mov3 dst1:alpha~0, src1:Undefined, dst2:beta~1, src2:Undefined, dst3:dup~2, src3:Undefined + [ 20] Mov2 dst1:gamma~3, src1:Undefined, dst2:delta~4, src2:Undefined [ 38] NewFunction dst:alpha~0, shared_function_data_index:0 [ 50] NewFunction dst:beta~1, shared_function_data_index:1 - [ 68] NewFunction dst:gamma~4, shared_function_data_index:2 - [ 80] NewFunction dst:dup~3, shared_function_data_index:3 - [ 98] NewFunction dst:delta~2, shared_function_data_index:4 + [ 68] NewFunction dst:gamma~3, shared_function_data_index:2 + [ 80] NewFunction dst:dup~2, shared_function_data_index:3 + [ 98] NewFunction dst:delta~4, shared_function_data_index:4 [ b0] Call dst:reg5, callee:alpha~0, this_value:Undefined, alpha [ d0] Call dst:reg6, callee:beta~1, this_value:Undefined, beta - [ f0] Call dst:reg7, callee:gamma~4, this_value:Undefined, gamma - [ 110] Call dst:reg8, callee:delta~2, this_value:Undefined, delta - [ 130] Call dst:reg9, callee:dup~3, this_value:Undefined, dup + [ f0] Call dst:reg7, callee:gamma~3, this_value:Undefined, gamma + [ 110] Call dst:reg8, callee:delta~4, this_value:Undefined, delta + [ 130] Call dst:reg9, callee:dup~2, this_value:Undefined, dup [ 150] NewArray dst:reg10, elements:[reg5, reg6, reg7, reg8, reg9] [ 178] Return value:reg10 diff --git a/Tests/LibJS/Bytecode/input/local-source-order.js b/Tests/LibJS/Bytecode/input/local-source-order.js new file mode 100644 index 0000000000..5aa4ac2e72 --- /dev/null +++ b/Tests/LibJS/Bytecode/input/local-source-order.js @@ -0,0 +1,12 @@ +// Local variables should be allocated indices in source order, not in +// alphabetical order. With names that sort in reverse alphabetical +// order, the difference is visible in the bytecode dump (~0 first, etc). + +function source_order_locals() { + let zebra = 1; + let yak = 2; + let aardvark = 3; + return zebra + yak + aardvark; +} + +source_order_locals();