Skip to content

feat: support parse-tree pattern matching - #192

Merged
tinovyatkin merged 5 commits into
mainfrom
feat/issue-162-tree-pattern-matching
Jul 25, 2026
Merged

feat: support parse-tree pattern matching#192
tinovyatkin merged 5 commits into
mainfrom
feat/issue-162-tree-pattern-matching

Conversation

@tinovyatkin

@tinovyatkin tinovyatkin commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add ANTLR's parse-tree pattern matching API: ParseTreePattern, ParseTreeMatch, ParseTreePatternMatcher, and a compile_parse_tree_pattern method on every generated parser (the Parser.compileParseTreePattern analog), so structural queries like <ID> = <expr>; compile against a grammar rule and match subtrees with rule/token tags and <label:name> bindings
  • implement rule-bypass ATNs as a data transform (ParserAtn::with_bypass_alternatives): the packed parser ATN is read through its borrowing views, rewritten per ANTLR ATNDeserializer's bypass generation (imaginary token type max_token_type + rule + 1 per rule, left-recursive precedence prefixes wrapped with the loop-back edge excluded), and re-packed — the existing ATN interpreter runs over it unchanged, so the parse hot path is untouched
  • keep max_token_type unchanged in the bypass ATN: Atom transitions match by exact label equality while wildcard/~x transitions are bounded by min..=max_token_type, so imaginary bypass tokens are unreachable by grammar wildcards
  • port the pattern tokenizer faithfully: delimiter/escape-aware split (customizable via set_delimiters), literal chunks lexed by the real lexer (lex_pattern_chunk bridge), tags resolved through the vocabulary/rule names into synthetic tokens tracked in a TokenId-keyed side table, and the full-pattern-consumption check from Tree pattern compilation doesn't check for a complete parse antlr/antlr4#413
  • document the feature in the README alongside the XPath section

Validation

  • cargo clippy --locked --all-targets --all-features -- -D warnings
  • cargo test --locked --all-features --workspace (306 runtime, 734 codegen, 27 CLI tests)
  • cargo run --release --quiet --bin antlr4-runtime-testsuite: 357 passed, 0 failed, 0 skipped — normal parsing is unaffected by the bypass transform
  • end-to-end proofs at every layer: the unmodified interpreter matches an imaginary token as a whole rule and renders the (rule <tag>) single-terminal subtree shape (src/atn/bypass.rs tests); compile() + lockstep match against a hand-built stat/expr ATN (src/tree_pattern.rs tests); and a generated-parser CLI test that compiles <expression> + <expression> against the left-recursive Calculator grammar, matches 2 + 8, and binds both operands

Conformance note (from the issue): pattern matching is not exercised by the cross-target runtime-testsuite descriptors — ANTLR covers it in per-target API unit tests (TestParseTreeMatcher), which the new unit/CLI tests mirror. This is API-completeness, not a conformance-number mover.

Closes #162.

Summary by CodeRabbit

  • New Features

    • Added ANTLR-style parse-tree pattern matching with reusable compilation, label bindings, and ParseTreeMatch results.
    • Generated parsers now expose compile_parse_tree_pattern to compile patterns for a chosen rule.
    • Added the public tree_pattern API (ParseTreePatternMatcher, ParseTreePattern, PatternLexer, lex_pattern_chunk, and related errors).
    • Exposed token vocabulary metadata via RecognizerData::vocabulary().
  • Documentation

    • Documented the parse-tree pattern syntax (placeholders, escapes, delimiters, and labeled bindings) and runtime matching behavior.
  • Tests

    • Added end-to-end coverage for successful matches, mismatches, and <EOF>-handling failures.

Add ANTLR's parse-tree pattern matching API — compileParseTreePattern,
ParseTreePattern, and ParseTreeMatch — so structural queries like
`<ID> = <expr>;` can be compiled against a grammar rule and matched
over parse trees, binding rule/token tags (with optional labels) to
subject nodes.

- ParserAtn::with_bypass_alternatives (src/atn/bypass.rs): mirrors
  ATNDeserializer's rule-bypass generation on the packed parser ATN by
  reading it into an adjacency model, adding one imaginary-token bypass
  block per rule (left-recursive precedence prefixes handled), and
  re-packing. The existing ATN interpreter runs over the result
  unchanged — normal parsing is untouched, and max_token_type stays
  unchanged so grammar wildcard/not-set transitions can never match an
  imaginary bypass token.
- src/tree_pattern.rs: delimiter/escape-aware pattern splitter,
  RuleTagToken/TokenTagToken analog tracked in a TokenId-keyed side
  table, ParseTreePatternMatcher::compile (lex literal chunks, inject
  tag tokens, interpret over the bypass ATN, enforce full-pattern
  consumption per antlr4#413), and the lockstep matchImpl walk with
  label binding.
- Generated parsers gain compile_parse_tree_pattern(pattern,
  rule_index, make_lexer), mirroring ANTLR's Parser API; the reusable
  matcher and lex_pattern_chunk bridge are exported from the runtime.
- Tests: bypass structural + end-to-end interpreter proofs, splitter
  snapshots, matcher/compile unit tests against a real hand-built ATN,
  and a CLI test that generates the left-recursive Calculator grammar
  and matches `<expression> + <expression>` against a real parse.
  Conformance sweep unaffected: 357 passed, 0 failed.

Closes #162.
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

Copy/Paste Detection

Found 3 duplication(s) across 9 changed Rust file(s) (threshold: 100 tokens).

Show duplications

Found a 22 line (132 tokens) duplication in the following files:

  • Starting at line 1100 of tests/antlr4_rust_gen_cli.rs
  • Starting at line 1200 of tests/antlr4_rust_gen_cli.rs
        "parser grammar Delegate;\ndelegated: {isTypeName()}? ID;\n",
    )
    .expect("delegate grammar should be writable");
    fs::write(&tokens, "lexer grammar Tokens;\nID: [a-z]+;\n")
        .expect("token grammar should be writable");

    let output = run_antlr4_rust_gen(&[
        root.as_os_str(),
        tokens.as_os_str(),
        OsStr::new("-I"),
        temp.path().as_os_str(),
        OsStr::new("--out-dir"),
        out.as_os_str(),
    ]);
    assert!(
        output.status.success(),
        "stdout: {}\nstderr: {}",
        utf8(&output.stdout),
        utf8(&output.stderr)
    );
    let parser = fs::read_to_string(out.join("root.rs")).expect("parser should be emitted");
    assert!(parser.contains("pub trait RootHooks"), "{parser}");
```rust

---

Found a 20 line (120 tokens) duplication in the following files:
* Starting at line 284 of src/atn/mod.rs
* Starting at line 939 of src/atn/parser_atn.rs

```rust
            Self::Range { start, stop, .. } => (*start..=*stop).contains(&symbol),
            Self::Set { set, .. } => set.contains(symbol),
            Self::NotSet { set, .. } => {
                (min_vocabulary..=max_vocabulary).contains(&symbol) && !set.contains(symbol)
            }
            Self::Wildcard { .. } => (min_vocabulary..=max_vocabulary).contains(&symbol),
            Self::Epsilon { .. }
            | Self::Rule { .. }
            | Self::Predicate { .. }
            | Self::Action { .. }
            | Self::Precedence { .. } => false,
        }
    }
}

/// Ordered set of integer intervals used by set and negated-set transitions.
///
/// Unicode grammars can contain very large ranges, so this stores normalized
/// intervals rather than expanding every code point into a flat set.
#[derive(Clone, Debug, Default, Eq, PartialEq)]

Found a 15 line (104 tokens) duplication in the following files:

  • Starting at line 905 of src/atn/parser_atn.rs
  • Starting at line 1712 of src/atn/parser_atn.rs
impl ParserTransitionData<'_> {
    pub const fn target(self) -> usize {
        match self {
            Self::Epsilon { target }
            | Self::Atom { target, .. }
            | Self::Range { target, .. }
            | Self::Set { target, .. }
            | Self::NotSet { target, .. }
            | Self::Wildcard { target }
            | Self::Rule { target, .. }
            | Self::Predicate { target, .. }
            | Self::Action { target, .. }
            | Self::Precedence { target, .. } => target,
        }
    }
```rust

</details>

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f9fd06f2-c17b-47a3-85e3-6664210df4b9

📥 Commits

Reviewing files that changed from the base of the PR and between f8cb477 and b4b2ee0.

📒 Files selected for processing (2)
  • src/tree_pattern.rs
  • tests/antlr4_rust_gen_cli.rs

📝 Walkthrough

Walkthrough

Adds ANTLR-style parse-tree pattern compilation and matching, rule-bypass ATN construction, public runtime APIs, generated parser support, end-to-end tests, and README documentation.

Changes

Parse-tree pattern matching

Layer / File(s) Summary
Rule-bypass ATN transformation
src/atn/bypass.rs, src/atn/parser_atn.rs, src/atn/mod.rs
Adds rule-bypass ATN construction, imaginary rule tokens, transition rewiring, left-recursive handling, builder indexing, and validation tests.
Pattern compilation and tree matching
src/tree_pattern.rs, src/lib.rs, src/recognizer.rs
Adds pattern splitting, tag parsing, lexer integration, compiled pattern trees, tree matching, label bindings, mismatch reporting, XPath lookup, and public exports.
Generated parser API and validation
src/bin/antlr4-rust-gen.rs, tests/antlr4_rust_gen_cli.rs, README.md, src/byte_stream.rs
Generates compile_parse_tree_pattern, validates matching through the calculator parser, documents the syntax and runtime support, and reflows module documentation.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GeneratedParser
  participant ParseTreePatternMatcher
  participant PatternLexer
  participant ParserAtn
  participant ParseTreePattern
  GeneratedParser->>ParseTreePatternMatcher: compile pattern and rule index
  ParseTreePatternMatcher->>PatternLexer: lex literal chunks
  PatternLexer-->>ParseTreePatternMatcher: return token specifications
  ParseTreePatternMatcher->>ParserAtn: interpret tags and tokens
  ParserAtn-->>ParseTreePatternMatcher: return compiled pattern tree
  ParseTreePatternMatcher-->>GeneratedParser: return ParseTreePattern
  GeneratedParser->>ParseTreePattern: match_tree subject
  ParseTreePattern-->>GeneratedParser: return bindings and mismatch
Loading

Possibly related issues

  • Issue 162 — Adds the requested parse-tree pattern compilation, tag matching, generated parser API, and match results.

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding parse-tree pattern matching support.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/issue-162-tree-pattern-matching

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Claude finished @tinovyatkin's task in 11m 38s —— View job


Code review — round 4 (b4b2ee0)

  • Gather context (diff, upstream v4.13.2 reference at antlr-upstream/)
  • Review src/atn/bypass.rs against ATNDeserializer.java:265-347
  • Review src/tree_pattern.rs against ParseTreePatternMatcher.java / ParseTreePattern.java
  • Review generated-parser API + tests
  • Verify candidate findings by probe (cargo test --lib with temporary probe tests, since reverted)
  • Gate: cargo clippy --locked --all-targets --all-features -- -D warningsclean

The two commits since my last pass hold up. Everything from rounds 1–3 is addressed. One new finding, all of it minor.


Verification of the incremental commits

b4b2ee0 (trailing <EOF> rejection) — correct and it closes the hole Codex described. Probed directly:

pattern (fixture stat : ID '=' expr ';') result
<ID> = <expr> ; <EOF> Err(StartRuleDoesNotConsumeFullPattern)
<ID> = <expr> ; <EOF> garbage Err(Tokenization)
<ID> = <expr> ; compiles, tree (stat <ID> = (expr <expr>) ;)

The descendants() scan can't false-accept: tokenize already guarantees at most one EOF-typed spec and it is last, so any EOF terminal in the tree is that token.

f8cb477 (per-source dedupe index) — semantics preserved. transitions_by_source[source] is append-ordered and contains only that source's indices, so find returns the same global index the old whole-list scan did, and the returned TransitionId identity is unchanged. Staleness after finish()'s sort_by_key is unreachable because finish consumes self, and transitions_from() (used by mark_precedence_decisions) still reads the unsorted self.transitions. ✔

Upstream parity — the non-obvious things I checked

  • markPrecedenceDecisions ordering. Upstream marks precedence decisions before the bypass block (ATNDeserializer.java:259 vs :265), so I checked whether re-deriving them in ParserAtnBuilder::finish (parser_atn.rs:1361) survives the rewrite. It does: is_precedence_decision depends on StarLoopEntry's **last out-edge → LoopEndRuleStop``, and for an LR rule the end state is the StarLoopEntry, so LoopEnd → RuleStop` is never retargeted and no edges are added at the entry. Flag survives. ✔
  • Interval-set identity across the re-pack. add_interval_set (parser_atn.rs:1268) always appends and never dedupes, so the ParserIntervalSetIds carried in the copied specs stay valid. ✔
  • add_transition's dedupe can't silently drop an alt. Retargeting maps a target to a bypass stop only when it's a rule end state, and no two rules share one — so two distinct edges out of one source can never collapse. ✔
  • Phase reordering (plan-all → move-all → retarget-all → link-all, vs. upstream's per-rule loop) is behaviour-preserving: the only per-rule edges upstream adds before the next rule's retarget pass target new states or that rule's own end state. ✔
  • split / parse_tag / matchImpl / getRuleTagToken / rule- and token-name resolution (last-wins, EOF-1) all match upstream, including the quirks (<expr:expr> binds twice; identical start/stop delimiters degrade to UnterminatedTag).
  • No panics on hostile input — probed "", <a<b>>, rule index 99, escape == start, empty escape. All return structured errors.

Finding

Minor · src/tree_pattern.rs:485 — the post-EOF guard is channel-blind, so a hidden-channel token after a trailing <EOF> tag falsely rejects the pattern.

if let Some(at) = specs
    .iter()
    .position(|spec| spec.token_type == TOKEN_EOF)
    .filter(|at| at + 1 < specs.len())

The guard exists because CommonTokenStream stops buffering at an EOF-typed token, so trailing specs vanish before the #413 full-consumption check can see them. But dropping an off-default-channel spec is harmless — the interpreter would skip it anyway. Confirmed with a probe using a chunk lexer that emits hidden whitespace:

"<ID> = <expr> ; <EOF>"   →  Err(StartRuleDoesNotConsumeFullPattern)   // correct
"<ID> = <expr> ; <EOF> "  →  Err(Tokenization { message:
    "EOF at pattern token 4 terminates the stream; 1 following token(s) would be ignored" })

One trailing space turns a legal pattern into a confusing error that talks about an "ignored token" the user never wrote. This only bites grammars that write WS : [ \t\r\n]+ -> channel(HIDDEN); rather than -> skip — a very common idiom (most of grammars-v4 hides whitespace and comments). The Calculator.g4 fixture uses -> skip, which is exactly why the new CLI test can't catch it.

Fix is small: reject only when a DEFAULT_CHANNEL spec follows the EOF position, and pair it by making trailing_eof (src/tree_pattern.rs:582) look at the last default-channel spec rather than specs.last().

Fix this →


Optional polish (take or leave)

  1. Pattern-parse errors leak raw imaginary token types. A rejected pattern now surfaces the interpreter's message, e.g. mismatched input '<EOF>' expecting {ID, 5} — where 5 is the bypass type for rule stat. Upstream renders the same bare number (its Vocabulary has no entry either), so this is parity, not a regression; but since CannotInvokeStartRule is now the primary diagnostic surface for a bad pattern, mapping types above max_token_type back to <rulename> would be a genuine improvement over upstream. Cheap: ParseTreePatternMatcher already knows max_token_type and the rule names.

  2. The throwaway parser in compile_parse_tree_pattern. The new CLI test has to build CalculatorParser::new(CommonTokenStream::new(CalculatorLexer::new(InputStream::new("")))) purely to reach a method that reads nothing from self — it's all module statics now that the matcher is cached. Upstream's Parser.compileParseTreePattern is an instance method too, so &self is right to keep; but emitting a sibling associated function (or a free compile_parse_tree_pattern in the generated module) would let callers skip the ceremony. Visible right in the test you just added.

  3. MATCH_STACK_RED_ZONE / MATCH_STACK_SIZE (tree_pattern.rs:31-32) duplicate the visitor's constants by value. If they're meant to stay in lockstep, a shared pub(crate) pair would say so.


Verdict: the design is right — implementing bypass alternatives as a data transform over the packed ATN, so the interpreter runs unchanged, is the correct call for this runtime and the tests prove it end-to-end at all three layers. Nothing here blocks merge; the one finding is a narrow false rejection worth a follow-up.
· branch feat/issue-162-tree-pattern-matching

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

📊 Source Code Metrics (this PR vs main)

File Cyclomatic Cognitive Functions LLOC MI
src/bin/antlr4-rust-gen.rs 2249 (main: 2248) 🔴 1415 ⚪ 498 (main: 497) 🔴 3938 (main: 3936) 🔴 0 ⚪
src/atn/parser_atn.rs 628 (main: 617) 🔴 308 (main: 306) 🔴 204 (main: 203) 🔴 926 (main: 922) 🔴 0 ⚪
src/tree_pattern.rs 211 🆕 93 🆕 76 🆕 351 🆕 0 🆕
src/atn/mod.rs 55 ⚪ 13 ⚪ 34 ⚪ 57 ⚪ 7.28 (main: 7.31) 🔴
src/recognizer.rs 42 (main: 41) 🔴 1 ⚪ 34 (main: 33) 🔴 48 (main: 47) 🔴 11.69 (main: 12.04) 🔴
src/atn/bypass.rs 105 🆕 82 🆕 25 🆕 244 🆕 0 🆕
src/lib.rs 4 ⚪ 3 ⚪ 1 ⚪ 7 ⚪ 32.82 (main: 33.59) 🔴

Generated by mehen v1.7.0 — the code quality watcher.

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.11976% with 165 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/tree_pattern.rs 92.87% 71 Missing ⚠️
src/atn/bypass.rs 89.53% 58 Missing ⚠️
src/atn/parser_atn.rs 34.54% 36 Missing ⚠️

📢 Thoughts on this report? Let us know!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 10

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/atn/bypass.rs`:
- Around line 577-752: Add a hand-built left-recursive ATN fixture and unit test
alongside the existing bypass tests, exercising the precedence-prefix detection
in rule_end_state and its InvalidData path as appropriate. Assert that
with_bypass_alternatives selects the expected rule end state and preserves the
loop-back edge in the StarLoopEntry → LoopEnd → RuleStop structure.
- Around line 165-223: Store each rule’s planned end state alongside its bypass
stop during the initial planning loop, then reuse that stored value when adding
bypass edges instead of calling rule_end_state on the mutated graph. Replace the
per-transition excluded.contains and retarget.iter().find scans with indexed or
hashed lookup structures, preserving excluded-edge skipping and end-state
retargeting while making the rewrite linear in the number of transitions and
rules.

In `@src/bin/antlr4-rust-gen.rs`:
- Around line 9257-9266: Update compile_parse_tree_pattern to cache the
RecognizerData and ParseTreePatternMatcher in module-level OnceLock values,
initializing each only once from metadata() and parser_atn(). Reuse the cached
matcher for every pattern compilation while preserving the existing lexer
callback behavior and error propagation.

In `@src/recognizer.rs`:
- Around line 100-103: Add #[must_use] to the vocabulary accessor method so it
matches the neighboring rule_names accessor and satisfies
clippy::must_use_candidate under the enforced CI lint configuration.

In `@src/tree_pattern.rs`:
- Around line 508-512: Update TreePattern’s bypass_token_type to reuse the
existing ParserAtn/bypass.rs imaginary-token helper instead of recomputing
max_token_type + rule_index + 1. Expose the helper as needed, propagate its
Result-based overflow error through the matcher, and remove the silent i32
fallback so ATN and matcher always produce the same token type.
- Around line 889-892: Update the split_no_tags_is_single_text_chunk test to
snapshot the complete chunks Vec using the existing insta snapshot pattern
established by neighboring split tests, replacing the hand-written assert_eq!
while preserving the current input and validation.
- Around line 516-543: The interpret method must reject pattern parses that
recover from syntax errors instead of returning an apparently valid PatternTree.
Configure the BaseParser created in interpret to suppress/remove inherited error
listeners before parsing, then check parser.number_of_syntax_errors() after
parse_atn_rule and return the appropriate parse error when it is nonzero, before
the EOF validation and PatternTree construction.
- Around line 1211-1216: Remove the placeholder test function
ignore_the_unused_bypass_stat and annotate the unused BYPASS_STAT constant with
#[allow(dead_code)] instead, preserving the constant for fixture documentation
without adding a meaningless test.
- Around line 356-365: Add a dedicated ParseTreePatternError variant for
failures from atn.with_bypass_alternatives in ParseTreePattern::new, carrying
the underlying message without a rule index. Map the transform error to this
variant and update its Display implementation so it reports the bypass-ATN
construction failure directly, while preserving CannotInvokeStartRule for actual
rule invocation errors.
- Around line 263-297: Update the PatternLexer trait and tokenize_chunk
documentation to state that non-EOF tokens, including off-default-channel
tokens, are preserved with their original channels; remove the claim that
whitespace and comments are excluded. Keep the contract consistent with
lex_pattern_chunk and its channel-preserving TokenSpec behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e2c6209c-1e88-4690-94d1-29b12f17135f

📥 Commits

Reviewing files that changed from the base of the PR and between e0f032b and 17c4995.

⛔ Files ignored due to path filters (5)
  • src/snapshots/antlr4_runtime__tree_pattern__tests__split_custom_delimiters.snap is excluded by !**/*.snap
  • src/snapshots/antlr4_runtime__tree_pattern__tests__split_interleaves_text_and_tags.snap is excluded by !**/*.snap
  • src/snapshots/antlr4_runtime__tree_pattern__tests__split_parses_labeled_tags.snap is excluded by !**/*.snap
  • src/snapshots/antlr4_runtime__tree_pattern__tests__split_rejects_malformed.snap is excluded by !**/*.snap
  • src/snapshots/antlr4_runtime__tree_pattern__tests__split_strips_escapes.snap is excluded by !**/*.snap
📒 Files selected for processing (9)
  • README.md
  • src/atn/bypass.rs
  • src/atn/mod.rs
  • src/atn/parser_atn.rs
  • src/bin/antlr4-rust-gen.rs
  • src/lib.rs
  • src/recognizer.rs
  • src/tree_pattern.rs
  • tests/antlr4_rust_gen_cli.rs

Comment thread src/atn/bypass.rs
Comment thread src/atn/bypass.rs
Comment thread src/bin/antlr4-rust-gen.rs Outdated
Comment thread src/recognizer.rs
Comment thread src/tree_pattern.rs Outdated
Comment thread src/tree_pattern.rs Outdated
Comment thread src/tree_pattern.rs Outdated
Comment thread src/tree_pattern.rs
Comment thread src/tree_pattern.rs
Comment thread src/tree_pattern.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 17c4995478

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/tree_pattern.rs
Comment thread src/tree_pattern.rs
Comment thread src/tree_pattern.rs
Comment thread src/bin/antlr4-rust-gen.rs
Correctness (verified by probes before fixing):
- reject patterns that only parse via error recovery: the pattern parse
  now removes error listeners and fails on any recorded syntax error,
  matching upstream's BailErrorStrategy; previously `<ID> <e:expr> ;`
  compiled Ok with a `<missing '='>` error node baked into the pattern
  tree (which then matched nothing) while printing to stderr
- reject overlapping tag delimiters (`<a<b>>`) as DelimitersOutOfOrder
  instead of panicking on an inverted text-chunk slice
- reject tokens after an EOF-typed pattern token: an `<EOF>` tag
  terminates the buffered stream, so a suffix (`<EOF> garbage`) was
  silently dropped before the full-consumption check could see it
- reserve `compile_parse_tree_pattern` in the generated parser's rule
  method names so a grammar rule of that name gets the `_rule` suffix
  instead of colliding with the emitted helper
- `match_rules` no longer fails open on a non-rule node view, and the
  rule-tag lookup returns the rule index structurally instead of a
  `usize::MAX` sentinel

API/robustness:
- generated `compile_parse_tree_pattern` caches the RecognizerData and
  matcher (and its bypass ATN) in module OnceLocks, mirroring ANTLR's
  `Parser.bypassAltsAtnCache`, instead of re-running the O(states +
  transitions) transform per call
- `ParserAtn::bypass_token_type` is now the single source of the
  imaginary-type formula; the matcher consumes it instead of re-deriving
  it with a silent overflow fallback
- dedicated `BypassAtn` error variant replaces the `usize::MAX`
  rule-index sentinel in CannotInvokeStartRule
- `set_delimiters` validates non-empty start/stop (upstream throws
  IllegalArgumentException) and now documents the deliberate empty-escape
  divergence; it returns Result
- `ParseTreePattern::find_all(tree, xpath, recognizer)` lands the
  upstream XPath pairing (we already ship XPath)
- `PatternLexer` trait doc aligned with the implemented channel-
  preserving contract; `RecognizerData::vocabulary` gains #[must_use]

Bypass transform cleanups:
- end states planned once and reused; the second `rule_end_state` call
  on the mutated graph is gone
- retarget map indexed by state and excluded edges in a BTreeSet, making
  the rewrite linear instead of O(transitions x rules)
- `ParserTransitionSpec::with_target` moved next to the spec enum
- new hand-built left-recursive fixture pins the precedence-prefix
  detection, the preserved loop-back edge, and the InvalidData path

Tests: 312 lib (+6), 734 codegen, 27 CLI; conformance 357/357.
@tinovyatkin

Copy link
Copy Markdown
Contributor Author

All review findings addressed in 1614a48 (full gate re-run: clippy --all-targets --all-features -D warnings clean, 312+734+27 tests, conformance 357/357).

Claude review disposition:

  1. Recovery-accepted patterns → fixed: listeners removed for the pattern parse + number_of_syntax_errors() > 0 rejected (CannotInvokeStartRule), regression-tested against your probe shapes (<ID> <e:expr> ;, <ID> = ;, = <expr> ;).
  2. Bypass ATN rebuilt per call → fixed: generated method caches RecognizerData + matcher in module OnceLocks (ANTLR's bypassAltsAtnCache analog).
  3. O(transitions × rules) retarget → fixed: state-indexed retarget map + BTreeSet exclusions.
  4. rule_end_state recomputed on mutated graph → fixed: end states planned once and reused.
  5. PatternLexer doc contradiction → fixed: trait doc now states the channel-preserving contract.

Smaller items: find_all(tree, xpath, recognizer) implemented (paired with the existing XPath, upstream-shaped, tested); set_delimiters validates non-empty start/stop and returns Result, with the deliberate empty-escape divergence documented; bypass_token_type unified on ParserAtn::bypass_token_type (no silent overflow fallback); match_rules fails closed on a non-rule view; tag_rule_index sentinel replaced by returning the rule index from rule_tag_of; placeholder test dropped; with_target now lives on ParserTransitionSpec next to target().

Codex P2s (all probe-confirmed before fixing): recovery acceptance, <a<b>> slice panic → DelimitersOutOfOrder, <EOF>-tag stream truncation → rejected with a Tokenization error (trailing <EOF> stays legal), and compile_parse_tree_pattern added to the generated reserved-method list.

- narrow `ParserAtn::set_count` and `ParserTransitionSpec::with_target`
  to pub(crate): both are internal plumbing for the bypass transform,
  and set_count was private before this feature
- guard `match_impl` recursion with stacker::maybe_grow, matching the
  runtime's other recursive tree descents (visitor walk, recognizer)
- restore upstream's epsilon-only check on the loop-end state in
  `rule_end_state` (`ATNDeserializer` requires `epsilonOnlyTransitions`
  before trusting the first edge; the port only checked the target kind)
- expose `ParseTreePattern::pattern_tree()` (upstream `getPatternTree`)
  for debugging patterns that compile but do not match
- docs: drop the stale README advice to hand-reuse the matcher (the
  generated method caches it per process since 1614a48); note the
  caching and the clean-parse requirement in the generated method's
  rustdoc; fix the pre-existing redundant-link rustdoc error in
  byte_stream.rs so `RUSTDOCFLAGS="-D warnings" cargo doc` is clean
@tinovyatkin

Copy link
Copy Markdown
Contributor Author

Follow-up review items addressed in 3be5c35:

  1. pubpub(crate) on ParserAtn::set_count and ParserTransitionSpec::with_target — confirmed nothing outside the crate needs them.
  2. match_impl stack guard — now wrapped in stacker::maybe_grow with the same red-zone/growth constants as the visitor walk. Agreed it's convention alignment rather than a live defect (the parse stages bound the practical depth first, as you measured), but the asymmetry is gone.
  3. Stale README reuse advice — reworded: the generated method caches the compiler per process; only the delimiter customization still points at ParseTreePatternMatcher.
  4. getPatternTree analog — added ParseTreePattern::pattern_tree() for exactly the debugging case you described (pattern compiles, silently fails to match). The other members (getMatcher, one-shot match/matches overloads, ParseTreeMatch::toString) stay skipped as borrow-awkward, per your read.
  5. epsilonOnlyTransitions guard — restored in rule_end_state, matching ATNDeserializer.java:305's full conjunction.

Also fixed the pre-existing byte_stream.rs redundant-link rustdoc error while touching docs, so RUSTDOCFLAGS="-D warnings" cargo doc --lib is now clean. Gate: clippy -D warnings clean, 308 lib tests, generated-parser pattern CLI test green.

`add_transition` deduped by scanning every previously added transition,
making a whole-ATN re-emit O(T^2). That cost was codegen-only until the
rule-bypass transform put it on a runtime path (the first
`compile_parse_tree_pattern` call), where review measured ~4x per
transition-count doubling (531 ms at 51k transitions).

Keep a `BTreeMap<AtnStateId, Vec<usize>>` of per-source transition
indices so duplicate detection scans one state's out-edges only.
Dedupe semantics and returned `TransitionId` identity are unchanged.
Measured on the review's synthetic single-edge chain (release):
6.4k/12.8k/25.6k transitions now 3.0/4.9/7.0 ms (was 6.4/25/98 ms
locally) — linear instead of quadratic.

Conformance sweep: 357 passed, 0 failed.
@tinovyatkin

Copy link
Copy Markdown
Contributor Author

Round-3 observation addressed in f8cb477 — thanks for measuring it.

O(T²) first-call costParserAtnBuilder::add_transition now keeps a BTreeMap<AtnStateId, Vec<usize>> of per-source transition indices, so the dedupe scans one state's out-edges instead of every transition added so far. Dedupe semantics and returned TransitionId identity are unchanged (same first-match-wins within a source; the index is append-ordered).

Reproduced your synthetic single-edge-chain benchmark locally (release, M-series):

transitions before after
6,400 6.4 ms 3.0 ms
12,800 25 ms 4.9 ms
25,600 98 ms 7.0 ms

~4× per doubling → linear. This also speeds up codegen-time ATN construction for large grammars, not just the first pattern compile.

Gate: clippy -D warnings clean, 312+734+27 tests, conformance 357/357.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f8cb477ebd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/tree_pattern.rs
A final `<EOF>` tag doubles as the pattern token stream's terminator, so
when the selected rule does not itself end in EOF the tag was silently
dropped: `<ID> = <expr> ; <EOF>` compiled to the same pattern tree as
`<ID> = <expr> ;`, and the lookahead full-consumption check could not
tell "the rule matched EOF" from "the tag was ignored".

After a parse that saw a trailing EOF spec, require an EOF terminal in
the produced tree; reject with StartRuleDoesNotConsumeFullPattern
otherwise (upstream silently drops the tag here, consistent with its
handling of a suffix after EOF, which we already reject).

Covers both directions: the unit fixture (`stat` never consumes EOF →
rejected) and the generated Calculator parser (`start : expression EOF`
consumes the tag → `<expression> <EOF>` compiles and matches a full
parse; `<expression> + <expression> <EOF>` against `expression` is
rejected).
@tinovyatkin
tinovyatkin merged commit 619ebcb into main Jul 25, 2026
13 checks passed
@tinovyatkin
tinovyatkin deleted the feat/issue-162-tree-pattern-matching branch July 25, 2026 10:25
@ophiarch ophiarch Bot mentioned this pull request Jul 25, 2026
tinovyatkin added a commit that referenced this pull request Jul 26, 2026
Switching the review workflow to `claude-code-base-action` (#216) was
necessary for `workflow_run` — the wrapper rejects it as an automation
event with no PR entity — but it silently cost the review its GitHub
identity. Comments now come from `github-actions[bot]` instead of
`claude[bot]`.

The identity was never an input we could set. `claude-code-action` trades
the job's OIDC token for a Claude GitHub App installation token in
`src/github/token.ts`, before it runs anything; the base action has no
such code, so nothing in this workflow could re-enable it. Confirmed by
comparing comment authors: PR #192 has a `claude[bot]` comment, the
#216-era runs do not.

Re-implement the exchange as a step, mirroring the wrapper: mint an OIDC
token with audience `claude-code-github-action` (Anthropic validates the
`aud` claim, so the default audience is rejected), POST it to
`api.anthropic.com/api/github/github-app-token-exchange`, and use the
returned installation token for every comment write. Revoke it in a final
step rather than letting it idle out its remaining ~40 minutes.

Identity is kept strictly cosmetic. `continue-on-error` plus a
`|| github.token` fallback on each consumer means an uninstalled app, a
changed endpoint, or a network blip downgrades authorship instead of
losing a review. `gh run cancel` on the quota path keeps using the
workflow token: the app token's grants are contents/pull-requests/issues,
with no `actions: write`.

Three details found by testing the step against a local stub rather than
by reading the upstream source:

* `ACTIONS_ID_TOKEN_REQUEST_{URL,TOKEN}` must be read as shell variables.
  The `env` expression context only exposes workflow/job/step-declared
  variables, so mapping these runner-injected ones in yields empty
  strings and the exchange is never attempted — it would have failed
  open, always, and looked like the app was uninstalled.
* `-w '%{http_code}'` is unusable as a success signal here: on a
  connection-teardown retry curl emits the template once per attempt, so
  the capture reads "000200" and matches no status test. A recovered
  transient fault would have cost the identity. Judge by curl's exit code
  with `--fail-with-body`, which still saves the error JSON.
* Bodies go to a file, never piped to jq. Under `-sS --retry` curl writes
  "Transient problem … will retry" to stderr, which a `2>&1` capture
  splices into the JSON.

Verified with a stub covering the happy path through a forced retry,
a permanent 401, an unreachable endpoint, and a missing OIDC context:
all four fall back cleanly and exit 0. actionlint clean.

Note this cannot take effect until it lands on the default branch — the
exchange refuses workflows absent from it, and `workflow_run` reads the
default-branch copy regardless.
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.

Support parse-tree pattern matching (compileParseTreePattern / ParseTreePattern / ParseTreeMatch)

1 participant