When an identifier was registered and its group already existed but
had no declaration_kind set, we failed to propagate it. This caused
var declarations to lose their annotation in AST dumps when the
identifier was referenced before its declaration.
ScopeCollector::add_declaration() was adding var declarations to the
top-level scope's m_var_declarations once per bound identifier and once
more after the for_each_bound_identifier loop - so a `var a, b, c`
would be added 4 times instead of 1.
The Script constructor iterates m_var_declarations and expands each
entry's bound identifiers, resulting in O(N²) work for a single var
statement with N declarators.
Running the Emscripten-compiled version of ScummVM with a 32,174-
declarator var statement, this produced over 1 billion entries,
consuming 14+ GB of RAM and blocking the event loop for 35+ seconds.
After this fix, this drops down to 200 MB and just short of 200ms.
When a function accesses the arguments object in non-strict mode, scope
analysis was skipping argument index assignment for all parameter
candidates. This is correct for regular parameters (which participate in
the sloppy-mode arguments-parameter linkage), but rest parameters never
participate in that linkage and should always get their argument index.
Destructured parameter bindings (e.g. the x and y in function f([x, y]))
were not receiving any local indices during scope analysis. This meant
they could not benefit from the fast local variable access path in the
bytecode interpreter.
The issue had two parts:
1. set_function_parameters() only registered plain Identifier parameters
with the IsParameterCandidate flag. BindingPattern parameters only
got IsForbiddenLexical, which caused their identifiers to be skipped
entirely during resolve_identifiers().
2. resolve_identifiers() unconditionally called set_argument_index() for
all parameter candidates, but get_index_of_parameter_name() only
finds plain Identifier parameters, not bindings inside patterns.
Fix this by registering destructured binding identifiers with
IsParameterCandidate so they participate in resolution, and then falling
back to a local variable slot when get_index_of_parameter_name() does
not find them. This is correct because the argument register holds the
whole object/array to be destructured, while the individual bindings
live in local variable slots.
has_declaration_in_current_function() only checked IsLexical and
IsVar flags, but parameters carry IsParameterCandidate instead.
This meant a parameter named "arguments" wasn't recognized as a
declaration, causing the scope analysis to incorrectly treat it
as an access to the arguments exotic object instead of a plain
parameter binding.
Add IsParameterCandidate to the flags check so parameters are
correctly recognized, allowing "arguments" to get its [argument:N]
index like any other parameter.
Replace the ScopePusher RAII class (which performed scope analysis
in its destructor chain during parsing) with a two-phase approach:
1. ScopeCollector builds a tree of ScopeRecord nodes during parsing
via RAII ScopeHandle objects. It records declarations, identifier
references, and flags, but does not resolve anything.
2. After parsing completes, ScopeCollector::analyze() walks the tree
bottom-up and performs all resolution: propagate eval/with
poisoning, resolve identifiers to locals/globals/arguments, hoist
functions (Annex B.3.3), and build FunctionScopeData.
Key design decisions:
- ScopeRecord::ast_node is a RefPtr<ScopeNode> to prevent
use-after-free when synthesize_binding_pattern re-parses an
expression as a binding pattern (the original parse's scope records
survive with stale AST node pointers).
- Parser::scope_collector() returns the override collector if set
(for synthesize_binding_pattern's nested parser), ensuring all
scope operations route to the outer parser's scope tree.
- FunctionNode::local_variables_names() delegates to its body's
ScopeNode rather than copying at parse time, since analysis runs
after parsing.
