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

staabm · 2024-12-17T10:42:11Z

phpstan-bot · 2024-12-17T10:58:17Z

This pull request has been marked as ready for review.

tests/PHPStan/Rules/Classes/data/bug-12281.php

ondrejmirtes

We also need to error when this attribute is used above interfaces. Also please check what happens above enums and traits (I'm not sure).

src/Rules/Classes/ClassAttributesRule.php

tests/PHPStan/Rules/Classes/data/bug-12281.php

staabm · 2024-12-17T17:58:32Z

We also need to error when this attribute is used above interfaces. Also please check what happens above enums and traits (I'm not sure).

I have tested this cases and linked the 3v4l.org examples in phpstan/phpstan#12281 (comment)

TL;DR:

enums not allowed
interfaces not allowed
traits not allowed

what do you think about adding a separate AllowDynamicPropertiesAttributeRule which targets ClassLike instead of adding this error to the existing rule?
(atm we don't have a rule which covers attributes on traits)

ondrejmirtes

Also:

Please do new TraitAttributesRule similar to ClassAttributesRule that will call AttributesCheck. We need the rule to hook onto Trait_ and fetch the reflection ourselves from ReflectionProvider.
Add AllowDynamicProperties check there too.

src/Rules/Classes/ClassAttributesRule.php

tests/PHPStan/Rules/Classes/data/bug-12281.php

src/Rules/Classes/ClassAttributesRule.php

src/Rules/Traits/TraitAttributesRule.php

ondrejmirtes · 2024-12-19T16:00:03Z

src/Rules/Traits/TraitAttributesRule.php

+		}
+
+		$errors = $this->attributesCheck->check(
+			$scope,


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.

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?

let me fix the build real quick

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

here you are

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.

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

Will have another look after sleeping over it

ondrejmirtes · 2024-12-19T16:01:10Z

Please keep this on 2.0.x. We don't need to be adding new features to 1.12.x anymore.

src/Rules/Traits/TraitAttributesRule.php

Co-authored-by: Markus Staab <[email protected]>

ondrejmirtes · 2024-12-20T08:41:37Z

I changed my mind, I'm going to adjust it myself, thanks!

Some attribute arguments actually depend on using class instead of the trait So we have to analyse the attributes in context of each class that uses the trait

ondrejmirtes · 2024-12-20T08:50:32Z

See my commit and reasoning in description: b1cebc2

ondrejmirtes · 2024-12-20T08:51:31Z

Please stop force-pushing here, I said I'm taking over. Thanks.

staabm · 2024-12-20T08:52:35Z

Please stop force-pushing here, I said I'm taking over. Thanks.

oh sorry, did not see your comments. I got it working just a moment ago ;)

ondrejmirtes · 2024-12-20T09:00:14Z

Thank you!

staabm · 2024-12-20T09:00:39Z

thanks

staabm force-pushed the readonly-dynm branch from f56a765 to ddb564c Compare December 17, 2024 10:46

staabm marked this pull request as ready for review December 17, 2024 10:57

staabm commented Dec 17, 2024

View reviewed changes

tests/PHPStan/Rules/Classes/data/bug-12281.php Show resolved Hide resolved

ondrejmirtes requested changes Dec 17, 2024

View reviewed changes

src/Rules/Classes/ClassAttributesRule.php Outdated Show resolved Hide resolved

src/Rules/Classes/ClassAttributesRule.php Show resolved Hide resolved

tests/PHPStan/Rules/Classes/data/bug-12281.php Show resolved Hide resolved

ondrejmirtes requested changes Dec 19, 2024

View reviewed changes

src/Rules/Classes/ClassAttributesRule.php Outdated Show resolved Hide resolved

src/Rules/Classes/ClassAttributesRule.php Outdated Show resolved Hide resolved

src/Rules/Classes/ClassAttributesRule.php Outdated Show resolved Hide resolved

ondrejmirtes reviewed Dec 19, 2024

View reviewed changes

tests/PHPStan/Rules/Classes/data/bug-12281.php Outdated Show resolved Hide resolved

clxmstaab force-pushed the readonly-dynm branch from 83a0ea6 to af3d0af Compare December 19, 2024 10:42

ondrejmirtes requested changes Dec 19, 2024

View reviewed changes

clxmstaab force-pushed the readonly-dynm branch 2 times, most recently from 05dfb04 to 4fc9574 Compare December 19, 2024 17:00

staabm changed the base branch from 1.12.x to 2.0.x December 19, 2024 17:00

clxmstaab force-pushed the readonly-dynm branch from 4fc9574 to 88402dc Compare December 19, 2024 17:06

Readonly classes cannot be combined with #[AllowDynamicProperties]

b96f17b

clxmstaab force-pushed the readonly-dynm branch from 88402dc to b96f17b Compare December 19, 2024 17:11

staabm commented Dec 19, 2024

View reviewed changes

src/Rules/Traits/TraitAttributesRule.php Outdated Show resolved Hide resolved

ondrejmirtes and others added 3 commits December 19, 2024 21:37

Update TraitAttributesRule.php

65a5889

Co-authored-by: Markus Staab <[email protected]>

work in progress.. crashing atm

0b750a9

fix class fake

ff83891

clxmstaab force-pushed the readonly-dynm branch from 5220c97 to ff83891 Compare December 20, 2024 07:55

clxmstaab force-pushed the readonly-dynm branch from ff83891 to 0999d0f Compare December 20, 2024 08:46

ondrejmirtes added 2 commits December 20, 2024 09:48

Use InTraitNode instead

b1cebc2

Some attribute arguments actually depend on using class instead of the trait So we have to analyse the attributes in context of each class that uses the trait

Fix

b2b833c

ondrejmirtes force-pushed the readonly-dynm branch from 0999d0f to b2b833c Compare December 20, 2024 08:50

clxmstaab force-pushed the readonly-dynm branch from b2b833c to 4d7274d Compare December 20, 2024 08:51

ondrejmirtes force-pushed the readonly-dynm branch 2 times, most recently from b85805f to b2b833c Compare December 20, 2024 08:52

This file can still be linted

b35a845

ondrejmirtes merged commit 202dd81 into phpstan:2.0.x Dec 20, 2024
425 of 426 checks passed

staabm deleted the readonly-dynm branch December 20, 2024 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

staabm commented Dec 17, 2024

phpstan-bot commented Dec 17, 2024

ondrejmirtes left a comment

staabm commented Dec 17, 2024 •

edited

Loading

ondrejmirtes left a comment

ondrejmirtes Dec 19, 2024

staabm Dec 19, 2024

staabm Dec 19, 2024

ondrejmirtes Dec 19, 2024

staabm Dec 19, 2024

ondrejmirtes Dec 19, 2024

staabm Dec 19, 2024 •

edited

Loading

ondrejmirtes commented Dec 19, 2024

ondrejmirtes commented Dec 20, 2024

ondrejmirtes commented Dec 20, 2024

ondrejmirtes commented Dec 20, 2024

staabm commented Dec 20, 2024

ondrejmirtes commented Dec 20, 2024

staabm commented Dec 20, 2024

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

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

Conversation

staabm commented Dec 17, 2024

phpstan-bot commented Dec 17, 2024

ondrejmirtes left a comment

Choose a reason for hiding this comment

staabm commented Dec 17, 2024 • edited Loading

ondrejmirtes left a comment

Choose a reason for hiding this comment

ondrejmirtes Dec 19, 2024

Choose a reason for hiding this comment

staabm Dec 19, 2024

Choose a reason for hiding this comment

staabm Dec 19, 2024

Choose a reason for hiding this comment

ondrejmirtes Dec 19, 2024

Choose a reason for hiding this comment

staabm Dec 19, 2024

Choose a reason for hiding this comment

ondrejmirtes Dec 19, 2024

Choose a reason for hiding this comment

staabm Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

ondrejmirtes commented Dec 19, 2024

ondrejmirtes commented Dec 20, 2024

ondrejmirtes commented Dec 20, 2024

ondrejmirtes commented Dec 20, 2024

staabm commented Dec 20, 2024

ondrejmirtes commented Dec 20, 2024

staabm commented Dec 20, 2024

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

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

staabm commented Dec 17, 2024 •

edited

Loading

staabm Dec 19, 2024 •

edited

Loading