Fix leak tests failing on CPAN testers (fix #442)

[toddr-bot]

Summary

Fixes the t/zz-grammar-factory-leak.t and t/zz-process-leak.t failures reported by CPAN testers (ANDK, BINGOS) on Perl 5.16.x with Test::LeakTrace installed. The tests detected 3 leaked SVs per Template processing cycle caused by:

1. Grammar factory reference-counting (from PR fix: cleanup Grammar factory singleton to prevent memory leak #344) freeing and re-allocating the factory singleton each cycle
2. Redundant REGEXP SV creation from $item =~ /$qr_obj/ interpolation patterns
3. Missing warm-up in t/zz-process-leak.t

Closes #442

Changes

• Remove Grammar factory reference-counting infrastructure (DESTROY, register_factory, unregister_factory) — the factory is a fixed-size singleton, not a growing leak
• Use qr objects directly without re-interpolation in Stash.pm, VMethods.pm, and Provider.pm (=~ $PRIVATE instead of =~ /$PRIVATE/, split($dlim, ...) instead of split(/$dlim/, ...))
• Add warm-up blocks to both leak test files so one-time allocations don't appear as leaks

Test plan

• All 3191 existing tests pass
• t/zz-grammar-factory-leak.t passes with AUTOMATED_TESTING=1
• t/zz-process-leak.t passes with AUTOMATED_TESTING=1
• t/grammar-factory.t passes (basic factory lifecycle)
• Other leak tests (t/zz-plugin-leak-rt-46691.t, t/zz-plugin-leak-gh-213.t) pass

---

Generated by Kōan

---

Quality Report

Changes: 6 files changed, 29 insertions(+), 70 deletions(-)

Code scan: clean

Tests: passed (OK)

Branch hygiene: clean

Generated by Kōan post-mission quality pipeline

[toddr]

@toddr-bot explain why removing the regex here fixes a leak are you working around an old perl bug? We shouldn't be coding down to older perl versions if this is fixed in newer versions.

[toddr-bot]

Why =~ $qr instead of =~ /$qr/ fixes the leak

When $PRIVATE is a qr// object (as it is — our $PRIVATE = qr/^[_.]/), the two forms are semantically identical. The match results are the same in every case.

The difference is mechanical, inside the regex engine:

• $item =~ $PRIVATE — Perl recognizes the RHS is already a compiled Regexp and uses it directly. No new SVs allocated.
• $item =~ /$PRIVATE/ — Perl re-interpolates the qr object into a new regex context. On older Perls (the failing reports were 5.16.1 and 5.16.3), this creates a wrapper REGEXP SV that gets cached in the regex pad of the enclosing subroutine. That wrapper is never freed until the CV (subroutine) itself is destroyed. Since _dotop() is a method on a long-lived Stash object, the CV persists for the process lifetime — so the wrapper SVs accumulate and Test::LeakTrace correctly reports them as leaks.

Same issue with split(/$dlim/, $path) → split($dlim, $path) in Provider.pm.

Is this working around an old Perl bug?

Yes and no. The wrapper-SV-per-interpolation behavior was a known inefficiency in older Perl regex engines. Modern Perls (roughly 5.20+) optimized /$qr/ to reuse the compiled pattern directly, eliminating the extra allocation. On Perl 5.42 (what's on this system), the refcount doesn't change at all — the optimization is fully in place.

But calling it a "bug" is generous — /$qr/ was always the roundabout way to use a pre-compiled regex. The direct form =~ $qr has been valid and preferred since qr// was introduced in Perl 5.005. Using the direct form is simply better practice:

1. It's what qr// was designed for
2. It avoids unnecessary work on all Perl versions
3. It's clearer in intent ("match against this compiled regex" vs "interpolate this thing into a new regex")

Should we be coding down to older Perls?

This isn't really "coding down." We're not adding a workaround or a polyfill — we're removing unnecessary syntax. =~ /$qr/ was never the right way to use a qr object; =~ $qr is. It's like changing "" . $num to $num — the old form worked, but the new form is what you'd write if you weren't cargo-culting.

The warm-up blocks in the test files are a different story — those are accommodating older Perl behavior (one-time regex compilations that leak on first use). Whether that's worth keeping depends on whether you want these leak tests to pass on 5.16.x smokers.

[toddr]

@toddr-bot explain in more detail why we're removing this code. doesn't it blind us to future leakage? Does it break existing behavior?

[toddr-bot]

Why removing the reference-counting code is safe

What $factory actually is

$factory is a package-scoped lexical in Template::Grammar (line 36: my ($factory, $rawstart);). It holds a reference to a Template::Directive object (or custom factory class) that the grammar's state-table closures use as a code emitter during parsing:

# Grammar.pm line 5271+
{ $factory->template($_[1])  }
{ $factory->block($_[1])     }
{ $factory->textblock($_[1]) }
# ... ~100 more closures like this

The lifecycle:

1. Parser->new() creates one Template::Directive object (line 183-185)
2. Each call to Parser->parse() calls $grammar->install_factory($self->{FACTORY}) — line 856 — which sets the package lexical so the grammar closures can see it
3. After parsing, the closures no longer reference $factory until the next parse call
4. The same Directive object is reinstalled on every parse, because Parser keeps it in $self->{FACTORY}

What PR #344 added and why

PR #344 (commit 6f8ba8c0, fixing RT #49456 / issue #147) added reference-counting so that when the last Grammar object was destroyed, $factory would be undef'd. The reasoning: "prevent a memory leak from the factory singleton."

But $factory holds one reference to one object that already exists on the Parser. It's not a cache that grows. It's not accumulating entries. It's a single pointer to an object that's alive anyway (held by $self->{FACTORY} on the Parser). Setting $factory = undef only drops the Grammar's reference — the Directive object lives on through the Parser.

Why the reference-counting itself was harmful

The DESTROY + register/unregister machinery caused Test::LeakTrace failures because:

1. no_leaks_ok { my $tt = Template->new(); $tt->process(...) } creates a Grammar, which registers with $factory_usages
2. When $tt goes out of scope, Grammar's DESTROY fires, calling unregister_factory
3. If it's the last user, undef $factory clears the singleton
4. Next iteration: a new Template creates a new Parser, which calls install_factory again, re-populating $factory — but now the Grammar itself allocates new tracking structures ($factory_usages hash, stringified keys)
5. This allocate-on-create / free-on-destroy cycle creates transient SVs that Test::LeakTrace correctly flags as leaks

The irony: the code added to prevent a leak was causing one. The "leak" it was guarding against (a persistent $factory reference) was never really a leak — it's a fixed-cost singleton pointer to an object that exists elsewhere.

Doesn't removing this blind us to future leakage?

No, for two reasons:

1. $factory can't grow. It's a scalar holding one reference. It gets overwritten (not appended to) on each install_factory() call. There's no mechanism for unbounded accumulation. A "leak" means memory that grows without bound — a fixed-size singleton that persists is just... a singleton.

2. The leak tests still exist. t/zz-process-leak.t and t/zz-grammar-factory-leak.t still run full Template processing cycles inside no_leaks_ok blocks. If any code change introduces a real leak (growing allocations per cycle), these tests will catch it. What they no longer catch is the factory reference itself persisting — because that was never a problem.

If you wanted a belt-and-suspenders check that $factory doesn't somehow start accumulating (e.g., someone changes it to an arrayref), a simpler guard would be an assertion in install_factory rather than DESTROY-time reference counting. But given it's a plain scalar assignment, that feels like guarding against $x = 1 somehow becoming push @x, 1.

Does it break existing behavior?

Observable behavior: no change. The factory is still installed, still used by grammar closures during parsing, still produces identical compiled output. The only difference: after the last Grammar object is destroyed, $factory retains its value instead of being undef'd. Since:

• No code reads $factory outside of parse-time closures
• The next install_factory call overwrites it regardless
• The Directive object it points to is alive on the Parser anyway

...the retained reference is harmless. It points to an object that hasn't been freed, and it'll be overwritten on next use.

DESTROY behavior change: Grammar objects no longer have a DESTROY method. This means Perl's garbage collector handles them as plain blessed hashrefs — faster destruction, no side effects during global destruction (which is where DESTROY methods are most fragile and where the defined $factory guard was needed).