ReviewOS

zig-utils/zig-js

Parser: lexical/var early errors are never checked in most nested function bodies

#930
Closed glennmichael123 opened this 8 days ago ·
8 days ago

Parent: #904 Related: #928 (found while adversarially reviewing that fix), #929 (the opposite direction: valid code rejected)

Invalid programs accepted

Every row below was executed against node and zig-js, not read off the code. node rejects each as a SyntaxError; zig-js parses each successfully (OK, or a later runtime error such as ReferenceError — either way the parser accepted it).

All 20 are pre-existing. A test262 binary built from the parser before #928 gives the same result on every one; #928 neither introduced nor fixed any of them.

The 15 test262 subtrees gated for #928 (11,890 tests, 0 fail) do not exercise these shapes, so a green corpus says nothing about them.

1. Function bodies the scope walk never reaches — 13 cases

The same program inside a function body is checked if that function is a declaration, and silently accepted if it is almost anything else:

programnodezig-js
(function(){ let x; var x; })();SyntaxErroraccepts
g0(function(){ let x; var x; });SyntaxErroraccepts
({ m(){ let x; var x; } });SyntaxErroraccepts
xx = () => { let y; var y; };SyntaxErroraccepts
function h(){ return function(){ let x; var x; } }SyntaxErroraccepts
var a = 1, b = function(){ let x; var x; };SyntaxErroraccepts
let a2 = 1, b2 = class { m(){ let x; var x; } };SyntaxErroraccepts
for (var i = function(){ let x; var x; };;) break;SyntaxErroraccepts
[function(){ let z; var z; }];SyntaxErroraccepts
class K { x = function(){ let z; var z; }; }SyntaxErroraccepts
let {a = function(){ let z; var z; }} = {};SyntaxErroraccepts
new function(){ let x; var x; };SyntaxErroraccepts
function f2(a = function(){ let x; var x; }){}SyntaxErroraccepts

Cause: recurseScope descends an expr_stmt only when the expression is a function or class, and a var_decl initializer only for single declarators. It never follows call arguments, object/array literals, assignments, return values, class field initializers, default parameter values, or the initializers inside a declaration group, destructuring declaration or for head. #928 preserved that reach exactly rather than widening it inside a performance change.

2. with bodies are never walked — 2 cases

programnodezig-js
{ let x; with ({}) { var x; } }SyntaxErroraccepts
with ({}) { let y; var y; }SyntaxErroraccepts

Cause: neither the scope walk nor collectVarNames has a .with_stmt arm.

3. Labelled function declarations are not counted — 3 cases

programnodezig-js
let g; lbl: function g(){}SyntaxErroraccepts
function g2(){ let f; lbl: function f(){} }SyntaxErroraccepts
{ let f; lbl: function f(){} }SyntaxErroraccepts

Cause: only a direct func_decl statement is checked as a var-scoped name, and the lexical seen set has no labeled_stmt arm, so a labelled function is invisible to both sides.

4. Strict duplicate block functions checked with sloppy strictness — 2 cases

programnodezig-js
function s(){ "use strict"; { function g(){} function g(){} } }SyntaxErroraccepts
class C2 { m(){ { function g(){} function g(){} } } }SyntaxErroraccepts

Cause (from reading, consistent with the executed result): addDecl decides rigidity from self.strict, but this check runs after the function body has been parsed, by which point parseFnBody's defer has already restored the enclosing sloppy strictness. Two block-level functions in strict code are then treated as Annex B.3.3 plain duplicates. Top-level strict code and class static blocks are checked during parsing and are unaffected.

Not verified by execution

Readings from the same review that I could not run, because test262 --eval has no module mode. Listed so they are not lost, and not claimed as confirmed:

  • module top-level lexical names are never compared with nested vars — let x; { var x; } as a module;
  • export_decl is not walked — export function f(){ let x; var x; }.

Suggested order

Each family is a separate root cause and can land separately. For each: pin the cases above as a parser test confirmed against node first, then change the walk, then re-run the #928 gate subtrees and the #928 frozen workloads — family 1 widens the walk's reach, so it is exactly the kind of change that could reintroduce the quadratic #928 removed.

8 days ago

Additional executed evidence for family 4 (strict duplicate block functions), found while gating #929: the switch-case form is affected too, not just plain blocks.

programnodezig-js
function g(){ "use strict"; { function f(){} function f(){} } }SyntaxErroraccepts
function g(){ "use strict"; switch (0) { case 0: function f(){} case 1: function f(){} } }SyntaxErroraccepts

Both fail identically with the parsers before and after #929, so they are not caused by it. The switch form fits the diagnosis already given: the whole switch is checked as one block scope, and addDecl reads rigidity from self.strict after the body parse has restored it.

A related but separate bug surfaced in the same corpus and is filed as #931: destructuring catch parameters wrongly allow a same-named var.

8 days ago

Two more executed cases for the reach families here, surfaced while fixing #931. Both are now rejected-by-rule but unreached: the #931 check is correct, but the lexical walk never visits these positions.

programnodezig-jsfamily
(function(){ try {} catch ([e]) { var e; } })()SyntaxErroraccepts1 — function body nested in an expression
try {} catch ([e]) { with ({}) { var e; } }SyntaxErroraccepts2 — with body

Since #931 moved a check into the walk, fixing families 1 and 2 now widens the coverage of that check too, not only of the lexical/var early error — worth including both in any pinned test for those families.

8 days ago

Family 4 is fixed and pushed in 3e792eed (fix(parser): preserve strictness in scope checks (#930)).

The scope walk runs after parseFnBody has restored the enclosing mode. It now temporarily uses FunctionNode.is_strict while checking that body, so duplicate plain functions in strict blocks and switch case blocks reject, while sloppy Annex B.3.3 duplicates remain valid. The mode is restored before checking sibling functions.

Evidence:

  • both reported block and switch programs: pre-fix OK 1; after ERR UnexpectedToken; Node 24.18 rejects both with SyntaxError;
  • pinned strict function, inherited-strict nested function, class method, block, and switch cases, plus sloppy and restoration controls;
  • parser units: 77 passed, 0 failed/leaked;
  • focused frontend: 26 passed;
  • before/after test262: block 21, switch 111, function-code 217, statement classes 4,367, expression classes 4,059 — 8,775 pass and 0 fail on both binaries, 0 flips.

Families 1–3 remain open on this issue.

8 days ago

Family 3 is fixed and pushed in 0a2c1cbb (fix(parser): count labelled function declarations (#930)).

The declaration collector now peels an Annex B.3.2 label chain to its FunctionDeclaration. The name follows the containing statement-list rules: var-scoped in Script/FunctionBody lists and lexical in Block/CaseBlock lists. Catch-parameter lexical conflicts use the same classification.

Evidence:

  • the three reported programs, plus block var/class conflicts, nested labels, CaseBlock conflicts, and a destructuring-catch conflict now reject as Node 24.18 does;
  • five Node-confirmed valid controls preserve top-level/function-body var coexistence and sloppy duplicate block/switch functions;
  • parser units: 78 passed, 0 failed/leaked;
  • focused frontend: 26 passed;
  • before/after test262 over labeled, block, switch, function-code, and try: 574 pass, 0 fail on both binaries, 0 flips.

Families 1 and 2 remain open.

8 days ago

Family 2 is fixed and pushed in ada64fff (fix(parser): traverse with bodies in declaration checks (#930)).

Both declaration walkers now descend through a with statement's body. The object environment changes runtime lookup but is not a lexical or var declaration boundary. This also extends #931's destructuring-catch conflict check through with, as requested.

Evidence:

  • the two reported lexical/var programs and the #931 catch program: pre-fix OK undefined; after ERR UnexpectedToken; Node 24.18 rejects all three;
  • pinned nested statement and lexical-for-head conflicts exercise both walkers, with function-boundary and outer-var controls preserved;
  • parser units: 79 passed, 0 failed/leaked;
  • before/after test262 over with, block, try, for, for-in, for-of, and function-code: 1,871 pass, 0 fail on both binaries, 0 flips.

Family 1 remains open.

8 days ago

Family 1 is fixed and pushed in bb48fb2 (fix(parser): check every function body as it is parsed (#930)), with the shapes from the #931 comment above pinned in f2bafeb.

Change. Every function body is now checked once, the moment parseFnBody or a block-bodied arrow finishes it, against a fresh scope. A function body is a fresh var scope, so nothing outside it can take part in its conflicts. The check also runs with the body's own strictness still in force, which the family 4 fix previously had to restore by hand. The post-parse scope walk no longer descends into functions or class members at all. Keeping that descent next to the per-body check would walk every nested body once per enclosing body, which is #928's quadratic.

Executed, Debug f4100cc (before) vs bb48fb2 (after), node v24.4.1 as the oracle:

programbeforeafternode
the 13 family-1 rows in the issue bodyacceptedSyntaxErrorSyntaxError
`${function(){ let x; var x; }}`;acceptedSyntaxErrorSyntaxError
({ get p(){ let x; var x; } });acceptedSyntaxErrorSyntaxError
async () => { let x; var x; };SyntaxErrorSyntaxErrorSyntaxError
(function(){ try {} catch ([e]) { var e; } })() (from the #931 comment)acceptedSyntaxErrorSyntaxError
[() => { try {} catch ([e]) { var e; } }];acceptedSyntaxErrorSyntaxError
(function(){ with ({}) { let q; var q; } })();acceptedSyntaxErrorSyntaxError
(function(){ "use strict"; { function g(){} function g(){} } })();acceptedSyntaxErrorSyntaxError
x = class { m(){ switch (0) { case 0: function g(){} case 1: function g(){} } } };acceptedSyntaxErrorSyntaxError

Twelve programs node accepts are unchanged and pinned as a single case. They cover distinct names, duplicate vars, a lexical shadow in a nested block, a var over an outer function's lexical name, sloppy Annex B duplicate block functions (an object literal method included), strict single block functions, names reused across or after sibling functions, and a var over a simple catch parameter.

Growth, as #933 item 6 requires. I re-measured all 18 #926/#928 rows and item 2's function-body rows at 256–2048, using the median of 18 samples a side with the two binaries interleaved. Every row grows about x2 per doubling before and after, at the top level and inside a function; the table is in bb48fb2. One reading, private_nested_single_inner after at 2048, came out at 636 µs. Three repeats give 267–271 µs, x2.00–x2.01. The same row run under taskpolicy -b (efficiency cores) gives 600–770 µs, median ~646, so that reading was the process landing on E-cores.

Conformance. Debug before and after, 0 fail on both sides, identical --diag output. The subtrees are test/language/statements 9,337, expressions 11,038, test/annexB 1,086, block-scope 145, function-code 217, eval-code 347, global-code 42, arguments-object 263, and test/built-ins/Function 509: 22,984 tests in all. Flips 0 cases. Parser units 215/215; focused frontend 58, runtime 17, vm 4.

All four families are fixed (4: 3e792ee, 3: 0a2c1cb, 2: ada64ff, 1: bb48fb2), so I'm closing this.

Sign in to comment on this issue.