Skip to content

Commit 46c50d4

Browse files
svnldwgsvnldwg
committed
#6 asserts that immutable properties are not public
1 parent d98c3e3 commit 46c50d4

File tree

8 files changed

+166
-28
lines changed

8 files changed

+166
-28
lines changed

‎src/Helper/Converter.php

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,20 @@ class Converter
1313
*
1414
* @return string[]
1515
*/
16-
public static function propertyStringNames(array $properties)
16+
public static function propertyStringNames(array $properties): array
1717
{
1818
return array_map(static function (Node\Stmt\Property $property): string {
19-
$firstProp = reset($property->props);
20-
if ($firstProp === false) {
21-
return '';
22-
}
23-
24-
return (string)$firstProp->name;
19+
return static::propertyToString($property);
2520
}, $properties);
2621
}
22+
23+
public static function propertyToString(Node\Stmt\Property $property): string
24+
{
25+
$firstProp = reset($property->props);
26+
if ($firstProp === false) {
27+
return '';
28+
}
29+
30+
return (string)$firstProp->name;
31+
}
2732
}

‎src/Rules/ImmutableObjectRule.php

Lines changed: 87 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use PHPStan\Analyser\Scope;
1010
use PHPStan\Reflection\ClassMemberReflection;
1111
use PHPStan\Rules\Rule;
12+
use PHPStan\Rules\RuleError;
1213
use PHPStan\Rules\RuleErrorBuilder;
1314
use Svnldwg\PHPStan\Helper\AnnotationParser;
1415
use Svnldwg\PHPStan\Helper\BackwardsIterator;
@@ -28,6 +29,13 @@ class ImmutableObjectRule implements Rule
2829
/** @var \PHPStan\Parser\Parser */
2930
private $parser;
3031

32+
/** @var string */
33+
private $currentClass = '';
34+
/** @var string[] */
35+
private $immutableProperties = [];
36+
/** @var bool */
37+
private $isImmutable = false;
38+
3139
public function __construct(\PHPStan\Parser\Parser $parser)
3240
{
3341
$this->parser = $parser;
@@ -40,49 +48,42 @@ public function getNodeType(): string
4048

4149
public function processNode(Node $node, Scope $scope): array
4250
{
43-
if (!$node instanceof Node\Expr\AssignOp && !$node instanceof Assign) {
51+
if (!$node instanceof Node\Expr\AssignOp
52+
&& !$node instanceof Assign
53+
&& !$node instanceof Node\Stmt\Property
54+
) {
4455
return [];
4556
}
4657

4758
if (!$scope->isInClass()) {
4859
return [];
4960
}
5061

51-
['properties' => $immutableProperties, 'hasImmutableParent' => $hasImmutableParent] = $this->getInheritedImmutableProperties($scope);
52-
53-
$nodes = $this->parser->parseFile($scope->getFile());
54-
$hasImmutableClassAnnotation = AnnotationParser::classHasAnnotation(self::WHITELISTED_ANNOTATIONS, $nodes);
62+
$this->detectImmutableProperties($scope);
5563

56-
if (!empty($immutableProperties)) {
57-
$classNode = NodeParser::getClassNode($nodes);
58-
if ($classNode !== null) {
59-
$classProperties = NodeParser::getClassProperties($classNode);
60-
$classPropertyNames = Converter::propertyStringNames($classProperties);
61-
$immutableProperties = array_merge($immutableProperties, $classPropertyNames);
62-
}
63-
}
64-
65-
if (empty($immutableProperties)) {
66-
$immutableProperties = AnnotationParser::propertiesWithWhitelistedAnnotations(self::WHITELISTED_ANNOTATIONS, $nodes);
64+
$isImmutable = $this->isImmutable;
65+
$immutableProperties = $this->immutableProperties;
66+
if (!$isImmutable && empty($immutableProperties)) {
67+
return [];
6768
}
6869

69-
if (!$hasImmutableParent && !$hasImmutableClassAnnotation && empty($immutableProperties)) {
70-
return [];
70+
if ($node instanceof Node\Stmt\Property) {
71+
return $this->assertImmutablePropertyIsNotPublic($node);
7172
}
7273

7374
while ($node->var instanceof Node\Expr\ArrayDimFetch) {
7475
$node = $node->var;
7576
}
7677

77-
if (!empty($immutableProperties)
78+
if (!$isImmutable
79+
&& !empty($immutableProperties)
7880
&& property_exists($node->var, 'name')
7981
&& !in_array((string)$node->var->name, $immutableProperties)
8082
) {
8183
return [];
8284
}
8385

84-
if (
85-
!$node->var instanceof Node\Expr\PropertyFetch
86+
if (!$node->var instanceof Node\Expr\PropertyFetch
8687
&& !$node->var instanceof Node\Expr\StaticPropertyFetch
8788
) {
8889
return [];
@@ -160,4 +161,69 @@ private function getInheritedImmutableProperties(Scope $scope): array
160161

161162
return ['properties' => $immutableParentProperties, 'hasImmutableParent' => $hasImmutableParent];
162163
}
164+
165+
/**
166+
* @param Scope $scope
167+
*/
168+
private function detectImmutableProperties(Scope $scope): void
169+
{
170+
$classReflection = $scope->getClassReflection();
171+
if ($classReflection === null) {
172+
$this->isImmutable = false;
173+
$this->immutableProperties = [];
174+
$this->currentClass = '';
175+
176+
return;
177+
}
178+
$currentClassName = $classReflection->getName();
179+
if ($this->currentClass === $currentClassName) {
180+
return;
181+
}
182+
183+
['properties' => $immutableProperties, 'hasImmutableParent' => $hasImmutableParent] = $this->getInheritedImmutableProperties($scope);
184+
185+
$nodes = $this->parser->parseFile($scope->getFile());
186+
$isImmutable = $hasImmutableParent || AnnotationParser::classHasAnnotation(self::WHITELISTED_ANNOTATIONS, $nodes);
187+
188+
if (!empty($immutableProperties)) {
189+
$classNode = NodeParser::getClassNode($nodes);
190+
if ($classNode !== null) {
191+
$classProperties = NodeParser::getClassProperties($classNode);
192+
$classPropertyNames = Converter::propertyStringNames($classProperties);
193+
$immutableProperties = array_merge($immutableProperties, $classPropertyNames);
194+
}
195+
}
196+
197+
if (empty($immutableProperties)) {
198+
$immutableProperties = AnnotationParser::propertiesWithWhitelistedAnnotations(self::WHITELISTED_ANNOTATIONS, $nodes);
199+
}
200+
201+
$this->immutableProperties = $immutableProperties;
202+
$this->isImmutable = $isImmutable;
203+
$this->currentClass = $classReflection->getName();
204+
}
205+
206+
/**
207+
* @param Node\Stmt\Property $property
208+
*
209+
* @throws \PHPStan\ShouldNotHappenException
210+
*
211+
* @return RuleError[]
212+
*/
213+
private function assertImmutablePropertyIsNotPublic(Node\Stmt\Property $property): array
214+
{
215+
$propertyName = Converter::propertyToString($property);
216+
$propertyIsImmutable = $this->isImmutable || in_array($propertyName, $this->immutableProperties);
217+
218+
if ($propertyIsImmutable && $property->isPublic()) {
219+
return [
220+
RuleErrorBuilder::message(sprintf(
221+
'Property "%s" is declared immutable, but is public and therefore mutable',
222+
$propertyName
223+
))->build(),
224+
];
225+
}
226+
227+
return [];
228+
}
163229
}

‎test/Fixture/ImmutableObjectRule/Failure/PublicImmutableProperty/Inheritance.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Svnldwg\PHPStan\Test\Fixture\ImmutableObjectRule\Failure\PublicImmutableProperty;
6+
7+
class Inheritance extends InheritanceParent
8+
{
9+
public $shouldNotBePublicBecauseParentIsImmutable;
10+
}

‎test/Fixture/ImmutableObjectRule/Failure/PublicImmutableProperty/InheritanceParent.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Svnldwg\PHPStan\Test\Fixture\ImmutableObjectRule\Failure\PublicImmutableProperty;
6+
7+
/** @psalm-immutable */
8+
class InheritanceParent
9+
{
10+
}

‎test/Fixture/ImmutableObjectRule/Failure/PublicImmutableProperty/PublicImmutableClassProperty.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Svnldwg\PHPStan\Test\Fixture\ImmutableObjectRule\Failure\PublicImmutableProperty;
6+
7+
/**
8+
* @psalm-immutable
9+
*/
10+
class PublicImmutableClassProperty
11+
{
12+
public $shouldNotBePublic;
13+
}

‎test/Fixture/ImmutableObjectRule/Failure/PublicImmutableProperty/PublicImmutableProperty.php

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Svnldwg\PHPStan\Test\Fixture\ImmutableObjectRule\Failure\PublicImmutableProperty;
6+
7+
class PublicImmutableProperty
8+
{
9+
/** @psalm-immutable */
10+
public $shouldNotBePublic;
11+
}

‎test/Fixture/ImmutableObjectRule/Success/ImmutableProperty.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66

77
class ImmutableProperty
88
{
9+
public $mutablesCanBePublic;
10+
911
/** @immutable */
1012
private $value;
1113

‎test/Integration/Rules/ImmutableObjectRuleTest.php

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,27 @@ public function provideCasesWhereAnalysisShouldFail(): iterable
8888
13,
8989
],
9090
],
91+
'public-immutable-class-property' => [
92+
__DIR__ . '/../../Fixture/ImmutableObjectRule/Failure/PublicImmutableProperty/PublicImmutableClassProperty.php',
93+
[
94+
'Property "shouldNotBePublic" is declared immutable, but is public and therefore mutable',
95+
12,
96+
],
97+
],
98+
'public-immutable-property' => [
99+
__DIR__ . '/../../Fixture/ImmutableObjectRule/Failure/PublicImmutableProperty/PublicImmutableProperty.php',
100+
[
101+
'Property "shouldNotBePublic" is declared immutable, but is public and therefore mutable',
102+
10,
103+
],
104+
],
105+
'public-immutable-property-inheritance' => [
106+
__DIR__ . '/../../Fixture/ImmutableObjectRule/Failure/PublicImmutableProperty/Inheritance.php',
107+
[
108+
'Property "shouldNotBePublicBecauseParentIsImmutable" is declared immutable, but is public and therefore mutable',
109+
9,
110+
],
111+
],
91112
];
92113

93114
foreach ($paths as $description => [$path, $error]) {

0 commit comments

Comments
 (0)