-
Notifications
You must be signed in to change notification settings - Fork 460
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
Fix invariance composition #2054
Fix invariance composition #2054
Conversation
Wow, it's not half as bad as I expected. The failing test in doctrine/collections seems correct: the tuple returned by the What do you think about this change @ondrejmirtes? It might start reporting new errors, like in the doctrine/collections case, but they are right; the code genuinely is not perfectly type-safe. |
I really appreciate it! But it's guaranteed to make a lot of CI builds red, so I'd put it into bleedingEdge now. One place you changed is a DI service, that's straightforward, the other one is a place when we cannot access DI easily so it's gonna need some static toggle, similarly how AccessorryArrayListType::intersectWith works. Thank you, looking forward to merge this! |
0836094
to
e1c24bc
Compare
I've added a feature toggle and updated the tests to cover both cases. I'll think about an explainer for the eventual release notes. I think this is going to deserve one :) |
e1c24bc
to
c1fe347
Compare
@@ -98,7 +98,7 @@ public function check( | |||
$messages[] = RuleErrorBuilder::message(sprintf($invalidTypeMessage, $referencedClass))->build(); | |||
} | |||
|
|||
$variance = TemplateTypeVariance::createInvariant(); | |||
$variance = TemplateTypeVariance::createStatic(); |
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.
What about this? What does it change? What's the impact? Shouldn't it be done under the feature toggle?
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.
When I fixed the invariance composition, this check reported all template types in @extends
as invariant. Therefore, this would not be allowed:
/** @template-covariant T */
interface Foo {}
/**
* @template-covariant T
* @implements Foo<T>
*/
class Bar implements Foo {}
I can add the feature toggle but I don't think it's necessary. This change has no practical impact because "static" variance has had the same composition logic as invariant until this PR. It even feels semantically more correct to me: the initial position in @extends
is not invariance, it is nothing, similar to other static contexts like static methods or the constructor.
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.
Alright, great 😊
Thank you very much! 😊 |
Thank you too! :) |
Now for the more complicated bit.
In my research for type projections, I've found this paper very resourceful; in section 3.1 which discusses variance composition, it says that "invariance transforms everything into invariance".
So, as also argued in #1492 (comment), this is indeed technically a bugfix. To make sure that the position variance is composed correctly, I've also replicated the test case from this PR in Kotlin whose type system I firmly trust.
Let's see what builds this breaks :)