New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for @template-contravariant #1492
Conversation
new GenericObjectType(C\Contravariant::class, [new ObjectType('DateTimeInterface')]), | ||
new GenericObjectType(C\Contravariant::class, [new ObjectType('DateTime')]), | ||
TrinaryLogic::createMaybe(), | ||
], | ||
[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I also add some data sets to dataGetReferencedTypeArguments
?
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:
If somebody can help with those that'd be greatly appreciated :) edit: Solved by adding the |
} | ||
if ($other->covariant()) { | ||
return self::createCovariant(); | ||
} | ||
return self::createInvariant(); | ||
} | ||
|
||
if ($this->invariant()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change has broken some things. But is this change wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is in fact correct, this has also been a bug in PHPStan since 2019. Given the example from the test:
class Base {}
class Extended extends Base {}
/**
* @template-contravariant K
*/
class B {
/**
* @param Invariant<Out<In<K>>> $l
*/
function a($l) {}
}
/**
* @param B<Extended> $b
*/
function f1($b) {}
/**
* @param B<Base> $b
*/
function f2($b) {
f1($b);
}
Without this change, PHPStan would effectively allow changing an invariant type parameter from Out<In<Extended>>
to Out<In<Base>>
, which is a bug.
@ondrejmirtes Can you verify I'm not wrong so I can fix the tests this has affected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has also been a bug in PHPStan since 2019
Can you please show the bug on an existing code on phpstan.org/try? Your example uses @template-contravariant
which is a new feature (so it couldn't be a bug in 2019).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing. Here's a similar example with @template-covariant
: https://phpstan.org/r/144953ee-3dfd-4606-a558-b373309ef9c5
It should have given the following error, but it currently passes: Template type K is declared as covariant, but occurs in invariant position in parameter a of method C::a().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any updates on this @ondrejmirtes? I'd love to finish this PR and get it merged
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? |
tests/PHPStan/Rules/Generics/data/method-signature-variance.php
Outdated
Show resolved
Hide resolved
87cedbf
to
200158e
Compare
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. |
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 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 |
Yeah, it's all caused by that change. Decoupling it sounds good, I'll do that. |
Awesome 👍 |
Hi, I'm cleaning up old and stale PRs. Please send a new PR if you're still interested, thanks. |
Closes phpstan/phpstan#3960