Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Readonly classes cannot be combined with #[AllowDynamicProperties] #3738

Merged
merged 7 commits into from
Dec 20, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix class fake
staabm committed Dec 20, 2024
commit ff83891b4524b9c7fdbf091c48356ec35efcb512
10 changes: 7 additions & 3 deletions src/Rules/Traits/TraitAttributesRule.php
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@

use Attribute;
use PhpParser\Node;
use PhpParser\Node\Stmt\Class_;
use PHPStan\Analyser\MutatingScope;
use PHPStan\Analyser\Scope;
use PHPStan\Reflection\ReflectionProvider;
@@ -41,12 +42,15 @@ public function processNode(Node $node, Scope $scope): array
if (!$this->reflectionProvider->hasClass($traitName->toString())) {
return [];
}
$classReflection = $this->reflectionProvider->getClass($traitName->toString());
$traitClassReflection = $this->reflectionProvider->getClass($traitName->toString());

if (!$scope instanceof MutatingScope) {
throw new ShouldNotHappenException();
}
$scope = $scope->enterTrait($classReflection);
$fakeClass = new Class_(null, [new Node\Stmt\TraitUse([$traitName])], ['startLine' => 1, 'endLine' => 1]);
$fakeClassReflection = $this->reflectionProvider->getAnonymousClassReflection($fakeClass, $scope);
$scope = $scope->enterClass($fakeClassReflection);
$scope = $scope->enterTrait($traitClassReflection);

$errors = $this->attributesCheck->check(
$scope,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to do something with $scope here. Please recreate this problem 3447391 but with a trait.

We probably need to do $scope->enterTrait() here before calling the check. Just do if !$scope instanceof MutatingScope then throw ShouldNotHappen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have adjusted the PR. I think I have reproduced the problem you are after.

adding $scope->enterTrait($classReflection) didn't work and throws:

2) PHPStan\Rules\Traits\TraitAttributesRuleTest::testBug12011
PHPStan\ShouldNotHappenException: Internal error.

C:\dvl\Workspace\phpstan-src-staabm\src\Analyser\ScopeContext.php:44
C:\dvl\Workspace\phpstan-src-staabm\src\Analyser\MutatingScope.php:2935
C:\dvl\Workspace\phpstan-src-staabm\src\Rules\Traits\TraitAttributesRule.php:49
C:\dvl\Workspace\phpstan-src-staabm\src\Analyser\FileAnalyser.php:111
C:\dvl\Workspace\phpstan-src-staabm\src\Analyser\NodeScopeResolver.php:482
C:\dvl\Workspace\phpstan-src-staabm\src\Analyser\NodeScopeResolver.php:354
C:\dvl\Workspace\phpstan-src-staabm\src\Analyser\NodeScopeResolver.php:842
C:\dvl\Workspace\phpstan-src-staabm\src\Analyser\NodeScopeResolver.php:307
C:\dvl\Workspace\phpstan-src-staabm\src\Analyser\FileAnalyser.php:212
C:\dvl\Workspace\phpstan-src-staabm\src\Analyser\Analyser.php:75
C:\dvl\Workspace\phpstan-src-staabm\src\Testing\RuleTestCase.php:177
C:\dvl\Workspace\phpstan-src-staabm\src\Testing\RuleTestCase.php:138
C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Rules\Traits\TraitAttributesRuleTest.php:78

at this point I have no idea what todo. could you fix the remaining thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me fix the build real quick

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Push the code that's crashing like that please, I will take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you are

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, we need to be in a class first before we can enter a trait.

I'd try two things: try to put the trait reflection into enterClass (but a different ShouldNotHappen might be thrown, not sure).

If that fails, we need to come up with some fake class to put into enterClass before we can enterTrait. It might be okay to simulate some anonymous Class_ node to use.

If it's too much work, don't worry and I'll merge the original code that doesn't use MutatingScope at all.

Copy link
Contributor Author

@staabm staabm Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have class-faking without a thrown exception working. the test-case is still failling though

Will have another look after sleeping over it

@@ -55,7 +59,7 @@ public function processNode(Node $node, Scope $scope): array
'class',
);

if (count($classReflection->getNativeReflection()->getAttributes('AllowDynamicProperties')) > 0) {
if (count($traitClassReflection->getNativeReflection()->getAttributes('AllowDynamicProperties')) > 0) {
$errors[] = RuleErrorBuilder::message('Attribute class AllowDynamicProperties cannot be used with trait.')
->identifier('trait.allowDynamicProperties')
->nonIgnorable()