-
Notifications
You must be signed in to change notification settings - Fork 42
Optimized compilation of string patterns #540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: hkmc2
Are you sure you want to change the base?
Changes from 4 commits
105fa99
592c813
e2fa0f7
a4f702f
eb7a728
4ab837a
cc60472
8aefa77
f6026af
10dfb09
8caf76d
2967a33
df8ba26
838ec0d
71e1715
5cbc7fd
42a1829
330a095
c83320a
3c80103
05920e9
c58e2b8
6095982
98995d9
994f9a8
476705f
df3eed0
8f66ead
90e213e
0173355
95343ed
07b5987
21cc53c
0db6ba6
5f092ec
5532a6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -150,7 +150,20 @@ class Compiler(using Context)(using tl: TL)(using Ctx, State, Raise) extends Ter | |
| .mkString("{", ", ", "}")}" | ||
| ): | ||
| val expandedPatterns = patterns.map(p => (p.label, p.expand(Set.empty))) | ||
| val heads = expandedPatterns.flatMap((_, p) => p.heads).toList | ||
| // String-shaped patterns (sequences and character classes) cannot take | ||
| // part in head-based specialization: how a string scrutinee is split is a | ||
| // decision global to the whole sequence. Whenever one is present, all | ||
| // string-shaped patterns — including plain string literals, whose heads | ||
| // would overlap the `Str` class — are absorbed into a single `Str` head | ||
| // whose branch runs one compiled automaton per label, like a lexer | ||
| // jointly matching several token rules (see `StringCompiler`). | ||
| val absorbStrings = expandedPatterns.exists((_, p) => StringCompiler.containsStringNode(p)) | ||
| val strSymbol = ctx.builtins.Str | ||
| val heads = expandedPatterns.flatMap((_, p) => p.heads).toList.filter: head => | ||
| !absorbStrings || (head match | ||
| case _: StrLit => false | ||
| case symbol: ClassLikeSymbol if symbol is strSymbol => false | ||
| case _ => true) | ||
| // This is the parameter of the current multi-matcher. | ||
| val scrutinee = VarSymbol(Ident("input")) | ||
| // Assemble branches for constructors and literals. | ||
|
|
@@ -170,12 +183,16 @@ class Compiler(using Context)(using tl: TL)(using Ctx, State, Raise) extends Ter | |
| case _: (syntax.Literal | ModuleOrObjectSymbol) => empty | ||
| val consequent = Split.Else(multiMatcherBranch(specialized, scrutinee, classFields)) | ||
| Branch(scrutinee.safeRef, head.toFlatPattern(classFieldArguments), consequent) | ||
| val stringBranch = if !absorbStrings then N else | ||
| val pattern = FlatPattern.ClassLike(strSymbol.safeRef, strSymbol, N, false)(Tree.Dummy) | ||
| val consequent = Split.Else(multiMatcherStringBranch(expandedPatterns, scrutinee)) | ||
| S(Branch(scrutinee.safeRef, pattern, consequent)) | ||
| // Assemble the default branch. | ||
| val default = | ||
| val specialized = expandedPatterns.specializeSet(N) | ||
| Split.Else(multiMatcherBranch(specialized, scrutinee, Map.empty)) | ||
| // Make a split that tries all branches in order. | ||
| val topmostSplit = branches.foldRight(default)(_ ~: _) | ||
| val topmostSplit = (branches ::: stringBranch.toList).foldRight(default)(_ ~: _) | ||
| val bodyTerm = SynthIf(topmostSplit) | ||
| log(s"Multi-matcher body:\n${topmostSplit.prettyPrint}") | ||
| (paramList(param(scrutinee)), bodyTerm) | ||
|
|
@@ -243,7 +260,76 @@ class Compiler(using Context)(using tl: TL)(using Ctx, State, Raise) extends Ter | |
| // Lastly, we return the matcher result, directly for singleton matchers | ||
| // and as a record otherwise. | ||
| Blk(bindings ::: tests.reverse, resultTerm) | ||
|
|
||
|
|
||
| /** The branch body for the absorbed `Str` head: each label's string-shaped | ||
| * fragment is compiled to its own whole-match automaton (see the note in | ||
| * `buildMultiMatcherBody`). The per-label result terms follow the same | ||
| * protocol as `multiMatcherBranch`: a Boolean in match-only mode, and a | ||
| * `MatchSuccess`/`MatchFailure` value in full mode. | ||
| */ | ||
| def multiMatcherStringBranch( | ||
| patterns: Set[(Label, ExPat)], | ||
| scrutinee: VarSymbol, | ||
| )(using ResultMode): Blk = | ||
| val z = (Nil: Ls[Statement], Nil: Ls[(Label, Term)]) | ||
| val (tests, resultTerms) = patterns.iterator.foldLeft(z): | ||
| case ((stmts, results), (label, pattern)) => | ||
| val fragment = StringCompiler.stringFragment(pattern).simplify | ||
| val resultTerm = fragment match | ||
| case And(Nil) => | ||
| // This label has no string-shaped alternative: it cannot match. | ||
| emptyMatchResult("not a string pattern") | ||
| // Note that each label needs its own compiler: a compiler instance | ||
| // accumulates the automaton states (and failure flag) of a single | ||
| // region. | ||
| case fragment => StringCompiler().compile(fragment, StringCompiler.Mode.Whole) match | ||
| case N => emptyMatchResult("rejected string pattern") | ||
| case S(compiled) => | ||
| val matchTableTerm = str(compiled.matchTable) | ||
| if isMatchOnly then | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Claude] Critical — This branch calls Whether the transform runs depends on whether the caller happens to bind the output ( Suggest factoring the decision into one predicate on In fairness: dropping |
||
| app(strPatMatchWhole, tup(fld(matchTableTerm), fld(scrutinee.safeRef)), "string match") | ||
| else if compiled.pure then | ||
| // An operation-free whole match preserves the scrutinee. | ||
| val matchedSymbol = TempSymbol(N, "stringMatched") | ||
| val call = app(strPatMatchWhole, tup(fld(matchTableTerm), fld(scrutinee.safeRef)), "string match") | ||
| SynthIf(Split.Let(matchedSymbol, call, | ||
| Branch(matchedSymbol.safeRef, | ||
| Split.Else(makeMatchSuccess(scrutinee.safeRef)) | ||
| ) ~: Split.Else(emptyMatchResult("string mismatch")))) | ||
| else | ||
| // Note: expanded patterns may aggregate sub-patterns from | ||
| // several source blocks, so their auto-computed location is | ||
| // not usable here; the helper pins the first action's own | ||
| // location instead (the surrounding terms are location-free). | ||
| val call = app(strPatParseWhole, | ||
| tup(fld(str(compiled.table)), fld(actionsTuple(compiled.actions, N)), fld(scrutinee.safeRef)), | ||
| "string parse") | ||
| val resultSymbol = TempSymbol(N, "parseResult") | ||
| val outputSymbol = TempSymbol(N, "stringOutput") | ||
| val slotSymbols = compiled.visibleSlots.map: (symbol, slot) => | ||
| (symbol, slot, TempSymbol(N, s"${symbol.name}$$")) | ||
| val bindingsTerm = makeBindings(slotSymbols.map: | ||
| (symbol, _, local) => RcdField(str(symbol.name), local.safeRef)) | ||
| val success = slotSymbols.foldRight( | ||
| Split.Else(makeMatchSuccess(outputSymbol.safeRef, bindingsTerm)): Split | ||
| ): | ||
| case ((_, slot, local), inner) => | ||
| Split.Let(local, callTupleGet(resultSymbol.safeRef, 1 + slot, "string binding"), inner) | ||
| SynthIf(Split.Let(resultSymbol, call, | ||
| Branch( | ||
| resultSymbol.safeRef, | ||
| // The engine returns null on failure and an array on success. | ||
| FlatPattern.Tuple(1, true), | ||
| Split.Let(outputSymbol, callTupleGet(resultSymbol.safeRef, 0, "string output"), success) | ||
| ) ~: Split.Else(emptyMatchResult("string mismatch")))) | ||
| val symbol = TempSymbol(N, label.asFieldName + "$") | ||
| (DefineVar(symbol, resultTerm) :: LetDecl(symbol, Nil) :: stmts, (label, symbol.safeRef) :: results) | ||
| val resultTerm = resultTerms.reverse match | ||
| case (_, term) :: Nil => term | ||
| case terms => Rcd(false, terms.map: (label, term) => | ||
| RcdField(str(label.asFieldName), term)) | ||
| Blk(tests.reverse, resultTerm) | ||
|
|
||
| import Pattern.* | ||
|
|
||
| /** Represent things that can be used as expressions in consequents. */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,20 +124,28 @@ class Instantiator(using tl: TL)(using Ctx, State, Raise): | |
| case SP.Wildcard() => Wildcard | ||
| case SP.Literal(literal) => Literal(literal) | ||
| case SP.Range(lower, upper, rightInclusive) => | ||
| // Currently, we expand the range pattern into a list of literals. After | ||
| // the `where` clause or chain patterns are implemented, we could directly | ||
| // expand the range pattern into a range test. | ||
| (lower, upper) match | ||
| case (StrLit(lower), StrLit(upper)) => | ||
| Or((lower.head to upper.head).map(c => Literal(StrLit(c.toString))).toList) | ||
| case (StrLit(lower), StrLit(upper)) if lower.nonEmpty && upper.nonEmpty => | ||
| // String ranges compare the first UTF-16 code unit, mirroring the | ||
| // previous expansion `(lower.head to upper.head)`. Keeping the range | ||
| // symbolic lets the string pattern compiler emit compact | ||
| // character-class transitions instead of wide disjunctions. | ||
| CharClass(lower.head.toInt, upper.head.toInt).withLocOf(pattern) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Claude] Critical —
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 Suggest
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 I also fixed the integer branch immediately below, which had the same bug ( I did not touch the Leaving this thread open for you. |
||
| case (IntLit(lower), IntLit(upper)) => | ||
| // Integer ranges are still expanded into a list of literals. After | ||
| // the `where` clause or chain patterns are implemented, we could | ||
| // directly expand the range pattern into a range test. | ||
| Or((lower to upper).map(i => Literal(IntLit(i))).toList) | ||
| case _ => | ||
| error(msg"Range patterns are not supported in pattern compilation." -> pattern.toLoc) | ||
| Never | ||
| case SP.Concatenation(left, right) => | ||
| error(msg"String concatenation is not supported in pattern compilation." -> pattern.toLoc) | ||
| Never | ||
| // Flatten nested concatenations into one sequence so that the string | ||
| // pattern compiler sees the whole `~`-spine at once. | ||
| def parts(pattern: Pat): Ls[Pat] = pattern match | ||
| case Concat(patterns) => patterns | ||
| case pattern => pattern :: Nil | ||
| Concat(parts(instantiate(left)) ::: parts(instantiate(right))).withLocOf(pattern) | ||
| case SP.Tuple(leading, spread) => | ||
| val instantiatedSpread = spread.map: | ||
| case (spreadKind, middle, trailing) => | ||
|
|
@@ -171,4 +179,9 @@ class Instantiator(using tl: TL)(using Ctx, State, Raise): | |
| acc | ||
| case N => true | ||
| instantiate(pattern) | ||
| case _: SP.Guarded => TODO("instantiate for Guarded") | ||
| case _: SP.Guarded => | ||
| // Guards may fail after consumption based on information the automaton | ||
| // cannot track, which would reintroduce backtracking; they are excluded | ||
| // from compiled patterns for now. | ||
| error(msg"Guarded patterns are not supported in pattern compilation." -> pattern.toLoc) | ||
| Never | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Claude] Critical — compiler crash when two labels of one multi-matcher reach the same transform.
Each label gets its own
StringCompilerhere, andExtractclosures 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 sameVarSymbols, becausecorrespondencecomes straight off the shared AST node.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 sameP2without@compilecompiles and runs; two labels in one multi-matcher crashes.This makes the
TODOabove theExtractcase inStringCompileroptimistic — 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; theTODO's own suggestion — hosting each definition's transforms as methods on the pattern object and referencing them by selection — fixes it properly.