Skip to content

Commit 251dcf3

Browse files
TomasVotrubaTomasVotruba
authored
[code-quality] Make ctor defined dynamic properties private, as most probably case (#7036)
* [code-quality] Make ctor defined dynamic properties private, as most probably case * improve CompleteDynamicPropertiesRector to be aware of context method it was created in * add private property in case of setUp test case * cleanup
1 parent 40393c5 commit 251dcf3

8 files changed

Lines changed: 304 additions & 232 deletions

File tree

‎rules-tests/CodeQuality/Rector/Class_/CompleteDynamicPropertiesRector/Fixture/constructor_private.php.inc‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<?php
2+
3+
namespace Rector\Tests\CodeQuality\Rector\Class_\CompleteDynamicPropertiesRector\Fixture;
4+
5+
class ConstructorPrivate
6+
{
7+
public function __construct()
8+
{
9+
$this->value = 'classStringCaseInSensitive';
10+
}
11+
}
12+
13+
?>
14+
-----
15+
<?php
16+
17+
namespace Rector\Tests\CodeQuality\Rector\Class_\CompleteDynamicPropertiesRector\Fixture;
18+
19+
class ConstructorPrivate
20+
{
21+
private string $value;
22+
public function __construct()
23+
{
24+
$this->value = 'classStringCaseInSensitive';
25+
}
26+
}
27+
28+
?>

‎rules-tests/CodeQuality/Rector/Class_/CompleteDynamicPropertiesRector/Fixture/setup_private.php.inc‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<?php
2+
3+
namespace Rector\Tests\CodeQuality\Rector\Class_\CompleteDynamicPropertiesRector\Fixture;
4+
5+
final class SetupPrivate
6+
{
7+
public function setUp()
8+
{
9+
$this->initialValue = 123;
10+
}
11+
}
12+
13+
?>
14+
-----
15+
<?php
16+
17+
namespace Rector\Tests\CodeQuality\Rector\Class_\CompleteDynamicPropertiesRector\Fixture;
18+
19+
final class SetupPrivate
20+
{
21+
private int $initialValue;
22+
public function setUp()
23+
{
24+
$this->initialValue = 123;
25+
}
26+
}
27+
28+
?>

‎rules/CodeQuality/NodeAnalyzer/LocalPropertyAnalyzer.php‎

Lines changed: 85 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@
1717
use PhpParser\NodeVisitor;
1818
use PHPStan\Analyser\Scope;
1919
use PHPStan\Type\MixedType;
20-
use PHPStan\Type\Type;
2120
use Rector\CodeQuality\TypeResolver\ArrayDimFetchTypeResolver;
21+
use Rector\CodeQuality\ValueObject\DefinedPropertyWithType;
2222
use Rector\NodeAnalyzer\PropertyFetchAnalyzer;
2323
use Rector\NodeNameResolver\NodeNameResolver;
2424
use Rector\NodeTypeResolver\Node\AttributeKey;
@@ -44,53 +44,73 @@ public function __construct(
4444
}
4545

4646
/**
47-
* @return array<string, Type>
47+
* @return DefinedPropertyWithType[]
4848
*/
4949
public function resolveFetchedPropertiesToTypesFromClass(Class_ $class): array
5050
{
51-
$fetchedLocalPropertyNameToTypes = [];
51+
$definedPropertiesWithTypes = [];
5252

53-
$this->simpleCallableNodeTraverser->traverseNodesWithCallable($class->getMethods(), function (Node $node) use (
54-
&$fetchedLocalPropertyNameToTypes
55-
): ?int {
56-
if ($this->shouldSkip($node)) {
57-
return NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
58-
}
53+
foreach ($class->getMethods() as $classMethod) {
54+
$methodName = $this->nodeNameResolver->getName($classMethod);
5955

60-
if ($node instanceof Assign && ($node->var instanceof PropertyFetch || $node->var instanceof ArrayDimFetch)) {
61-
$propertyFetch = $node->var;
56+
$this->simpleCallableNodeTraverser->traverseNodesWithCallable(
57+
$classMethod->getStmts(),
58+
function (Node $node) use (&$definedPropertiesWithTypes, $methodName): ?int {
59+
if ($this->shouldSkip($node)) {
60+
return NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
61+
}
6262

63-
$propertyName = $this->resolvePropertyName(
64-
$propertyFetch instanceof ArrayDimFetch ? $propertyFetch->var : $propertyFetch
65-
);
63+
if ($node instanceof Assign && ($node->var instanceof PropertyFetch || $node->var instanceof ArrayDimFetch)) {
64+
$propertyFetch = $node->var;
6665

67-
if ($propertyName === null) {
68-
return NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
69-
}
66+
$propertyName = $this->resolvePropertyName(
67+
$propertyFetch instanceof ArrayDimFetch ? $propertyFetch->var : $propertyFetch
68+
);
7069

71-
if ($propertyFetch instanceof ArrayDimFetch) {
72-
$fetchedLocalPropertyNameToTypes[$propertyName][] = $this->arrayDimFetchTypeResolver->resolve(
73-
$propertyFetch,
74-
$node
75-
);
76-
return NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
77-
}
70+
if ($propertyName === null) {
71+
return NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
72+
}
7873

79-
$fetchedLocalPropertyNameToTypes[$propertyName][] = $this->nodeTypeResolver->getType($node->expr);
80-
return NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
81-
}
74+
if ($propertyFetch instanceof ArrayDimFetch) {
75+
$propertyType = $this->arrayDimFetchTypeResolver->resolve($propertyFetch, $node);
8276

83-
$propertyName = $this->resolvePropertyName($node);
84-
if ($propertyName === null) {
85-
return null;
86-
}
77+
$definedPropertiesWithTypes[] = new DefinedPropertyWithType(
78+
$propertyName,
79+
$propertyType,
80+
$methodName
81+
);
8782

88-
$fetchedLocalPropertyNameToTypes[$propertyName][] = new MixedType();
83+
return NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
84+
}
8985

90-
return null;
91-
});
86+
$propertyType = $this->nodeTypeResolver->getType($node->expr);
87+
88+
$definedPropertiesWithTypes[] = new DefinedPropertyWithType(
89+
$propertyName,
90+
$propertyType,
91+
$methodName
92+
);
93+
94+
return NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
95+
}
96+
97+
$propertyName = $this->resolvePropertyName($node);
98+
if ($propertyName === null) {
99+
return null;
100+
}
101+
102+
$definedPropertiesWithTypes[] = new DefinedPropertyWithType(
103+
$propertyName,
104+
new MixedType(),
105+
$methodName
106+
);
107+
108+
return null;
109+
}
110+
);
111+
}
92112

93-
return $this->normalizeToSingleType($fetchedLocalPropertyNameToTypes);
113+
return $this->normalizeToSingleType($definedPropertiesWithTypes);
94114
}
95115

96116
private function shouldSkip(Node $node): bool
@@ -139,18 +159,40 @@ private function shouldSkipPropertyFetch(PropertyFetch $propertyFetch): bool
139159
}
140160

141161
/**
142-
* @param array<string, Type[]> $propertyNameToTypes
143-
* @return array<string, Type>
162+
* @param DefinedPropertyWithType[] $definedPropertiesWithTypes
163+
* @return DefinedPropertyWithType[]
144164
*/
145-
private function normalizeToSingleType(array $propertyNameToTypes): array
165+
private function normalizeToSingleType(array $definedPropertiesWithTypes): array
146166
{
147-
// normalize types to union
148-
$propertyNameToType = [];
149-
foreach ($propertyNameToTypes as $name => $types) {
150-
$propertyNameToType[$name] = $this->typeFactory->createMixedPassedOrUnionType($types);
167+
$definedPropertiesWithTypesByPropertyName = [];
168+
foreach ($definedPropertiesWithTypes as $definedPropertyWithType) {
169+
$definedPropertiesWithTypesByPropertyName[$definedPropertyWithType->getName()][] = $definedPropertyWithType;
170+
}
171+
172+
$normalizedDefinedPropertiesWithTypes = [];
173+
174+
foreach ($definedPropertiesWithTypesByPropertyName as $propertyName => $definedPropertiesWithTypes) {
175+
if (count($definedPropertiesWithTypes) === 1) {
176+
$normalizedDefinedPropertiesWithTypes[] = $definedPropertiesWithTypes[0];
177+
continue;
178+
}
179+
180+
$propertyTypes = [];
181+
foreach ($definedPropertiesWithTypes as $definedPropertyWithType) {
182+
/** @var DefinedPropertyWithType $definedPropertyWithType */
183+
$propertyTypes[] = $definedPropertyWithType->getType();
184+
}
185+
186+
$normalizePropertyType = $this->typeFactory->createMixedPassedOrUnionType($propertyTypes);
187+
$normalizedDefinedPropertiesWithTypes[] = new DefinedPropertyWithType(
188+
$propertyName,
189+
$normalizePropertyType,
190+
// skip as multiple places can define the same property
191+
null
192+
);
151193
}
152194

153-
return $propertyNameToType;
195+
return $normalizedDefinedPropertiesWithTypes;
154196
}
155197

156198
/**

‎rules/CodeQuality/NodeAnalyzer/MissingPropertiesResolver.php‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Rector\CodeQuality\NodeAnalyzer;
6+
7+
use PhpParser\Node\Stmt\Class_;
8+
use PHPStan\Reflection\ClassReflection;
9+
use Rector\CodeQuality\ValueObject\DefinedPropertyWithType;
10+
use Rector\NodeAnalyzer\PropertyPresenceChecker;
11+
12+
final readonly class MissingPropertiesResolver
13+
{
14+
public function __construct(
15+
private ClassLikeAnalyzer $classLikeAnalyzer,
16+
private PropertyPresenceChecker $propertyPresenceChecker,
17+
) {
18+
}
19+
20+
/**
21+
* @param DefinedPropertyWithType[] $definedPropertiesWithTypes
22+
* @return DefinedPropertyWithType[]
23+
*/
24+
public function resolve(Class_ $class, ClassReflection $classReflection, array $definedPropertiesWithTypes): array
25+
{
26+
$existingPropertyNames = $this->classLikeAnalyzer->resolvePropertyNames($class);
27+
28+
$missingPropertiesWithTypes = [];
29+
30+
foreach ($definedPropertiesWithTypes as $definedPropertyWithType) {
31+
// 1. property already exists, skip it
32+
if (in_array($definedPropertyWithType->getName(), $existingPropertyNames, true)) {
33+
continue;
34+
}
35+
36+
// 2. is part of class docblock or another magic, skip it
37+
if ($classReflection->hasProperty($definedPropertyWithType->getName())) {
38+
continue;
39+
}
40+
41+
// 3. is fetched by parent class on non-private property etc., skip it
42+
$hasClassContextProperty = $this->propertyPresenceChecker->hasClassContextProperty(
43+
$class,
44+
$definedPropertyWithType
45+
);
46+
47+
if ($hasClassContextProperty) {
48+
continue;
49+
}
50+
51+
// it's most likely missing!
52+
$missingPropertiesWithTypes[] = $definedPropertyWithType;
53+
}
54+
55+
return $missingPropertiesWithTypes;
56+
}
57+
}

‎rules/CodeQuality/NodeFactory/MissingPropertiesFactory.php‎

Lines changed: 45 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,36 +5,71 @@
55
namespace Rector\CodeQuality\NodeFactory;
66

77
use PhpParser\Modifiers;
8+
use PhpParser\Node;
89
use PhpParser\Node\PropertyItem;
910
use PhpParser\Node\Stmt\Property;
10-
use PHPStan\Type\Type;
11+
use Rector\CodeQuality\ValueObject\DefinedPropertyWithType;
12+
use Rector\Php\PhpVersionProvider;
13+
use Rector\PHPStanStaticTypeMapper\Enum\TypeKind;
14+
use Rector\StaticTypeMapper\StaticTypeMapper;
15+
use Rector\ValueObject\MethodName;
16+
use Rector\ValueObject\PhpVersionFeature;
1117

1218
final readonly class MissingPropertiesFactory
1319
{
1420
public function __construct(
15-
private PropertyTypeDecorator $propertyTypeDecorator
21+
private PropertyTypeDecorator $propertyTypeDecorator,
22+
private PhpVersionProvider $phpVersionProvider,
23+
private StaticTypeMapper $staticTypeMapper
1624
) {
1725
}
1826

1927
/**
20-
* @param array<string, Type> $fetchedLocalPropertyNameToTypes
21-
* @param string[] $propertyNamesToComplete
28+
* @param DefinedPropertyWithType[] $definedPropertiesWithType
2229
* @return Property[]
2330
*/
24-
public function create(array $fetchedLocalPropertyNameToTypes, array $propertyNamesToComplete): array
31+
public function create(array $definedPropertiesWithType): array
2532
{
2633
$newProperties = [];
27-
foreach ($fetchedLocalPropertyNameToTypes as $propertyName => $propertyType) {
28-
if (! in_array($propertyName, $propertyNamesToComplete, true)) {
29-
continue;
34+
foreach ($definedPropertiesWithType as $definedPropertyWithType) {
35+
$visibilityModifier = $this->isFromAlwaysDefinedMethod($definedPropertyWithType)
36+
? Modifiers::PRIVATE
37+
: Modifiers::PUBLIC;
38+
39+
$property = new Property($visibilityModifier, [
40+
new PropertyItem($definedPropertyWithType->getName()),
41+
]);
42+
43+
if ($this->isFromAlwaysDefinedMethod(
44+
$definedPropertyWithType
45+
) && $this->phpVersionProvider->isAtLeastPhpVersion(PhpVersionFeature::TYPED_PROPERTIES)) {
46+
$propertyType = $this->staticTypeMapper->mapPHPStanTypeToPhpParserNode(
47+
$definedPropertyWithType->getType(),
48+
TypeKind::PROPERTY
49+
);
50+
if ($propertyType instanceof Node) {
51+
$property->type = $propertyType;
52+
$newProperties[] = $property;
53+
54+
continue;
55+
}
3056
}
3157

32-
$property = new Property(Modifiers::PUBLIC, [new PropertyItem($propertyName)]);
33-
$this->propertyTypeDecorator->decorateProperty($property, $propertyType);
58+
// fallback to docblock
59+
$this->propertyTypeDecorator->decorateProperty($property, $definedPropertyWithType->getType());
3460

3561
$newProperties[] = $property;
3662
}
3763

3864
return $newProperties;
3965
}
66+
67+
private function isFromAlwaysDefinedMethod(DefinedPropertyWithType $definedPropertyWithType): bool
68+
{
69+
return in_array(
70+
$definedPropertyWithType->getDefinedInMethodName(),
71+
[MethodName::CONSTRUCT, MethodName::SET_UP],
72+
true
73+
);
74+
}
4075
}

0 commit comments

Comments
 (0)