False positive on mixed attribute usage on promoted property

[RobertMe]

Bug report

When mixing both attributes that target parameters and attributes target properties on a promoted property an (incorrect) error will be reported for the attribute targeted at properties, that it isn't targeting properties and parameters.

(Found when mixing #[SensitiveParameter] with Symfony validator constraints on a promoted property)

Code snippet that reproduces the problem

https://phpstan.org/r/302d5bf8-304b-49aa-b013-7757886fcf4c

Expected output

No errors, instead it fails on PropAttr not having property or parameter target

Did PHPStan help you today? Did it make you happy in any way?

No response

[RobertMe]

The reason for this seems to be quite simple. This because ParamAttributeRule runs the AttributesCheck twice. Once for TARGET_PROPERTY and once for TARGET_PARAMETER. But in this case both will fail. When it runs for TARGET_PROPERTY it fails on ParamAttr because it can not be placed on a promoted property. And when it runs for TARGET_PARAMETER it fails for PropAttr as it can't be placed on a parameter.

I think the code can be simplified to just one run, with TARGET_PARAMETER | TARGET_PROPERTY. I've made the change locally and all tests seem to pass (including my added test). So will try to create a PR today or tomorrow. (But I'm wondering if the test isn't too simple, and there was a reason to go for the current solution.)

[ondrejmirtes]

Hi, PHPStan is right here. See this example in PHP runtime: https://3v4l.org/cg0Uj

[RobertMe]

Hmm. My actual code is using #[SensitiveParameter] combined with Symfony validator constraints. Those constraints seemingly work fine, while they have TARGET_PROPERTY. But I guess that's because the attributes are only read by their property, and not by the parameters (in other words: Symfony inspects the properties, and gets the attributes on that. And there is no code actually reading the attributes of the parameters). So as long as no code is iterating over the parameters and creating the attributes everything seemingly works fine and the properties do have the attributes. But as soon as you try to read properties of the parameters it errors.

And I believe that PHPStan is still slightly wrong then? Because it states that PropAttr doesn't have "parameter or property target". While it should be "doesn't have parameter target", not mentioning "property".

Furthermore the code now thus seems to support a case which isn't supported by PHP (i.e.: the code I changed was incorrect anyway). Which is also confirmed by running the test file: https://3v4l.org/Vr19G
According to the unit test it should only fail on Foo once, and Qux once (for usage on a plain param).
https://github.com/phpstan/phpstan-src/blob/91600a812c97b9f3ee200651eeb39f285d25a8a8/tests/PHPStan/Rules/Functions/ParamAttributesRuleTest.php#L53C4-L60

While PHP itself gives more errors, for every usage of Qux

Attribute "ParamAttributes\Foo" cannot target parameter (allowed targets: class)
Attribute "ParamAttributes\Qux" cannot target parameter (allowed targets: property)
Attribute "ParamAttributes\Qux" cannot target parameter (allowed targets: property)
Attribute "ParamAttributes\Qux" cannot target parameter (allowed targets: property)
Attribute "ParamAttributes\Qux" cannot target parameter (allowed targets: property)

So I guess the code I changed, should actually be dropped altogether? (/the conditional promoted property stuff should be dropped).

[ondrejmirtes]

I don't know, you can try sending a PR and we'll see what happens, either it makes sense or it doesn't 😊

[RobertMe]

Well, it would mean simply dropping these lines:
https://github.com/phpstan/phpstan-src/blob/91600a812c97b9f3ee200651eeb39f285d25a8a8/src/Rules/Functions/ParamAttributesRule.php#L30-L43

But... I think that's wrong too, and even more wrong then the current code. So then we can return to my original issue / PR 😃
Because as long as you don't try to instantiate the attribute everything works fine. You just mustn't try to instantiate a wrongly used / target attribute.
I.e.: see this example: https://3v4l.org/DGJGS
This works as one would expect. PHP throws two errors, which is when trying to instantiate PropAttr when reading the attributes of the param, and when trying to instantiate ParamAttr when reading the attributes on the property. So as long as one is "careful" in actually "reading" (/instantiating) the attribute it's just fine. And obviously one should be "careful" with that, because one just shouldn't go instantiate attributes used in a place it wasn't targeted for. And I don't think it's uncommon at all to apply property targeted attributes to promoted properties.

So to conclude I think this issue still is valid, and the PR is fine. But there should be a feature (request) for an additional rule which checks that in case when one does ->getAttributes(Foo::class) that not only Foo must be an attribute (i.e.: have the #[Attribute] attribute), but that the "origin" of the ->getAttributes (i.e.: ReflectionClass / ReflectionProperty / ReflectionMethod / ReflectionParameter / ...) also lines up with the target of said attribute.
So this piece of code should result in an error:

#[Attribute(Attribute::TARGET_CLASS)]
class Attr {}

function test() {}

$f = new ReflectionFunction('test');
$f->getAttributes(Attr::class);

So something like "Attr attribute is not a valid attribute for a function".

[ondrejmirtes]

Attributes with property target cannot be used above promoted properties. Pure and simple. It's PHPStan's job to report this.

You have the following options:

* Ignore this error if you think it's safe to have such code: https://phpstan.org/user-guide/ignoring-errors

• Argue over at Symfony framework that validator constrant attributes should also be allowed for parameters
• Argue over at php-src so that promoted properties also support attributes that are only for property targets

[RobertMe]

So to conclude, you would say that phpstan/phpstan-src#528 shouldn't have been merged before, and that #4418 should have been closed, and stay closed?

In other words: you already came back on the "PHP doesn't allow this, and neither should PHPStan" before, so are you now reverting back to the original "PHPStan shouldn't support this"? If so I will create a PR to revert phpstan/phpstan-src#528

And to be clear: no, I don't like the PHP behavior either. But it does work, and it clearly is used (as others already requested this feature well over 2 years ago, and my issue and PR just report a minor defect in the current implementation where applying both incorrectly reports an error, while applying either of them works just fine).

[RobertMe]

@ondrejmirtes could you please clarify your position in this?

You mentioned earlier that PHPStan should report an error when a TARGET_PROPERTY attribute is used on a promoted property. But shouldn't this result in an error as well then?
https://phpstan.org/r/99646bfd-4833-4c11-a403-9a9592479ecd
Because in this example there is no error, while an attribute which is only allowed on properties is used on a promoted property.

Furthermore, as I mentioned earlier. If this is your test case to show that PHP doesn't allow property attributes on promoted properties:

$t = new ReflectionClass(Test::class);
$a = $t->getConstructor()->getParameters()[0]->getAttributes();

foreach ($a as $aa){
    $aa->newInstance();
}

(as per your comment: #10298 (comment))
Then the test code: https://github.com/phpstan/phpstan-src/blob/21e1a8fea3415010f69405a2f0ad36bfa0bc2898/tests/PHPStan/Rules/Functions/data/param-attributes.php#L71-L84 should also result in more errors, as per https://3v4l.org/Vr19G (which results in an error for every property attribute Qux being read on a parameter). While this clearly isn't the case.

This is the case because of phpstan/phpstan-src#528, which fixes the in 2021, by @dbrekelmans reported, issue #4418 to add support for property attributes on parameters. While initially you took the same stance: "Not true, PHPStan is right. Runtime proof: https://3v4l.org/tBjj2". After later comments by @dbrekelmans, @dktapps, @michaelzangerle, @derrabus and @iquito you reconsidered the issue and came to the conclusion that it should be supported. So I really don't understand where the current stance on "PHP doesn't support this, and neither should PHPStan" comes from.

As @iquito wrote:

any library would only instantiate their own attributes and can easily avoid this fatal error

And returning to my example. When only reading the attributes for their intended target, everything works fine without any errors: https://3v4l.org/5HXZD
So when reading using:

$paramAttr = $t->getConstructor()->getParameters()[0]->getAttributes(ParamAttr::class)[0];
$propAttr = $t->getProperty('test')->getAttributes(PropAttr::class)[0];

var_dump($paramAttr->newInstance());
var_dump($propAttr->newInstance());

Where the parameter targeted property is fetched using ->getAttributes() on a ReflectionParameter and the property targeted property is fetched using ->getAttributes() on a ReflectionProperty.

So the only case were PHP will actually throw an error is either the contrived example where you're iterating over all attributes (and instantiating all of them), or the (unlikely) case where the (library) developer makes an incorrect ->getAttributes(...) call. And while PHPStan may (/should) detect such a scenario, that's something on rule on getAttributes(...) should detect. And isn't something which should be detected by the attribute placement. Because it, once again, is perfectly valid to place an attribute with property target on a promoted property, and this will never result in an error, unless the "reflection" side (->getAttributes(...)) is implemented incorrectly, which should be detected by a rule on ->getAttributes(...), and not give lots and lots of false positives on the actual attribute usage/placement.

[ondrejmirtes]

You're right, some errors were missed. The next release is going to include them (if you enable bleeding edge): phpstan/phpstan-src@25d1552

Thanks.

Otherwise my points from this comment stand: #10298 (comment)

Yeah, sure, if the user is careful and does not access something that would throw an error, your code might be fine. But this point is true for the whole field of static analysis.

If you're careful and only pass classes that have method doFoo to this function then the reported error is wrong. But it's really easy to make the mistake the analyser points out here: https://phpstan.org/r/100094a9-c03c-47ff-a34a-be3637fb6a10

[dktapps]

Not sure why I got notified about this, but my two cents is that it should be reported at the point of accessing the parameter attributes, and not at the declaration site.

IMO, the way this should have been implemented in php-src is to just not apply the attribute to the parameter at all if the attribute didn't have TARGET_PARAMETER flags. Not much we can do about that here though ...

[RobertMe]

You're right, some errors were missed. The next release is going to include them (if you enable bleeding edge): phpstan/phpstan-src@25d1552

I think this is already deployed on the demo / try page, correct? I haven't gotten the time/means to go over the change yet. But when I rerun my example from the original report and enabling bleeding edge it will error on the #[ParamAttr], which is incorrect also. The example code is perfectly fine and works perfectly fine. Also when reading the attributes for their intended purposes (so reading the parameter attribute using getParameters()...->getAttributes() and the (promoted) properties attribute using getProperty(...)->getAttributes()).

Yeah, sure, if the user is careful and does not access something that would throw an error, your code might be fine. But this point is true for the whole field of static analysis.

If you're careful and only pass classes that have method doFoo to this function then the reported error is wrong. But it's really easy to make the mistake the analyser points out here: https://phpstan.org/r/100094a9-c03c-47ff-a34a-be3637fb6a10

I'm sorry, but this completly contrived example doesn't make any sense to me. Of course this should result in an error because there is no guarantee such method exits.
But to return to the attributes case there are two possible validations:

1. Can the attribute be placed where it is placed (i.e.: if an atrribute has TARGET_PROPERY, is it actually placed on a property?). This is what PHPStan currently does, works fine, except my reported issue where placing both a param only and property only attribute on a promoted property fails
2. When reflecting the attributes (i.e. Reflection*->getAttributes(...)) the given class name must be an attribute which tagets the Reflection* type (so when calling getAttributes() on a ReflectionParameter the given class name must be an attribute with at least TARGET_PARAMETER)

And I'm reporting an issue with 1., and you keep coming back to 2..
So to come back to your comparison it's more like always giving an error on calling doFoo() because the call side doesn't have any guarantee that any given Foo implementation does have a doFoo() method. While there obviously is (/must be) an accompanying rule which checks that any class ... implements Foo actually does have all methods defined in the interface.
And the same applies here. One rule to check whether the attribute is placed where it may be placed (and a target property attribute may be placed on a promoted property), and another rule which checks that the given attribute for getAttributes does have at least the target of the Reflection* type is called on.
While currently PHPStan is/might be giving an enormous amount of errors on attribute placement, when the attribute placement is 100% correct and working. And the only place where such error you are constantly referring to can occur is in a library which exposes and reads (/reflects on) the attribute.

[ondrejmirtes]

We can agree to disagree.