Skip to content

fix: prevent escape fallback from swallowing malformed constructs and enable error test suite - #130

Merged
fluentfuture merged 2 commits into
google:masterfrom
adi-IL:fix/regex-escape-fallback-and-error-tests
Aug 23, 2026
Merged

fix: prevent escape fallback from swallowing malformed constructs and enable error test suite#130
fluentfuture merged 2 commits into
google:masterfrom
adi-IL:fix/regex-escape-fallback-and-error-tests

Conversation

@adi-IL

@adi-IL adi-IL commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary of Changes

  1. Prevent Escape Fallback Swallowing Malformed Constructs:

    • In RegexParsers.java, the generic escaped character fallback string("\\").then(...) previously only excluded 'x' via isNot('x'). When specific escape constructs failed due to malformed or incomplete syntax (such as \N{} empty name, \N{abc unclosed name, \u123z invalid hex digits, \u incomplete unicode escape, \08 or \0 invalid octal escape), the parser fell through and consumed the leading character as a literal escaped character (Literal("N"), Literal("u"), Literal("0")).
    • Added .as("character name") to the \N{...} name parser.
    • Updated the generic escape fallback predicate to noneOf("0123456789xNu"), preventing digits, x, N, and u from falling through to literal single-character escapes when their specific syntax fails.
  2. Enable and Expand Error Test Suite:

    • Renamed RegexParserErrorTests.java to RegexParserErrorTest.java (matching Maven Surefire's default **/*Test.java include pattern), allowing all 75 error tests to be automatically discovered and executed during standard mvn test runs.
    • Added tests in RegexParserErrorTest.java covering \u123z, \u, \08, and \0.
    • Updated ParsersRegexCheckTest.java in mug-errorprone to assert the compile-time diagnostic expecting <4 hex digits> for \u123z.

Validation

Executed full clean test suite across all 16 reactor modules:

  • mvn clean test passed with 0 failures and 0 errors.
  • dot-parse: 2,480 tests passed (including all 75 tests in RegexParserErrorTest).
  • mug-errorprone: 929 tests passed.

@fluentfuture

fluentfuture commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Thanks for submitting the fix!

AI identified a minor improvement. Please verify if it's applicable.

While this excludes digits, x, N, and u, it omits k, p, and P:

  • Unclosed / Malformed Property (\p{foo / \P{foo): When \p{foo fails to parse as a valid property, the parser falls through to string("\\").then(one(noneOf("..."))) and successfully matches \p as a literal 'p', parsing \p{foo as Literal("p{foo") instead of throwing ParseException.
  • Malformed Named Backreference (\k<foo): When \k<foo (missing closing >) fails word().between("\k<", ">"), it falls through and parses as Literal("k<foo").

Recommended Fix
Include k, p, P in the exclusion set.

The full list:

noneOf("0123456789xNuckpP")

.map(Character::codePointOf)
.map(Character::toString),
string("\\").then(one(isNot('x'), "escaped char")).map(String::valueOf)));
string("\\").then(one(noneOf("0123456789xNu"), "escaped char")).map(String::valueOf)));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use noneOf("0123456789xNuckpP")

@adi-IL

adi-IL commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review @fluentfuture! Added c, k, p, and P to the exclusion set (noneOf("0123456789xNuckpP")) and added test cases in RegexParserErrorTest covering \p{foo, \P{foo, \k<foo, and \c.

@fluentfuture
fluentfuture merged commit e34619e into google:master Aug 23, 2026
12 checks passed
@fluentfuture

Copy link
Copy Markdown
Collaborator

AI suggested me to also change the one(isLetter, "category") to one("[a-zA-Z]").as("category") because JDK regex allows only ascii for the unbraced form.

If the use of isLetter is intentional, please speak up.

@adi-IL

adi-IL commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Oh yeah that makes total sense, definitely was not intentional. In JDK regex the unbraced single letter properties like \pL are strictly ASCII [a-zA-Z], and non-ASCII characters like \pα get rejected with an unknown property error.

Switching to one("[a-zA-Z]").as("category") is definitely the right call and fits dot-parse prefix pruning nicely too.

Thanks again for the quick review and merge.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants