Design flaw in FinalizeClassesWithoutChildrenRector

[staabm]

Bug Report

Subject	Details
Rector version	e.g. v0.11.5 (invoke vendor/bin/rector --version)

FinalizeClassesWithoutChildrenRector is relying on FamilyRelationsAnalyzer to decide whether classes can be marked final or not. FamilyRelationsAnalyzer itself is working on the current state-of-reflection of the PHPStan-ReflectionProvider:

https://github.com/rectorphp/rector-src/blob/52398c81f9e88bde858cd2d79d481ae66190abc7/src/FamilyTree/Reflection/FamilyRelationsAnalyzer.php#L34

this means at the time of running FinalizeClassesWithoutChildrenRector the rule is trying to identify possible existing sub-classes, so it can know whether the current class can be marked final or not.

BUT the rector rule does not take into account that the list of classes FamilyRelationsAnalyzer works on depends on which classes have already been scanned/processed. That means its pretty easy to trigger a situation in which FinalizeClassesWithoutChildrenRector marks a class as final even though subclasses exist (but just have not been processed by rector/phpstan yet).

IMO all logic which is built on FamilyRelationsAnalyzer has this flaw.

there is a article on https://getrector.com/documentation/static-reflection-and-autoload which tries to solve this by a few more workarounds, but none of them work for us. we have a proper PSR-4 setup this does not help to fix the issue.

there are so many bug reports about this problem, and I think most of the time these are related to the described problem.

---

the only way I could think of the FinalizeClassesWithoutChildrenRector might work would be to use a collector beforehand which builts a list of all classes before the actual FinalizeClassesWithoutChildrenRector rule is invoked.

since collectors have been dropped recently, I think the only usefull way forward is dropping FinalizeClassesWithoutChildrenRector completely and all rectors which currently rely on FamilyRelationsAnalyzer. the results of this rules currently heavily depend on the current rector cache-state, whether the analysis is run in parallel or not and on the order files will be scanned (which depends on the filesystem).

Minimal PHP Code Causing Issue

Expected Behaviour

[TomasVotruba]

Thank you, this is known drawback of this rule.

How would you improve it?

[staabm]

I don't see a way to improve it. maybe it would help to remove it from the known set-lists so people don't use it accidentally

[staabm]

we would need a full picture of all known classes within FamilyRelationsAnalyzer which would mean it would only work if we run FinalizeClassesWithoutChildrenRector after everything else and only when no cache is used (so only on a full project analysis run)

[staabm]

we could do inefficient stuff like trigger static reflection of all classes registered in the composer autoloader, so at least the hacks mentioned on https://getrector.com/documentation/static-reflection-and-autoload would take effect.

but I think this would be slow and require lots of memory.

in the end I am not sure the feature is "stable enough" or maybe its something which can be considered to be deleted before 1.0 is released

[staabm]

looking at the PHP-CS rule for "final-classes" they strike a pretty hard balance:

If you want to subclass a class, mark the parent class as abstract and create two child classes, one empty if necessary: you’ll gain much more fine grained type-hinting. If you need to mock a standalone class, create an interface, or maybe it’s a value-object that shouldn’t be mocked at all. If you need to extend a standalone class, create an interface and use the Composite pattern. If you aren’t ready yet for serious OOP, go with FinalInternalClassFixer, it’s fine.

so they say, that if you need a non-final baseclass turn it abstract. thats a pretty rough idea, but its something which could be enforced with a PHPStan rule

[TomasVotruba]

maybe it would help to remove it from the known set-lists so people don't use it accidentally

Sounds good :+1:

[TomasVotruba]

in the end I am not sure the feature is "stable enough" or maybe its something which can be considered to be deleted before 1.0 is released

Agreed :+1: Everytime I use this rule it's going back and forth to skip files that are not abstract, but they're extended. Sometimes it's 30-60 minutes of pointless work.

---

I was thinking about standalone tool for a while, due to the full-context issue. Looking into it :roll_eyes:

[staabm]

I was thinking about standalone tool for a while, due to the full-context issue. Looking into it 🙄

Makes sense. Maybe this tool should bundle other rector rules which also rely on the same underlying class

[TomasVotruba]

Saying that, I see there are few more rules that "claim" to check children classes using FamilyRelationsAnalyzer, but actually bear the same problem.

• FinalizePublicClassConstantRector
• AddMethodCallBasedStrictParamTypeRector
• Rules adding return type checking child types

How would you approach those?

E.g. the FinalizePublicClassConstantRector does not bring much value, as final class resolves it in much better way.

[staabm]

E.g. the FinalizePublicClassConstantRector does not bring much value, as final class resolves it in much better way.

I agree that these rector does not provide much value. but from a domain problem view-point it would be a good fit togehter with FinalizeClassesWithoutChildrenRector in a new utilitly, like you suggested. (Adding final to constants only after the utility decided the class cannot be marked final at the class-level)

AddMethodCallBasedStrictParamTypeRector

I don't see yet why this rule has the same problem. it seems to only work on a single class and only on private methods, which seems to be independent from possible subclasses... why do you think it has the same problem, maybe I am overlooking something?

Rules adding return type checking child types

Atm I can't remember whether I had problems with other rules.

[staabm]

I also had a look at StringableForToStringRector, which adds implements Stringable by using FamilyRelationsAnalyzer. I don't understand what the value of this rule is, because as I understand the Stringable its mostly usefull at the typing level (e.g. used as a parameter or return type).

I don't see why one would want to add implements Stringable since its auto-implemented by php-src when a __toString() method exists.

[TomasVotruba]

@todo myself, handle stringable rule and close this one

[TomasVotruba]

Closing as rule was deprecated and behavior disabled now :+1:

[staabm]

Thanks