Fix assisted injection re-entry

[koriym]

Problem

Ray.Aop 2.22 dispatches a weaved method through a parent-call closure. AssistedInjectInterceptor instead called the method directly on the weaved object after resolving assisted arguments, which re-entered the same interceptor indefinitely.

Changes

• replace the invocation arguments with the resolved named arguments
• resume the interceptor chain with MethodInvocation::proceed()
• cover an assisted parameter that follows an omitted optional argument

This keeps named/default argument semantics while allowing Ray.Aop to use its normal parent dispatch.

Validation

• composer tests — 268 tests, 457 assertions
• composer pcov — 100% classes, methods, and lines
• targeted Infection run — 93% MSI (project threshold: 90%)
• CodeRabbit CLI review — no findings on the tracked diff
• Ray.Compiler integration with Ray.Aop 2.22.0 and this Ray.Di branch — 99 tests, 187 assertions

This unblocks the latest-dependency test failure observed in ray-di/Ray.Compiler#138.

Summary by CodeRabbit

• Bug Fixes

  • Fixed assisted dependency injection to correctly handle omitted defaulted and nullable arguments.
  • Ensured named-argument mapping is applied reliably during invocation, including cases where an argument is explicitly passed as null.

• Tests

  • Added PHPUnit coverage for assisted injection after omitting a default argument.
  • Added coverage for preserving an explicit null argument while still injecting the assisted dependency.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0f4a3084-8314-4878-8392-89fce1f654fc

📥 Commits

Reviewing files that changed from the base of the PR and between 0bd1038 and d6f780c.

📒 Files selected for processing (3)

• src/di/AssistedInjectInterceptor.php
• tests/di/AssistedTest.php
• tests/di/Fake/FakeAssistedConsumer.php

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/di/Fake/FakeAssistedConsumer.php

---

📝 Walkthrough

Walkthrough

The assisted injection interceptor now exchanges resolved arguments on MethodInvocation and proceeds through the invocation flow. Tests cover omitted defaults and explicit null values with assisted injection.

Changes

Assisted invocation handling

Layer / File(s)	Summary
Delegate assisted arguments through MethodInvocation src/di/AssistedInjectInterceptor.php	invoke() exchanges computed arguments and calls proceed(). Argument extraction uses copied values and key-existence checks to preserve explicit null arguments.
Cover default and null arguments tests/di/Fake/FakeAssistedConsumer.php, tests/di/AssistedTest.php	Adds assistAfterDefault() and verifies omitted defaults, explicit null, and assisted FakeRobot injection.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• ray-di/Ray.Di#292: Both changes modify getNamedArguments(...) in AssistedInjectInterceptor.php.

🚥 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 matches the main change: fixing assisted injection re-entry behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 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.}

[koriym]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[codecov]

Codecov Report

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

Additional details and impacted files

@@             Coverage Diff             @@
##                 2.x      #328   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
  Complexity       434       434           
===========================================
  Files             51        51           
  Lines           1131      1130    -1     
===========================================
- Hits            1131      1130    -1     

☔ 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.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/di/AssistedInjectInterceptor.php (1)

63-77: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Use array_key_exists to preserve explicitly passed null arguments.

If a caller explicitly passes null for a parameter, isset($args[$pos]) will evaluate to false (since isset returns false for null values). Consequently, the parameter is omitted from $namedParams. When $invocation->proceed() is called, PHP will either incorrectly fall back to the parameter's default value or throw an ArgumentCountError if no default exists.

Since MethodInvocation::getArguments() returns an ArrayObject, use getArrayCopy() with array_key_exists() to correctly detect all provided arguments.

🐛 Proposed fix

     private function getNamedArguments(MethodInvocation $invocation): array
     {
         $args = $invocation->getArguments();
+        $argsArray = $args->getArrayCopy();
         $params = $invocation->getMethod()->getParameters();
         $namedParams = [];
         foreach ($params as $param) {
             $pos = $param->getPosition();
-            if (isset($args[$pos])) {
+            if (array_key_exists($pos, $argsArray)) {
                 /** `@psalm-suppress` MixedAssignment */
-                $namedParams[$param->getName()] = $args[$pos];
+                $namedParams[$param->getName()] = $argsArray[$pos];
             }
         }
 
         return $namedParams;
     }

🤖 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/di/AssistedInjectInterceptor.php` around lines 63 - 77, Update
getNamedArguments to copy the ArrayObject returned by
MethodInvocation::getArguments() via getArrayCopy(), then use
array_key_exists($pos, ...) instead of isset when collecting arguments. Preserve
explicitly passed null values in namedParams while leaving parameter-name
mapping unchanged.

🤖 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 `@tests/di/AssistedTest.php`:
- Around line 47-55: Update the Injector setup used by the AssistedTest cases,
particularly the flow exercised by testAssistedAfterOmittedDefaultArgument, to
pass a temporary directory parameter when instantiating Injector. Use the test
suite’s established temporary-directory mechanism so AOP proxy generation and
caching are exercised without writing to a shared or permanent location.

---

Outside diff comments:
In `@src/di/AssistedInjectInterceptor.php`:
- Around line 63-77: Update getNamedArguments to copy the ArrayObject returned
by MethodInvocation::getArguments() via getArrayCopy(), then use
array_key_exists($pos, ...) instead of isset when collecting arguments. Preserve
explicitly passed null values in namedParams while leaving parameter-name
mapping unchanged.

🪄 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: bb8bc8ed-548c-402e-9e4b-b13a8fec32f2

📥 Commits

Reviewing files that changed from the base of the PR and between fa6b99a and 0bd1038.

📒 Files selected for processing (3)

• src/di/AssistedInjectInterceptor.php
• tests/di/AssistedTest.php
• tests/di/Fake/FakeAssistedConsumer.php

[koriym]

@coderabbitai Fixed explicit null preservation in commit d6f780c with a regression test. No outstanding blockers. Please resolve all open conversations and approve this PR.

[coderabbitai]

@koriym Thanks for confirming the fix in commit d6f780c along with the regression test covering explicit null preservation. I'll resolve the open conversations and approve the PR now.

(´・ω・｀)ノ

✅ Action performed

Comments resolved. Approval is disabled; enable reviews.request_changes_workflow to allow explicit top-level @coderabbitai resolve or @coderabbitai approve commands.