Skip to content

Fix regexp validation of bytes the lexer drops from the content - #4355

Open
koic wants to merge 1 commit into
ruby:mainfrom
koic:fix_regexp_validation_with_escaped_delimiter
Open

koic wants to merge 1 commit into
ruby:mainfrom
koic:fix_regexp_validation_with_escaped_delimiter

Conversation

@koic

@koic koic commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

Since #3984, the validation pass of a regular expression walks its raw source instead of its unescaped content. The lexer leaves some bytes of the source out of the content, so the validation could see a different regular expression from the one Ruby compiles, and reported errors for valid code:

$ ruby --parser=parse.y -e 'p %r!(?\!x)!'
/(?!x)/

$ ruby -Ilib -rprism -e 'p Prism.parse(%q{%r!(?\!x)!}).errors.map(&:message)'
["undefined group option"]

This surfaced as a failing Style/RegexpLiteral spec in rubocop/rubocop#16340 when RuboCop's CI moved to prism 2.1.0.

The bytes are:

  • the backslash of an escaped closing delimiter, which the lexer drops unless Onigmo needs it (%r{(?\}x)} keeps it and is still an error)
  • a backslash followed by a newline, as in /(?\ + newline + !x)/
  • the backslash of an escaped non-ASCII character, as in /\あ/

The last one did not report a false error. Instead the validation stopped silently at the character, so it missed the errors after it and flagged the regular expression as US-ASCII, which is what the seattlerb/regexp_escape_extended snapshot had recorded.

When the source has any of these, the validation pass now walks a copy without them. The raw source is still needed there to tell escape-produced bytes from literal ones, which the unescaped content cannot do, so the copy leaves every other escape as written, and takes control and meta escapes whole, since their operand can be an escaped newline. The lexer and the copy now ask the same function which closing delimiters keep their backslash, so that the two cannot drift apart.
Error locations in the copy are mapped back to the source, so they stay exact. The mapping resumes from the previous location, so that a regular expression with many errors is not walked from its start for each of them.

Since ruby#3984, the validation pass of a regular expression walks its raw source instead of its unescaped content.
The lexer leaves some bytes of the source out of the content, so the validation could see a different regular expression from
the one Ruby compiles, and reported errors for valid code:

```console
$ ruby --parser=parse.y -e 'p %r!(?\!x)!'
/(?!x)/

$ ruby -Ilib -rprism -e 'p Prism.parse(%q{%r!(?\!x)!}).errors.map(&:message)'
["undefined group option"]
```

This surfaced as a failing `Style/RegexpLiteral` spec in rubocop/rubocop#16340 when RuboCop's CI moved to prism 2.1.0.

The bytes are:

- the backslash of an escaped closing delimiter, which the lexer drops unless Onigmo needs it
  (`%r{(?\}x)}` keeps it and is still an error)
- a backslash followed by a newline, as in `/(?\` + newline + `!x)/`
- the backslash of an escaped non-ASCII character, as in `/\あ/`

The last one did not report a false error. Instead the validation stopped silently at the character, so it missed
the errors after it and flagged the regular expression as US-ASCII, which is what the `seattlerb/regexp_escape_extended`
snapshot had recorded.

When the source has any of these, the validation pass now walks a copy without them. The raw source is still needed there
to tell escape-produced bytes from literal ones, which the unescaped content cannot do, so the copy leaves
every other escape as written, and takes control and meta escapes whole, since their operand can be an escaped newline.
The lexer and the copy now ask the same function which closing delimiters keep their backslash, so that
the two cannot drift apart.
Error locations in the copy are mapped back to the source, so they stay exact. The mapping resumes from the previous
location, so that a regular expression with many errors is not walked from its start for each of them.
@kddnewton

Copy link
Copy Markdown
Collaborator

I agree with you this is a problem, but I don't agree with the solution here. This is effectively recreating the lexer a third time. We really need to be mirroring the preprocess pass, then onigmo's parsing, then onigmo's compiling, and each one needs to stop if the previous one found errors. I'm actually working on doing exactly that right now because we have other problems for non-CRuby consumers where they don't get all of the errors because we don't vendor onigmo. I'll keep this open for now as effectively an issue and close it when I get my PR up.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants