RFC: Trait expects interface

Weirdan

4 years ago

On Wed, Jan 5, 2022 at 12:36 AM Kirill Nesmeyanov <nesk@xakep.ru> wrote:

> Since «traits» are often an indicator of not very good code and many may not use them quite correctly, for example, as helpers, I suggest adding support for the `expects` keyword to indicate that the trait is part of the code decomposition taking into account ISP.

Prior art: @psalm-require-extends and @psalm-require-implements Psalm
annotations: https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-require-extends

--
Best regards,
Bruce Weirdan                                     mailto:weirdan@gmail.com

Dusk

4 years ago

On Jan 4, 2022, at 14:35, Kirill Nesmeyanov <nesk@xakep.ru> wrote:

> Since «traits» are often an indicator of not very good code and many may not use them quite correctly, for example, as helpers, I suggest adding support for the `expects` keyword to indicate that the trait is part of the code decomposition taking into account ISP.

I like this idea, but I think it'd be even more useful if it could be used with classes as well as interfaces. This would allow traits to reject their use outside an intended class hierarchy.

(And, while we're at it: could it be useful for a trait to expect another trait?)

Larry Garfield

4 years ago

On Tue, Jan 4, 2022, at 4:35 PM, Kirill Nesmeyanov wrote:

> Hello internals!
>
> Since «traits» are often an indicator of not very good code and many
> may not use them quite correctly, for example, as helpers, I suggest
> adding support for the `expects` keyword to indicate that the trait is
> part of the code decomposition taking into account ISP.
>
> For example:
> ```
> // Definition
>
> trait LoggerTrait expects LoggerInterface
> {
>     // ...
> }
>
> // Usage
>
> class MyService
> {
>     use LoggerTrait; // Fatal Error: Class MyService
> expects LoggerInterface to be implemented
> }
>
> class MyService2 implements LoggerInterface
> {
>     use LoggerTrait; // OK
> }
> ```
>
> How relevant do you think this idea/proposal is? And what possible
> problems or solutions will this entail in the future?

I can't say this has ever been an issue for me when using traits.  I've never had a trait that needed to be used by a class with a given interface.  What I usually have is a trait that is a full or mostly implementation of an interface. So this would be much more useful to me:

trait LoggerTrait implements LoggerInterface { ... }

(Perhaps some way to indicate that it mostly implements, and the rest are abstract methods?  Or require the other methods to be explicitly abstract?)

I think it's subtly different, as it approaches the question from the other direction.

--Larry Garfield

Кирилл Несмеянов

4 years ago

azjezz

4 years ago

> Hello internals!
>

> Since «traits» are often an indicator of not very good code and many may not use them quite correctly, for example, as helpers, I suggest adding support for the `expects` keyword to indicate that the trait is part of the code decomposition taking into account ISP.
>

> For example:
>

>     // Definition
>

>     trait LoggerTrait expects LoggerInterface
>     {
>         // ...
>     }
>

>     // Usage
>

>     class MyService
>     {
>         use LoggerTrait; // Fatal Error: Class MyService expects LoggerInterface to be implemented
>     }
>

>     class MyService2 implements LoggerInterface
>     {
>         use LoggerTrait; // OK
>     }
>

>

> How relevant do you think this idea/proposal is? And what possible problems or solutions will this entail in the future?
>

> ---------------------------------------------------------------------------------------------------------------------------
>

> Kirill Nesmeyanov

Hello Kirill

I really like to have this feature, this is not just useful for implementing interfaces, but also to enforce where a trait can be used.

e.g: if `ReadUserInputTrait` should only be used in console commands, so putting a requirement to implement the console command interface, will ensure that the trait is not misused.

However, i personally prefer the HackLang syntax of doing this.

Which is:

```
trait Foo
{
require implements Bar;
require extends Baz;
}
```

This syntax also will result in no new keywords, which is always preferred, but not necessarily a requirement.

ref: https://docs.hhvm.com/hack/traits-and-interfaces/trait-and-interface-requirements

Regards,

Saif Eddin Gmati.

https://void.tn

Rowan Collins

4 years ago

On 04/01/2022 22:35, Kirill Nesmeyanov wrote:

> Since «traits» are often an indicator of not very good code and many may not use them quite correctly, for example, as helpers, I suggest adding support for the `expects` keyword to indicate that the trait is part of the code decomposition taking into account ISP.

Hi Kirill,

I'm a little confused what problem this is trying to solve - in what way
does using a trait in the "wrong" inheritance hierarchy constitute "abuse"?

On 04/01/2022 23:10, Bruce Weirdan wrote:

> Prior art: @psalm-require-extends and @psalm-require-implements Psalm
> annotations:https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-require-extends

... and on 05/01/2022 12:36, Saif Eddin Gmati wrote:

> ref:https://docs.hhvm.com/hack/traits-and-interfaces/trait-and-interface-requirements

The examples in both of these cases appear to be using the requirements
primarily as short-hand for listing abstract methods in the trait.

For instance:

interface FooInterface {
     public function doFoo();
}
trait SillyFooTrait {
    require implements FooInterface;
    public function doFooTwice() {
        $this->doFoo();
        $this->doFoo();
   }
}

Is essentially equivalent to:

trait SillyFooTrait {
    abstract public function doFoo();
    public function doFooTwice() {
        $this->doFoo();
        $this->doFoo();
   }
}


In other words, the requirement is there because it is actually a
requirement for the trait to work correctly, not because of some
perceived "correct" use of the trait. This doesn't seem to match your
reasoning for proposing the syntax, so maybe I'm missing something?


Regards,

--
Rowan Tommins
[IMSoP]

David Gebler

4 years ago

On Tue, Jan 4, 2022 at 10:35 PM Kirill Nesmeyanov <nesk@xakep.ru> wrote:

> How relevant do you think this idea/proposal is? And what possible
> problems or solutions will this entail in the future?
>

I'm not convinced there's a reasonable need for it. The very nature of
finding yourself in a situation where you want any class using a trait to
also require other things outside the trait kind of suggests you really
want to be using interfaces or abstract classes anyway.

There is a similar concept for what I think you're trying to achieve in
Java, though, which could also be useful in PHP if it was feasible within
the engine - the ability to provide a default method implementation on
interfaces themselves.

Short of that, we can already effectively get there with the tools we have;
an abstract class can use a trait and define abstract or concrete methods,
and in doing so can implement one or more interfaces. Obviously traits can
also declare abstract methods but I assume it's the identity/type aspect of
an interface you want here which isn't satisfied by that approach.

Also worth noting, although I can't say I'm familiar with the mechanics of
traits in PHP's implementation, my understanding has always been that
they're effectively compiler-level copy and paste in to a class so I'm not
sure what interfaces are implemented by the class (or conversely allowing a
trait to implement an interface) would be readily achievable.

That's my two cents, good luck with whatever you're trying to do anyway.

- David

Chase Peeler

4 years ago

On Wed, Jan 5, 2022 at 2:17 PM David Gebler <davidgebler@gmail.com> wrote:

> On Tue, Jan 4, 2022 at 10:35 PM Kirill Nesmeyanov <nesk@xakep.ru> wrote:
>
> > How relevant do you think this idea/proposal is? And what possible
> > problems or solutions will this entail in the future?
> >
>
> I'm not convinced there's a reasonable need for it. The very nature of
> finding yourself in a situation where you want any class using a trait to
> also require other things outside the trait kind of suggests you really
> want to be using interfaces or abstract classes anyway.
>
> There is a similar concept for what I think you're trying to achieve in
> Java, though, which could also be useful in PHP if it was feasible within
> the engine - the ability to provide a default method implementation on
> interfaces themselves.
>
> Short of that, we can already effectively get there with the tools we have;
> an abstract class can use a trait and define abstract or concrete methods,
> and in doing so can implement one or more interfaces. Obviously traits can
> also declare abstract methods but I assume it's the identity/type aspect of
> an interface you want here which isn't satisfied by that approach.
>
> Also worth noting, although I can't say I'm familiar with the mechanics of
> traits in PHP's implementation, my understanding has always been that
> they're effectively compiler-level copy and paste in to a class so I'm not
> sure what interfaces are implemented by the class (or conversely allowing a
> trait to implement an interface) would be readily achievable.
>
> That's my two cents, good luck with whatever you're trying to do anyway.
>
> - David
>
>
> > --
> > Kirill Nesmeyanov
>

First, I'm someone that mainly uses traits to implement the functionality
defined in an interface. I think that's one of the best uses for them.
However, I'm personally not a huge fan of overly restrictive things. For
instance, while there are definitely some use cases for them, I need a
REALLY good reason to justify making a property/method private instead of
protected, or making a class final.

As such, I think this would be better if it didn't throw a fatal error.
When you make it optional, however, I think you are left with something
that can be handled with an attribute just as well as a new keyword:
#[Expects('MyInterface')]
trait foo { }

However, there might be value in generating a notice/warning, and I think
that would require a keyword, correct? (Not that up to speed on
annotations). Another option might be two support two new keywords:
requires and expects. The former would throw an error if the interface
isn't implemented while the latter will throw a warning/notice/nothing.

Another option (and I haven't thought about this one enough to decide if I
like it) would be to have the expected interface automatically implemented
in the using class. This would allow the trait to be written under the
assumption it has access to the methods defined in the interface, and will
then throw an error if any of the methods are not implemented in the using
class:

interface foo {
function a();
function b();
}

trait bar expects foo {
function c(){
return $this->a() + $this->b();
}
}

class baz {
use foo;
}

The above would throw an error since a() and b() are never implemented and
baz is implementing the foo interface. You can currently get the same
behavior if you define a() and b() as abstract in the trait. However, this
doesn't give you the added benefit of utilizing the interface automatically
within the type system. The more I think about it, the less I like this
idea, since it doesn't require that much additional work to make the code
clearer by explicitly implementing the interface on the class if you want
it implemented. However, I'll go ahead and leave it here because it might
help generate some other ideas.

--
Chase Peeler
chasepeeler@gmail.com

Larry Garfield

4 years ago

On Wed, Jan 5, 2022, at 2:35 PM, Chase Peeler wrote:

> First, I'm someone that mainly uses traits to implement the functionality
> defined in an interface. I think that's one of the best uses for them.
> However, I'm personally not a huge fan of overly restrictive things. For
> instance, while there are definitely some use cases for them, I need a
> REALLY good reason to justify making a property/method private instead of
> protected, or making a class final.

I am much the same.

> As such, I think this would be better if it didn't throw a fatal error.
> When you make it optional, however, I think you are left with something
> that can be handled with an attribute just as well as a new keyword:
> #[Expects('MyInterface')]
> trait foo { }
>
> However, there might be value in generating a notice/warning, and I think
> that would require a keyword, correct? (Not that up to speed on
> annotations). Another option might be two support two new keywords:
> requires and expects. The former would throw an error if the interface
> isn't implemented while the latter will throw a warning/notice/nothing.
>
> Another option (and I haven't thought about this one enough to decide if I
> like it) would be to have the expected interface automatically implemented
> in the using class. This would allow the trait to be written under the
> assumption it has access to the methods defined in the interface, and will
> then throw an error if any of the methods are not implemented in the using
> class:
>
> interface foo {
>   function a();
>   function b();
> }
>
> trait bar expects foo {
>    function c(){
>        return $this->a() + $this->b();
>    }
> }
>
> class baz {
>   use foo;
> }
>
> The above would throw an error since a() and b() are never implemented and
> baz is implementing the foo interface. You can currently get the same
> behavior if you define a() and b() as abstract in the trait. However, this
> doesn't give you the added benefit of utilizing the interface automatically
> within the type system. The more I think about it, the less I like this
> idea, since it doesn't require that much additional work to make the code
> clearer by explicitly implementing the interface on the class if you want
> it implemented. However, I'll go ahead and leave it here because it might
> help generate some other ideas.

I... still don't see any use in this annotation.

Stepping back and ignoring the syntax for a moment, there's two different things here:

1. "This trait expects to be used in a class that has these other methods on it".
2. "This trait mostly/fully fulfills interface X, because it has methods a, b, and c."

For point 1, we already have that.  It's called abstract methods in traits.  This is a solved problem that requires no further resolution.  At best it would be a shorthand to copying a few methods from an interface into the trait and sticking "abstract" in front of them.  I really don't see a need for that.

For point 2, that's mainly useful as a way to signal to other developers "hey, this trait has all but one method of the LoggerInterface, that's how you'd use it", and to signal static analyzers and refactoring tools the same thing so that they can be auto-updated if you tweak the interface.  I can see a use for point 2, and it would make my life a bit easier, but it's overall not high priority.

--Larry Garfield

David Gebler

4 years ago

On Wed, Jan 5, 2022 at 11:05 PM Larry Garfield <larry@garfieldtech.com>
wrote:

> On Wed, Jan 5, 2022, at 2:35 PM, Chase Peeler wrote:
>
> For point 2, that's mainly useful as a way to signal to other developers
> "hey, this trait has all but one method of the LoggerInterface, that's how
> you'd use it", and to signal static analyzers and refactoring tools the
> same thing so that they can be auto-updated if you tweak the interface.  I
> can see a use for point 2, and it would make my life a bit easier, but it's
> overall not high priority.
>
>

I think existing constructs provide 99% of this benefit anyway.

interface I { public function hello(); }
abstract class A implements I { abstract public function hello(); }
trait T { public function hello() { echo 'hello'; } }
class C extends A { use T; }
// or alternatively
class D implements I { use T; }
// or
abstract class E implements I { use T; }
class F extends E { ... }

$c = new C;
$c->hello(); // $c is instanceof I
$d = new D;
$d->hello(); // $d is instanceof I
$f = new F;
$f->hello(); // $f is instanceof I

It's not quite as neat as directly having a way of annotating T to say it
implements I, but it does the job insofar as your IDE and tools should be
able to immediately pick up a change in interface I is not fulfilled by
anything relying on trait T to be of type I, either directly or by
inheritance.

The remaining 1% of benefit could probably be achieved just by naming
convention:

interface LoggerInterface { ... }
trait LoggerInterface_FileLogger { ... }

But I still think both goals would be better achieved with something like:

interface LoggerInterface {
default public function error(string $message) {
$this->logger->log('error', $message);
}
}

in the manner of Java...no idea how easy or not it would be for someone a
little more experienced than me working on PHP core to do this though.

- David

Chase Peeler

4 years ago

On Wed, Jan 5, 2022 at 6:05 PM Larry Garfield <larry@garfieldtech.com>
wrote:

> On Wed, Jan 5, 2022, at 2:35 PM, Chase Peeler wrote:
>
> > First, I'm someone that mainly uses traits to implement the functionality
> > defined in an interface. I think that's one of the best uses for them.
> > However, I'm personally not a huge fan of overly restrictive things. For
> > instance, while there are definitely some use cases for them, I need a
> > REALLY good reason to justify making a property/method private instead of
> > protected, or making a class final.
>
> I am much the same.
>
> > As such, I think this would be better if it didn't throw a fatal error.
> > When you make it optional, however, I think you are left with something
> > that can be handled with an attribute just as well as a new keyword:
> > #[Expects('MyInterface')]
> > trait foo { }
> >
> > However, there might be value in generating a notice/warning, and I think
> > that would require a keyword, correct? (Not that up to speed on
> > annotations). Another option might be two support two new keywords:
> > requires and expects. The former would throw an error if the interface
> > isn't implemented while the latter will throw a warning/notice/nothing.
> >
> > Another option (and I haven't thought about this one enough to decide if
> I
> > like it) would be to have the expected interface automatically
> implemented
> > in the using class. This would allow the trait to be written under the
> > assumption it has access to the methods defined in the interface, and
> will
> > then throw an error if any of the methods are not implemented in the
> using
> > class:
> >
> > interface foo {
> >   function a();
> >   function b();
> > }
> >
> > trait bar expects foo {
> >    function c(){
> >        return $this->a() + $this->b();
> >    }
> > }
> >
> > class baz {
> >   use foo;
> > }
> >
> > The above would throw an error since a() and b() are never implemented
> and
> > baz is implementing the foo interface. You can currently get the same
> > behavior if you define a() and b() as abstract in the trait. However,
> this
> > doesn't give you the added benefit of utilizing the interface
> automatically
> > within the type system. The more I think about it, the less I like this
> > idea, since it doesn't require that much additional work to make the code
> > clearer by explicitly implementing the interface on the class if you want
> > it implemented. However, I'll go ahead and leave it here because it might
> > help generate some other ideas.
>
> I... still don't see any use in this annotation.
>
> Stepping back and ignoring the syntax for a moment, there's two different
> things here:
>
> 1. "This trait expects to be used in a class that has these other methods
> on it".
> 2. "This trait mostly/fully fulfills interface X, because it has methods
> a, b, and c."
>
> For point 1, we already have that.  It's called abstract methods in
> traits.  This is a solved problem that requires no further resolution.  At
> best it would be a shorthand to copying a few methods from an interface
> into the trait and sticking "abstract" in front of them.  I really don't
> see a need for that.
>
>

Agreed. It would also allow IDEs and what not to determine the trait has
access to the interface methods that you didn't explicitly define as
abstract, but I still agree that it would just be shorthand that isn't
really needed.

> For point 2, that's mainly useful as a way to signal to other developers
> "hey, this trait has all but one method of the LoggerInterface, that's how
> you'd use it", and to signal static analyzers and refactoring tools the
> same thing so that they can be auto-updated if you tweak the interface.  I
> can see a use for point 2, and it would make my life a bit easier, but it's
> overall not high priority.
>
>

I also agree.

I was focusing more on fleshing out the idea in general. I think the idea
is interesting enough that it's worth discussing even if it doesn't seem
like it's worth pursuing in its current form.

> --Larry Garfield
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>

--
Chase Peeler
chasepeeler@gmail.com

Robert Korulczyk

4 years ago

> For point 1, we already have that.  It's called abstract methods in traits.  This is a solved problem that requires no further resolution.  At best

it would be a shorthand to copying a few methods from an interface into the trait and sticking "abstract" in front of them.  I really don't see a need
for that.

You can say exactly the same about traits in general - you don't need traits at all, since you can copy method implementation into multiple classes,
in the same way as you're proposing to copy abstract methods declarations. But traits are implemented and they're useful - they reduce the amount of
code duplication, which means less work and a smaller surface for errors. It is the same case for this proposal - if you don't need to copy&paste
method declarations from an interface, that means less code duplication and possible errors. It should also make it easier for SCA tools to understand
the code since they no longer need to know the whole project to understand that method from trait is implementation of method from interface (which is
really tricky right now since it depends on context - trait can at the same implement and not implement the interface, because it depends on class
where it is used). And finally, you could use `{@inheritdoc}` in some meaningful way in traits - right now there is no straightforward way to inherit
phpdoc from the interface, and often this is what you want to do.

--
Regards,
Robert Korulczyk

Rowan Collins

4 years ago

On 6 January 2022 15:21:28 GMT, Robert Korulczyk <robert@korulczyk.pl> wrote:

> It should also make it easier for SCA tools to understand
>the code since they no longer need to know the whole project to understand that method from trait is implementation of method from interface (which is
>really tricky right now since it depends on context - trait can at the same implement and not implement the interface, because it depends on class
>where it is used).

Your other points make sense, but I don't think this one does - there are no implicit interfaces in PHP*, so all any tool cares about is:

1) Does the class declaration say that it implements an interface?
2) Does it actually contain the methods needed to do so, through any combination of direct implementation, inheritance, and trait usage?

Knowing that a particular trait *could be* used to implement a particular interface without further code doesn't really tell the tool anything - it still has to resolve the list of methods on the class itself, and test those against the "implements" clause. This is particularly true if the class uses the "as" and "insteadOf" clauses when including the trait, which nullify any promises the trait could make.

* other than "Stringable", whose purpose and implementation continue to baffle me

Regards,

--
Rowan Tommins
[IMSoP]

Chase Peeler

4 years ago

On Thu, Jan 6, 2022 at 11:20 AM Rowan Tommins <rowan.collins@gmail.com>
wrote:

> On 6 January 2022 15:21:28 GMT, Robert Korulczyk <robert@korulczyk.pl>
> wrote:
> > It should also make it easier for SCA tools to understand
> >the code since they no longer need to know the whole project to
> understand that method from trait is implementation of method from
> interface (which is
> >really tricky right now since it depends on context - trait can at the
> same implement and not implement the interface, because it depends on class
> >where it is used).
>
> Your other points make sense, but I don't think this one does - there are
> no implicit interfaces in PHP*, so all any tool cares about is:
>
> 1) Does the class declaration say that it implements an interface?
> 2) Does it actually contain the methods needed to do so, through any
> combination of direct implementation, inheritance, and trait usage?
>
> Knowing that a particular trait *could be* used to implement a particular
> interface without further code doesn't really tell the tool anything - it
> still has to resolve the list of methods on the class itself, and test
> those against the "implements" clause. This is particularly true if the
> class uses the "as" and "insteadOf" clauses when including the trait, which
> nullify any promises the trait could make.
>
>
> I think the advantage would come within the trait. If I say the trait

expects an interface with two methods, then the tool can act as if the
interfaces methods exist in the trait even if they aren't explicitly
defined. As others have pointed out, though, you can get the same behavior
from declaring the methods not implemented in the trait as abstract.

> * other than "Stringable", whose purpose and implementation continue to
> baffle me
>
> Regards,
>
> --
> Rowan Tommins
> [IMSoP]
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>

--
Chase Peeler
chasepeeler@gmail.com

Rowan Collins

4 years ago

On 6 January 2022 16:25:04 GMT, Chase Peeler <chasepeeler@gmail.com> wrote:

> I think the advantage would come within the trait. If I say the trait
>expects an interface with two methods, then the tool can act as if the
>interfaces methods exist in the trait even if they aren't explicitly
>defined. As others have pointed out, though, you can get the same behavior
>from declaring the methods not implemented in the trait as abstract.

This is where I wonder if people are talking at cross purposes.

The *only* advantage I've understood of the proposal is to save on copy and paste of abstract methods from an interface into a trait, and everything else is just restating that in more complicated ways. So, yes, static analysers could use the information about requiring an interface in exactly the same way they can already use abstract method signatures, to validate the trait itself - but that wouldn't be any *easier* for the tool, and in fact would be extra logic for them to implement, so it seems an odd thing to bring up.

But some comments seem to be hinting at some *separate* advantage, to do with "correct usage" of the trait, which I haven't grasped. It's possible that the mention of static analysis relates to that in some way, and I'm just completely missing the point.

Regards,

--
Rowan Tommins
[IMSoP]

Alexandru Pătrănescu

4 years ago

On Thu, Jan 6, 2022 at 7:12 PM Rowan Tommins <rowan.collins@gmail.com>
wrote:

>
> But some comments seem to be hinting at some *separate* advantage, to do
> with "correct usage" of the trait, which I haven't grasped. It's possible
> that the mention of static analysis relates to that in some way, and I'm
> just completely missing the point.
>

Yes, traits are a language construct that has in general more negative
implications than positive so it's good to keep an eye on their usage.
One of the okish usages is to define some implementation for an interface
that classes can use.
Limiting that trait to be used only by classes implementing the interface
is seen as a restriction placed on the trait that would not allow it to be
used in other places when it might have a negative impact.

Just as a note, on the composition vs inheritance line, I see traits
somewhere in the middle.
I'll always prefer to go for full composition, even if that means a bit
more boilerplate. The implementation can very well sit in a class and have
it as a dependency of the class that needs it.

Alex

Rowan Collins

4 years ago

On 6 January 2022 17:36:41 GMT, "Alexandru Pătrănescu" <drealecs@gmail.com> wrote:

>Yes, traits are a language construct that has in general more negative
>implications than positive so it's good to keep an eye on their usage.
>One of the okish usages is to define some implementation for an interface
>that classes can use.
>Limiting that trait to be used only by classes implementing the interface
>is seen as a restriction placed on the trait that would not allow it to be
>used in other places when it might have a negative impact.

The examples I've generally seen of problematic trait use are more about what's in the trait itself, rather than where it's used. I can't really picture what would be the "negative impact" of using somebody else's well-designed trait (and if it's not well designed, it presumably wouldn't make use of the new syntax). Can you give any example scenarios that you think mandating an interface would help avoid?

Regards,

--
Rowan Tommins
[IMSoP]

Chase Peeler

4 years ago

On Thu, Jan 6, 2022 at 12:37 PM Alexandru Pătrănescu <drealecs@gmail.com>
wrote:

> On Thu, Jan 6, 2022 at 7:12 PM Rowan Tommins <rowan.collins@gmail.com>
> wrote:
>
> >
> > But some comments seem to be hinting at some *separate* advantage, to do
> > with "correct usage" of the trait, which I haven't grasped. It's possible
> > that the mention of static analysis relates to that in some way, and I'm
> > just completely missing the point.
> >
>
> Yes, traits are a language construct that has in general more negative
> implications than positive so it's good to keep an eye on their usage.
> One of the okish usages is to define some implementation for an interface
> that classes can use.
> Limiting that trait to be used only by classes implementing the interface
> is seen as a restriction placed on the trait that would not allow it to be
> used in other places when it might have a negative impact.
>
>

And that's what I don't like about this. I'm totally OK with allowing
developers to do things the "wrong" way. It might end up that it's the
"right" way for their use-case.

> Just as a note, on the composition vs inheritance line, I see traits
> somewhere in the middle.
> I'll always prefer to go for full composition, even if that means a bit
> more boilerplate. The implementation can very well sit in a class and have
> it as a dependency of the class that needs it.
>
> Alex
>

>  but that wouldn't be any *easier* for the tool, and in fact would be

extra logic for them to implement, so it seems an odd thing to bring up.

I think most people see the tools as black boxes to make THEIR lives
easier. They aren't concerned about whether it's added complexity to the
tool itself. If PhpStorm allows me to reference
$this->someMethodFromAnInterface() without defining
someMethodFromAnInterface() as an abstract method in the trait because I
include "expects <interface>", then that means things are "simpler" for me
as a developer. I don't really think this makes things that much simpler
though.

--
Chase Peeler
chasepeeler@gmail.com

Robert Korulczyk

4 years ago

> Your other points make sense, but I don't think this one does - there are no implicit interfaces in PHP*, so all any tool cares about is:
>
> 1) Does the class declaration say that it implements an interface?
> 2) Does it actually contain the methods needed to do so, through any combination of direct implementation, inheritance, and trait usage?
>
> Knowing that a particular trait *could be* used to implement a particular interface without further code doesn't really tell the tool anything - it still has to resolve the list of methods on the class itself, and test those against the "implements" clause.

You're talking about classes that use traits, but I'm talking about traits themselves. If I open a class in editor, there is a straightforward way to
check if specific method is implementation of an interface - check interfaces implemented by class and if one of them have this method, then this
method is implementation of this interface, and editor can properly mark it and handle navigation between implementation in class and declaration in
interface. So even if I have a big project, you need to only load a subset of classes used by this project in order to find methods that are
implementations of interfaces in this particular class.

If I open trait in my editor, it is a lot more complicated. Editor needs to scan the whole project, find all classes that use this trait, find all
interfaces that are implemented by these classes, and then find matches. And you may still end up with confusing results, because if you have class
`A` that implements `FooInterface` and uses `FooTrait`, and also class `B` that uses `FooTrait`, but NOT implements `FooInterface`, then if you ask if
`FooTrait::someMethod()` is implementation of `FooInterface::someMethod()`, the answer is "yes and no".

Also, I'm not sure how it works now, but about 2 years ago I got rid of most of traits in one project, because navigation between trait and interface
worked so badly in PhpStorm, that it was easier to deal with code duplication than broken "implements" detection. At the same time navigation between
classes and interfaces worked without any problem.

--
Regards,
Robert Korulczyk

Rowan Collins

4 years ago

On 6 January 2022 19:51:46 GMT, Robert Korulczyk <robert@korulczyk.pl> wrote:

>You're talking about classes that use traits, but I'm talking about traits themselves. If I open a class in editor, there is a straightforward way to
>check if specific method is implementation of an interface - check interfaces implemented by class and if one of them have this method, then this
>method is implementation of this interface, and editor can properly mark it and handle navigation between implementation in class and declaration in
>interface.

Ah, I see what you're getting at now. I've never particularly had the need to jump between an interface and a trait, but then I very rarely use traits at all.

In your use cases, are these traits generally *using* the interface methods, or *implementing* them? For instance, you might have a trait that *requires* the LoggerInterface because it makes use of its methods, so you want to jump to their documentation when viewing those uses; or you might have a trait that helps *implement* the LoggerInterface, and you want to link each method to the definition in the interface that it implements. Is one of these situations more common than the other, in your experience?

Regards,

--
Rowan Tommins
[IMSoP]

Robert Korulczyk

4 years ago

> In your use cases, are these traits generally *using* the interface methods, or *implementing* them?

It is both. However "implementing" case is more problematic, since "using" can be quite reliably handled by adding `assert($this instanceof
LoggerInterface)` or type hints in trait methods - SCA tools should handle this and PHP will complain if interface is not implemented. But there is no
easy way to say "`FooTrait::someMethod()` is implementation of `FooInterface::someMethod()`" that PHP and SCA will understand. And I think this
proposal handles this quite well, since it does not introduce implicit "implements" (IMO introducing another way of "injecting" interfaces to classes
will just hurt readability) - you still needs to add interface to "implements" section in class definition, but traits have more tools to define its
purpose and ensure that classes have proper definition.

--
Regards,
Robert Korulczyk

Rowan Collins

4 years ago

On 06/01/2022 23:53, Robert Korulczyk wrote:

> But there is no easy way to say "`FooTrait::someMethod()` is
> implementation of `FooInterface::someMethod()`" that PHP and SCA will
> understand. And I think this proposal handles this quite well

I'm not convinced it does, actually. Consider the following trait:

trait PropertyCount {
    public function count(): int {
        return count(get_object_vars($this));
    }
}

This trait CAN be used to implement the built-in Countable interface,
and it might be useful to label it as such; but does it really make
sense to say that classes MUST implement that interface?

Even if we put it as a requirement, we can't guarantee that the class
will actually use the trait's implementation of the interface, because
this would still be valid:

class Foo implements Countable {
    private $whatever;

    use PropertyCount {
        count as getPropertyCount;
    }

    public function count(): int {
        return 0;
    }
}


It feels like this use case would work better with an annotation like
/** @can-implement Countable */ since it is really just documentation
about possible uses.

Regards,

--
Rowan Tommins
[IMSoP]