Implement @phpstan-consistent-templates

[canvural]

This PR adds a new tag @phpstan-consistent-templates and new checks for it. Fixes one part of phpstan/phpstan#5512

There are some parts I am not sure of.

• Naming of the hasConsistentTemplates Everything else, like internal, final, has naming with isXX. But isConsistentTemplates did not make sense to me.
• Wording of the error messages. I am open to suggestions to improve/change the error messages.
• Currently all variants of the tag is supported. Is this good?
• Currently it is implemented in a way that the errors will only appear if the class has @extends or @implements tag. Just class Foo extends Bar will not produce these errors even Bar has @phpstan-consistent-templates

Lastly, there is this edge case.

<?php

/**
 * @template T
 * @template K
 * @psalm-consistent-templates
 */
class Bar {}

/**
 * @template T
 * @psalm-consistent-templates
 */
interface Foo {}

/**
 * @template T
 * @template K
 * @extends Bar<T, K>
 * @implements Foo<T>
 */
class Baz extends Bar implements Foo {}

Basically if a class both extends and implements something, and if those parent classes have different number of template types, the child class can never satisfy both.

[canvural]

On second thought maybe it should be it's own rule, with in the bleeding edge. Because Psalm version of the tag is already in use. I'll change it.

[ondrejmirtes]

I think it's not necessary, it's going to be really rare. I've also enabled conditional return types related rules for everyone :)

Some ideas (I haven't seen the code yet):

• This feature will allow us to usefully interpret static<T, U, V>. Because without consistent templates, we cannot assume how @template tags look above all child classes, but with consistent templates enforced, we can. So this should definitely be part of this PR. Try to have a method marked with generic static<...> tag, call the method on a subclass, it should result in SubClass<T, U, V>.
• There could be a new rule (this one is definitely for bleeding edge) - if someone uses generic static<...> in their code, we should tell them they need @phpstan-consistent-templates above the class. The easiest way to do this is to incorporate the logic in IncompatiblePhpDocTypeRule.

[canvural]

This feature will allow us to usefully interpret static<T, U, V>. Because without consistent templates, we cannot assume how @template tags look above all child classes, but with consistent templates enforced, we can. So this should definitely be part of this PR. Try to have a method marked with generic static<...> tag, call the method on a subclass, it should result in SubClass<T, U, V>.

Yes, that's the goal from phpstan/phpstan#5512 I was planning to do that in another PR. But sure it can be part of this one.

There could be a new rule (this one is definitely for bleeding edge) - if someone uses generic static<...> in their code, we should tell them they need @phpstan-consistent-templates above the class. The easiest way to do this is to incorporate the logic in IncompatiblePhpDocTypeRule.

If the logic will be in IncompatiblePhpDocTypeRule it's not gonna be a new rule, is it? Do you mean have a parameter for bleeding edge toggle, and do the checks according to that?

[ondrejmirtes]

Yes, I often add a boolean parameter in bleedingEdge that changes behaviour of existing rules 😊

But if you find the implementation inelegant, certainly feel free to implement it in a separate rule. For one, it only applies to methods in classes and not functions, so IncompatiblePhpDocTypeRule might not be great after all.

[canvural]

I added a commit to correctly infer static<...> types, if the class has the tag. I think static<...> only makes sense for return types. So I didn't add it to property or method argument types. And about that, should IncompatiblePhpDocTypeRule give error if someone uses static<..> in property or argument? (that's a separate PR)

I'll implement the second part about the IncompatiblePhpDocTypeRule later.

[ondrejmirtes]

Some ideas about validating static<...> I got meanwhile

• I'm not sure about IncompatiblePhpDocTypeRule anymore
• Because we should also validate that the stuff inside static<...> is correct, and the best place to do that is GenericObjectTypeCheck.
• So I imagine this is what should be done:

1. If @phpstan-consistent-templates is present above the class, validate the inner types of static<...> to see if they match @template above the class (in both bleeding edge and without bleeding edge).
2. If @phpstan-consistent-templates is not present, by default don't do anything.
3. If @phpstan-consistent-templates is not present, in bleeding edge it should be reported to add @phpstan-consistent-templates.

[ondrejmirtes]

It's funny a little because one of the places where GenericObjectTypeCheck is called from is IncompatiblePhpDocTypeRule :) So I guess the changes should be tested in IncompatiblePhpDocTypeRuleTest by writing some new sample classes.

[canvural]

The current problem I'm having is that static<...> return type tag is already resolved to GenericObjectType when docblock is resolved in IncompatiblePhpDocTypeRule Should I inspect the $docComment->getText() to figure out if static<...> is used? Do you have any idea of the top of your head?

[ondrejmirtes]

@canvural Yeah, I realized that we might need a new GenericStaticObjectType for this so that the information is kept when going out from TypeNodeResolver::resolveGenericTypeNode().

Same as GenericObjectType extends ObjectType, GenericStaticObjectType should extend StaticType.

It's also useful because we should be able to validate that static<X, Y> is truly returned from a method that typehints @return static<X, Y>.

static works by preserving StaticType when we're inside the class and work mainly with $this, but is converted to the final type (specific subclass) when a method is called from the outside.

The hardest part is going to be to figure out the correct relationships between ObjectType, StaticType, GenericObjectType, GenericStaticType when asked about isSuperTypeOf() about each other. But there's already some precedens about this from the current types.

[ondrejmirtes]

Before PHPStan 1.7.0 is out I'll take a look at the current state of this PR and decide what we can cut from my demands to make it part of that release :)

[canvural]

@ondrejmirtes I actually implemented all the rule changes and GenericStaticObjectType (maybe with some errors), but couldn't find time to clean it up and push :/ Was planning to do it tomorrow since I'll have some free time.

[ondrejmirtes]

Looking forward to it 😊

[canvural]

@ondrejmirtes I pushed the changes. I did the rule changes from here. But did not implement isSuperTypeOf for GenericStaticObjectType Because I'm not sure where that check would be used. What kind of test case would use isSuperTypeOf on GenericStaticObjectType?

Other than that, I guess this is ready for a review.

[ondrejmirtes]

@canvural Type::isSuperTypeOf() is the most useful and important methods of all :) See https://phpstan.org/developing-extensions/type-system#querying-a-specific-type for more details.

It describes the relationship between types. The best way to test it is through type normalization (https://phpstan.org/developing-extensions/type-system#type-normalization) which means to add test cases in TypeCombinatorTest::dataUnion() and TypeCombinatorTest::dataIntersection(). In case of union, the more general type wins and in case of incompatible types, they remain as a union (like string|int).

In case of intersection, the more specific type wins and in case of incompatible types, the result is NeverType.

I'd expect GenericStaticObjectType to be tested against ObjectWithoutClassType, ObjectType (compatible class), ObjectType (incompatible class), StaticType, GenericObjectType, and GenericStaticObjectType.

[canvural]

@ondrejmirtes Thank you for the detailed explanation. But I already knew all that. I can implement that, and add the tests. But my question was more specific to in which scenarios the isSuperTypeOf would be called for GenericStaticObjectType? Currently it uses the parent's (StaticType) implementation and it seems to be fine. Also GenericStaticObjectType resolves to GenericObjectType as soon as the method is called outside of the class context.

All that said, I'll probably continue to work on this next week. Because tomorrow morning I'm leaving for the phpday! I'll make sure to say hi to you! 😊

[ondrejmirtes]

Oh wow, I'm looking forward to that a lot! I kind of hoped to see more familiar faces aside from other speakers so it's great my wishes are gonna be fulfilled 😊

[ondrejmirtes]

And about your question:

But my question was more specific to in which scenarios the isSuperTypeOf would be called for GenericStaticObjectType?

You're in a class method like and have code like this:

if (rand(0, 1)) {
    $a = $this->doFoo(); // returns static<T, U>
} else {
    $a = $this->doBar(); // returns static<Foo, Bar>
}

// $a is union of $a from if+else branches

GenericStaticObjectType remains GenericStaticObjectType here but we need to union the types from if and else. Of course you can come up with other (basically an infinite number) of examples.

The relationship between GenericStaticObjectType and StaticType is the same as GenericObjectType and ObjectType, so you can draw some inspiration from there.

Also it just occured to me that GenericStaticObjectType should probably be called just GenericStaticType :)

[ondrejmirtes]

PHPStan 1.7.0 is on its way to be released, I think we can put this one even in 1.7.x or maybe in 1.8.0 depending on when you find time to finish this :) Thanks.

[canvural]

It's fine by me. It's not urgent to get this merged. But I guess I'll spend some time on this this week. Then we'll see.

[mitelg]

I can provide another example if needed: https://phpstan.org/r/5b601b92-3518-4f51-9de6-0387b28ebfd5

as discussed in phpstan/phpstan#9975

[mxr576]

Just bumped into this issue with my generic collection class as well.

[arderyp]

any hope for making it into 2.x?

[ondrejmirtes]

Superseded by: 0c1f22a

I realized we don't need to enforce @phpstan-consistent-templates for generic static type to work.

I couldn't have done it without your PR, it saved me a lot of time figuring out how the static type even works, so thank you!