Repository navigation
fix(repl): skip regex literals when validating input - #36375
Conversation
`deno_ast::lex` runs without the parser, so a regex literal comes back as a division operator followed by however the rest of the pattern tokenizes. A character class containing a quote produces a stray string literal that swallows the closing `]`, leaving the `[` orphaned on the bracket stack, so `/[']/` either reported mismatched pairs or blocked waiting for more input. Reuse `find_regex_literal_end`, which already handles character classes and escapes for syntax highlighting, to skip the tokens a regex literal covers. Deciding that a `/` starts a regex is a guess, since expression position isn't tracked, so once the guess has been acted on the validator only suppresses errors and never reports one: a wrong "valid" is recovered by V8 reporting the real syntax error, but a wrong "invalid" wedges the REPL. Fixes denoland#24963.
Deno Individual Contributor License AgreementAll contributors have signed the CLA. Thank you! This is an automated message from CLA Assistant |
|
Nice diagnosis, and reusing One blocking issue though: the monotonicity guard is incomplete, so the The final guard checks
The first one is the sharp case: the slash-scan finds its "closing Suggested fix — compute the result first, then suppress any non- let result = if let Some(error) = queued_validation_error {
error
} else if !stack.is_empty() || in_template || ends_with_dot {
ValidationResult::Incomplete
} else {
ValidationResult::Valid(None)
};
if skipped_regex && !matches!(result, ValidationResult::Valid(_)) {
// We guessed that a `/` started a regex and skipped over it. If that left
// the input looking unbalanced the guess was probably wrong, so hand it to
// V8 rather than rejecting it or blocking on more input.
return ValidationResult::Valid(None);
}
resultI tried this locally: both cases above go back to Smaller things, none blocking:
No security or performance concerns; the scan is O(line) per slash on |
|
thanks, that was a good catch, I have reproduced both cases, and applied the restructure with all the four conditions. I have taken note of the three minor points too. |
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Nice — reusing find_regex_literal_end (already line-scoped: it returns None on \n/\r) instead of the "two slashes and bail" heuristic is the right call, and the "a skipped regex may only ever suppress, never report" invariant is what makes this safe without tracking expression position.
Checked the cases I was worried about:
- Multi-line input with a division on each line — the newline bail in
find_regex_literal_endmeans nothing is skipped, soIncompletestill works and the REPL keeps reading. - A line that genuinely contains two
/s in division position now trips the rescan and suppresses. That's not a regression: the olddiv_token_count >= 2path already returnedValid(None)for exactly those inputs. ends_with_dotis assigned before thecontinueon a detected regex, so a trailing.inside a skipped span can't leak through as a pending member access.- A string token that starts inside the regex span but ends past it gets dropped wholesale, which can leave the rest of the line mis-tokenized — but that lands in
Valid(None)via the suppression rule, so V8 reports the real error. Fine.
Tests cover the reported case, division, the unterminated-regex Incomplete path, and the two ambiguous-position cases. CI is green. LGTM.
2 participants
Summary
deno_ast::lexruns the lexer without the parser, so it never recognizes regex literals. A literal comes back as a division operator followed by however the rest of the pattern happens to tokenize, and a character class containing a quote produces a stray string literal that swallows the closing]. That leaves the[orphaned on the bracket stack, so typinglet re = /[']/;in the REPL either reported "Mismatched pairs" or blocked waiting for more input.The repo already had the piece needed to fix this.
find_regex_literal_endscans a regex literal correctly, tracking character classes and escapes, but it was only wired into syntax highlighting.validate()instead counted/tokens per line and bailed once it saw two, with a comment conceding that detecting regex literals properly was too complicated.This reuses that helper in
validate()to skip the tokens a regex literal covers.Problem
Deciding whether a
/starts a regex or is a division depends on expression position, which this code does not track.highlight_linehas aregex_allowedflag for it, and its own comment notes the flag is knowingly imperfect after)and++/--, which is fine for coloring.That tradeoff is not fine for validation, where a wrong guess decides whether the REPL accepts input or hangs. So rather than porting that flag, this change carries a stricter rule: once a regex literal has been skipped, the validator may only suppress an error, never report one. If skipping leaves the input looking unbalanced, it returns
Valid(None)and lets V8 decide.That makes the change strictly monotonic. A wrong "valid" is recovered immediately because V8 reports the real syntax error, whereas a wrong "invalid" or "incomplete" wedges the REPL, which is the bug being fixed. The existing division counter is kept as the fallback for a
/that is not a regex.Fixes #24963.
Test Plan
Four tests added to the existing test module in
cli/tools/repl/editor.rs:validate_regex_literal_with_quotescovers the reported case,/["]/, the issue's originalfunction fn() { if (/[']/); }, both quote kinds with flags, a pattern with escaped slashes, and a regex on the second line of multi-line input.validate_division_is_not_a_regexchecksa / banda / b / cstill parse.validate_unterminated_regex_is_incompletechecksconst re = /[still asks for more input.validate_ambiguous_slash_never_reports_an_errorcovers the two cases the position heuristic cannot resolve,if (x) /re/.test(y)andx++ /2/ y, asserting neither produces an error.Verified the new tests fail before the change.
cargo test -p deno --lib validate_passes 19 tests, including every pre-existingvalidate_*test.One note on formatting:
tools/format.jsandtools/lint.jsneed adenobinary I could not build locally, so I ran the underlying formatter directly with the flags from.dprint.json,rustfmt --config imports_granularity=item --config group_imports=StdExternalCrate --check cli/tools/repl/editor.rs, which exits clean. Plaincargo fmtis not equivalent, since it omits both config flags.AI disclosure
The fix and tests here were written with Claude Code, working from my direction on the approach, in particular the decision that skipping a regex may only suppress a validation error and never report one.