Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php

declare(strict_types=1);

namespace Rector\Tests\TypeDeclaration\Rector\ClassMethod\NarrowObjectReturnTypeRector;

use Iterator;
use PHPUnit\Framework\Attributes\DataProvider;
use Rector\Testing\PHPUnit\AbstractRectorTestCase;

final class DisableReturnAbstractOrInterfaceTest extends AbstractRectorTestCase
{
#[DataProvider('provideData')]
public function test(string $filePath): void
{
$this->doTestFile($filePath);
}

public static function provideData(): Iterator
{
return self::yieldFilesFromDirectory(__DIR__ . '/FixtureSkipReturnAbstractOrInterface');
}

public function provideConfigFilePath(): string
{
return __DIR__ . '/config/disable_return_abstract_or_interface_configured_rule.php';
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

namespace Rector\Tests\TypeDeclaration\Rector\ClassMethod\NarrowObjectReturnTypeRector\FixtureSkipReturnAbstractOrInterface;

use Rector\Tests\TypeDeclaration\Rector\ClassMethod\NarrowObjectReturnTypeRector\Source\AbstractTalk;
use Rector\Tests\TypeDeclaration\Rector\ClassMethod\NarrowObjectReturnTypeRector\Source\ConcreteConferenceTalk;

final class SkipReturnAbstractClass
{
public function create(): AbstractTalk
{
return new ConcreteConferenceTalk();
}
}

?>
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

namespace Rector\Tests\TypeDeclaration\Rector\ClassMethod\NarrowObjectReturnTypeRector\FixtureSkipReturnAbstractOrInterface;

use Illuminate\Container\Container;
use Psr\Container\ContainerInterface;

final class SkipReturnInterface
{
public function create(): ContainerInterface
{
return new Container();
}
}

?>
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

declare(strict_types=1);

use Rector\Config\RectorConfig;
use Rector\TypeDeclaration\Rector\ClassMethod\NarrowObjectReturnTypeRector;

return static function (RectorConfig $rectorConfig): void {
$rectorConfig->ruleWithConfiguration(NarrowObjectReturnTypeRector::class, [
NarrowObjectReturnTypeRector::IS_ALLOW_ABSTRACT_AND_INTERFACE => false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

I just find this option name confusing. Could you name it INCLUDE_STANDALONE_CLASS?

Copy link
Member Author

@samsonasik samsonasik Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means the value need to be true, default false, but "include" term means "also", which a bit contradictive imo.

For better word, I think it can be ALLOW_CONTRACT_RETURN

It is about adding return type which return type is contract.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include standalone class should be false by default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is nothing todo with standalone class or not, it is about returning abstract or interface as return type, whether class extends another class or not, if the method return interface, this config will skip.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I've checked the original report again.

In that case we don't need any configuration. That case should be skipped, as it would create breaking code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we had discussion about this on original PR with @Orest-Divintari and @ruudk , the use of replace interface/abstract class to concrete class is valid,

But as @calebdw and @WyriHaximus pointed, there are use case which keep interface/abstract as return type make sense, usually on unit test purpose which still use mock object everywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but how do you make a boundary which interface should be replaced and which not? This rule replace e.g. all getRule() in phsptan tests with exact return type of the rule. It doesn't add any value, as parent return type is generic on purpose. It's part of contract.

Once we start adding such configuration, this rule turns into manual labour I want to avoid. This rule should only replace one object type with more specific one. Not all interface types with exact returns, which sparks last 2 reports.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into this tomorrow. Closing to avoid further configuration on this rule.

Copy link
Member Author

@samsonasik samsonasik Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Common interface type to be skipped:

  • Interface return with Psr\\ prefix
  • Interface return with Interop\\ prefix

Common interface type should be allowed to be replaced:

  • Doctrine\Common\Collections\Collection
  • PhpParser\Node

This is what I proposed in old PR 3 weeks ago

and closed.

]);
};
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ public function refactor(Node $node): array|Expr|null|int
if (! $node->var instanceof ArrayDimFetch) {
return null;
}

return $this->createExplicitMethodCall($node->var, 'set', $node->expr);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,30 @@
use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfoFactory;
use Rector\BetterPhpDocParser\ValueObject\Type\FullyQualifiedIdentifierTypeNode;
use Rector\Comments\NodeDocBlock\DocBlockUpdater;
use Rector\Contract\Rector\ConfigurableRectorInterface;
use Rector\NodeTypeResolver\TypeComparator\TypeComparator;
use Rector\PhpParser\AstResolver;
use Rector\PhpParser\Node\BetterNodeFinder;
use Rector\Rector\AbstractRector;
use Rector\Reflection\ReflectionResolver;
use Rector\StaticTypeMapper\StaticTypeMapper;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\ConfiguredCodeSample;
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;

/**
* Narrows return type from generic object or parent class to specific class in final classes/methods.
*
* @see \Rector\Tests\TypeDeclaration\Rector\ClassMethod\NarrowObjectReturnTypeRector\NarrowObjectReturnTypeRectorTest
*/
final class NarrowObjectReturnTypeRector extends AbstractRector
final class NarrowObjectReturnTypeRector extends AbstractRector implements ConfigurableRectorInterface
{
/**
* @var string
*/
public const IS_ALLOW_ABSTRACT_AND_INTERFACE = 'is_allow_abstract_and_interface';

private bool $isAllowAbstractAndInterface = true;

public function __construct(
private readonly BetterNodeFinder $betterNodeFinder,
private readonly ReflectionResolver $reflectionResolver,
Expand All @@ -50,7 +58,7 @@ public function getRuleDefinition(): RuleDefinition
return new RuleDefinition(
'Narrows return type from generic object or parent class to specific class in final classes/methods',
[
new CodeSample(
new ConfiguredCodeSample(
<<<'CODE_SAMPLE'
final class TalkFactory extends AbstractFactory
{
Expand All @@ -70,8 +78,12 @@ protected function build(): ConferenceTalk
}
}
CODE_SAMPLE
,
[
self::IS_ALLOW_ABSTRACT_AND_INTERFACE => true,
]
),
new CodeSample(
new ConfiguredCodeSample(
<<<'CODE_SAMPLE'
final class TalkFactory
{
Expand All @@ -91,6 +103,10 @@ public function createConferenceTalk(): ConferenceTalk
}
}
CODE_SAMPLE
,
[
self::IS_ALLOW_ABSTRACT_AND_INTERFACE => true,
]
),
],
);
Expand Down Expand Up @@ -120,6 +136,10 @@ public function refactor(Node $node): ?Node
return null;
}

if (! $this->isAllowedReturnsAbstractOrInterface($returnType->toString())) {
return null;
}

if (! $classReflection->isFinalByKeyword() && ! $node->isFinal()) {
return null;
}
Expand Down Expand Up @@ -159,6 +179,34 @@ public function refactor(Node $node): ?Node
return $node;
}

/**
* @param array<string, bool> $configuration
*/
public function configure(array $configuration): void
{
$this->isAllowAbstractAndInterface = $configuration[self::IS_ALLOW_ABSTRACT_AND_INTERFACE] ?? true;
}

private function isAllowedReturnsAbstractOrInterface(string $actualReturnClass): bool
{
if ($this->isAllowAbstractAndInterface) {
return true;
}

$actualObjectType = new ObjectType($actualReturnClass);
$classReflection = $actualObjectType->getClassReflection();

if (! $classReflection instanceof ClassReflection) {
return false;
}

if ($classReflection->isInterface()) {
return false;
}

return ! $classReflection->isAbstract();
}

private function updateDocblock(ClassMethod $classMethod, string $actualReturnClass): void
{
$phpDocInfo = $this->phpDocInfoFactory->createFromNode($classMethod);
Expand Down