Fix: ChoiceRule.Synthesize stack-overflows on a self-referential choice with a parantheses alternative - #148
Conversation
…ferential choices
A self-referential ChoiceRule (e.g. a boolean/arithmetic expression grammar
where one alternative is itself, directly or via a transparent `parantheses`
wrapper) could recurse indefinitely during synthesis and crash with a stack
overflow.
Root cause: SynthesisPlan's memoized "block" for a (rule, element) pair is
only consulted when a ChoiceRule evaluates its own alternatives via
SynthesisPlan.CanSynthesize. Every other combinator (SequenceRule and, by
inheritance, ParanthesesRule; also OneOrMoreRule/ZeroOrOneRule/QuoteRule)
calls a nested rule's CanSynthesize/Synthesize directly, bypassing that
memoization. So when a transparent parantheses alternative (a SequenceRule
with no metaclass of its own) reaches back into the same ChoiceRule for its
inner content, ChoiceRule.CanSynthesize is re-entered as a raw call with no
memory that it's already in progress for this exact element it just
re-evaluates every alternative from scratch, including the same parantheses
alternative, forever. ChoiceRule.Synthesize made this unconditional: unlike
CanSynthesize, it allocated a brand-new SynthesisPlan on every call instead
of accepting one, discarding what little protection existed between
recursive Synthesize steps.
Fix: give ChoiceRule its own thread-local reentrancy guard (a per-instance,
per-thread HashSet of in-progress semantic elements, using reference
equality) checked at the top of both CanSynthesize and Synthesize. This is
confined to ChoiceRule and doesn't require changing the Rule.Synthesize
signature.
Verified with a minimal repro (Container.Value=Expr; Expr: ParenExpr | Atom;
parantheses ParenExpr: '(' Expr ')';) that previously stack-overflowed on
Parser.Update(element) after mutating a nested leaf it now resynthesizes
correctly.
⚡ Risk Assessment —
|
| Files | Summary |
|---|---|
ChoiceRule recursion guardAnyText/AnyText.Core/Rules/ChoiceRule.cs |
Adds ThreadLocal<HashSet> to track in-progress synthesis, preventing infinite recursion in self-referential choice rules during CanSynthesize and Synthesize operations |
Changelog updateAnyText/AnyText.history |
Adds history entry documenting the stack overflow fix for self-referential ChoiceRule with parentheses alternative |
Sequence Diagram
sequenceDiagram
participant Caller
participant ChoiceRule
participant InProgressSet as _synthesisInProgress
participant Alternatives
Caller->>ChoiceRule: CanSynthesize(element)
alt element in InProgressSet
ChoiceRule->>Caller: return false
else element not in set
ChoiceRule->>InProgressSet: Add(element)
loop For each alternative
ChoiceRule->>Alternatives: CanSynthesize(element)
end
ChoiceRule->>InProgressSet: Remove(element)
ChoiceRule->>Caller: return result
end
Dig Deeper With Commands
/review <file-path> <function-optional>/chat <file-path> "<question>"/roast <file-path>
Runs only when explicitly triggered.
|
My first idea was that the grammar is wrong, because AnyText uses the same syntax like EBNF but the grammar is in fact a PEG instead of a CFG, meaning that the order of a choice is important. However, the parser could clearly differentiate between an opening parenthesis and an atom, rendering the grammar correct and so yes, this is a bug. At very least, AnyText should warn people about this. The downside of the solution with the thread-local variable is memory usage, but given that you clean the hashset after use, it is only transient and hence acceptable. So thank you very much :) |
ChoiceRule.Synthesizestack-overflows on a self-referential choice with aparanthesesalternativeSummary
When a grammar rule is a self-referential
ChoiceRule(i.e. one of its alternatives is,directly or indirectly, itself) and one of the alternatives is an untyped
paranthesesrulewrapping that same choice, calling
Parser.Update(element)(or any other path that invokesRule.Synthesizefor that choice without an existing, matching parse tree) recurses foreverand crashes the process with a stack overflow. It reproduces with a 6-line grammar and
2-level type hierarchy — nothing exotic, no arithmetic precedence, no ambiguity beyond
"this thing could always be wrapped in redundant parens."
This matters for any grammar modeling a recursive boolean/arithmetic expression language with
optional parenthesization (a very common shape — see
UVL.anytext'sConstraintrule in thissame test suite, which has the identical shape). It's specifically triggered by the model → text
(
Synthesize) direction; parsing (Match) is unaffected.Minimal reproduction
Recursion.anytext:Program.cs:Expected
Actual
Unhandled stack overflow (process terminates, cannot be caught) with a stack trace that
alternates indefinitely between
ChoiceRule.SynthesizeandSequenceRule.Synthesize/SynthesizeCore:Note: the recursive choice must be reached through an assigned/containment feature
(
AssignRule→QuoteRule→ChoiceRule, as inContainer.Value) — I could not reproduce itby calling
Updatedirectly on a root-rule element of the same shape, which apparently takes adifferent code path (
SynthesizeParseObjectreusing the existing parse tree structurally ratherthan freely re-deriving alternatives from scratch).
Root cause
Two things compound:
1. The paren-wrapping alternative always "can synthesize" any element the choice can
represent at all.
ParenExpris aparanthesesrule (ParanthesesRule : SequenceRule,AnyText.Core/Model/ParanthesesRule.cs) with noModelElementRule<T>type gate and nometaclass of its own — it's fully transparent (
ParanthesesRuleApplication.GetValuejustreturns
Inner[1].GetValue(...)). SoParanthesesRule.CanSynthesize→
SequenceRule.CanSynthesize(
SequenceRule.cs:337-340)→ delegates straight to the enclosing choice's
CanSynthesizefor the same semantic element.Because
ParenExpris declared beforeAtominExpr's alternative list,ChoiceRule.Synthesizepicks it first (
ChoiceRule.cs:190-199,Array.Findreturns the first match).2. Nothing stops the choice from picking that same alternative again on the way back in.
SynthesisPlanis supposed to guard against this:ChoiceRule.CanSynthesizecallssynthesisPlan.BlockRecursion(this, semanticElement)(
ChoiceRule.cs:182-187), whichwrites
falseintoSynthesisPlan._decisions[(rule, semanticObject)](
SynthesisPlan.cs). But that dictionary is onlyever read from
SynthesisPlan.CanSynthesize(Rule, object, ParseContext)(
SynthesisPlan.cs:20-28) — and thatmethod is only called from
ChoiceRule's own alternative loop(
Array.Exists(Alternatives, r => synthesisPlan.CanSynthesize(r.Rule, ...))). Every othercombinator —
SequenceRule.CanSynthesize(
SequenceRule.cs:337-340),OneOrMoreRule,ZeroOrOneRule,QuoteRule— calls the inner rule'sCanSynthesize/Synthesizedirectly, bypassing
SynthesisPlanentirely. So whenParenExpr's sequence reaches back intoExpr(the sameChoiceRule) for its middle element,ChoiceRule.CanSynthesizeis re-entered asa raw call: it writes
BlockRecursionagain (a no-op re-write of an entry nobody reads back) andevaluates all alternatives from scratch — there's no check at entry for "is
(this, semanticElement)already in progress?" It picksParenExpragain. Forever.ChoiceRule.Synthesize(the actual tree-construction method, not just theCanSynthesizeprobe)makes it unconditional, because unlike
CanSynthesizeit doesn't even accept aSynthesisPlanparameter — it allocates a fresh one on every call
(
ChoiceRule.cs:190-192):So even the partial protection
CanSynthesizehas (within a single top-level probe) is thrownaway at every recursive
Synthesizestep. Net effect:Expr → ParenExpr → Expr → ParenExpr → …,printing
((((((...x...)))))with no depth bound until the stack overflows.Originally suggested fix
ChoiceRule.Synthesizeshould accept/thread aSynthesisPlan(mirroringCanSynthesize'ssignature) instead of creating a new one on every call.
ChoiceRule.CanSynthesize(andSynthesize, once it takes a plan) should check whether(this, semanticElement)already has an in-progress/blocked entry in the plan beforeevaluating alternatives, and short-circuit if so, rather than only ever writing to that entry
for other call sites to observe.
Either change alone would fix this reproduction; both together would close the gap for
SequenceRule/OneOrMoreRule/etc. as well, since they'd now be threading a plan that theChoiceRulethey eventually reach will actually consult on re-entry.Not what was actually implemented — see below. Threading
SynthesisPlanthroughRule.Synthesizewould mean changing an abstract method signature implemented by 22 other ruletypes across the codebase (
SequenceRule,LiteralRule,EnumRule,AssignRule,QuoteRule,OneOrMoreRule,ZeroOrOneRule,ParanthesesRule, etc.), which is a much larger and riskierchange than this bug warrants.
Fix (implemented)
Instead of threading a
SynthesisPlaneverywhere,ChoiceRulenow carries its own thread-localreentrancy guard: a per-instance
ThreadLocal<HashSet<object>>of semantic elements currentlybeing synthesized by this choice rule, checked at the top of both
CanSynthesizeandSynthesizeand cleared in afinallyblock. If a semantic element is already in progress forthis rule (i.e. we've re-entered
ChoiceRule.CanSynthesize/Synthesizefor the same(this, element)pair via aSequenceRule/ParanthesesRule/etc. bypassingSynthesisPlan), thesecond attempt fails immediately instead of re-evaluating alternatives and recursing again.
This is confined entirely to
ChoiceRule.cs— no signature changes, no other rule types touched.Uses
ReferenceEqualityComparersince semantic elements are model instances; thread-local (ratherthan a plain instance field) because
Ruleinstances represent grammar structure and may beshared across concurrently-running parsers (e.g. multiple documents in
AnyText.Lsp).Verification
Parser.Update(container)on the mutatedAtomno longer stack-overflows; producesEdit count: 1as expected.AnyText.Testssuite: 214/214 passing, no regressions.Workaround
List typed/terminal alternatives before any transparent
paranthesesalternative in aself-referential choice:
This isn't a complete fix on its own (an actual instance of a different recursive alternative
that's also representable via parens could still hit the same gap), but for grammars dominated by
leaf/typed alternatives (identifiers, literals, comparisons) it avoids the crash in practice.
Summary by MergeMonkey