PHPStan 2.1.12 compatibility

[ruudk]

PHPStan just released 2.1.12 and I checked out the Dependabot bump going from 2.1.11 to 2.1.12 and noticed that all my rules that disallow classes except when allowInMethodsWithAttributes now report issues.

<?php

declare(strict_types=1);

return [
    'parameters' => [
        'disallowedClasses' => [
            [
                'allowInUse' => true,
                'class' => EventRecorder::class,
                'allowInMethodsWithAttributes' => [
                    AsCommandHandler::class,
                ],
                'allowIn' => [
                    '**/tests/*',
                ],
            ],
        ],
    ],
];

I'll debug it further, but just wanted to hint it here already.

@spaze Maybe you can kick off a new CI builds that pulls in the latest PHPStan version to confirm?

[spaze]

All builds use the latest versions already by running composer update (except where --prefer-lowest is added):

phpstan-disallowed-calls/.github/workflows/php.yml

Line 70 in 4ece18a

run: composer update --no-progress --no-interaction ${{ matrix.update-options }}

The builds and tests are also executed automatically daily in the evening because a similar thing happened once already (#249, #251). I tried to run the workflow manually now as well, and it has failed. I'd know about it in a few hours, guaranteed :-)

[spaze]

Not sure the test failure is related to your problem though. It seems to be related to phpstan/phpstan-src#3925 and also tests are passing on my local, which is interesting. Asked phpstan/phpstan#12888

[spaze]

I have ignored the error that was breaking tests in #324. It was breaking them only on PHP 7.4/PHPStan 2 so I don't think it was related. The tests are now green with PHPStan 2.1.12. Let me know if you find something or if there's anything I could do.

[ruudk]

I will, thanks for the fast response and action. We'll narrow it down!

[ruudk]

Some code that is failing:

final readonly class SomeHandler
{
    #[AsCommandHandler]
    #[WrapInTransaction]
    public function __invoke(SomeCommand $command, Actor $actor, EventRecorder $eventRecorder) : void
    {}
}

with this config:

<?php
return [
    'parameters' => [
        'disallowedClasses' => [
            [
                'allowInUse' => true,
                'class' => EventRecorder::class,
                'allowInMethodsWithAttributes' => [
                    AsCommandHandler::class,
                ],
                'allowIn' => [
                    '**/tests/*',
                ],
            ],
        ],
    ],
];

And this code

final readonly class SomeCommand
{
    #[AsCommandHandler]
    #[IgnoreException(LocationNotFoundException::class)]
    public function __invoke(SomeCommand $command, Actor $actor, EventRecorder $eventRecorder) : void
    {}
}

with this config

<?php
return [
    'parameters' => [
        'disallowedAttributes' => [
            [
                'attribute' => IgnoreException::class,
                'allowInMethodsWithAttributes' => [
                    AsCommandHandler::class,
                ],
            ],
        ],
    ],
];

Interestingly, all the errors are related to my 2 disallowed configs that use allowInMethodsWithAttributes.

Are we sure the tests are fully covering these cases?

[ruudk]

Somehow it seems to fall in this condition:

[ruudk]

Ok, I know what's going on here.

Since 2.1.12 we seem to fall in

phpstan-disallowed-calls/src/Allowed/Allowed.php

Line 267 in 1d61d45

return $scope->isInClass() ? $scope->getClassReflection()->getNativeReflection()->getMethod($function->getName())->getAttributes() : [];

While with previous version, we would go in here:

phpstan-disallowed-calls/src/Allowed/Allowed.php

Lines 271 to 279 in 1d61d45

	if ($node instanceof ClassMethod && $scope->isInClass()) {
	return $scope->getClassReflection()->getNativeReflection()->getMethod($node->name->name)->getAttributes();
	} elseif ($node instanceof Function_) {
	return $this->reflector->reflectFunction($node->name->name)->getAttributes();
	}
	$attributes = $this->attributesWhenInSignature->get($scope);
	if ($attributes !== null) {
	return $attributes;
	}

[ruudk]

Found the issue! 🎉
It's because I have a constructor, and the tests that guard this, don't have a constructor.

Because of phpstan/phpstan-src@da74773#diff-dc817f2bab8672057a95375a542e68599164f6ab2380e4cccd1b50583e295d85 the code above now breaks. See the change with the MutatingScope.

Not sure if this should be handled differently in this library, or if this was an issue introduced in PHPStan.

/cc @staabm Thanks for your amazing contributions. Just wanted to let you know we're dealing with this issue. Maybe you already have an insight.

I'll create a PR with a failing test.

[ruudk]

Will create the failing test tomorrow.

[staabm]

Feel free to ping me when a reproducer is available as a unit test

[ruudk]

Created a reproducer in #325

[spaze]

Thanks @ruudk for the repro test. Have been looking into this for the past few hours but can't really understand what's going on and most importantly why. But I have managed to make your reproducer a bit shorter :-) I don't think it's related to the code you mention in #325's description (added in #315) and although I'm not really proud of that piece of code, and maybe because of that, the shorter reproducer doesn't use any attributes to not hit or need that code.

My reproducer is in #326 and if you check it, the rule triggers on FullyQualified nodes, and there are just two in the analyzed code, and once the node is processed, it will only return the node class name, and the scope function name.

The issue is that for both nodes the scope function name is __construct, which I don't think is right – the nodes are in the foo and bar method params. Previously, or with shouldNarrowMethodScopeFromConstructor = null, or when there's no constructor, the scope function name is null. I don't properly understand the new narrowing feature yet so I don't really know whether this is an intentional change or if it's a bug and where.

Maybe @staabm would help me understand the problem here or explain the behaviour?