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
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 2 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ lint:
--exclude tests/PHPStan/Rules/Properties/data/non-abstract-hooked-properties-in-class.php \
--exclude tests/PHPStan/Rules/Properties/data/hooked-properties-in-class.php \
--exclude tests/PHPStan/Rules/Properties/data/hooked-properties-without-bodies-in-class.php \
--exclude tests/PHPStan/Rules/Classes/data/bug-12281.php \
--exclude tests/PHPStan/Rules/Traits/data/bug-12281.php \
src tests

cs:
Expand Down
1 change: 1 addition & 0 deletions conf/config.level0.neon
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ rules:
- PHPStan\Rules\Regexp\RegularExpressionPatternRule
- PHPStan\Rules\Traits\ConflictingTraitConstantsRule
- PHPStan\Rules\Traits\ConstantsInTraitsRule
- PHPStan\Rules\Traits\TraitAttributesRule
- PHPStan\Rules\Types\InvalidTypesInUnionRule
- PHPStan\Rules\Variables\UnsetRule
- PHPStan\Rules\Whitespace\FileWhitespaceRule
Expand Down
32 changes: 31 additions & 1 deletion src/Rules/Classes/ClassAttributesRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
use PHPStan\Node\InClassNode;
use PHPStan\Rules\AttributesCheck;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use function count;
use function sprintf;

/**
* @implements Rule<InClassNode>
Expand All @@ -28,12 +31,39 @@ public function processNode(Node $node, Scope $scope): array
{
$classLikeNode = $node->getOriginalNode();

return $this->attributesCheck->check(
$errors = $this->attributesCheck->check(
$scope,
$classLikeNode->attrGroups,
Attribute::TARGET_CLASS,
'class',
);

$classReflection = $node->getClassReflection();
if (
$classReflection->isReadOnly()
|| $classReflection->isEnum()
|| $classReflection->isInterface()
) {
$typeName = 'readonly class';
$identifier = 'class.allowDynamicPropertiesReadonly';
if ($classReflection->isEnum()) {
$typeName = 'enum';
$identifier = 'enum.allowDynamicProperties';
}
if ($classReflection->isInterface()) {
$typeName = 'interface';
$identifier = 'interface.allowDynamicProperties';
}

if (count($classReflection->getNativeReflection()->getAttributes('AllowDynamicProperties')) > 0) {
$errors[] = RuleErrorBuilder::message(sprintf('Attribute class AllowDynamicProperties cannot be used with %s.', $typeName))
->identifier($identifier)
->nonIgnorable()
->build();
staabm marked this conversation as resolved.
Show resolved Hide resolved
}
}

return $errors;
}

}
51 changes: 51 additions & 0 deletions src/Rules/Traits/TraitAttributesRule.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php declare(strict_types = 1);

namespace PHPStan\Rules\Traits;

use Attribute;
use PhpParser\Node;
use PHPStan\Analyser\Scope;
use PHPStan\Node\InTraitNode;
use PHPStan\Rules\AttributesCheck;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use function count;

/**
* @implements Rule<InTraitNode>
*/
final class TraitAttributesRule implements Rule
{

public function __construct(
private AttributesCheck $attributesCheck,
)
{
}

public function getNodeType(): string
{
return InTraitNode::class;
}

public function processNode(Node $node, Scope $scope): array
{
$originalNode = $node->getOriginalNode();
$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

$originalNode->attrGroups,
Attribute::TARGET_CLASS,
'class',
);

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

return $errors;
}

}
24 changes: 24 additions & 0 deletions tests/PHPStan/Rules/Classes/ClassAttributesRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,28 @@ public function testBug12011(): void
]);
}

public function testBug12281(): void
{
if (PHP_VERSION_ID < 80200) {
$this->markTestSkipped('Test requires PHP 8.2.');
}

$this->checkExplicitMixed = true;
$this->checkImplicitMixed = true;
$this->analyse([__DIR__ . '/data/bug-12281.php'], [
[
'Attribute class AllowDynamicProperties cannot be used with readonly class.',
05,
],
[
'Attribute class AllowDynamicProperties cannot be used with enum.',
12,
],
[
'Attribute class AllowDynamicProperties cannot be used with interface.',
15,
],
]);
}

}
16 changes: 16 additions & 0 deletions tests/PHPStan/Rules/Classes/data/bug-12281.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php // lint >= 8.2

namespace Bug12281;

#[\AllowDynamicProperties]
readonly class BlogData { /* … */ }

/** @readonly */
#[\AllowDynamicProperties]
class BlogDataPhpdoc { /* … */ }
staabm marked this conversation as resolved.
Show resolved Hide resolved

#[\AllowDynamicProperties]
enum BlogDataEnum { /* … */ }

#[\AllowDynamicProperties]
interface BlogDataInterface { /* … */ }
96 changes: 96 additions & 0 deletions tests/PHPStan/Rules/Traits/TraitAttributesRuleTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
<?php declare(strict_types = 1);

namespace PHPStan\Rules\Traits;

use PHPStan\Php\PhpVersion;
use PHPStan\Rules\AttributesCheck;
use PHPStan\Rules\ClassCaseSensitivityCheck;
use PHPStan\Rules\ClassForbiddenNameCheck;
use PHPStan\Rules\ClassNameCheck;
use PHPStan\Rules\FunctionCallParametersCheck;
use PHPStan\Rules\NullsafeCheck;
use PHPStan\Rules\PhpDoc\UnresolvableTypeHelper;
use PHPStan\Rules\Properties\PropertyReflectionFinder;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleLevelHelper;
use PHPStan\Testing\RuleTestCase;
use const PHP_VERSION_ID;

/**
* @extends RuleTestCase<TraitAttributesRule>
*/
class TraitAttributesRuleTest extends RuleTestCase
{

private bool $checkExplicitMixed = false;

private bool $checkImplicitMixed = false;

protected function getRule(): Rule
{
$reflectionProvider = $this->createReflectionProvider();
return new TraitAttributesRule(
new AttributesCheck(
$reflectionProvider,
new FunctionCallParametersCheck(
new RuleLevelHelper($reflectionProvider, true, false, true, $this->checkExplicitMixed, $this->checkImplicitMixed, false),
new NullsafeCheck(),
new PhpVersion(80000),
new UnresolvableTypeHelper(),
new PropertyReflectionFinder(),
true,
true,
true,
true,
),
new ClassNameCheck(
new ClassCaseSensitivityCheck($reflectionProvider, false),
new ClassForbiddenNameCheck(self::getContainer()),
),
true,
),
);
}

public function testRule(): void
{
$this->analyse([__DIR__ . '/data/trait-attributes.php'], [
[
'Attribute class TraitAttributes\AbstractAttribute is abstract.',
8,
],
[
'Attribute class TraitAttributes\MyTargettedAttribute does not have the class target.',
20,
],
]);
}

public function testBug12011(): void
{
if (PHP_VERSION_ID < 80300) {
$this->markTestSkipped('Test requires PHP 8.3.');
}

$this->checkExplicitMixed = true;
$this->checkImplicitMixed = true;

$this->analyse([__DIR__ . '/data/bug-12011.php'], [
[
'Parameter #1 $name of attribute class Bug12011Trait\Table constructor expects string|null, int given.',
8,
],
]);
}

public function testBug12281(): void
{
$this->analyse([__DIR__ . '/data/bug-12281.php'], [
[
'Attribute class AllowDynamicProperties cannot be used with trait.',
11,
],
]);
}

}
26 changes: 26 additions & 0 deletions tests/PHPStan/Rules/Traits/data/bug-12011.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php // lint >= 8.3

namespace Bug12011Trait;

use Attribute;


#[Table(self::TABLE_NAME)]
trait MyTrait
{
private const int TABLE_NAME = 1;
}

class X {
use MyTrait;
}

#[Attribute(Attribute::TARGET_CLASS)]
final class Table
{
public function __construct(
public readonly string|null $name = null,
public readonly string|null $schema = null,
) {
}
}
19 changes: 19 additions & 0 deletions tests/PHPStan/Rules/Traits/data/bug-12281.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php // lint >= 8.2

namespace Bug12281Traits;

#[\AllowDynamicProperties]
enum BlogDataEnum { /* … */ } // reported by ClassAttributesRule

#[\AllowDynamicProperties]
interface BlogDataInterface { /* … */ } // reported by ClassAttributesRule

#[\AllowDynamicProperties]
trait BlogDataTrait { /* … */ }

class Uses
{

use BlogDataTrait;

}
30 changes: 30 additions & 0 deletions tests/PHPStan/Rules/Traits/data/trait-attributes.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

namespace TraitAttributes;

#[\Attribute]
abstract class AbstractAttribute {}

#[AbstractAttribute]
trait MyTrait {}

#[\Attribute]
class MyAttribute {}

#[MyAttribute]
trait MyTrait2 {}

#[\Attribute(\Attribute::TARGET_PROPERTY)]
class MyTargettedAttribute {}

#[MyTargettedAttribute]
trait MyTrait3 {}

class Uses
{

use MyTrait;
use MyTrait2;
use MyTrait3;

}
Loading