Eliminate double dispatch with parent FCC (-47% call time)

[koriym]

Summary

Eliminates double-dispatch bottleneck: replaces _intercept() indirection with direct parent::method(...) FCC in generated proxy code. Removes _isAspect recursion guard, call_user_func_array, and one full trip through the proxy per intercepted call.

Why this matters

This is the deepest fixed cost of Ray.Aop: every intercepted call pays it, and nothing above the library can amortize it away. The win is not end-user “feel” of a single request — it is unit cost of the dispatch layer.

• Overhead ratio ~20× → ~10×: the structural tax of interception is halved at the point where it is always charged.
• Correctness on the same path: nested interception via getThis() no longer corrupts _isAspect into infinite recursion; $1-style attribute args are no longer eaten by preg_replace backreferences.
• Microseconds per call compound as calls × apps × time. Judging this layer by UI snappiness is the wrong metric; fixed-cost and safety are the right ones.

Benchmark

Metric	Before	After	Change
Intercepted call	2.99 µs	1.58 µs	-47%
Overhead ratio	19.7x	9.9x	-50%

Scripts are not kept on this branch (removed as one-off measurement tools). To re-run if needed, check out the commit that still has them and run php benchmarks/bench.php:

• tree: https://github.com/koriym/Ray.Aop/tree/fb405ca088bbaae90d01b5011aa26ee942db79ac/benchmarks
• bench.php: https://github.com/koriym/Ray.Aop/blob/fb405ca088bbaae90d01b5011aa26ee942db79ac/benchmarks/bench.php

git checkout fb405ca088bbaae90d01b5011aa26ee942db79ac
composer install
php benchmarks/bench.php

Numbers depend on machine / PHP build; treat as relative before/after on the same host. Absolute µs are less important than the overhead-ratio change above.

Key Changes

• AopCode: generated methods now inline parent::METHOD(...) FCC
• ReflectiveMethodInvocation: lazy ArrayObject, FCC spread, optional $parentCall (BC-safe)
• InterceptTrait: removed _isAspect flag and _intercept() (28→20 lines)
• ReadOnlyInterceptTrait: same removal (26→20 lines)
• AopPostfixClassName: hash includes AopCode::GENERATION for cache invalidation
• insert(): preg_replace → strrpos+substr_replace (fixes backreference bug)
• addMethods(): in_array O(n*m) → isset O(1)

Tests

PHPUnit green. Added reentrant interceptor and readonly class interception tests, plus $1 attribute codegen regression.

Review follow-up

Addressed review feedback: Psalm/PHPStan on ReflectiveMethodInvocation, codecov ignore for unreachable insert() guard, gitignore for generated tmp proxies, CHANGELOG Unreleased, regression tests, mutation escapes on hot paths.

Note

CodeRabbit skipped while this PR was draft; re-run after ready-for-review.

[coderabbitai]

Warning

Review limit reached

@koriym, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 91628d2c-7e22-4bc5-a1a5-1b1829752b46

📥 Commits

Reviewing files that changed from the base of the PR and between 3ec09a8 and fd3ddb7.

📒 Files selected for processing (6)

• src/AopCode.php
• src/AopPostfixClassName.php
• src/InterceptTrait.php
• src/MethodSignatureString.php
• tests/AopCodeTest.php
• tests/AopPostfixClassNameTest.php

📝 Walkthrough

Walkthrough

Generated proxies now dispatch through ReflectiveMethodInvocation, including readonly classes, with generation-based identities and simplified interception traits. Compiler binding checks, argument handling, code generation, and regression tests were updated accordingly.

Changes

Proxy weaving and interception

Layer / File(s)	Summary
Generated interception dispatch src/AopCode.php, src/ReflectiveMethodInvocation.php, src/InterceptTrait.php, src/ReadOnlyInterceptTrait.php, src/InterceptTraitState.php, src/MethodSignatureString.php	Generated methods use normal or readonly proceed templates, direct callable invocation, lazy argument objects, and simplified interception traits.
Weaving lifecycle and proxy identity src/Compiler.php, src/AopPostfixClassName.php, src/Weaver.php, .gitignore, CHANGELOG.md, composer.json	Unmatched bindings skip weaving, generated class hashes include AopCode::GENERATION, and repository metadata records the updated implementation.
Dispatch and code-generation validation tests/AopCodeTest.php, tests/CompilerTest.php, tests/AopPostfixClassNameTest.php, tests/Fake/*, tests/script/*, tests/tmp_unerase/*	Tests cover proceed generation, readonly interception, deterministic proxy names, literal $1 handling, empty bindings, and reentrant interception; obsolete generated fixtures are removed.

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

Sequence Diagram(s)

sequenceDiagram
  participant Compiler
  participant AopCode
  participant ReflectiveMethodInvocation
  Compiler->>AopCode: generate intercepted method body
  AopCode->>ReflectiveMethodInvocation: construct invocation with arguments and parentCall
  ReflectiveMethodInvocation->>ReflectiveMethodInvocation: proceed through the bound callable

Loading

Possibly related PRs

• ray-di/Ray.Aop#227: Both modify MethodSignatureString and generated method signature handling.
• ray-di/Ray.Aop#236: Both modify readonly-class interception plumbing.
• ray-di/Ray.Aop#238: Both refactor readonly interception traits and bindings initialization.

Suggested reviewers: ngmy

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: replacing double dispatch with parent first-class callables for faster interception.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (4d9f809) to head (fd3ddb7).

Additional details and impacted files

@@             Coverage Diff             @@
##                 2.x      #260   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
+ Complexity       228       227    -1     
===========================================
  Files             29        29           
  Lines            584       593    +9     
===========================================
+ Hits             584       593    +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[koriym]

@koriym Full review below. The Required / Recommended sections are written as self-contained instructions for the implementing agent — they can be executed without re-deriving the analysis.

Verdict

The architecture is sound. Removing _isAspect + the second trip through the proxy is not only faster — it fixes a real bug. Verified empirically on 2.x base: an interceptor that calls another intercepted method on the same instance via $invocation->getThis() hits infinite recursion (the _isAspect flag gets flipped back by the nested call, so the outer proceed() re-enters interception forever). testReentrantInterceptorCrossMethodCall is therefore a genuine regression test: it hangs on base, passes on this branch.

Verified during review — no action needed on these:

• Cache invalidation is complete. Both Weaver::weave() and Compiler::compile() derive file/class names from AopPostfixClassName, so the GENERATION bump invalidates every cached-proxy path. Stale files from older versions are simply never loaded.
• The enum change in MethodSignatureString is correct and inseparable from the insert() change. The old preg_replace treated the inserted code as a replacement string and unescaped \\→\; the '\\' . var_export(...) prefix existed only to compensate. With substr_replace nothing unescapes, so the prefix had to go. testVariousMethodSignaturesInPhp81 requires the generated file, so a double backslash would parse-error there — covered.
• Argument mutation still works. getArguments() → lazy ArrayObject → getArrayCopy() spread in proceed() is covered end-to-end by testInterceptorCanModifyArguments.
• By-ref parameters: no regression. Both old and new dispatch pass by value (func_get_args() loses references either way); the FCC spread just no longer emits the old must be passed by reference, value given warning that call_user_func_array produced.
• readonly path is valid. Generated $this->_state->bindings[...] accesses a trait-owned private property from the using class — legal — and removing $isAspect makes InterceptTraitState fully immutable, which is what a readonly class wants.

Required — CI is currently red

1. Psalm: 4 errors in src/ReflectiveMethodInvocation.php

MixedPropertyTypeCoercion: $this->callable expects 'callable(mixed...):mixed', parent type Closure|(list{T:...as object, non-empty-string}) provided
InvalidReturnType / InvalidReturnStatement: getArguments() 'ArrayObject<int, mixed>' vs 'ArrayObject<int<0, max>, mixed>'
PossiblyInvalidPropertyAssignmentValue: $this->argumentsObject

Fix the constructor by branching (this also restores the runtime assert(is_callable(...)) that this PR silently dropped), and give $parentCall a full callable signature in the docblock:

/**
 * ...
 * @param (Closure(mixed...): mixed)|null $parentCall Direct parent-method closure (avoids double-dispatch through proxy)
 */
public function __construct(...)
{
    if ($parentCall !== null) {
        $this->callable = $parentCall;
    } else {
        $callable = [$this->object, $this->method];
        assert(is_callable($callable));
        $this->callable = $callable;
    }

    $this->arguments = $arguments;
}

Fix getArguments() with an inline @var (resolves all three variance errors); type the property docblock as @var ArgumentList|null:

public function getArguments(): ArrayObject
{
    if ($this->argumentsObject === null) {
        /** @var ArgumentList $argumentsObject */
        $argumentsObject = new ArrayObject($this->arguments);
        $this->argumentsObject = $argumentsObject;
    }

    return $this->argumentsObject;
}

⚠️ After restructuring, the // @phpstan-ignore assign.propertyType comment on the old one-liner may become unmatched — PHPStan fails on unmatched ignores. Run composer sa and delete any ignore it reports as unused. The two @phpstan-ignore callable.nonCallable in proceed() should stay. Acceptance: composer sa exits 0.

While there: private array $arguments; is assigned only in the constructor — make it private readonly array $arguments;.

2. Codecov patch: 1 uncovered line in src/AopCode.php

The insert() guard if ($lastBrace === false) { return; } is defensively unreachable (by the time insert() runs, resolveInterceptTrait() has always emitted a }). Mark it:

if ($lastBrace === false) {
    return; // @codeCoverageIgnore
}

Project precedent: Weaver.php:46, Compiler.php. Acceptance: codecov patch = 100%.

Recommended

3. Remove machine-specific build artifacts from git

The proxy postfix is crc32(filemtime . bindings . classDir) — these files can never match on another machine, and bench.php deletes tracked files at startup, dirtying the tree:

• Delete benchmarks/tmp/Ray_Aop_Benchmark_BenchService_1152959859.php; add benchmarks/tmp/.gitkeep instead.
• Delete tests/tmp_unerase/Ray_Aop_FakeWeaverMock_4090727845.php (keep the .gitkeep this PR already adds).
• Add to .gitignore: /benchmarks/tmp/*.php and /tests/tmp_unerase/*.php.

Note: the directories must continue to exist — Compiler::__construct throws NotWritableException if the dir is missing (bench.php and testWeaveLoadsCompiledAopFile both depend on this). Hence .gitkeep, not ignoring the whole directory.

4. CHANGELOG.md — add Unreleased entries

• Changed: proxy dispatch is now a direct parent::method(...) first-class callable (no double dispatch through the proxy); intercepted-call overhead roughly halved. Generated proxies are invalidated via AopCode::GENERATION — existing script dirs regenerate once after upgrade (stale files are ignored, not loaded).
• Fixed: infinite recursion when an interceptor invoked another intercepted method on the same instance via getThis(). Such nested calls are now intercepted normally (previously interception state was corrupted). Codegen no longer corrupts generated method code containing $1-style sequences (preg_replace backreference bug in AopCode::insert()).
• Removed (internal API): AopCode::INTERCEPT_STATEMENT public const, _intercept(), _isAspect; AopCode::resolveInterceptTrait() is now private.

5. Add a regression test for the backreference fix

The PR claims the preg_replace backreference fix, but nothing pins it — on base, a $1 inside an attribute argument is silently stripped (no parse error, wrong code), so the existing eval test can't catch it.

• In tests/Fake/FakePhp8Types.php, add method26 annotated with #[FakeMarker4(['a$1b'], 1)] (FakeMarker4 already accepts array $a, int $b).
• In testVariousMethodSignaturesInPhp81, extend the bind loop from 25 to 26 so the generated file (which is required) includes it.
• Add an assertion (there or in a dedicated test): assertStringContainsString('a$1b', $code). This fails on base ($1 becomes empty → ab), passes on this branch.

6. Fix a now-vacuous assertion

tests/AopCodeTest.php:231 (testEmptyBindingsDoesNotAddMethods) still asserts absence of _intercept(__FUNCTION__ — trivially true now that nothing ever generates it. Replace with assertStringNotContainsString('ReflectiveMethodInvocation', $code).

7. Housekeeping

• Update the PR description: it says 16 files, +141/−196; the PR is actually 20 files, +415/−166 (benchmarks were added after the body was written). It also says "CodeRabbit review completed", but CodeRabbit skipped this PR (draft) — once CI is green, mark ready for review so it actually runs.
• Update the branch with latest 2.x (fix: re-emit weaved class file when declared in-process but missing on disk #261 merged after this branch was cut; Compiler.php isn't touched here so no conflict expected, but CI should run against the combined state).

Nits (optional)

• benchmarks/bench.php: the comment says "100K iterations" but $iterations = 5_000.
• Generated method bodies currently end with return $__aop->proceed(); } — closing brace glued to the statement with trailing spaces. Appending \n after proceed(); in the templates (or in the enclosing sprintf in addMethods()) makes generated files easier to read when debugging.
• benchmarks/micro_bench.php: imports Ray\Aop\ReflectiveMethodInvocation but then uses the FQCN inline; a few lines have trailing whitespace. Benchmarks are outside phpcs scope (composer cs covers src tests only), so cosmetic.

Overall acceptance: composer tests (cs + phpunit + sa) green locally, codecov patch/project green, full CI matrix green, then un-draft.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/Compiler.php (1)

115-128: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider early-return in hasBoundMethod for efficiency.

The loop sets $hasMethod = true but continues iterating all remaining bindings unnecessarily. An early return true on first match would be more efficient, especially with many bindings.

♻️ Proposed refactor

     foreach ($bindingMethods as $bindingMethod) {
         if (! method_exists($class, $bindingMethod)) {
             continue;
         }

-        $hasMethod = true;
+        return true;
     }

-    return $hasMethod;
+    return false;

🤖 Prompt for 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.

In `@src/Compiler.php` around lines 115 - 128, Update hasBoundMethod to return
true immediately when method_exists finds the first matching binding method, and
remove the now-unnecessary mutable $hasMethod state; retain the final false
result when no binding methods exist on the class.

🤖 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/ReflectiveMethodInvocation.php`:
- Around line 56-63: Update manual construction of ReflectiveMethodInvocation,
especially in FakeWeaved, to pass the appropriate parent::method(...) callable
as $parentCall. Ensure the null-$parentCall fallback in
ReflectiveMethodInvocation is not used by these callers, preventing re-entry
into overridden methods while preserving generated proxy behavior.

---

Nitpick comments:
In `@src/Compiler.php`:
- Around line 115-128: Update hasBoundMethod to return true immediately when
method_exists finds the first matching binding method, and remove the
now-unnecessary mutable $hasMethod state; retain the final false result when no
binding methods exist on the class.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: a418aeab-41dc-4cdb-a255-20763a944e30

📥 Commits

Reviewing files that changed from the base of the PR and between 4d9f809 and 3ec09a8.

📒 Files selected for processing (21)

• .gitignore
• CHANGELOG.md
• composer.json
• src/AopCode.php
• src/AopPostfixClassName.php
• src/Compiler.php
• src/InterceptTrait.php
• src/InterceptTraitState.php
• src/MethodSignatureString.php
• src/ReadOnlyInterceptTrait.php
• src/ReflectiveMethodInvocation.php
• src/Weaver.php
• tests/AopCodeTest.php
• tests/AopPostfixClassNameTest.php
• tests/CompilerTest.php
• tests/Fake/FakePhp82ReadOnlyClass.php
• tests/Fake/FakePhp8Types.php
• tests/Fake/FakeReentrantInterceptor.php
• tests/script/method_stmts_gen.php
• tests/tmp_unerase/.gitkeep
• tests/tmp_unerase/Ray_Aop_FakeWeaverMock_1899453090.php

💤 Files with no reviewable changes (4)

• tests/tmp_unerase/Ray_Aop_FakeWeaverMock_1899453090.php
• src/InterceptTraitState.php
• tests/script/method_stmts_gen.php
• src/ReadOnlyInterceptTrait.php