phpstan-src icon indicating copy to clipboard operation
phpstan-src copied to clipboard

Add support for @template-contravariant

Open autaut03 opened this issue 2 years ago • 7 comments

Closes https://github.com/phpstan/phpstan/issues/3960

autaut03 avatar Jul 01 '22 23:07 autaut03

Also, the tests are failing due to those errors missing. I haven't had time to debug why PHPStan doesn't report them, although I do believe they should be:

-94: Template type K is declared as contravariant, but occurs in invariant position in parameter l of method MethodSignatureVariance\B::a().
-127: Template type K is declared as contravariant, but occurs in invariant position in return type of method MethodSignatureVariance\B::l().
-130: Template type K is declared as contravariant, but occurs in invariant position in return type of method MethodSignatureVariance\B::m().

If somebody can help with those that'd be greatly appreciated :)

edit: Solved by adding the if($this->invariant()) return invariant(); check. Need to verify this is ok

autaut03 avatar Jul 01 '22 23:07 autaut03

All the newly reported errors are technically all correct. It doesn't make sense that changing of variance for an extended class is allowed. It is currently covered by other PHPStan rules, but is there a point in doing that?

https://phpstan.org/r/fa55f2f8-8798-47fd-99d7-06c007e44be2

autaut03 avatar Jul 02 '22 00:07 autaut03

Sorry, I can't review or merge this PR, the build is pretty broken. Please make it green first.

ondrejmirtes avatar Aug 17 '22 19:08 ondrejmirtes

Sorry, I can't review or merge this PR, the build is pretty broken. Please make it green first.

Sure. I was just waiting for a response in a thread above. I'll make it green.

autaut03 avatar Aug 17 '22 19:08 autaut03

Yeah, at the same time I'm not sure about the failures, I need to see how the fixes look like. Is the code that causes the errors the added if ($this->invariant()) {? Does the @template-contravariant feature need that addition or not?

Maybe we could decouple that fix from the rest of the feature, it looks pretty blocking to me.

So maybe the build would be come green by removing the if ($this->invariant()) { and you could send another PR with that to have a discussion about that?

ondrejmirtes avatar Aug 17 '22 19:08 ondrejmirtes

Yeah, at the same time I'm not sure about the failures, I need to see how the fixes look like. Is the code that causes the errors the added if ($this->invariant()) {? Does the @template-contravariant feature need that addition or not?

Maybe we could decouple that fix from the rest of the feature, it looks pretty blocking to me.

So maybe the build would be come green by removing the if ($this->invariant()) { and you could send another PR with that to have a discussion about that?

Yeah, it's all caused by that change. Decoupling it sounds good, I'll do that.

autaut03 avatar Aug 17 '22 19:08 autaut03

Awesome 👍

ondrejmirtes avatar Aug 17 '22 20:08 ondrejmirtes

Hi, I'm cleaning up old and stale PRs. Please send a new PR if you're still interested, thanks.

ondrejmirtes avatar Oct 16 '22 10:10 ondrejmirtes