Fixed "for" statement processing - #1118
Open
xeioex wants to merge 10 commits into
Open
Conversation
xeioex
force-pushed
the
fix_parse_for_in
branch
from
September 1, 2026 05:04
21e1c82 to
d1cfa99
Compare
xeioex
changed the title
Previously, the "for" initializer was parsed speculatively as a LeftHandSideExpression, and njs_parser_for_expression_map_reparse() resumed parsing whenever that parse produced no node. njs_parser_reject() rewinds the state stack only, so the resume continued from a later token with an abandoned function scope still on parser->scope, and generated bytecode with a scope that is never instantiated at run time. The fix is to remove that landing state together with its frame, so a rejected speculative parse reports a SyntaxError, and to dispatch the initializer starts that cannot begin a LeftHandSideExpression to <Expression> directly. Routing "await" through that dispatch also applies [~In] to it, so a bare "in" in an await initializer is now a SyntaxError. Found by OSS-Fuzz.
Previously, the [In] parameter lived in the lexer, in a byte array indexed by
bracket nesting depth. The lexer advanced the depth while prereading, the
parser wrote at the depth it had consumed to, so a delimiter in a conditional
consequent left the write in the wrong slot and the restriction was lost:
for (a ? (b) : c in d; false;);
The fix is to carry the parameter in njs_parser_stack_entry_t: a frame
records the value in effect when it was pushed and restores it when popped,
including on the njs_parser_reject() landing frame. An arrow initializer now
gets [~In] as well, which it escaped before.
This also allows to remove the [In] support from the lexer. The parser
owns the parameter now. njs_lexer_t drops from 104 to 80 bytes and no longer
allocates.
Previously, an invalid for-in target was a ReferenceError when the
initializer happened to parse as a LeftHandSideExpression and a SyntaxError
otherwise, so the same target changed class once parenthesized:
for (a = 1 in {}); SyntaxError
for ((a = 1) in {}); ReferenceError
The fix is to report a SyntaxError for both, as the specification and V8 do.
The type is observable through Function(), so this is a compatibility change.
njs keeps ReferenceError for invalid assignment targets elsewhere.
Previously, the initializer was parsed twice: speculatively as a
LeftHandSideExpression so that the token after it could be inspected, and
then the rest of the expression was grafted onto that left-hand side by four
continuation states. A dispatch list and an arrow pre-check existed only to
keep inputs away from the speculative parse.
The fix is to let [~In] end the initializer: the "in" operator is no longer
part of the expression, so the header is a single <Expression> and
njs_parser_for_var_in_of_expression(), which already classified the other
routes, classifies this one too. Its post-hoc for-in detection goes with it.
A declaration header stopping at "in" now reports an invalid target instead
of an unexpected token:
for (var a, b in []);
The first token's text was carried through the header only to be quoted in
the removed diagnostic, so the allocation goes with it.
Previously, use_lhs told njs_parser_exponentiation_expression() to skip the unary part and njs_parser_assignment_expression() to skip the arrow check, so that an expression could resume from an already parsed left-hand side. The fix is to remove it: its only writer went with the speculative "for" initializer parse.
Previously, njs_parser_cover_parenthesized_expression(), its two continuations and njs_parser_binding_identifier_pattern() formed a closed island: every reference to them came from inside it, and the only thing linking it to the parser was a "(void)" cast left with a TODO in 2020. It honours neither contract this series introduces. Two of its states are reject landing states that never inspected parser->ret, so with a ")" or a "," in hand they would consume the token and pop the stack after a reject. Its parenthesized expression opens no [+In] region. The fix is to delete the island rather than repair it. Arrow parameters are not going to be completed through this production, and the binding pattern entry points it dispatched to are stubs reachable on their own.
Previously, the kind of a declaration being parsed lived in a mutable
parser->var_type, written once when "var", "let" or "const" was seen and read
for every declarator in the list. A declaration nested in a declarator
initializer overwrote it, so the declarators after it were recorded with the
wrong kind and lexical ones lost their per-iteration binding:
let out = [];
for (let a = function() { var z; }, b = 0; b < 2; b++) out.push(() => b);
out[0]() + ',' + out[1]()
gave "2,2" instead of "0,1", because "b" was recorded as a var.
The fix is to carry the kind in njs_parser_stack_entry_t, as the [In]
parameter already is, so a continuation restores the kind in effect when it
was pushed. njs_parser_stack_entry_t does not grow.
Previously, the declaration route of a for-in loop stored the resolved variable index in the AST wrapper njs_parser_node_t, and read it back one generator state later. The other two routes keep the same value in ctx->index_next_value, a field njs_generator_loop_ctx_t already has for it. The inconsistency was the only difference between njs_generate_for_in_body_wo_decl() and njs_generate_for_in_body(): the rest was duplicated verbatim, including a guard that is dead in the copy. The fix is to use the context on the declaration route as well and delete the duplicate. One AST field is no longer written by the generator, and one of the three shape-selected for-in paths is gone. The emitted code is unchanged.
Previously, njs_parser_for_var_binding_or_var_list() peeked one token past a declaration binding and, on "in", built the for-in nodes itself, consumed the tokens and used its own right-hand-side and body states. A declaration header therefore never reached the classifier the expression header uses, and the two carried separate copies of the same completion. The fix is to parse an identifier binding once under [~In] and let njs_parser_for_var_in_of_expression() classify the token it stopped at. The "in" case now accepts a declaration result, checks there that a for-in declaration has one binding and no initializer, and continues through the same right-hand-side, body and NJS_TOKEN_FOR_IN construction as an expression target. njs_parser_for_var_in_statement() and its completion go with the duplication, and the restricted-identifier check is left to the ordinary declaration parser, which reports it identically. A binding pattern still leaves this route before the classifier, into the handlers that report it as unsupported. The tree shape is unchanged, so the generator still selects a for-in route the way it did, and the emitted code is identical. The declaration node now takes its source line from the ordinary declaration parser, so a header split across lines reports the line of "in" rather than of the binding; this shows only in the AST dump, not in generated code or backtraces.
Previously, njs_generate_for_in_statement() worked out what a for-in header declared from the shape of the tree: a declaration was recognised as a STATEMENT wrapper whose right child held the kind, and the kind and the name were reached through that wrapper. The wrapper was navigated again in the object and body states, and njs_generate_for_let_update() walked it to find lexical bindings. Nothing named the contract, and the property route used the same "foreach->left->right" to mean the property key. The fix is to let the parser put the declaration node under the "in" node directly, so the target's token type says what it is: VAR, LET or CONST for a declaration, PROPERTY or PROPERTY_REF for a member target, and NAME for a plain one. An unrecognised target is now an internal error instead of falling into the plain-name route. njs_generate_let_update() takes the single declaration a for-in header has, and njs_generate_for_let_update() keeps the list walk the classic "for" needs. The instruction stream is unchanged. The code map for PROPERTY_NEXT on the declaration route now carries the line of the declaration instead of the zero the removed wrapper held; backtraces are unaffected.
xeioex
force-pushed
the
fix_parse_for_in
branch
from
September 2, 2026 15:00
5b581d5 to
69edc85
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a systematic fix of the issue from PR 910.
Very good simplification of the parser/generator, -600 lines net.