Fix catastrophic backtracking in numeric regexes - #50
Conversation
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe numeric regex patterns replace an ambiguous nested quantifier with ChangesNumeric regex backtracking fix
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change removes catastrophic backtracking from numeric parsing patterns while preserving tested matching and capture behavior. The updated patterns and regression coverage indicate no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #50 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 464 464
=========================================
Hits 464 464 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Capture group 2 of both numeric patterns was an ambiguous nested quantifier:
Every string the inner group matches begins and ends with a digit, so concatenating two of them yields another string of the same shape. The outer
*added no strings to the language, only exponentially many ways to split one digit run across iterations — which the engine explores in full whenever the overall match fails.numericRegex.exec('1'.repeat(30) + '!')on Node/V8: 15.6 s → 0.08 ms. Growth is now flat rather than 4× per two digits.Equivalence
?and*accept the same language here, so this is a pure performance change:,or_.A new test asserts this empirically: it reconstructs the old pattern from the new one and compares
execresults across ~30 inputs covering separators, decimals, exponents, fractions, mixed numbers, signs, malformed separators, and trailing invalid characters. The existing 550-case fixture suite is unchanged and passes.Scope
numericRegexWithTrailingInvalidcarries the same construct and is fixed alongsidenumericRegex— the two must stay in sync, and consumers embedding its source in an anchored context would hit the same blowup. In isolation it does not backtrack today, because its permissive(\s*[^.\d/].*)?tail means a match almost never fails.numericQuantitywas never affected, for that reason. I also profiledromanNumeralRegex,romanNumeralUnicodeRegex,vulgarFractionsRegex,superSubDigitsRegex,normalizeDigits,parseRomanNumerals, andnumericQuantityunder currency/percentage/comma-decimal options at growing input sizes; none show superlinear growth, so no other pattern needed changing.Changes
src/constants.ts— the two literals, plus a JSDoc note onnumericRegexexplaining why group 2 is?and not*.src/index.test.ts— newnumeric regex backtrackingsuite: the equivalence comparison above, a source assertion guarding against a revert, and a timing regression test.CHANGELOG.md— entry under Unreleased → Fixed.bunx tsc,bun run build,bun run test(100% coverage held),bun run fmt --check, andbun run lintall pass locally.Summary by CodeRabbit
Bug Fixes
Tests
Documentation