Do not report constructor unused parameter if class is an Attribute class

[carlos-granados]

When defining Attribute classes usually the constructor parameters are not used, as the values are retrieved using reflection.

This PR modifies the UnusedConstructorParametersRule class so that unused constructor parameters are not reported if the class is an Attribute class.

Fixes phpstan/phpstan#7165

[carlos-granados]

There is a failing check but seems unrelated to the PR 🤔

[calebdw]

Simply promoting the property should fix the error:

[\Attribute]
class MyAttribute
{
    public function __construct(public string $name) {}
}

[ondrejmirtes]

Thank you!

[vdauchy]

Hi @ondrejmirtes ,

Just saw this and I really don't understand as this might hide real issues... the [\Attribute] might be used on any type of class, not just simple DTOs. The argument of 'this might be access through Reflection` is true for eveything...

In fact the access to property shall indeed be done like @calebdw described, meaning it shall be a public property.

Note to people using Reflection to access Attributes's constructor's parameters: To access attribute's values Reflection can be used with newInstance() to instanciate the attribute like so:

$attibute = (new \ReflectionObject($object))->getAttributes(MyAttribute::class)[0]->newInstance();

// Now you can access the public property with all type safety \o/
var_dump($attibute->name);

[ondrejmirtes]

@vdauchy You might access the attribute arguments through reflection with https://www.php.net/manual/en/reflectionattribute.getarguments.php which is what I think the author phpstan/phpstan#7165 is doing and that's why PHPStan reported false positives to them.

[ilmiont]

@ondrejmirtes

Can we at least have an option to disable this please?

This is a really strange change as @vdauchy outlined above.

We seem to have adjusted PHPStan to disable a potentially useful warning based on the requirements of one user in phpstan/phpstan#7165. Their code would almost certainly be better adjusted to call newInstance() on the ReflectionAttribute, then access the property in the standard way.

The original example

<?php
declare(strict_types=1);

namespace XX\XXX\Structs\Attribute;

use Attribute;

#[Attribute(Attribute::TARGET_PROPERTY)]
final class ArrayList
{
    public function __construct(string $structName)
    {}
}

class Ball
{
    #[ArrayList('balablub')]
    public array $colors = [];
}

class Example
{

    public function readArgumentValue(): void
    {
        $reflection = new \ReflectionClass(Ball::class);
        $reflectionProperties = $reflection->getProperties();
        $reflectionProperty = $reflectionProperties[0];
        $reflectionAttributes = $reflectionProperty->getAttributes();
        $reflectionAttribute = $reflectionAttributes[0];
        $reflectionArguments = $reflectionAttribute->getArguments();
        $reflectionArgument = $reflectionArguments[0]; // balablub
        echo $reflectionArgument;
    }
}

(new Example()) -> readArgumentValue();

Yes, this works, and the pattern is alluded to in the manual. But there is also a very important caveat:

Only after calling ReflectionAttribute::newInstance(), objects of the attribute class are instantiated and the correct matching of arguments is validated, not earlier.

Hence I don't believe this pattern should be routinely used to access Attributes data, because despite strict_types=1, the following code also runs successfully despite the type mismatch:

<?php
declare(strict_types=1);

namespace XX\XXX\Structs\Attribute;

use Attribute;

#[Attribute(Attribute::TARGET_PROPERTY)]
final class ArrayList
{
    public function __construct(string $structName)
    {}
}

class Ball
{
    // int being passed to string parameter
    #[ArrayList(12345)]
    public array $colors = [];
}

class Example
{

    public function readArgumentValue(): void
    {
        $reflection = new \ReflectionClass(Ball::class);
        $reflectionProperties = $reflection->getProperties();
        $reflectionProperty = $reflectionProperties[0];
        $reflectionAttributes = $reflectionProperty->getAttributes();
        $reflectionAttribute = $reflectionAttributes[0];
        $reflectionArguments = $reflectionAttribute->getArguments();
        $reflectionArgument = $reflectionArguments[0]; // 12345
        echo $reflectionArgument;
    }
}

(new Example()) -> readArgumentValue();

How the example almost certainly should be written (note that this is also shorter!)

<?php
declare(strict_types=1);

namespace XX\XXX\Structs\Attribute;

use Attribute;

#[Attribute(Attribute::TARGET_PROPERTY)]
final class ArrayList
{
    public function __construct(public readonly string $structName)
    {}
}

class Ball
{
    #[ArrayList('balablub')]
    public array $colors = [];
}

class Example
{

    public function readArgumentValue(): void
    {
        $reflection = new \ReflectionClass(Ball::class);
        $reflectionProperties = $reflection->getProperties();
        $reflectionProperty = $reflectionProperties[0];
        $reflectionAttributes = $reflectionProperty->getAttributes();
        $reflectionAttribute = $reflectionAttributes[0];
        echo $reflectionAttribute -> newInstance() -> structName;
    }
}

(new Example()) -> readArgumentValue();

---

As it stands, we now have the potential for unused constructor parameters to be missed in any class that happens to be an Attribute and I am not convinced the "benefit" is real. I really don't think that phpstan/phpstan#7165 should ever have been accepted as a bug.

As vdauchy suggested, you may as well remove all unused parameter checks altogether, because any parameter "might" be used in Reflection :S

[ondrejmirtes]

because any parameter "might" be used in Reflection :S

I don't think this is true outside of Attributes. Reflection doesn't have any concept of function call arguments, outside of Attributes.

Sorry, I don't have any plans adding this back for attribute classes. It's too much of an edge case, I don't think it's worth it to pollute the configuration parameters or the documentation.