Skip to content

feat: resolve nested rules from the registered rule set - #45

Open
RobinDev wants to merge 1 commit into
tempestphp:mainfrom
RobinDev:feat/rule-contexts
Open

RobinDev wants to merge 1 commit into
tempestphp:mainfrom
RobinDev:feat/rule-contexts

Conversation

@RobinDev

@RobinDev RobinDev commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Rewritten to follow the direction in #13: the parser is the single rule registry, tokens pull from it, and a rule declares which tokens it applies to.

The API

interface Rule
{
    public function shouldParse(Parser $parser): bool;
    public function parse(Parser $parser): ?Token;

    public function supportsToken(Token $token): bool;
    public function addTokenSupport(string $token): static;
    public function removeTokenSupport(string $token): static;
}
// In the token
$parser->forToken($this)->parse($this->content);

// In the parser
public function forToken(Token $token): self
{
    if (isset($this->cache[$token::class])) {
        return $this->cache[$token::class];
    }

    $clone = clone $this;

    $clone->activateRules(array_values(array_filter(
        $this->rules,
        static fn (Rule $rule) => $rule->supportsToken($token),
    )));

    $this->cache[$token::class] = $clone;

    return $clone;
}

IsRule supplies the three methods, and Parser::getRule() / Markdown::getRule() return a registered instance to reconfigure.

The thirteen forToken($this, [ … ]) lists are gone. So are the new BoldRule() calls they contained: nested content now runs the registered instances, which is what makes removal, reconfiguration and addition propagate at all.

The one place I deviate

The proposal has each rule hold an explicit $supportedTokens allowlist. I measured what that costs on the current code: thirteen tokens, thirteen rules, 98 edges. Transposing the matrix does not shrink it — TextRule would list 13 tokens, LinkRule and SocialHandleRule 12, BoldRule and ItalicRule 11.

That matters because the matrix is what broke in the first place. #47 existed because those lists drifted: fifteen valid CommonMark/GFM combinations rendered as literal text, purely because a rule was added to some lists and not the others. An allowlist that every rule has to keep complete is the same failure mode, re-signed.

So $tokenSupport is an override map rather than an allowlist, and the default answer comes from the context the rule is written for:

public function supportsToken(Token $token): bool
{
    return $this->tokenSupport[$token::class]
        ?? in_array(RuleContext::INLINE, $this->contexts, strict: true);
}
  • RuleContext::INLINE — runs inside every token that holds inline content.
  • RuleContext::BLOCK — runs on the document, and on the containers that opt in.

The whole matrix is then 16 declarations instead of 98, and all sixteen are real:

// Rules do not nest in the token they produce
BoldRule:          removeTokenSupport(BoldToken, BoldAndItalicToken)
ItalicRule:        removeTokenSupport(ItalicToken, BoldAndItalicToken)
BoldAndItalicRule: removeTokenSupport(BoldToken, ItalicToken, BoldAndItalicToken)
StrikethroughRule: removeTokenSupport(StrikethroughToken)
LinkRule:          removeTokenSupport(LinkToken)
SocialHandleRule:  removeTokenSupport(LinkToken)

// Block rules name the containers they nest in
HeadingRule:       addTokenSupport(DivToken)
QuoteRule:         addTokenSupport(DivToken, QuoteToken)
PreRule:           addTokenSupport(DivToken, ParagraphToken)
DivRule:           addTokenSupport(ParagraphToken)

addTokenSupport() and removeTokenSupport() are still the escape hatch — they are how those sixteen are written, so the extension path and the built-in path are the same path.

This also answers the requirement the allowlist version misses. An extension that adds a block rule almost always adds its token with it. Under an allowlist, its content parses nothing until the extension calls addTokenSupport(MyToken::class) on every registered inline rule — including the ones a different extension registers later. Under a context default, a custom token gets the inline rules with nothing declared at all; RuleRegistryTest::a_custom_token_gets_the_registered_inline_rules is that case.

What this costs

  • Rule gains three methods. use IsRule; is the one-line migration; the custom rules in MarkdownTest show it.
  • ParagraphRule and RawRule lose readonly. Token support is mutable state, and a readonly class cannot hold it.
  • new Parser(rules: [new HeadingRule()]) now really means that rule only. Nested content used to arrive with a list the token rebuilt, so an isolated parser silently got inline parsing it never asked for. It no longer does, which is most of the test diff: the isolated rule tests now register the rules they depend on, and the cases that asserted '' for "no rule matched" assert the literal text instead, because TextRule is registered and that is what a parser does. Say the word if you would rather keep the old convenience — I would rather surface it than paper over it.
  • getRule() drops the sub-parser cache. Rules are mutable and shared, so a sub-parser built from a rule stops being valid the moment someone reconfigures it — this is the "this should be properly cached" note in your sketch. getRule() is the only way to reach a registered instance, so invalidating there closes the window exactly when it opens, at no cost on the parsing path. Without it, getRule(…)->addTokenSupport(…) silently does nothing after the first parse.

Evidence

  • 377 tests green. RuleRegistryTest covers the three requirements from Make tokens and rules more configurable #13 plus the custom-token case and the stop-char leak below.
  • Every document in a personal dataset renders byte-identically to main. This refactor changes no output.
  • Faster than main on both fixtures: −5.5% small, −2.4% large, averaged over four interleaved runs. The earlier +9.7% the bot reported on 01-small was the per-document cache clear; it is gone.

One bug this had to fix

Stop chars were stored on the rule ($rule->stopChars .= …). That was harmless while every rule set had its own instances. Once sets share the registered instance, one set's stop chars leak into the next and **bold** stops parsing on the second call. activateRules() now clones a NeedsStopChars rule when the set's stop chars differ from the ones it carries, so the registry instance keeps its own and each set gets a copy.

@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown

Benchmark Results

Comparison of feat/rule-contexts against main (b4cd28e8892eb3049af2bc67612f34149ada5aa7).

Open to see the benchmark results

No benchmark changes above ±5%.

Generated by phpbench against commit 032f9b4

@RobinDev RobinDev changed the title feat: resolve nested rules from the configured rule set feat: resolve nested rules from the registered rule set Sep 15, 2026
@RobinDev

RobinDev commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on main now that #43 is in, and the +9.73% the bot flagged on 01-small is fixed — it was mine, not noise.

The cause was the sub-parser cache being cleared at the start of every top-level parse(). MarkdownBench reuses one Parser across Revs(3) × Iterations(5), so that rebuilt every sub-parser on every rev; on a small document the rebuild dominates the parse.

It was there for one reason: rules are mutable now, so a sub-parser built from a rule goes stale the moment someone calls addTokenSupport() on it. getRule() is the only way to reach a registered instance, so invalidating there closes the window exactly when it opens, and costs nothing on the parsing path:

public function getRule(string $rule): ?Rule
{
    $this->cache = [];
    // …
}

RuleRegistryTest::token_support_can_be_added_on_a_registered_rule parses, reconfigures, then parses again — it fails without that line.

Measured after the change, four interleaved runs against main:

Fixture main this PR
01-small 0.2045 ms 0.1933 ms −5.5%
02-large 13.348 ms 13.022 ms −2.4%

Faster on both, which is what I would expect: nested content no longer instantiates a fresh new BoldRule() and friends on every cache miss, it reuses the registered instances.

Also re-verified behaviour after the rebase: 377 tests green, and a personal dataset of real-world Markdown renders byte-identically to main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant