Optimized compilation of string patterns

[chengluyu]

This PR support compiling supported string patterns into automata for whole and prefix matching. String patterns are compiled into automata encoded in string literals and the runtime can decode and execute automata. Transformations are done in the second pass after matching.

[LPTK]

👎

[LPTK]

This case should probably be moved out of this file.

[LPTK]

This is a crazy scheme. It reparses a string on every match. Clearly, this should be cached into a top-level field of the current file.

[LPTK]

[Claude] Automated review of the automaton-based compilation of string patterns.

The design is sound — Frisch–Cardelli two-pass matching, right-linear SCC detection, deferred frames for the value operations that follow a tail call — and StringCompiler.scala is unusually well documented. The implementation is not there yet.

8 critical defects: 7 silent miscompilations and 1 compiler crash. All 8 are regressions: every program below produces the correct answer on hkust-taco/hkmc2 and the wrong one here. None is caught by the suite, which is fully green (814 tests, WASM included, no golden rewrites) — which is exactly the situation AGENTS.md warns about.

program	base	this PR
pattern Y = ("a" ~ X) | (("b"~"c") => "T"), pattern X = ("d" ~ Y) | "e"; "zae" is ("z" ~ Y) as r	"zae"	"()z"
pattern Funny = "" ~ "" ~ ""; 42 is Funny	TypeError	true
pattern Half = ("a" ..< "z") ~ "!"; "z!" is Half	false	true
pattern Listed = (((Digit as h) ~ (Listed as t)) => h+","+t) | ("" => "$"); "123" is Listed	"1,2,3,$"	"3,2,3,$"
@compile (("a" => print("ran")) ~ "b") in condition position	clean "unsupported" error	transform never runs
pattern P2 = Box(T ~ "c") | Box(T ~ "d") under @compile	clean "unsupported" error	AssertionError: already defined: w
Bracket(Str) as v, for pattern Bracket(pattern P, inner) = "[" ~ (P as inner) ~ "]"	"no"	"[ab]" (should be "ab")

Each is on its own inline comment below, with the trace and a suggested fix. Every repro was run on this branch and again on a worktree of the base ref, except the effect-handler one (C2), which is traced and reproduced at the engine level but not run end to end under :effectHandlers — that comment says so.

Method: nine reviewers each took one area of the change (NFA construction, NFA reduction, reverse determinization and encoding, the StrPat engine, SplitCompiler gating, the multi-matcher and specialize, Instantiator/Pattern, tests and goldens, performance and AGENTS.md conformance). Every finding was then handed to three independent verifiers instructed to refute it — does the code actually do this, is the path reachable from surface syntax, and is the defect introduced here or pre-existing. 61 raised, 50 survived. Only the 8 critical ones are inline here; there are 26 further major findings (unbounded reverse determinization, regionSupported admitting &/! that build then hard-fails on, unapplyStringPrefix swallowing those errors into a constant failure, Concat flattening re-associating ~, a compiler hang on polymorphic-recursive parametric patterns) available on request.

Two small things not worth their own comment:

• The diff strips the trailing indentation from 7 pre-existing blank lines (ups/Compiler.scala ×1, ups/Pattern.scala ×2, Runtime.mls ×4). AGENTS.md asks that indentation whitespace never be stripped and that a PR carry no gratuitous empty-line changes.
• Compiled.resultPrefixSize is dead code, while the result-array layout it documents is open-coded at three call sites.

A regression-test file pinning all 8 with :expect + :fixme is ready and passes as written; each block starts failing as soon as its bug is fixed. Happy to push it here if useful.

[LPTK]

[Claude] Critical — the purity of a recursive component depends on the order its alternatives are written in.

The optimistic true on the line above (for an instantiation already in visiting) is sound for the query that made the assumption, but getOrElseUpdate memoizes it region-wide. Since Or(ps) => ps.forall(...) short-circuits, a body whose first alternative reaches back into the SCC gets pureMemo := true written for the callee before the second alternative reveals the Extract. Two members of one SCC then disagree about purity.

The pure-subtree shortcut (l.355) then fires for a fragment containing a same-SCC tail reference: it emits Op.Mark on the way in and puts the matching Op.Slice on an exit state the goto can never reach. reduceStates prunes that state, so the fragment pushes zero values instead of the one the completion protocol promises, and the enclosing deferred Op.Add underflows.

pattern Y = ("a" ~ X) | (("b" ~ "c") => "T")
pattern X = ("d" ~ Y) | "e"

if "zae" is ("z" ~ Y) as r then r else "no"
//| = "()z"        <- base: "zae"

Writing Y's two alternatives in the other order makes it correct again, which is what pins the cause on the memo rather than on the grammar.

Purity is a property of the SCC condensation, not of an individual traversal. Suggest computing it once bottom-up after computeSccs(), so all members of an SCC agree by construction. If the recursive formulation is kept, thread a "consulted an assumption" flag out of loop and only memoize when it is false. The softAssert at l.480 does not catch this: it guards sccContext.pure && needValue, which is exactly the combination that does not arise on this path.

[LPTK]

[Claude] Addressed — fix written and verified locally (not pushed here yet).

isPureDeep now keeps results that consulted the in-cycle assumption out of the memo, so a later query recomputes them exactly. The assumption itself stays (it is what makes the traversal well-founded); only its caching was unsound. One consequence worth noting: the traversal can no longer short-circuit on the first impure element, because it has to visit every branch to learn whether any of them consulted the assumption — so forall became a fold over (pure, assumed).

The two spellings of the grammar now agree:

pattern Y = ("a" ~ X) | (("b" ~ "c") => "T")
pattern X = ("d" ~ Y) | "e"
if "zae" is ("z" ~ Y) as r then r else "no"      //| = "zae"

pattern Y2 = (("b" ~ "c") => "T") | ("a" ~ X2)
pattern X2 = ("d" ~ Y2) | "e"
if "zae" is ("z" ~ Y2) as r then r else "no"     //| = "zae"

This is the minimal sound fix and it does not disturb the surrounding design. The stronger version I suggested in the original comment — computing purity once over the SCC condensation after computeSccs() — is still worth doing: it would make all members of an SCC agree by construction rather than by the memo being careful, and it would restore the short-circuit. I left that to you as it changes the shape of the analysis.

Not addressed, and related: the softAssert at l.480 does not cover the invariant that actually broke here (the pure-subtree shortcut must not be entered for a fragment that can reach a same-SCC reference). Worth adding while this is fresh.

Leaving this thread open for you.

[LPTK]

[Claude] Critical — a transform that raises an algebraic effect corrupts the parse.

parseWhole/parsePrefix are not annotated @mayNotRaiseEffects, but the closure is applied from inside the engine's own loop, which the handler transformation never instruments. When a transform suspends, runtime.curEffect is set, apply returns Unit, and parseRun pushes that onto valStack and keeps walking — running every remaining committed-path transform with () in place of the suspended value.

Reproduced by driving the shipped engine directly with the CommaSep(Email) table from the EmailAddress.mls golden plus one effectful action: the effectful transform runs and sets curEffect, and the outer head :: tail transform then still runs, with () for the inner result.

The MLscript shape that reaches this is any compiled string region with an effectful => under :effectHandlers, e.g. if s is ((("a".."z") as c) => h.ask(c)) ~ "!" then …. ups/regex/CompiledSemantics.mls already exercises that shape with a pure print, so the path is live. To be precise about the evidence: I traced this and reproduced it at the engine level, but did not run the handler-level program end to end — so treat the MLscript-level manifestation as traced rather than measured.

The clean fix is to hoist transform application out of the engine: have parseRun return the committed call plan (action ids plus argument tuples, in commit order) and apply the closures in the generated user block, where handler lowering applies. As a stopgap that at least turns silent corruption into something diagnosable, check runtime.curEffect !== null right after actions.[actionId].apply(...) and call runtime.illegalEffect. Either way this wants a :effectHandlers regression test with an effectful => under ~, :fixme-marked if the real fix is deferred.

[LPTK]

[Claude] Critical — use sites of parametric string patterns lose their extraction parameters.

This predicate requires defn.patternParams.nonEmpty and a matching argument count, but never looks at defn.extractionParams. When it fires, the site compiles an in-place automaton that knows only the region's own output, so the extraction binding is dropped and the whole matched string comes back instead.

pattern Bracket(pattern P, inner) = "[" ~ (P as inner) ~ "]"

if "[ab]" is Bracket(Str) as v then v else "no"
//| = "[ab]"       <- should be "ab"; base: "no"

With two extraction parameters the site should yield a tuple and instead yields the input string. Alias in makeMatchSplit binds symbol -> output, so ... as v faithfully receives the wrong output — nothing downstream can notice.

The cheap correct gate is to also require defn.extractionParams.isEmpty here, and let those definitions keep the unapply route (worth a line of comment saying why). Projecting the extraction parameters out of compiled.visibleSlots would be better, but that needs Instantiator/StringCompiler to keep definition-internal Renames visible through Synonym, which they currently do not.

For completeness: matching the extraction argument directly — Bracket(Str, x) — returns "no" on both branches, so that half is pre-existing rather than a regression from this PR.

[LPTK]

[Claude] Critical — a string region matches a non-string scrutinee.

Both call sites emitted here go straight to StrPat.matchWhole/parseWhole with no Str head test, unlike Compiler.buildMultiMatcherBody, which guards its absorbed Str branch with FlatPattern.ClassLike(Str, …). matchWhole then reads .length off whatever it is handed.

pattern Funny = "" ~ "" ~ ""

42 is Funny            //| = true    <- base: TypeError
[] is Funny            //| = true    <- base: TypeError
42 is @compile Funny   //| = false   <- the multi-matcher gets this right

The encoded table for Funny is "1,0;;1,1;10", so starts[seedRev] == '1' and any scrutinee with a falsy or zero length is reported as a match. On the parsing entry point the same gap surfaces differently — trimStart(42) walks into Error: StrPat: no viable transition (this is a compiler bug), blaming the compiler for what is a user type error.

Wrapping both emitted sites in the same Branch(scrutinee(), FlatPattern.ClassLike(ctx.builtins.Str.safeRef, ctx.builtins.Str, N, false)(Tree.Dummy), …) ~: alternative that the multi-matcher already uses fixes it. Defensively, matchWhole and parseRun should also reject a non-string input rather than silently reading .length off it.

[LPTK]

[Claude] Addressed — fix written and verified locally (not pushed here yet).

Both emitted call sites now sit under the same Str branch that Compiler.buildMultiMatcherBody already uses for the absorbed head, so the two paths agree. The generated code is what you would want — a single guard, scrutinee referenced directly, no extra temporary:

define ab as fun ab(x) {
  let stringMatched;
  match x
    Str =>
      set stringMatched = runtime.StrPat.matchWhole("4,0;97,98,99;...;0001", x);
      match stringMatched
        true => return 1
        else => return 0
      end
    else
      return 0
  end
}

pattern Funny = "" ~ "" ~ ""
42 is Funny        //| = false     (was: true)
[] is Funny        //| = false     (was: true)
trimStart(42)      //| = 42        (was: "no viable transition (this is a compiler bug)")

Not addressed: the defensive half of the suggestion. matchWhole and parseRun still read .length off whatever they are handed, so they remain wrong if any future call site forgets the guard. A cheap typeof input !== "string" early return would make the engine safe on its own — your call whether the runtime should pay for that.

Leaving this thread open for you.

[LPTK]

[Claude] Critical — compiler crash when two labels of one multi-matcher reach the same transform.

Each label gets its own StringCompiler here, and Extract closures are interned per instance (actionSources.indexWhere(_ eq term)). A definition reached from two labels therefore emits its transform lambda twice, and both copies bind the same VarSymbols, because correspondence comes straight off the shared AST node.

class Box(val v)
pattern T = (("a" ~ "b") as w) => [w]
pattern P2 = Box(T ~ "c") | Box(T ~ "d")

fun f2(x) = if x is @compile P2 as y then y else "no"
//| /!!!\ Uncaught error: java.lang.AssertionError: assertion failed: already defined: w
//| 	at: hkmc2.codegen.SymbolRefresherWalker.assertUpdate(SymbolRefresher.scala:15)

The whole definition is lost and every later use fails with ReferenceError: f2 is not defined. Minimal pair: one label (Box(T ~ "c") alone) compiles and runs; the same P2 without @compile compiles and runs; two labels in one multi-matcher crashes.

This makes the TODO above the Extract case in StringCompiler optimistic — the hazard is not only "when a simplifier pass duplicates a subtree containing both", it fires on ordinary two-label input. Threading one interning table through all labels of a multi-matcher body would fix it; the TODO's own suggestion — hosting each definition's transforms as methods on the pattern object and referencing them by selection — fixes it properly.

[LPTK]

[Claude] Critical — ..< silently becomes ..= inside a ~ sequence.

rightInclusive is bound in the enclosing match and never used here, so the emitted CharClass is always inclusive. The comment is right that this mirrors the previous (lower.head to upper.head) expansion — but that expansion was only reachable from the @compile path, whereas routing Concatenation through Instantiator now makes it the default for ordinary code.

pattern Half = ("a" ..< "z") ~ "!"
"z!" is Half        //| = true     <- base: false

pattern Plain = "a" ..< "z"
"z" is Plain        //| = false    <- still correct, via makeRangeTest

So the same range means two different things depending on whether it sits inside a sequence. Nothing caught it because no test in the tree uses ..< on strings — only ..=.

Suggest CharClass(lo, if rightInclusive then hi else hi - 1), collapsing to Never when the class becomes empty, or an explicit inclusivity field on CharClass. Worth noting the IntLit branch a few lines below has the same bug (lower to upper regardless of the flag) — pre-existing rather than introduced here, but it is right there.

[LPTK]

[Claude] Addressed — fix written and verified locally (not pushed here yet).

The upper bound is now lowered by one for an exclusive range, collapsing to Never when that empties the class:

pattern Half = ("a" ..< "z") ~ "!"
"z!" is Half        //| = false    (was: true)
"y!" is Half        //| = true

pattern Plain = "a" ..< "z"
"z" is Plain        //| = false    (unchanged — this path always honoured the flag)

I also fixed the integer branch immediately below, which had the same bug (lower to upper regardless of rightInclusive). That one is pre-existing rather than introduced by this PR, but it is literally the adjacent case and the same two lines, so it seemed worse to leave it. Shout if you would rather it were split out.

I did not touch the lower.nonEmpty && upper.nonEmpty guard on this case. It is unreachable — Elaborator.isInvalidStringBounds already rejects any bound whose length is not exactly 1, which also rules out astral characters, since a surrogate pair has length 2 — so a bound reaching here is always a single code unit. Removing the guard would send those to the "Range patterns are not supported in pattern compilation" arm, whose message would be misleading. Left as is, but it is dead defensive code either way.

Leaving this thread open for you.

[LPTK]

[Claude] Critical — the outer activations of a recursive component read the innermost activation's bindings.

Binding slots are global to the region and rewritten at every recursion level, and this loop exports whatever the global store holds at the end. Op.Defer captures the slots a frame reads before writing them, which is the right rule for h in the shape below — but only when the frame is created; through unapply the outermost transform ends up reading the last-written value instead.

pattern Digit = "0" ..= "9"
pattern Listed = (((Digit as h) ~ (Listed as t)) => h + "," + t) | ("" => "$")

if "1"   is Listed as r then r    //| = "1,$"        <- correct
if "12"  is Listed as r then r    //| = "2,2,$"      <- base: "1,2,$"
if "123" is Listed as r then r    //| = "3,2,3,$"    <- base: "1,2,3,$"

fun viaCompile(s) = if s is @compile Listed as r then r else "NOPE"
viaCompile("123")                 //| = "1,2,3,$"    <- correct

One definition, three different answers depending on how it is referenced, with no diagnostic. Depth 1 is correct, which is why UpsBugsBacklog.mls's Input case does not expose it — the greedy wildcard eats the whole string, so there is only ever one activation.

Making unapply compile the whole body as one region when containsStringSeq(pd.pattern) && regionSupported(pd.pattern, Set.empty) — exactly the condition already used for unapplyStringPrefix — puts the Transform inside the automaton, where Op.Defer capture already does the right thing. Short of that, StringCompiler.compile should refuse to export a visible slot that can be written more than once (i.e. bound inside a recursive SCC), and softAssert it.

[LPTK]

[Claude] Critical — @compile silently drops a transform's effects.

This branch calls matchWhole regardless of compiled.actions, while makeStringRegionSplit documents the opposite rule and follows it: "Transforms … run exactly once on the committed parse, even when the match is only used as a condition". ups/regex/CompiledSemantics.mls pins that rule — for the inline path only.

fun probe(x)  = if x is ("a" => print("ran")) ~ "b" then "yes" else "no"
probe("ab")   //| > ran
              //| = "yes"

fun probe2(x) = if x is @compile (("a" => print("ran2")) ~ "b") then "yes" else "no"
probe2("ab")  //| = "yes"     <- "ran2" is never printed

Whether the transform runs depends on whether the caller happens to bind the output (MatchOnly vs Full), so adding @compile — documented as an optimisation switch — changes observable behaviour.

Suggest factoring the decision into one predicate on Compiled that both call sites consult, e.g. def needsParsing(outputNeeded: Bool, bindingsNeeded: Bool): Bool = !pure && (outputNeeded || bindingsNeeded || actions.nonEmpty || visibleSlots.nonEmpty). At minimum, if isMatchOnly && compiled.actions.isEmpty then here, plus a softAssert that the matchWhole path is only taken when actions.isEmpty.

In fairness: dropping Extract in ResultMode.MatchOnly is pre-existing base behaviour for every pattern shape, so the underlying policy question is broader than this PR. What is new is that two paths now disagree for the same pattern, silently.