Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118367
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-758-ge0d20a54e1-fm-20220729.001-ge0d20a54
Mime-Version: 1.0
Message-ID: <6a5c06d3-3816-4d1b-8c9b-6884346812eb@www.fastmail.com>
In-Reply-To: <14237a5f-0522-48b4-8d3f-8dc346dd0870@www.fastmail.com>
References: <14237a5f-0522-48b4-8d3f-8dc346dd0870@www.fastmail.com>
Date: Sun, 07 Aug 2022 13:56:14 -0500
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] RFC Idea: Introducing a ReflectionWithAttributes interface -
 Looking for feedback
From: larry@garfieldtech.com ("Larry Garfield")

On Sun, Aug 7, 2022, at 6:56 AM, Ollie Read wrote:
> Hello all,
>
> A while back, I posted on this mailing list with a link to a gist that 
> had a brain dump proposal for improvement to reflection essentially, 
> but I'd like to start again by including more detail here and by 
> breaking it up.
>
> The current implementation of attributes with PHP is great, but it 
> feels like it's lacking a bit. What I'd like to do is introduce 3 new 
> methods that fall more in line with the way that other child elements 
> are handled within reflection and then introduce an interface called 
> ReflectionWithAttributes, which can be implemented by all Reflector 
> implementations that have attributes.
>
> I would like to put all of this in an RFC and would be more than happy 
> to have a go at doing the work, too, should it be something people are 
> interested in, but I wanted to put it on here first to get some 
> feedback, so here's the breakdown/my thinking.
>
> ## The Interface
> Attributes are unlike most other parts of reflection in that their 
> usage and effect on a codebase or application are entirely arbitrary. 
> Because of this, a lot of applications and packages will likely have 
> handlers and wrappings around the reflection of attributes, and having 
> a "ReflectionWithAttributes" interface would allow developers to handle 
> this without having to create rather annoyingly long union types for 
> all the reflection classes that have attributes.
>
> As far as I can tell, the introduction of this interface would not be a 
> disruption in the slightest, so there's no real downside to doing so.
>
> ## The Methods
> There are several methods that developers could benefit from with the 
> above interface.
>
> ### getAttributeTarget
> Of all the methods, this is the one I'm most uncertain about, but the 
> idea is simple. ReflectionWithAttribute::getAttributeTarget returns the 
> corresponding constant in Attribute that is relevant to the current 
> implementation. So ReflectionClass::getAttributeTarget would return 
> Attribute::TARGET_CLASS.
>
> The idea behind this is that a developer can accept the type 
> ReflectionWithAttribute, and won't have to do any instanceof checks to 
> figure out what sort of attributes they're dealing with in a situation 
> where that's important before retrieving the actual attributes.
>
> I'm relatively certain this would be trivial to implement, as the 
> implementation will know what it is.
>
> ### getAttribute
> There is currently a way to retrieve all of the attributes, but not 
> just one. There are ways to do this in userland, but I'd wager that a 
> good percentage of the time, there's only one attribute there.
>
> The idea behind this method is to bring this more inline with other 
> parts of reflection, where the other methods have singular versions to, 
> ie, getMethods, getMethod, getProperties, getProperty, etc, etc.
>
> Again, I suspect the implementation of this would be trivial, as it 
> could essentially wrap getAttributes but just return the first item, as 
> it would have an identical signature, except for the singular return 
> type or null. There's also the option of throwing an exception if 
> there's more than one match to avoid unforeseen side effects.
>
> ### hasAttribute
> This method is again an attempt to bring attributes more inline with 
> the rest of reflection, as we have hasMethod, hasProperty, etc.
>
> Much like getAttribute, this method would have an identical signature 
> to getAttributes, but would return a bool instead. Again, this could 
> just wrap getAttributes returning that its size is greater than 0.
>
> ### getNumberOfAttributes
> For all intents and purposes, this method is identical to hasAttribute 
> except that it returns the actual size from getAttributes. In fact, 
> hasAttribute could probably wrap this. The idea behind this method is 
> to mirror the ReflectionFunctionAbstract::getNumberOfParameters method.
>
> Like the others, its signature would be the same, except that it 
> returns an int. 
>
> ## Conclusion
> Would love to hear any thoughts on this, I know that a few developers 
> that I have spoken to outside of this are fond of the idea, and as far 
> as I can tell, the implementation wouldn't be too difficult, and 
> shouldn't cause any disruptions at all. And as I said, I'm more than 
> happy to take on the task of implementing this, rather than expect 
> someone else to do it.
>
> If you're interested, the brain dump gist is here: 
> https://gist.github.com/ollieread/34c878bf120ee70f9d2a869cb7a242d1#attributes

Having an interface that applies to "things that have  attributes" I am fully behind.  In my own attribute library[1] dealing with that right now is an absolute mess, requiring either this:

public function getInheritedAttribute(\ReflectionClass|\ReflectionMethod|\ReflectionProperty|\ReflectionParameter|\ReflectionClassConstant $target, string $name): ?object
{
  // ...
}

which is just gross, or this:

public function getInheritedAttribute(\Reflector $target, string $name): ?object
{
  // ...
}

The latter of which works at runtime kinda by accident, but is technically wrong as Reflector does not define the attribute methods.

So I'm all for adding a proper interface here.  However, I am unclear why it needs to be separate from Reflector.  Reflector is already used by all the same classes that can have attributes, no?  Wouldn't just adding the attribute methods to that interface be sufficient?

I'm mixed on the new methods.  The current equivalent of getAttribute() is:

$r->getAttributes($attrClass, \ReflectionAttribute::IS_INSTANCEOF)[0] ?? null;

Which I don't mind.  My main concern there would be how it interacts with repeating/non-repeating attributes.  Can you call getAttribute() on a repeating attribute?  Can you call getAttributes() on a non-repeating? Do they behave any differently?  There's likely answers to these questions, but they would need to be asked.

Would hasAttribute() and getNumberOfAttributes() have any performance benefit over calling count() on getAttributes()?  Again, this isn't a pain point I've had with attributes yet.  If so, I'd combine them into an attributeCount() method that returns an int, and if it returns 0 you know it wasn't defined.

[1] https://github.com/Crell/AttributeUtils