fix(parser): restrict continue targets to iteration statement labels - #646
Conversation
`a: { for (;;) { continue a; } }` parsed. A `continue` may only target a label
of an iteration statement it is nested in, and here `a` labels a block, so
ContainsUndefinedContinueTarget makes it an early SyntaxError. The same hole
accepted a label on an `if`, on a `switch` case, on a `try` block and on a block
nested inside a loop.
`isValidLabel` in src/common.ts walks the label set chain outwards from the
`continue` and cleared its "must be inside an iteration statement" requirement at
the first set marked `loop`, then accepted the label wherever it turned up further
out, whatever statement it was attached to.
`parseIterationStatementBody` marks an iteration statement's body set with `loop`,
and a labelled statement declares its label into the set it is itself parsed with,
so the set directly above a `loop` set holds that iteration statement's own labels.
The walk now tracks whether the set it is looking at is such a set and only accepts
a `continue` label declared there. `break` passes `isIterationStatement` as 0 and is
unchanged.
Drops the two `language/statements/block/labeled-continue.js` lines from
test262/whitelist.txt.
commit: |
3cp
left a comment
3cp
left a comment
There was a problem hiding this comment.
Thx! Only might need two small adjustments.
| > 1 | a: { b: for (;;) { continue a; } } | ||
| | ^ continue statement must be nested within an iteration statement" |
There was a problem hiding this comment.
The error location can be fixed by swapping parseIdentifer and if (!isValidLabel) in src/parser.ts.
There are two usages in src/parser.ts. Take one as example:
// one in parseContinueStatement
const { tokenValue } = parser;
if (!isValidLabel(parser, labels, tokenValue, /* requireIterationStatement */ 1))
parser.report(Errors.UnknownLabel, tokenValue);
label = parseIdentifier(parser, context | Context.AllowRegExp);There was a problem hiding this comment.
Also the printed error "continue statement must be nested within an iteration statement" sounds very confusing.
@fisker any suggestion to improve that error message? It probably should reflect the label was not for a loop statement.
Acorn uses "Unsyntactic break/continue" for all types of error. It isn't much better.
There was a problem hiding this comment.
Acorn uses "Unsyntactic break/continue" for all types of error. It isn't much better.
Agree. And babel also use the same, I guess because they started as a fork of acorn.
Oxc uses
Label 'a' does not denote an iteration statement
better?
|
@fisker we probably need to improve the type def for |
Validate the label before parseIdentifier consumes it, in both parseContinueStatement and parseBreakStatement, so the reported error points at the label itself instead of the token after it. Reword the iteration statement error to "Label 'a' does not denote an iteration statement", the wording Oxc uses, and pass the label name to it. Snapshots regenerated.
|
Thank you! |
3 participants
a: { for (;;) { continue a; } }parses. Acontinuemay only target a label of an iteration statement it is nested in, andahere labels a block, so ContainsUndefinedContinueTarget makes it an early SyntaxError. The same hole accepts a label on anif, on aswitchcase, on atryblock, and on a block nested inside a loop. V8 and Acorn reject all of them, and the whitelisted test262 fixturelanguage/statements/block/labeled-continue.jsis exactly this shape.isValidLabelinsrc/common.tswalks the label set chain outwards from thecontinueand cleared its "must be inside an iteration statement" requirement at the first set markedloop, then accepted the label wherever it turned up further out. Any loop between thecontinueand the label satisfied the check, whatever statement the label was actually attached to.parseIterationStatementBodymarks an iteration statement's body set withloop, and a labelled statement declares its label into the set it is itself parsed with, so the set directly above aloopset holds that iteration statement's own labels. The walk now tracks whether the set it is looking at is such a set, and only accepts acontinuelabel declared there.breakpassesisIterationStatementas 0 and is untouched, soa: { for (;;) { break a; } }still parses; there is a test for that.Verification on Node 22 against
mainat 56416fa. The seven newly rejected shapes fail on unmodifiedmainwith "Expect a ParserError thrown" and pass with the change; six accepted shapes are added alongside them, includinga: b: for (;;) { continue a; },a: for (;;) { for (;;) continue a; }anda: while (x) { b: { continue a; } }.vitest runis 91860 passing over 139 files with no existing snapshot touched. Full test262 at the pinned commit 7710052 goes from 8694 to 8696 invalid programs rejected while valid programs accepted stays at 94569, so nothing valid was tightened out, and the twolabeled-continue.jswhitelist lines come out. The ast alignment, jsx alignment and production suites pass, andeslint,prettier --check,tsc,cspell,knipandgenerate-unicode.mjs --checkare clean.