Skip to content
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 contravariant test which fails due to missing in the from part #702

Draft
wants to merge 2 commits into
base: 8.2.x
Choose a base branch
from

Conversation

DanielBadura
Copy link
Contributor

We have a situation right now where we want to enable the use to use the psr-20 clock interface. Beforehand we already introduced our own ClockInterface with the exact api the psr-20 one would have. And now, since it was released, we wanted to get rid of our own interface from our internal code but still providing it. So our interface now extends from the psr-20 one and we changed a signature which required our interface to the new psr-20 interface. In my opinion this should be ok, since the old interface is still allowed.

I provided a TestCase here to reproduce this. Also here as ref the PR where we want to achieve this: patchlevel/event-sourcing#334

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's reduce this further to a test case around

final class TypeIsContravariantTest extends TestCase

Comment on lines +138 to +139
interface A extends B {}
interface B {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming in this test makes things a bit complex to follow.

Let's rename A and B to make the sorting more clear instead.

B used in both examples as "child interface".

@Ocramius
Copy link
Member

Weird, this should work:

interface ParentInterface {}
interface ChildInterface extends ParentInterface {}
            'interface to parent interface is contravariant (2)'                            => [
                new Identifier('ChildInterface'),
                new Identifier('ParentInterface'),
                true,
            ],
            'interface to child interface is contravariant (2)'                            => [
                new Identifier('ParentInterface'),
                new Identifier('ChildInterface'),
                false,
            ],

Possibly just a swapped conditional in the code?

@DanielBadura
Copy link
Contributor Author

I think the logic in TypeIsContravariant is correct. But the problem in my case is that the ChildInterface is not extending the ParentInterface in the base. So if we change my test and add the B Interface and A is extending then B in the base, then my test is green with no changes in the to part.

So i assume if I split my PR mentioned into 2 PR's would not result in a failing CI. So the two PR's would be:

  1. Add psr-20, add our interface extends the psr-20 one
  2. change signature from referring class

From this point i think, if we want support this, we would need to adjust ParameterTypeContravarianceChanged as this knows about 2 code bases and not TypeIsContravariant which is working without this knowledge and assuming this is the same codebase.

@Ocramius
Copy link
Member

Ocramius commented Nov 28, 2022

But the problem in my case is that the ChildInterface is not extending the ParentInterface in the base.

I see, so that's what the test is picking up.

Can we please represent that in a test case around TypeIsCovariant? It could be solvable, perhaps 🤔

@DanielBadura
Copy link
Contributor Author

So i added now a testcase for TypeIsCovariant which mostly looks the same as for ParameterTypeContravarianceChanged. I also tried to figure out how to solve this and i think i come to the conclusion that this problem is not really solvable.

With the 2 codebases and one missing the crucial classes we cannot easily perform an implementsInterface check. Getting the ChildInterface from the new codebase of ParentInterface is not really possible is it? And if we could get this information how would be perfoming the check that ChildInterface from the one codebase is the same as the ChildInterface from the other one?

@Ocramius
Copy link
Member

We could build our own implementsInterface check, no? 🤔

Would need to get all parent interfaces, turn them to a list<class-string>, then check that?

@DanielBadura
Copy link
Contributor Author

Yeah we could check this on this basis, if we can retrieve all child interfaces / classes of ParentInterface from the second codebase. AFAIK there is not possibility in reflection to retrieve that. Am I missing here something?

@Ocramius
Copy link
Member

Would need to build an in-memory representation of the inheritance tree in both versions, then compare the two trees :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants