fix(regex): never memoize a never-match program beside a missing fancy fallback - #9799
fix(regex): never memoize a never-match program beside a missing fancy fallback#9799proggeramlug wants to merge 1 commit into
Conversation
…y fallback Found by the regex lane (PerryTS#9796 item 2) against the construction cache added earlier on this branch. `REGEX_CACHE` holds a never-match placeholder for a lookbehind / backreference pattern whose real program lives in `FANCY_CACHE`, and the two maps have independent clear-on-overflow caps. `FANCY_CACHE` can therefore drop a pattern `REGEX_CACHE` still answers for, and `build_and_install_programs` then reads the pair (never-match, no fancy) — which matches nothing. Left to the maps alone that state heals: the next `REGEX_CACHE` clear makes the pattern recompile and repopulate both. A site-cache entry never heals — a construction that hits it is born built and never consults the maps again — so memoizing the incoherent pair makes a lookbehind literal PERMANENTLY non-matching, silently, for the life of the thread. That permanence is this branch's regression, and this is its fix: the triple is only remembered against the text when it is coherent. The placeholder becomes a process-wide singleton so the question is an `Arc::ptr_eq` rather than a pattern-text compare. This is the narrow guard, not the whole repair: PerryTS#9796 item 2 also REBUILDS the missing fallback so the header itself stops mis-matching, which fixes the underlying transient defect (present on main) that this only declines to make permanent. Keep both; this one becomes a defensive invariant once that lands. The same hole exists for `REPEAT_MATCHER_CACHE` — a cleared repeat matcher is memoized as `repeat: None` beside a real std program, changing capture semantics — and is reported to that lane rather than guessed at here. Test: `an_incoherent_program_pair_is_never_memoized_by_the_site_cache` reads `Some(true)` and then `-1` without the guard.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe regex runtime now shares its never-match program and detects it by identity. Site-cache installation skips incoherent standard/fancy program pairs. Tests cover cache eviction, non-memoization, and recovery. ChangesRegex cache coherence
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change prevents lookbehind and backreference patterns from being permanently cached as non-matching after cache desynchronization, with regression coverage for recovery. It is ready to merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Same bug, and the diagnosis matches mine exactly — but this fix leaves half of it standing, and I think the half it leaves is the visible one. Declining to memoize stops the incoherent triple from being inherited by later constructions. It does not stop the header that is being built right now from getting Two more things it does not cover:
#9801 fixes it the other way round: repair the missing program in Its test is the discriminator if you want to check this locally: Happy either way as long as the header is repaired — if you would rather keep this PR as the landing vehicle, take the repair block from #9801's |
|
Closing in favour of #9801, which is strictly stronger and which I would rather have. Same defect, same attribution — I reached it from #9764's side after the regex lane reported it, and my analysis agreed with theirs on the point that matters: the incoherence pre-dates What #9801 does that this does not:
Nothing here is worth carrying separately once #9801 lands. Its test covers the same lookbehind-after-a- |
The defect, on main today
REGEX_CACHEholds a never-match placeholder ([^\s\S]) for a pattern onlyfancy-regexcan match — a lookbehind, a backreference — with the real programin
FANCY_CACHE. The two maps are capped independently and eachclear()swholesale on overflow (
evict_regex_cache_if_full), soFANCY_CACHEcan drop apattern that
REGEX_CACHEstill answers for.When that has happened, nothing repopulates the fallback:
get_or_compile_regexreturns early on aREGEX_CACHEhit;compile_and_cache_regex_checkedreturnstrueimmediately when the key isalready in
REGEX_CACHE.So
build_and_install_programsreads the pair(never-match, fancy: None)—which matches nothing.
On its own that state heals. The next
REGEX_CACHEclear makes the patternrecompile, and the recompile repopulates both maps. A site-cache entry does
not heal.
site_cache::install_programsremembers the triple against thepattern text, and
js_regexp_newthen installs it eagerly on every laterconstruction of that text —
regex_ptrnon-null, soensure_regex_compiledshort-circuits,
lookup_fancy_regexreturnsNoneoff the header, and the mapsare never consulted again. A lookbehind or backreference literal is then
permanently non-matching, silently, for the life of the thread.
The construction cache landed in the last merge train; this closes the window it
opened.
The desynchronisation is reachable, not theoretical
REGEX_CACHEalso takes entries from ordinary patterns, so it can hit its capand clear on an ordinary insert while
FANCY_CACHEkeeps everything. Fromthere
FANCY_CACHEreaches its own cap first, clears, and every fancy patterncompiled since that earlier clear is left with a placeholder in
REGEX_CACHEand no fallback anywhere.
The change
(
never_match_program), so "is this entry a placeholder?" is anArc::ptr_eqand not a pattern-text compare. Both places that used to buildone now share it.
build_and_install_programsmemoizes the triple only when it is coherent:never a placeholder with no fancy program beside it. Declining costs one
recompile on a later construction and restores the self-healing behaviour.
Correctness
New test
an_incoherent_program_pair_is_never_memoized_by_the_site_cache. Itbuilds a lookbehind, clears
FANCY_CACHEthe way the overflow guard does,rebuilds, and asserts (a) the incoherent pair is not remembered against the text
and (b) the literal matches again once the maps heal.
Verified it can fail. With the guard's condition replaced by
if trueitfails on its own named assertion —
left: Some(true),right: Some(false)—and, without the first assertion, the recovered construction returns
-1instead of matching.
cargo test -p perry-runtime --release regex -- --test-threads=1: 99 passed,0 failed.
Scope, and what this is NOT
This is the narrow guard: it stops the site cache making the breakage
permanent. It does not repair the underlying transient defect — a header built
during the incoherent window still carries a never-match program and no
fallback. #9796 item 2 fixes that properly, by rebuilding the missing fallback
beside the placeholder, and should land regardless of this PR. With it in place
this guard's condition can never be true and it degrades to a defensive
invariant, which is a fine thing for it to be.
One hole of the same shape is left open deliberately, and reported rather than
guessed at:
REPEAT_MATCHER_CACHEhas the same independent cap. A clearedrepeat matcher is memoized as
repeat: Nonebeside a real std program, whichthe placeholder test above cannot see, and
lookup_repeat_matcherthen answersNonefor a pattern whose ECMA-262 RepeatMatcher capture semantics areobservable — wrong captures rather than no match. That belongs with #9796, which
already owns that function.
Reported by the regex lane (#9796 item 2); confirmed against this code before
being fixed.
Summary by CodeRabbit
Bug Fixes
Tests