Check abstract method signatures coming from traits

[nikic]

RFC: https://wiki.php.net/rfc/abstract_trait_method_validation

Abstract method signatures coming from traits are currently not being validated, or at least not in the most common case where the abstract method is implemented directly by the using class. If it is implemented by a parent class, or by a child class, then the method is already validated.

This fixes the behavior to always validate abstract methods from traits. Additionally it removes the requirement that abstract methods in multiple traits must be bidirectionally compatible. That is, the code from https://3v4l.org/VrpaV will now compile without error.

[nicolas-grekas]

This will break legitimate use of such "incompatible" methods.
Since traits are not types, they should not enforce any contracts.
That's the same for properties used inside traits: they are expected to be declared and have the corresponding semantics.
Same for methods: the traits provide only an implementation that might use some methods "to be implemented". Their signature, from a type pov, shouldn't be enforced. The only thing that matters is that they work in practice.

Please reconsider. There are other tools to enforce signatures: (abstract) classes and interfaces.

[nikic]

@nicolas-grekas Can you please provide an example of a "legitimate use"? Note that this only affects abstract methods.

I find it hard to imagine why a trait would be specifying illegal abstract signatures.

[nicolas-grekas]

Sure: check this trait: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpClient/Response/ResponseTrait.php

It declares:
abstract protected static function perform(ClientState $multi, array &$responses): void;

Which is implemented in different forms in consuming classes:
private static function perform(CurlClientState $multi, array &$responses = null): void
private static function perform(NativeClientState $multi, array &$responses = null): void
protected static function perform(ClientState $multi, array &$responses): void

Depending on the use case. The implementations inside the trait don't care about these details.

[nikic]

@nicolas-grekas For exotic use cases like that, why not just drop that method from the trait (that is, move to @method), instead of ruining it for everyone? That method declaration is clearly and unambiguously wrong, but I understand that you use it to work around other type system deficiencies. That's what doc blocks are generally used for.

[nikic]

(The type system deficiency being that you can't write use ResponseTrait<XyzClientState>.)

[nicolas-grekas]

why not just drop that method from the trait

Could work, but I don't have to right now, so we can use abstract methods for documentation purpose. I find it better than annotations because e.g. syntax is enforced.

Note that a trait should never enforce any method's visibility to their consumers.

All in all, I think this provides no benefits over existing tool: if one wants to enforce a contract, there are tools for it, (abstract) classes and interface. That a trait needs some stubs to be filled in, fine, but abstract methods shouldn't be used for that.

Would allowing private abstract methods on traits work for this purpose?

[nikic]

All in all, I think this provides no benefits over existing tool: if one wants to enforce a contract, there are tools for it, (abstract) classes and interface.

You are making use of the exactly the thing this PR intends to provide: You are declaring an abstract method in the trait with the requirement that it be implemented by the using class. Your particular case just has a fuzzy notion of what "implement" means (due to type system deficiencies) and makes use of the incomplete validation to make that slip through. People regularly want to do that kind of stuff independently of traits:

interface EventHandler {
    public function handle(Event $e);
}
class SpecificEventHandler implements EventHandler {
    public function handle(SpecificEvent $e);
}

That's basically the same situation as you have, just without the trait, and it's just about as incorrect here as with the trait.

That a trait needs some stubs to be filled in, fine, but abstract methods shouldn't be used for that.

What else could abstract methods in traits possibly be used for? Unless they are used to actually provide abstract methods (i.e. the trait is used in abstract classes only) this is the only purpose they can serve that I can see.

Would allowing private abstract methods on traits work for this purpose?

Private abstract methods would make sense in addition to provide signatures for purely internal requirements of the trait.

[nicolas-grekas]

I mostly agree with you. My main concern emerging from this discussion is about method visibility.
An abstract protected or public method on a trait can be needed to compose an abstract class. Such composing abstract class could then decide to rename the method when using the trait - or turn a protected method into a public one, etc. These composition options are what traits are for. But once the trait system starts to complain about a missing unrelated method (because eg it's been renamed on import), then it plays against itself and destroys its value proposition.

private abstract methods on traits look really the correct way to me, they have the perfect semantics.

[muglug]

@nicolas-grekas you're also violating another (currently unenforced) contract:

It declares:
abstract protected static function perform(ClientState $multi, array &$responses): void;
Which is implemented in different forms in consuming classes:
private static function perform(CurlClientState $multi, array &$responses = null): void

Here the trait is saying "I want users of this trait to define a protected method perform", but CurlResponse.php instead defines a private method.

I don't think it's terrible, but it's now something that Psalm (latest master) differentiates from more serious method access overriding: https://psalm.dev/r/a2e30a5828

[nicolas-grekas]

Hello @muglug.

I would have used a private abstract declaration if it were allowed by the engine.

But the code is not violating any contracts because by their very definition, traits do not define any contracts.

That's the reason why you can rename or change the visibility of any of their methods when using them, check https://www.php.net/manual/en/language.oop5.traits.php

[nicolas-grekas]

@smarr are you the author of the original PHP RFC that introduced traits? We would be super happy to have your feedback here! Thank you :)

[smarr]

@nicolas-grekas sorry it has been a very long time. So, my opinion is probably not as useful as you may hope.

If I understand the discussion correctly, the underlying question here seems to be how precisely a trait can express its requirements. Given that traits where designed before type annotations where a thing in PHP, I never really thought about it before. And I'll admit that I haven't spent nearly enough time on the issue, before starting to type this comment.

However, my general feeling is that abstract methods in traits are intended to express the requirements that the trait has on an implementing class. As such, I would expect a typed language to validate these requirements, and decide whether what is offered is actually compatible.

Back in the day, this was simply whether a required method was there, and perhaps checking that it had the right number of arguments. Now with types being a thing, I would expect those also to be validated as in any other case.
The trait says that it needs a ClientState, so, if you provide it with a NativeClientState, that should better be compatible. If those two classes are not in the same hierarchy, perhaps it's better to introduce an interface that defines what they have in common and require that in the trait's abstract method?

Again, I am fairly out of touch with what has been happening, and I do not know PHP's type system, or philosophy on how to combine types with the dynamic nature of the language. So, please take this comment with the necessary grain of salt.

[nicolas-grekas]

Thanks a lot for your feedback @smarr. I think that clears my objections.

The validation done by traits should not account for the visibility, isn't it @nikic?

Having abstract private methods (in traits only) would still be nice IMHO for expressing the need to implement a method without forcing a public/protected visibility. Maybe that's for a separate PR?

[smarr]

Just to add another note on visibility: Visibility requirements would seem odd to me, because the trait is going to be flattened into the class and thus has full access. So, I would think that a trait should be satisfied when the class provides a private method to it.

[nikic]

I've update the PR to now:

• Check visibility per the usual rules. I.e. you can always make it more visible but not less.
• Check that staticness matches.
• Allow abstract private methods inside traits.

I think visibility in traits should follow the same rules we have everywhere else, i.e. if you have abstract protected function foo() in the trait, you can have either protected function foo() or public function foo() in the implementation.

If you don't care about the visibility of the method, then you can use abstract private function foo(), because that allows arbitrary visibility under our usual rules.

[nicolas-grekas]

Check visibility per the usual rules. I.e. you can always make it more visible but not less.

This is not required from the pov of the trait. I think this should be removed.
The class should be able to decide for the public/protected API it wants to provide, not the trait.

[nikic]

Check visibility per the usual rules. I.e. you can always make it more visible but not less.

This is not required from the pov of the trait. I think this should be removed.
The class should be able to decide for the public/protected API it wants to provide, not the trait.

I kinda see your point, but don't see the value in making this behave differently from all the rest of PHP. The visibility is generally considered part of the method contract just like everything else, and there already is a way (under this proposal) to say that you're fine with any visibility: abstract private foo(); That makes it completely clear that the method is a private requirement of the trait and you're free to implement it at whatever visibility you want.

Something to keep in mind is that there's also the possibility that the using class is abstract, in which case a child class may be providing the implementation of the abstract trait method. That's a scenario where visibility concerns do very much matter, and I think it would be odd to treat that case differently from the others.

[nicolas-grekas]

It's a matter of responsibility: let's say a trait is provided by a third-party library and it defines a public or protected method.

Then a consumer is forced to provide that same method in their API. This reduces the usefulness of traits for no reasons - ie traits are meant for horizontal reusability, and this rule makes them less reusable, while they don't require it to ensure they will work.

[nikic]

To reiterate: If the trait does not care about the visibility of the implementing method, it can use an abstract private method:

trait T {
    abstract private function neededByTrait(): string
}

class C {
    use T;

    // This is fine.
    private function neededByTrait(): string {}
    // This is also fine!
    protected function neededByTrait(): string {}
    // This is also fine!
    public function neededByTrait(): string {}
}

The only reason why the trait would use something other than abstract private is if it does want to enforce a minimum visibility on the implementing class. If it doesn't want to, it would be using private...

Why would the trait want to enforce a minimum visibility? Maybe the implementation requires it:

trait T {
    abstract public function callback();

    public function registerCallback() {
        passSomewhere([$this, 'callback']); // Requires public visibility!
    }
}

I'd not write that code and use Closure::fromCallable() instead, but this is just to illustrate that visibility can be an implementation requirement.

More likely though the trait is a helper to implement some contract, in which case the method may need to be public. However, this should usually be enforced by a separate implementation of an interface, so enforcing it from the trait side is not strictly necessary.

Now, there is one more reason why one would use an abstract protected method and not want the visibility enforced: Backwards compatibility with PHP versions that don't support abstract private. I think this is what you're really concerned about here. If that's the case, then please do say so, because there's a lot of difference between "we should do this because that's how it should work" (which I disagree with) and "we should do this because it helps migrating code" (which I'm sympathetic to).

[nicolas-grekas]

we should do this because that's how it should work

Yes, that's what I mean. A library author should not be able to force a consumer to adopt a public/protected method. That's way past the responsibilities the language should pass to authors of libraries (when using traits).

we should do this because it helps migrating code

This is a side effect of the previous point. Let's say Symfony is defining abstract protected method in traits: turning that into a private method would be a BC break. i.e PHP8 would force a major version on Symfony OR it would force a BC break on code that alias these protected methods to private and are just fine.

I think you really need to reconsider.

[theodorejb]

A library author should not be able to force a consumer to adopt a public/protected method.

Don't libraries do this all the time with abstract classes? Why should traits have a different behavior?

Let's say using Symfony is defining abstract protected method in traits

It sounds like this is a hypothetical. But regardless, if a consumer implements the method as private, that seems likely to be a bug. If a library trait defines a protected or public method, there may be other parts of the library that expect to be able to call the method.

[nicolas-grekas]

Traits are supposed to be horizontal reusability units. The fact that we can alias or change the visibility of a method when using them is the big hint here if one doesn't get why they shouldn't enforce visibility. To enforce some visibility, there are already dedicated language constructs: classes and interfaces.

[Girgias]

I really don't see why the visibility shouldn't be enforced, as said by @nikic if the methods within the trait don't care about the visibility of the method they depend on, it can just be declared as abstract private. And to me being able to restrict the visibility is clearly nonsensical, horizontal re-usability or not.

Edit: also it is possibly to explicitly change the visibility of methods in traits currently: https://www.php.net/manual/en/language.oop5.traits.php#language.oop5.traits.visibility

[gharlan]

@nikic Would this be possible?

trait T 
{
    abstract private function execute(): void;
}

class C
{
    use T {
        execute as executeForT;
    }

    private function executeForT(): void
    {
        // do stuff for the trait implementation
    }

    public function execute(Foo $foo): Bar
    {
        // public method for doing something other
    }
}

[nikic]

@gharlan No, this would not be possible. This is the reason why use T { x as y } creates aliases, not replacements. The original function name must remain available for use by the trait.

[nikic]

Ooops, closed the wrong PR by accident.

[nikic]

I've updated the PR and RFC to no longer validate visibility, for better backwards-compatibility with the existing abstract protected pattern.

[TysonAndre]

Suggested change

	/* For backwards-compatiblity reasons, visibility is not enforced here. */
	/* For backwards-compatibility reasons, visibility is not enforced here. */

[TysonAndre]

EDIT: Never mind, created 5433 to document the change to ReflectionMethod

EDIT: Sorry, I forgot this was mentioned in https://externals.io/message/109377 , but documentation still applies

The way this affects ReflectionMethod->isConstructor should be documented in UPGRADING (e.g. for public function __construct() in an interface), and https://www.php.net/manual/en/reflectionmethod.isconstructor.php .

In php <= 7.4, it returned false for abstract constructors. I'd still think this change of returning true is more correct.
In php 8.0, it now returns true.

    protected function canMockMethod(ReflectionMethod $method)
    {
        if ($method->isConstructor() ||
            $method->isFinal() ||
            $method->isPrivate() ||
            isset($this->blacklistedMethodNames[$method->getName()])) {
            return false;
        }

        return true;

[nikic]

Uh, how is this related to this patch? This code is about generating error messages.

[TysonAndre]

Sorry, I was in a hurry and misread what this was doing and posted the comment for the wrong PR before reading the thread - it's reading ZEND_ACC_CTOR, but not setting it

The relevant change[1] is that now the class entry members
serialize_func, unserialize_func, constructor, destructor and clone are
set, if the respective methods are declared on an interface.

https://github.com/php/php-src/pull/3846/files#diff-3a8139128d4026ce0cb0c86beba4e6b9L5549-R5605