A bundle of generic variance enhancements

[jiripudil]

After #2064, I've had a few more eureka moments about generic variance. Honestly, I'm starting to feel like I'm fighting a hydra: I cut off one head and two more grow back 😅 as a result, I'm proposing several changes.

Sorry for not opening separate pull requests, but all these changes sort of build upon one another, and I believe they make the most sense as a whole package. I'm opening a draft PR so that you can take a look at the changes in their interconnectedness, and then I can split this commit by commit. It's 100% ready for review.

Disallow class template types in static methods

Other languages with generics such as Java, Kotlin, or TypeScript do not allow template type references in static members (with the obvious exception of a static method referencing its own template types). I think PHPStan should follow suit.

After all, static members are shared, even between various instances of the type constructor, while the type parameter is bound to its instances. In terms of (pseudo)code, you can't just call Collection<Dog>::create(). You make the factory itself generic and call Collection::create<Dog>() instead.

I hope this change should be relatively safe; I seriously doubt people are using class template types in static methods because the semantics of that are just so unclear. But if necessary, I believe it wouldn't be that difficult to add a feature toggle for this, even in the context of all the surrounding changes.

Do not check variance in constructor

Turns out you were almost right: there really should be a similar exception for constructor, except there isn't :) the check has so far behaved in the way I described in reply to your comment, but that way is indeed incorrect. I've changed the code so that the constructor is ignored as well.

This potentially relaxes the whole check: I imagine it might have previously discovered variance issues with promoted properties. But I consider it to be just a fortunate side effect rather than a feature. PHPStan should distinguish between the constructor parameter which should be omitted from the variance check, and the promoted property which should be included in the variance check just as any other property (more on that below).

Explicitly disallow defining type parameters in constructor

While methods are allowed to define their own type parameters and become generic, the constructor is a special case and should be exempt from this. I expect nobody is doing this, anyway, but I think it can't hurt to forbid it explicitly.

Check generic variance rules for properties

All these changes gradually build up towards the final one: generic variance check for properties.

Properties are treated as invariant, therefore this new rule will be most useful with invarianceComposition enabled (#2054). The only exception is native readonly properties: I believe we can safely consider them covariant because PHPStan already makes sure that they are initialized in the constructor and only read ever since.

Similarly to methods, private properties are ignored and static properties are prevented from referencing template types altogether.

I've hidden this new rule behind a bleeding-edge feature toggle because I expect it might break a build or two for projects (although hopefully not that many since private properties are ignored).

[ondrejmirtes]

Disallow class template types in static methods

I disagree with this one. Right, it doesn't make sense because you can't set it when talking about an instance like Foo<Bar>, but static members can still use class-level templates when specified in @extends or @implements.

As an experiment, I'd accept this for final classes. Only in bleeding edge though.

Do not check variance in constructor

👍

Explicitly disallow defining type parameters in constructor

I agree, needs to be in bleedingEdge though only.

Check generic variance rules for properties

👍

Next time, please send at least 4 separate PRs 😂 And only one-by-one, to avoid conflicts, and to apply learnings from the previous PRs in further ones.

[jiripudil]

Next time, please send at least 4 separate PRs

Yeah, sorry about that. I had most of the code prototyped for the properties check, and then I kept bumping into all these edge cases that I thought needed to be solved beforehand. As I said, this PR was meant as an overview of the whole context, and I'm ok with rebasing any changes that come out of this and sending the PRs one by one. It seems that only the static bit is causing a stir, anyway :)

static members can still use class-level templates when specified in @extends or @implements.

I'm not sure I understand. Could you give me an example of what you mean?

I've taken some time for further research, and I must stand my ground. I'll try to build my case from a different perspective, though: properties.

This is no longer a question of variance, it's more a question of implementation details. Consider the following code:

/** @template T */
class Foo
{
    /** @var T */
    public static mixed $foo;
}

Because PHPStan only does type erasure, there is a singular property at runtime: Foo::$foo, shared for all possible values of the type parameter. If PHPStan allowed this (which it would because the template type is invariant), you could write code that obviously breaks at runtime: (Pardon the pseudocode. You can actually access the generic type statically via some class-string tricks, but let's keep the code simple.)

Foo<string>::$foo = '✌️';
functionThatAcceptsInt(Foo<int>::$foo);

This must not be allowed. I believe we can agree on this one :)

---

Looping back to static methods, I think template type references should be disallowed there too, for the sake of consistency:

It's consistent for the end user: however obvious it might be, you don't need to explain to them why template types can be used in static methods, but not in static properties. People get the same error message in both cases.

It's also consistent implementation-wise. There are languages such as C# that have actual compiled generics; this allows them to treat Foo<string> and Foo<int> as two different classes in two different places in memory. PHPStan sits on a different bench, alongside Java, Kotlin and TypeScript that I've originally mentioned: all of them do type erasure, and thus, in the static context, they only have one Foo, and no type parameter.

It's only on the level of instances where the type parameters make any actual distinction. It's the instances that are actually parameterized by them – one instance of Foo contains strings, while the other holds integers. And if type parameters are instance-bound, I think static methods shouldn't be allowed to reference them, in the same way that they are not allowed to reference the instance-bound $this. So that it's also consistent with how the language already works.

---

Admittedly, if variance rules are properly enforced, I don't think having template type references in static methods could lead to any actual runtime errors.

But they feel useless, most of all: I've established before that class-level template types cannot be used in static properties. What purpose is left for them in static methods if they can't be used to access state?

Without this ability, static methods are essentially just class-scoped functions, and if functions need to be generic, they have to be generic themselves.

Without this ability, Foo<string>::bar() and Foo::bar<string>() are just two ways of doing the same thing, the latter of which is more practical to use in PHP and more consistent with the rest of PHP and PHPStan.

Yes, this might be opinionated and perhaps it should only be part of some optional, stricter ruleset, but I really think it should be there somewhere.

Sorry for the long read. Looking forward to your thoughts!

[ondrejmirtes]

I love these brainstormings! I imagine this is how Einstein wrote letters to his friends too :) (Of course I'm no Einstein, but it has a similar vibe, figuring out things together.)

I thought about it and you're probably right. What I meant for static methods was that you can do @extends ParentClass<int> at one place and @extends ParentClass<string> in a different place. And you can pass this information to the parent through a constant probably: https://phpstan.org/r/d9711806-0338-4408-945e-227933664e16 But it's not perfect.

So yeah, it doesn't make much sense. But we need to disable it only for bleedingEdge, because we need to see the feedback - most of it it's gonna be wrong usage, but maybe someone comes up with something valid. And I don't want to make everyone's builds red again :)

What I'd love to see is for FileTypeMapper be modified, so that we don't need a new special rule for this, the existing rules should pick up that T doesn't exist. Similarly how it's done for properties: https://phpstan.org/r/cac52022-9bff-4723-b39a-70512bd41c92

I had to fix some code so that @template T above a property doesn't mean anything: 53563e9

So the fix for static methods should be similar - it shouldn't see @template from above the class.

And oh, I just realized something about "Explicitly disallow defining type parameters in constructor":

I think it's valid to have @template above a constructor. Obviously it doesn't make sense to use it in @return but they can be used for example for conditional type in parameter, or to interact between themselves, like this:

@param callable(T): void $cb
@param T[] $items

And BTW since we are cleaning up generics, what do you think about union vs. intersection in this example?

@template T
@param T $a
@param T $b

And when such a function is called with new stdClass(), new \Exception(), what the T should be inferred as? :)

Currently it's a union, but I've heard that intersection might make sense too, and it's the case in some languages.

Also, callable parameters complicate that. PHPStan doesn't union them because we need functions like array_map or the constructor above to work properly (the callable parameter T limits what the array T[] in a different function parameter can be). I've found the commit where I made this work but I'm sure it's not always perfect 01f99c1

And one more challenge for generics is: Should we generalize the type or not? :) When you have @template T + @param T + @return T, what should happen if we pass 1 into the function? For @template T, int is returned. And I used a horrible hack so that if you write @template T of int, 1 is returned 🤦

I'm more inclined now towards not generalizing (because it causes problems), but we need to solve a new Collection[1, 2, 3]) problem - it should be Collection<int>. Fortunately there's a suggestion what to do about it phpstan/phpstan#6732 (comment), unfortunately it's really complicated to implement it. I think we need CFG first (phpstan/phpstan#8192).

[jiripudil]

I love these brainstormings!

Me too :)

What I'd love to see is for FileTypeMapper be modified, so that we don't need a new special rule for this, the existing rules should pick up that T doesn't exist.

I love the idea!

I think it's valid to have @template above a constructor.

My train of thought was that the constructor is part of the class's "signature" rather than a separate method because that's how it is in many other languages. But unlike many other languages, PHP doesn't have any special syntactical construct for constructors, so I guess you're right.

And BTW since we are cleaning up generics, what do you think about union vs. intersection in this example?

Yes, it seems that both can be seen in the wild, so probably both make sense.

Personally, I lean toward the intersection. When authoring such a function, I would assume you expect the type of both parameters to be the same (or at least compatible) – otherwise, you would have explicitly used two template types.

Intersection gives you that: if the types are compatible, you get their common denominator, so to speak, and if they're not, you get never and the function call should cause an error. (On a side note, #2110.)

And one more challenge for generics is: Should we generalize the type or not?

It's an interesting problem, and I see you already have a suggested solution :) I'll read through the linked issue and see if any idea comes up.

[jiripudil]

So, I've found some time to take a look at this again

What I'd love to see is for FileTypeMapper be modified, so that we don't need a new special rule for this, the existing rules should pick up that T doesn't exist. Similarly how it's done for properties: https://phpstan.org/r/cac52022-9bff-4723-b39a-70512bd41c92

It's fairly easy to alter the code so that static methods do not see class-level template types, but I must say I'm struggling with static properties. If I understand it correctly, they share the same NameScope with instance properties, and I'm not sure how to hide template types from one kind of properties and not the other – at least not on FileTypeMapper level. @ondrejmirtes do you think you could nudge me in the right direction?

If it turns out too complex – or perhaps even if not –, I still can see some value in reporting a distinct error for this – if nothing else, it should be more easily google-able for people who encounter it, and it leaves space for a potential targeted hint or a link to an explanation if people are confused by this.

[ondrejmirtes]

Can you please share how you did it for static methods? So that I might have an easier job figuring out how to did it with static properties :)

[jiripudil]

Closing, the less controversial changes have been implemented in other PRs (#2311, #2313, #2314), and I've changed my mind about the importance and/or justification of the other ones :)