Skip to content

fix panic in analyze-string#13

Merged
frankarensmeier merged 1 commit into
mainfrom
fix/analyze-string-optional-groups
Apr 28, 2026
Merged

fix panic in analyze-string#13
frankarensmeier merged 1 commit into
mainfrom
fix/analyze-string-optional-groups

Conversation

@frankarensmeier

Copy link
Copy Markdown
Contributor

Fix panic in analyze-string with non-participating optional groups

When optional groups (e.g. ([0-9]{1,2})?) don't participate in a match, they produce zero-length group entries. For these entries, when no parent group end-event exists at the same position, the start and end events were inserted in the wrong order (end before start). This caused on_group_end() to pop from an empty stack, panicking with unwrap() on None.

Fix: swap the event insertion order in the or_insert_with branch from (-i, i) to (i, -i), matching the and_modify branch which already inserts start before end.

Add regression test with regex ([0-9]{1,2})?\s?(...)?...\s?([0-9]{4}) matching "1949" where groups 1 and 2 are optional and don't participate.

fix: fix panic in analyze-string with non-participating optional groups

When optional groups (e.g. `([0-9]{1,2})?`) don't participate in a
match, they produce zero-length group entries. For these entries, when
no parent group end-event exists at the same position, the start and
end events were inserted in the wrong order (end before start). This
caused `on_group_end()` to pop from an empty stack, panicking with
`unwrap()` on `None`.

Fix: swap the event insertion order in the `or_insert_with` branch
from `(-i, i)` to `(i, -i)`, matching the `and_modify` branch which
already inserts start before end.

Add regression test with regex `([0-9]{1,2})?\s?(...)?...\s?([0-9]{4})`
matching "1949" where groups 1 and 2 are optional and don't participate.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes a panic in Regex::analyze when optional capturing groups don’t participate and end up represented as zero-length captures, by ensuring zero-length group “start/end” events are always inserted in a safe order.

Changes:

  • Adjust event insertion order for zero-length capture groups to insert “start” before “end”, preventing on_group_end() from popping an empty stack.
  • Add a regression test covering optional groups that don’t participate for a realistic date-like pattern.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
regexml/src/analyze_string.rs Fixes action ordering for zero-length group events in the or_insert_with path to avoid panics.
regexml/tests/test_qt_analyze_string.rs Adds a regression test exercising optional non-participating groups to ensure analyze() no longer panics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@frankarensmeier frankarensmeier merged commit ce4142b into main Apr 28, 2026
5 checks passed
frankarensmeier added a commit to frankarensmeier/xee that referenced this pull request Apr 28, 2026
Remove 10 now-passing tests from vendor/xslt-tests/filters:
- analyze-string: 033, 034, 042, 077, 083, 095, 100
- position: 2201
- result-document: 1501, 1502

These pass thanks to:
- regex-group() scoping fix for stylesheet functions (8b0d9ef)
- regexml 0.2.2 fix for optional group panics (Paligo/regexml#13)
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