Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DeadCode] Skip @template tag on RemoveUselessVarTagRector #6396

Merged
merged 8 commits into from
Oct 26, 2024
Merged

[DeadCode] Skip @template tag on RemoveUselessVarTagRector #6396

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
130c2e9
[DeadCode] Skip @template tag on RemoveUselessVarTagRector
samsonasik Oct 24, 2024
e3f340b
add fixutres
samsonasik Oct 24, 2024
02b566a
Fix by get @template from ClassReflection
samsonasik Oct 26, 2024
6864c5f
Fix
samsonasik Oct 26, 2024
cf3ca19
ensure scope exists
samsonasik Oct 26, 2024
0ca294c
cs fix
samsonasik Oct 26, 2024
9d019dc
Fix
samsonasik Oct 26, 2024
d61c756
[ci-review] Rector Rectify
actions-user Oct 26, 2024
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
26 changes: 26 additions & 0 deletions ...ts/DeadCode/Rector/Property/RemoveUselessVarTagRector/Fixture/non_existing_object.php.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

namespace Rector\Tests\DeadCode\Rector\Property\RemoveUselessVarTagRector\Fixture;

use Rector\Tests\DeadCode\Rector\Property\RemoveUselessVarTagRector\Source\Properties;

final class NonExistingObject
{
/** @var SomeNonExistingObject */
private Properties|null $properties;
}

?>
-----
<?php

namespace Rector\Tests\DeadCode\Rector\Property\RemoveUselessVarTagRector\Fixture;

use Rector\Tests\DeadCode\Rector\Property\RemoveUselessVarTagRector\Source\Properties;

final class NonExistingObject
{
private Properties|null $properties;
}

?>
14 changes: 14 additions & 0 deletions ...ests/DeadCode/Rector/Property/RemoveUselessVarTagRector/Fixture/skip_template_tag.php.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php

namespace Rector\Tests\DeadCode\Rector\Property\RemoveUselessVarTagRector\Fixture;

use Rector\Tests\DeadCode\Rector\Property\RemoveUselessVarTagRector\Source\Properties;

/**
* @template TProperties of Properties|null
*/
final class SkipTemplateTag
{
/** @var TProperties */
private Properties|null $properties;
}
5 changes: 5 additions & 0 deletions rules-tests/DeadCode/Rector/Property/RemoveUselessVarTagRector/Source/Properties.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?php

namespace Rector\Tests\DeadCode\Rector\Property\RemoveUselessVarTagRector\Source;

interface Properties{}
7 changes: 7 additions & 0 deletions rules/DeadCode/PhpDoc/DeadVarTagValueNodeAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use PHPStan\Type\UnionType;
use Rector\NodeTypeResolver\TypeComparator\TypeComparator;
use Rector\StaticTypeMapper\StaticTypeMapper;
use Rector\StaticTypeMapper\ValueObject\Type\NonExistingObjectType;

final readonly class DeadVarTagValueNodeAnalyzer
{
Expand All @@ -35,6 +36,12 @@ public function isDead(VarTagValueNode $varTagValueNode, Property $property): bo
$propertyType = $this->staticTypeMapper->mapPhpParserNodePHPStanType($property->type);
$docType = $this->staticTypeMapper->mapPHPStanPhpDocTypeNodeToPHPStanType($varTagValueNode->type, $property);

// NonExistingObjectType may refer to @template tag defined in class
if ($docType instanceof NonExistingObjectType && ! str_contains($docType->getClassName(), '\\')) {
dump($docType);
return false;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@dragosprotung this can be tweak solution, but it will cause invalid skipped real non existing object type, see fixture /~https://github.com/rectorphp/rector-src/pull/6396/files#diff-d9d0d07e6a9125b667f68974544d4c6567c6b31da986c9644d725c188257f378

There was 1 failure:

1) Rector\Tests\DeadCode\Rector\Property\RemoveUselessVarTagRector\RemoveUselessVarTagRectorTest::test with data set #3 ('/home/runner/work/rector-src/...hp.inc')
Failed on fixture file "non_existing_object.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 final class NonExistingObject
 {
+    /** @var SomeNonExistingObject */
     private Properties|null $properties;

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this is something that is acceptable. If the referenced type is somehow not discoverable by Rector, is the job of DeadVarTagValueNodeAnalyzer to remove the tag ?

Also, could we not check if $docType is a template NonExistingObjectType->getClassName() = TType?

Copy link
Member Author

Choose a reason for hiding this comment

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

The patch seems needs to be in ObjectTypeSpecifier to locate @template if exists before marking as NonExistingObjectType

rector-src/rules/TypeDeclaration/PHPStan/ObjectTypeSpecifier.php

Lines 58 to 70 in d415260

// probably in same namespace
if ($scope instanceof Scope) {
$namespaceName = $scope->getNamespace();
if ($namespaceName !== null) {
$newClassName = $namespaceName . '\\' . $className;
if ($this->reflectionProvider->hasClass($newClassName)) {
return new FullyQualifiedObjectType($newClassName);
}
}
}
// invalid type
return new NonExistingObjectType($className);

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you might be right. Unfortunatly I do not have enought knowlage of PHPStan to find out if the type is a template or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be a problem if the unknown type is not removed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@template tag should be detected, unknown type can be just typo missing FQCN on class name, which should be removed as pointed to invalid class target.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed by get @template from ClassReflection 02b566a

current limitation is currently only detect single @template that can be improved in separate PR when possible :)

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 seems cause invalid rectify 366194b , I will check more :)

Copy link
Member Author

Choose a reason for hiding this comment

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

should be fixed now :)


if ($propertyType instanceof UnionType && ! $docType instanceof UnionType) {
return ! $docType instanceof IntersectionType;
}
Expand Down
Loading