-
Notifications
You must be signed in to change notification settings - Fork 509
Readonly classes cannot be combined with #[AllowDynamicProperties]
#3738
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
Changes from all commits
b96f17b
65a5889
0b750a9
ff83891
b1cebc2
b2b833c
b35a845
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
at this point I have no idea what todo. could you fix the remaining thing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let me fix the build real quick There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here you are There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
} |
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 { /* … */ } |
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, | ||
], | ||
]); | ||
} | ||
|
||
} |
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, | ||
) { | ||
} | ||
} |
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; | ||
|
||
} |
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; | ||
|
||
} |
Uh oh!
There was an error while loading. Please reload this page.