Skip to content

fix: start-state merging in MergeEquivalentStates and nested unbounded repetitions - #911

Merged
Seven-Streams merged 2 commits into
mlc-ai:mainfrom
Seven-Streams:fix/2026-09-20/fsm_merge
Sep 20, 2026
Merged

Seven-Streams merged 2 commits into
mlc-ai:mainfrom
Seven-Streams:fix/2026-09-20/fsm_merge

Conversation

@Seven-Streams

Copy link
Copy Markdown
Member

Two correctness bugs in the grammar compilation pipeline, both reachable from ordinary EBNF, regex and JSON-schema input on main. They were found by diffing the regular-grammar flattener from #901 against GrammarMatcher; the flattener was right both times and the matcher side was wrong.

1. MergeEquivalentStates merges the start state away (over-acceptance)

The "equivalent successor" rule merges two states that have the same single predecessor, the same incoming labels and the same end-ness. When the start state's only incoming edge is its own self-loop, it passes that test, and a state reached from the start state by the loop label looks equivalent to it. But the start state is also entered without taking any edge, so the merged automaton accepts input that never consumed the looped class. Three-state automata escape only because of the NumStates() < 4 early return.

  • root ::= [a-z]* [a-z] "c" "d" accepts cd; root ::= [a-z]* [a-z] [0-9] [0-9] accepts 12.
  • JSON schema {"type": "string", "pattern": "^a*acd$"} accepts "cd", through the regex FSM builder.

Fix: skip the start state in that rule, as candidate and as sibling. The "equivalent precursor" rule is unaffected, since states with identical outgoing edges have the same future however they are entered.

2. Nested unbounded repetitions reject everything (under-acceptance)

RepetitionRangeExpander inlines a single-element rule into the repetition that references it, but copied the element verbatim instead of visiting it. For root ::= b{1,2}, b ::= a{2,} the inlined a{2,-1} was therefore never expanded into recursion, an unbounded kRepeat edge reached the parser, and the parser's repeat_count >= Upper() check against -1 never let it enter the repeated rule.

  • Regex (a{2,}){1,2}, (a{2,}){2}, (a{1,}){1,2} and the EBNF form above reject every input.

Fix: visit the inlined element (VisitExpr) so nested repetitions go through the expander like any other.

Validation

  • New tests: XGrammarFSMTest.MergeEquivalentStatesKeepsStartStateSeparate (hand-built four-state NFA), test_character_class_star_followed_by_same_class (EBNF and schema-pattern paths), and test_bounded_repetition_of_unbounded_repetition (four regex cases plus the EBNF form). All fail before their fix.
  • xgrammar_test 78/78. Full tests/python with HF_TOKEN: 4851 passed, 3 skipped, 0 failed.
  • No output change for unaffected grammars: the per-rule FSMs of the builtin JSON grammar, several JSON schemas (bounded strings, patterns, formats, numbers), Kimi K3 and DeepSeek V4 structural tags are byte-for-byte identical before and after, and the full serialized compiled grammar of a 1024-char bounded string schema (23.5 MB including the token mask cache) is identical as well. So no goldens change.
  • 600 random character-class grammars and a set of nested-repetition regexes (((a{2,}){1,2}){1,2}, (a{2,}b{1,}){1,3}, (a{130,}){1,2}) agree with Python's re.
  • Performance: A/B against the unfixed main build on the same host, alternating rounds. Compile time and mask latency for the builtin JSON grammar, a 1024-char bounded string, an email regex, a schema pattern, 300 bounded strings, 400 properties with max_whitespace_cnt and large repetitions are all within run-to-run noise; the compiled artifacts being identical, the fixes add only a per-state comparison in the merge pass and one visitor dispatch per inlined element.

Signed-off-by: yuchuan <yuchuan.7streams@gmail.com>
Signed-off-by: yuchuan <yuchuan.7streams@gmail.com>
Copilot AI lite review requested due to automatic review settings September 20, 2026 19:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Seven-Streams
Seven-Streams merged commit 26a71ab into mlc-ai:main Sep 20, 2026
51 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.

2 participants