Skip to content

Add support for @template-contravariant#1492

Closed
autaut03 wants to merge 4 commits intophpstan:1.8.xfrom
autaut03:template-contravariant
Closed

Add support for @template-contravariant#1492
autaut03 wants to merge 4 commits intophpstan:1.8.xfrom
autaut03:template-contravariant

Conversation

@autaut03
Copy link
Copy Markdown

@autaut03 autaut03 commented Jul 1, 2022

autaut03
autaut03 commented Jul 1, 2022
View reviewed changes
autaut03
autaut03 commented Jul 1, 2022
View reviewed changes
tests/PHPStan/Type/Generic/GenericObjectTypeTest.php Outdated
Copy link
Copy Markdown
Author

@autaut03 autaut03 Jul 1, 2022

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?

@autaut03
Copy link
Copy Markdown
Author

autaut03 commented Jul 1, 2022 •
edited
Loading

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
autaut03 commented Jul 2, 2022
View reviewed changes
src/Type/Generic/TemplateTypeVariance.php Outdated
Copy link
Copy Markdown
Author

@autaut03 autaut03 Jul 2, 2022

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?

Copy link
Copy Markdown
Author

@autaut03 autaut03 Jul 4, 2022

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?

Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes Jul 10, 2022

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).

Copy link
Copy Markdown
Author

@autaut03 autaut03 Jul 10, 2022

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().

Copy link
Copy Markdown

@oprypkhantc oprypkhantc Jul 29, 2022 •
edited
Loading

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

@autaut03
Copy link
Copy Markdown
Author

autaut03 commented Jul 2, 2022

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

ondrejmirtes
ondrejmirtes requested changes Jul 10, 2022
View reviewed changes
@autaut03 autaut03 force-pushed the template-contravariant branch from 87cedbf to 200158e Compare July 11, 2022 11:31
@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Aug 17, 2022

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

@ondrejmirtes ondrejmirtes mentioned this pull request Aug 17, 2022
Open
@autaut03
Copy link
Copy Markdown
Author

autaut03 commented Aug 17, 2022

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.

@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Aug 17, 2022

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?

@autaut03
Copy link
Copy Markdown
Author

autaut03 commented Aug 17, 2022

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.

@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Aug 17, 2022

Awesome 👍

@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Oct 16, 2022

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

This was referenced Dec 5, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

+1 more reviewer

@oprypkhantc oprypkhantc oprypkhantc left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Contravariant bounds in generics

3 participants

@autaut03 @ondrejmirtes @oprypkhantc