Skip to content
This repository has been archived by the owner on Jul 8, 2023. It is now read-only.

Cannot provide ad-hoc implementation of static function that returns self type. #253

Closed
jmalloc opened this issue Mar 6, 2022 · 2 comments
Labels

Comments

@jmalloc
Copy link
Member

jmalloc commented Mar 6, 2022

Given:

abstract class C {
    abstract static function fun(): self;
}

Phony::partialMock(
    [
        C::class,
        [
            'static fun' => function () {
                return new C();
            },
        ]
    ]
);

The following fatal error occurs:

Fatal error: Declaration of PhonyMock_C_8::fun() must be compatible with C::fun(): C in vendor/eloquent/phony/src/Mock/MockFactory.php(91) : eval()'d code on line 5

This makes some sense, as that closure is obviously not returning the self type, however adding the self type doesn't/can't work in this scenario either, as in most cases it would refer to the type of the surrounding class (such as a PHPUnit test suite implementation, for example).

FWIW, this is the error given when naively adding :self to the closure:

Call to undefined method ReflectionFunction::getDeclaringClass()

Which I assume is simply because it's a closure, it does not have a declaring class (ReflectionFunction, not ReflectionMethod).

As a workaround I've implemented this static function by hand-rolling a class in my test suite, but this had a number of undesirable knock-on effects, including preventing verifications using onStatic() from working for this function.

My PHP is pretty rusty at this point so I'm having trouble coming up with any good solutions, but figured I may as well document this issue before I forget the details.

@ezzatron
Copy link
Contributor

Okay, so there's quite a lot to unpack here.

Firstly, while there definitely is a tricky problem in your actual use-case in Recoil, the example above is not actually a good one, because it can be fixed by simply adding a return type to your ad hoc implementation:

abstract class C {
    abstract static function fun(): self;
}

Phony::partialMock(
    [
        C::class,
        [
            // adding the return type C satisfies the abstract parent class' "self" type
            'static fun' => function (): C {
                return new C();
            },
        ]
    ]
);

But after trying out your actual use-case in Recoil, I know that this is not a solution for you, and the problem lies in the fact that you're actually using a trait with an abstract method, not an abstract class.

To explain what's going on, Phony looks at the ad hoc method implementation when generating the ::fun() static method in the mock class. So when the mock class for your original example above is generated, it looks something like this (greatly simplified):

class PhonyMock_C_8 extends C
{
    public static function fun()
    {
        // ...
    }
}

The missing return type comes from the function signature of your ad hoc method implementation, and explains why you get:

Fatal error: Declaration of PhonyMock_C_8::fun() must be compatible with C::fun()

When you apply my fix of adding a return type, you instead get:

class PhonyMock_C_8 extends C
{
    public static function fun(): C
    {
        // ...
    }
}

And this works just fine because PHP can understand that C satisfies the original self type specified in the abstract C::fun() method.

This behaviour of looking to the ad hoc implementation for the function signature is usually useful, because it allows for parameter type widening and return type narrowing in the mock class implementation of overridden methods. But the downside is that it's kind of impossible in some cases to provide an ad hoc implementation that conforms to the overridden method's signature. The cases I can think of include:

  • When the overridden method uses the static type.
    • This is because static basically means "the class name of whatever class implements the function", which would be the mock class itself, which is not known until after the mock class is generated.
  • When the overridden method is from a trait, is abstract, and uses the self type.
    • This is because you cannot use a trait name as a return type, so again, you need the mock class itself, which is not known until after the mock class is generated.

The only way I can think to "solve" this problem, is by adding a feature to Phony's ad hoc mocks that allows you to configure whether the mock generation looks at the ad hoc implementation or the original method for the function signature to use. I've opened #267 to explore that option.

Now, as if all that weren't enough of a rabbit hole, there's a compounding issue here too. Phony currently can't mock a trait that uses the self type at all. I've opened #268 for that.

@ezzatron
Copy link
Contributor

ezzatron commented Mar 5, 2023

Closing, since the core issues are covered by #267 and #268.

@ezzatron ezzatron closed this as completed Mar 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants