Add extensible ClassReflection::getAllowedSubTypes()

[jiripudil]

...to allow restricting inheritance hierarchies in userland – but also internally; I've rewritten the current DateTimeInterface and enums support into this extension point.

Closes phpstan/phpstan#7493

[ondrejmirtes]

This is so nice! I'm blown away 🤯 I have a few minor points to make but I'll wait until the build is green :)

[ondrejmirtes]

I guess with this extension in place, we should be able to write a rule in phpstan-src that actually checks an extending class in regard to ClassReflection::getAllowedSubTypes()?

[ondrejmirtes]

And yeah, sorry, the build isn't going to be absolutely green because of some failures I have in https://github.com/phpstan/phpstan/actions/workflows/extension-tests-run.yml now.

[jiripudil]

It seems most of the failures relate to the null-safe operator I've used in ObjectType. Am I not allowed to use it, even though composer.json requires PHP ^8.0?

[ondrejmirtes]

@jiripudil I think it's a question of configuring Rector to downgrade it correctly here https://github.com/phpstan/phpstan-src/blob/1.7.x/build/rector-downgrade.php. (DowngradeNullsafeToTernaryOperatorRector is missing there).

[jiripudil]

Also, I've realized this probably won't play well with generics, be it generic parent, generic descendant, or both. I don't have a use case for that right now, but I can imagine someone might have. I'll give it some thought to see if it's possible to implement it later with the current API.

[staabm]

do we know a use-case where we would have several extensions for the same type?

I am wondering whether this loop should jus return whatever the first subtype class extensions' getAllowedSubTypes returns (which supports the current type)?

[ondrejmirtes]

I'd really welcome the rule that checks allowed extends/implements here :) Otherwise it's 👍

[ondrejmirtes]

Can you also test that:

1. DateTime CAN be extended by userland classes
2. DateTimeInterface CANNOT be implemented by userland classes

https://3v4l.org/f7V26

[jiripudil]

Good point, while we're at it, I've also added an extension (and test) for Throwable

[jiripudil]

Hm. Turns out you can add an explicit implements DateTimeInterface to a class as long as it is also a descendant of DateTime or DateTimeImmutable (https://3v4l.org/nOU76). The same goes for Throwable.

I could account for that in the rule, but I'm afraid it might not work that well for other use cases – including my original use case of sealed classes.

I'm starting to think this is trying to accomplish two different things, and perhaps we should only focus on one here – provide better type narrowing for restricted hierarchies. I guess we could come up with better names to make it clear that the extension only focuses on this and that it alone doesn't give any guarantees that the inheritance hierarchy is actually restricted, leaving it up to the applications (and phpstan-extensions) to enforce that via a custom rule based on their specific needs and conventions.

Having said that, I think that PHP natives like DateTimeInterface and Throwable should be covered by PHPStan, but that a more targeted rule which takes the aforementioned quirks into account would be a better fit in this case.

WDYT?

[ondrejmirtes]

It's complicated. This new rule has to somehow account for repeated implements DateTimeInterface because PHPStan will already include an extension that describes this so if this rule wouldn't allow it it would lead to false positives. (AllowedSubTypesRule should allow this https://3v4l.org/nOU76 without any errors reported.).

I think we can achieve that - if we find a piece of code that violates the constraints set up by extensions but the piece of code duplicates a fact that's already true from an inherited parent, this rule shouldn't report this?

So - DateTime already implements DateTimeInterface, class Foo extends DateTime, therefore class Bar extends Foo implements DateTimeInterface shouldn't be reported because DateTime and Foo already implement DateTimeInterface.

Is this doable? I think it will get use all the benefits we want still :)

[jiripudil]

Well, there are other edge cases, such as https://3v4l.org/nbLp6 – having a userland interface extend DateTimeInterface or Throwable is perfectly valid, but you can only implement that interface if the implementation extends a valid implementation such as DateTime or Exception. So the rule would have to account for this possible indirection too and make distinctions between parent class hierarchy, directly implemented interfaces, indirectly implemented interfaces, etc. It sounds quite complex, but it should be doable :)

The question is, are you ok with this rule (or rules) specifically targeting DateTimeInterface and Throwable, and there being no built-in general rule covering custom AllowedSubTypes extensions? That way, all restricted hierarchies that are supported out-of-the-box would be covered by PHPStan (hence no false positives), and userland AllowedSubTypes extensions would be allowed (and expected) to enforce their conventions.

For example, I would like sealed classes/interfaces to be strict:

#[Sealed(permits: [ImplementationOne::class, ImplementationTwo::class])]
interface SealedInterface {}

// I want this to fail, SubSealedInterface is not in "permits":
interface SubSealedInterface
    extends SealedInterface {}

// and this too (it's an error in Kotlin, and I'd like it to be consistent)
class SubImplementation
    extends ImplementationOne
    implements SealedInterface {}

Writing a custom SealedClassRule will allow me to not only tailor the behaviour to my expectations but also provide a more helpful error message.

[ondrejmirtes]

I think we have two possible paths to take here:

1. Include the new rule in this PR but continue hardcoding sealed DateTimeInterface class hierarchy as it is now on 1.8.x. So DateTimeInterfaceAllowedSubTypesClassReflectionExtension and ThrowableAllowedSubTypesClassReflectionExtension wouldn't be part of this PR. This means that the new rule would only target custom allowedSubTypes extensions.
2. Leave things as they are now in this PR but remove AllowedSubTypesRule.

I think 1) makes more sense because people aren't required to implement their custom (very repetetive) rules, they just have to provide custom extension to define the class hierarchy. The downside is that PHPStan internals will be a bit more ugly but that's fine.

WDYT? After this we can implement a completely separate rule to detect situations when the user shouldn't implement DateTimeInterface, Throwable etc...

[jiripudil]

Okay, that makes sense to me :)

[ondrejmirtes]

This rule isn't registered. Level 0 would be fine 👍

[jiripudil]

Oops, added, thanks.

[ondrejmirtes]

Perfect! I especially like the code generalization in ObjectType and StaticType about enum subtraction :) Thank you very much.

[ondrejmirtes]

FYI I've just written a documentation about this feature: https://phpstan.org/developing-extensions/allowed-subtypes Feel free to add anything!