[Experiment] Rework child classes detection on DynamicSourceLocatorProvider

[samsonasik]

@nuryagdym @TomasVotruba this is experimental for rectorphp/rector#8557

Closes rectorphp/rector#8557

[samsonasik]

Ok, here investigation:

1 Tried back to Rector 0.19.15 before the FinalizeClassesWithoutChildrenRector is deprecated, tested with:

git clone [email protected]:samsonasik/ci4-album.git rector-0.x

got output:

➜  ci4-album git:(rector-0.x) ✗ vendor/bin/rector --dry-run
 49/49 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
2 files with changes
====================

1) src/Domain/Exception/DuplicatedRecordException.php:12

    ---------- begin diff ----------
@@ @@

 use DomainException;

-class DuplicatedRecordException extends DomainException
+final class DuplicatedRecordException extends DomainException

which shouldnot as the class has child. I will check if there is possible way to "init" the classes to be resolved by phpstan Reflection.

[samsonasik]

@TomasVotruba @staabm It somehow can be resolved by patch:

-            if (!$classReflection->isSubclassOf($desiredClassReflection->getName())) {
+            if ($desiredClassReflection->getName() !== $classReflection->getName() && !$classReflection->isSubclassOf($desiredClassReflection->getName())) {
                continue;
            }

it seems isSubclassOf() definition make same class name as true on the removed FamilyRelationsAnalyzer::getChildrenOfClassReflection() on rector 0.19.5

rector-src/src/FamilyTree/Reflection/FamilyRelationsAnalyzer.php

Line 39 in 171fddb

if (! $classReflection->isSubclassOf($desiredClassReflection->getName())) {

I will try apply the child detection back.

[staabm]

I cannot follow, but you are right that isSubclassOf requires a actual subclass and does not include equality.

you can use is if you need the equality check to also return true.

[samsonasik]

It seems the classes only get the paths that scanned:

        $classReflections = $this->privatesAccessor->getPrivateProperty($this->reflectionProvider, 'classes');
        var_dump(count($classReflections));

Running on actual path only got 6 classes:

➜  ci4-album git:(rector-0.x) vendor/bin/rector process src/Domain/Exception/RecordNotFoundException.php  --dry-run --clear-cache
 0/1 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░]   0%int(6)
string(46) "Album\Domain\Exception\RecordNotFoundException"
string(15) "DomainException"
string(14) "LogicException"
string(9) "Exception"
string(9) "Throwable"
string(10) "Stringable"

which should not, I will continue investigating if there is a way to init somewhere.

[staabm]

ClassReflection only works on paths in the analyzed path or scan* configurations, thats expected and correct from PHPStan point of view (and composer autoloading I guess).

[samsonasik]

@staabm thank you, I remember that on old phpstan, the code is working fine without the tweak, and on some point of phpstan release, it no longer working, I will continue investigating as it is interesting, I expect it should work with at least run on no path provided:

vendor/bin/rector

which means all classes loaded.

[samsonasik]

@TomasVotruba @staabm I finally got it working prototype 🎉 , tested on Rector 0.19.5 before the FinalizeClassesWithoutChildrenRector deprecated that test sub classes.

the getClass() need to be called to trigger collect classes property of MemoizingReflectionProvider 9a2fdbd

Before

➜  ci4-album git:(rector-0.x) ✗ vendor/bin/rector process --dry-run --clear-cache
 49/49 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
2 files with changes
====================

1) src/Domain/Exception/DuplicatedRecordException.php:12

    ---------- begin diff ----------
@@ @@

 use DomainException;

-class DuplicatedRecordException extends DomainException
+final class DuplicatedRecordException extends DomainException

After

➜  ci4-album git:(rector-0.x) ✗ vendor/bin/rector process --dry-run --clear-cache
 49/49 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

                                                                                                                        
 [OK] Rector is done!                                                                                                   

I will check segmentation fault on package tests

[samsonasik]

@nuryagdym @TomasVotruba @staabm it finally working again 🎉 🎉 🎉 , here the story:

---

Long time ago, the children classes detection mostly always working, and mostly just require composer dump-autoload, until on some point release of phpstan, it no longer working, even autoload the classes early,

Turns out, the PHPStan has NewOptimizedDirectorySourceLocator which seems a brand new source locator, which somehow, make the PHPStan ReflectionProvider require to touch it to trigger classes to make it collected.

I tested in my own module: samsonasik/ci4-album#16 and it seems manually apply the code, and it finally working fine.

This PR added back the feature, and the handle in ClassMethodReturnTypeOverrideGuard which detect if child classes override the return type, it will not be allowed, see fixture https://github.com/rectorphp/rector-src/pull/5735/files#diff-d52d2b21cf8f834596c1ab5e7bee4fae94181a109dec119b506d538f8017f8d6

and e2e test for the proof for bin/rector command usage f5f5973

Limitation:

• Directly pass to single file will only get classes on single file, which expected.

➜  ci4-album git:(rector-0.x) ✗ vendor/bin/rector process src/Domain/Exception/DuplicatedRecordException.php --dry-run --clear-cache
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
1 file with changes
===================

1) src/Domain/Exception/DuplicatedRecordException.php:12

    ---------- begin diff ----------
@@ @@

 use DomainException;

-class DuplicatedRecordException extends DomainException
+final class DuplicatedRecordException extends DomainException

so the run needs to apply on the parent and child classes contains, eg, both parent and child classes files:

➜  ci4-album git:(rector-0.x) ✗ vendor/bin/rector process src/Domain/Exception/DuplicatedRecordException.php src/Domain/Track/TrackDuplicatedRectorException.php  --dry-run --clear-cache
 2/2 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

                                                                                                                        
 [OK] Rector is done!                                                                                                   
                                                                                                                        

or the directory containst both:

➜  ci4-album git:(rector-0.x) ✗ vendor/bin/rector process src/ --dry-run --clear-cache 
 36/36 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

                                                                                                                        
 [OK] Rector is done!                                                                                                                                                                                                                    

or just without arg:

➜  ci4-album git:(rector-0.x) ✗ vendor/bin/rector
 36/36 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

                                                                                                                        
 [OK] Rector is done!     

If this PR merged, we can un-deprecate FinalizeClassesWithoutChildrenRector and make it works out off the box, in separate PR.

[TomasVotruba]

Thank you 👍