Skip to content

fix(builtins): size regex_match from the pattern; null for non-participating groups (fixes #629)#652

Merged
InauguralPhysicist merged 1 commit into
InauguralSystems:mainfrom
Nitjsefnie-OSC:fix-629-regex-captures
Jul 17, 2026
Merged

fix(builtins): size regex_match from the pattern; null for non-participating groups (fixes #629)#652
InauguralPhysicist merged 1 commit into
InauguralSystems:mainfrom
Nitjsefnie-OSC:fix-629-regex-captures

Conversation

@Nitjsefnie

Copy link
Copy Markdown
Contributor

Fixes #629, both bugs, with your prescribed shape: the regmatch_t array is sized re.re_nsub + 1 (overflow-guarded through the repo's own xmalloc_array/safe_size_mul idiom, freed on both the no-match and success paths), regexec gets that count, and the participation test moved from the loop condition into the body — a non-participating group now emits null instead of terminating emission.

Repro → fixed

On main: regex_match of ["ab", "(x)?(a)(b)"]["ab"] (matched, zero groups) and a 17-group pattern returns 16 elements. On this branch: ["ab", null, "a", "b"] (Python parity: (None, 'a', 'b')) and the 17-group pattern returns all 18.

Why null for absent groups: it's the language's own precedent — VAL_NULL is first-class, builtins.c:5514/5540 already append make_null() into result lists (a borrowed singleton, so list_append_owned is leak-safe by the existing pattern), assert_eq against null is an established test idiom, and it stays distinguishable from "" (a group that matched empty). Pinned in docs/BUILTINS.md along with the no-group-cap guarantee.

Tests + verification

New #629 marker block in tests/test_regex.eigs (15→34 checks): the issue's repro + control, non-participating group in the middle and at the end, and a 17-group pattern with groups 16/17 present. Mutation-checked twice (implementer, then independently re-run with a rebuild between every swap): reverting only src/builtins.c fails the new asserts with the truncated results; restored, all pass. Full suite 2890/2890; make asan (with detect_leaks=1) clean over the repros and both regex test files — the dynamic-allocation path specifically; no new -Wall -Wextra warnings; --lint on the touched .eigs clean (the C file isn't lintable by the .eigs linter, as established).

One runner note, deliberately not touched (scope): run_all_tests.sh:1260 hard-codes "Regex (15 checks)" as that slot's declared tally — the 19 new asserts run and gate (the block is all-or-nothing on its pass marker) but the reported total stays 2890. Happy to bump the declared count in a follow-up or here if you prefer.


🤖 This PR was written with AI assistance (Claude), directed and verified by a human-supervised workflow.

… non-participating groups (InauguralSystems#629)

regex_match used a fixed regmatch_t[16] and a loop whose CONDITION tested
`matches[i].rm_so >= 0`. Two bugs followed:

  1. POSIX sets rm_so = -1 for a capture group that did not participate
     (e.g. an unmatched optional `(x)?`). With that test in the loop
     condition, the first non-participating group TERMINATED emission,
     deleting every later capture: `regex_match of ["ab", "(x)?(a)(b)"]`
     returned `["ab"]` instead of the full match plus three groups.

  2. Groups past 15 silently vanished — a 17-group pattern returned 16
     elements. Both break the docs/BUILTINS.md invariant that group n sits
     at index n on a successful match.

Fix: size the array from the compiled pattern (`re.re_nsub + 1`) via the
overflow-guarded xmalloc_array (re_nsub is attacker-controlled through the
pattern but POSIX-bounded), pass that count to regexec, free it on every
path, and move the participation test into the loop BODY — a group with
rm_so < 0 now emits `null` (distinct from `""`, mirroring Python's
`(None, 'a', 'b')`) instead of stopping the loop. Existing error / no-match
behavior is unchanged.

Tests: the issue's two repro cases, non-participating group in the middle
and at the end, and a 17-group pattern asserting all groups are present.
docs/BUILTINS.md pins the null encoding and the no-group-cap guarantee.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@InauguralPhysicist

Copy link
Copy Markdown
Collaborator

Merging. Verified independently, including the two things this rewrite actually risks.

Both bugs fixed, Python parity confirmed: regex_match of ["ab", "(x)?(a)(b)"]["ab", null, "a", "b"]; the 17-group pattern returns all 18; no-match still []; a zero-group pattern still ["abc"].

Your null-over-"" argument isn't just precedent — it's load-bearing, and it discriminates:

(a*)(b) on "b"  ->  ["b", "",   "b"]     group 1 matched EMPTY
(x)?(b) on "b"  ->  ["b", null, "b"]     group 1 ABSENT

Those two are genuinely different facts, and emitting "" for both would make them indistinguishable — permanently, with no way for a caller to recover the difference. Worth the paragraph you gave it.

The refcount protocol is the thing I checked hardest, because it's CLAUDE.md's first hard-won rule and list_append_owned(result, make_null()) looks like it decrefs a shared singleton to death. It doesn't, and your "borrowed singleton, leak-safe by the existing pattern" is exactly right — g_null_singleton is { .refcount = 1000000, .arena = 1 }, so val_decref's !v->arena short-circuits to a no-op and free_value guards on arena besides. Three belts. Citing 5514/5540 as the precedent was the right way to establish it.

The other risk is that you replaced a fixed stack array with a heap allocation. Frees on both the no-match and success paths, xmalloc_array guarding the +1 and the * sizeof. I ran ASan+UBSan with detect_leaks=1 over the repros (including the no-match early-free path) and both regex test files: clean. Suite 2890/2890.

Your runner-tally note is right, and it's now a pattern worth fixing properlyrun_all_tests.sh:1261 declares Regex (15 checks) while test_regex.eigs carries 34 asserts. That's the second time you've spotted this (the other was test_string_math's declared 75 vs 110). Leaving it out of this PR was correct — it's a systematic drift across the runner, not a regex problem, and bundling it would bury a good fix in bookkeeping. I'm filing it as its own issue, credited to you, rather than have you keep noticing it PR after PR.

@InauguralPhysicist
InauguralPhysicist merged commit 347ea3f into InauguralSystems:main Jul 17, 2026
16 checks passed
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.

regex_match drops every capture after a non-participating group, and hard-caps at 15 groups

2 participants