Bug Report
| Subject |
Details |
| Rector version |
v2.6.4 |
DirectInstanceOverMockArgRector calls ScopeFetcher::fetch($node) as the first
statement of refactor(), before any guard runs:
public function refactor(Node $node)
{
$scope = ScopeFetcher::fetch($node); // <- throws here
if (!$scope->isInClass()) {
return null;
}
$classReflection = $scope->getClassReflection();
if (!$classReflection->is(PHPUnitClassName::TEST_CASE)) {
return null;
}
...PHPStan assigns no scope to the arguments of a new expression that mixes an
unpacked spread with a named argument filling a required constructor
parameter — it cannot reorder the arguments, so it never descends into them.
The rule then throws:
System error: "Scope not available on "PhpParser\Node\Expr\MethodCall" node.
Fix scope refresh on changed nodes first"
Two things make this painful:
- The file does not have to be a test. The
TestCase check sits after the
scope fetch, so the crash happens in ordinary application code.
- The rule ships in the
phpunit-code-quality prepared set, so
->withPreparedSets(phpunitCodeQuality: true) makes any project with this
argument shape fail its whole Rector run.
The rule only rewrites mocks of Symfony's Request / RequestStack, so it had
nothing to do with the code it crashed on.
Minimal PHP Code Causing Issue
Reproduced locally with only DirectInstanceOverMockArgRector enabled:
<?php
final class SomeData
{
public function __construct(
public string $user,
public ?bool $flag = null,
) {}
}
final class SomeController
{
public function go($request)
{
$data = new SomeData(
...$request->updateData(),
user: (string) $request->user()->getKey(),
);
}
}Config:
return RectorConfig::configure()
->withPaths([__DIR__])
->withRules([DirectInstanceOverMockArgRector::class]);The scope is missing on the $request->user() method call inside the (string)
cast, in the named argument that follows the spread.
Both of these variants pass — the required parameter and the spread are each
necessary to trigger it:
// gives $user a default -> OK
final class SomeData
{
public function __construct(
public string $user = '',
public ?bool $flag = null,
) {}
}
// drops the spread -> OK
$data = new SomeData(
user: (string) $request->user()->getKey(),
);Expected Behaviour
Rector should skip the node instead of crashing.
Two changes would help:
- Move the scope fetch after the cheap guards, or make it tolerate a missing
scope. The rule needs the scope only to confirm the class is a TestCase;
with no scope it can return null.
- More generally,
ScopeFetcher::fetch() throwing on nodes PHPStan never
visited turns a PHPStan limitation into a hard failure of the whole run.
A nullable ScopeFetcher::fetchOrNull() for rules that can degrade
gracefully would stop one rule from taking down a full-tree run.
Bug Report
DirectInstanceOverMockArgRectorcallsScopeFetcher::fetch($node)as the firststatement of
refactor(), before any guard runs:PHPStan assigns no scope to the arguments of a
newexpression that mixes anunpacked spread with a named argument filling a required constructor
parameter — it cannot reorder the arguments, so it never descends into them.
The rule then throws:
Two things make this painful:
TestCasecheck sits after thescope fetch, so the crash happens in ordinary application code.
phpunit-code-qualityprepared set, so->withPreparedSets(phpunitCodeQuality: true)makes any project with thisargument shape fail its whole Rector run.
The rule only rewrites mocks of Symfony's
Request/RequestStack, so it hadnothing to do with the code it crashed on.
Minimal PHP Code Causing Issue
Reproduced locally with only
DirectInstanceOverMockArgRectorenabled:Config:
The scope is missing on the
$request->user()method call inside the(string)cast, in the named argument that follows the spread.
Both of these variants pass — the required parameter and the spread are each
necessary to trigger it:
Expected Behaviour
Rector should skip the node instead of crashing.
Two changes would help:
scope. The rule needs the scope only to confirm the class is a
TestCase;with no scope it can return
null.ScopeFetcher::fetch()throwing on nodes PHPStan nevervisited turns a PHPStan limitation into a hard failure of the whole run.
A nullable
ScopeFetcher::fetchOrNull()for rules that can degradegracefully would stop one rule from taking down a full-tree run.