From 3cb7730fb06e77310b4dfa4ae5ff75d41df8c737 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Hansl=C3=ADk?= Date: Thu, 22 Sep 2022 14:18:12 +0200 Subject: [PATCH 1/2] Fixed ReflectionProperty::getModifiers() for readonly properties --- src/Reflection/ReflectionProperty.php | 9 +++++++ .../Reflection/ReflectionPropertyTest.php | 24 +++++++------------ 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/Reflection/ReflectionProperty.php b/src/Reflection/ReflectionProperty.php index fb3dfa962..125e67ce5 100644 --- a/src/Reflection/ReflectionProperty.php +++ b/src/Reflection/ReflectionProperty.php @@ -37,6 +37,14 @@ class ReflectionProperty { + /** + * We cannot use CoreReflectionProperty::IS_READONLY because it does not exist in PHP < 8.1. + * Constant is public, so we can use it in tests. + * + * @internal + */ + public const IS_READONLY = 128; + private CompiledValue|null $compiledDefaultValue = null; private function __construct( @@ -135,6 +143,7 @@ public function getModifiers(): int $val += $this->isPublic() ? CoreReflectionProperty::IS_PUBLIC : 0; $val += $this->isProtected() ? CoreReflectionProperty::IS_PROTECTED : 0; $val += $this->isPrivate() ? CoreReflectionProperty::IS_PRIVATE : 0; + $val += $this->isReadOnly() ? self::IS_READONLY : 0; return $val; } diff --git a/test/unit/Reflection/ReflectionPropertyTest.php b/test/unit/Reflection/ReflectionPropertyTest.php index a02353238..1f9f9c16c 100644 --- a/test/unit/Reflection/ReflectionPropertyTest.php +++ b/test/unit/Reflection/ReflectionPropertyTest.php @@ -12,7 +12,6 @@ use PhpParser\Node\Stmt\Property; use PhpParser\Node\Stmt\PropertyProperty; use PHPUnit\Framework\TestCase; -use Reflection; use ReflectionProperty as CoreReflectionProperty; use Roave\BetterReflection\Reflection\Exception\ClassDoesNotExist; use Roave\BetterReflection\Reflection\Exception\NoObjectProvided; @@ -156,32 +155,25 @@ public function testGetDocCommentReturnsEmptyStringWithNoComment(): void self::assertSame('', $property->getDocComment()); } - /** @return list}> */ + /** @return list */ public function modifierProvider(): array { return [ - ['publicProperty', CoreReflectionProperty::IS_PUBLIC, ['public']], - ['protectedProperty', CoreReflectionProperty::IS_PROTECTED, ['protected']], - ['privateProperty', CoreReflectionProperty::IS_PRIVATE, ['private']], - ['publicStaticProperty', CoreReflectionProperty::IS_PUBLIC | CoreReflectionProperty::IS_STATIC, ['public', 'static']], + ['publicProperty', CoreReflectionProperty::IS_PUBLIC], + ['protectedProperty', CoreReflectionProperty::IS_PROTECTED], + ['privateProperty', CoreReflectionProperty::IS_PRIVATE], + ['publicStaticProperty', CoreReflectionProperty::IS_PUBLIC | CoreReflectionProperty::IS_STATIC], + ['readOnlyProperty', CoreReflectionProperty::IS_PUBLIC | ReflectionProperty::IS_READONLY], ]; } - /** - * @param list $expectedModifierNames - * - * @dataProvider modifierProvider - */ - public function testGetModifiers(string $propertyName, int $expectedModifier, array $expectedModifierNames): void + /** @dataProvider modifierProvider */ + public function testGetModifiers(string $propertyName, int $expectedModifier): void { $classInfo = $this->reflector->reflectClass(ExampleClass::class); $property = $classInfo->getProperty($propertyName); self::assertSame($expectedModifier, $property->getModifiers()); - self::assertSame( - $expectedModifierNames, - Reflection::getModifierNames($property->getModifiers()), - ); } public function testIsPromoted(): void From d44ff48cad4f6969de4a288c6b033ea906ce02e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Hansl=C3=ADk?= Date: Thu, 22 Sep 2022 14:13:48 +0200 Subject: [PATCH 2/2] Removed ReflectionProperty::getAst() to prevent memory leaks --- src/Reflection/ReflectionClass.php | 12 +- src/Reflection/ReflectionProperty.php | 220 +++++++++++++----- test/unit/Reflection/ReflectionClassTest.php | 4 - test/unit/Reflection/ReflectionObjectTest.php | 2 - .../Reflection/ReflectionPropertyTest.php | 90 ++++--- 5 files changed, 216 insertions(+), 112 deletions(-) diff --git a/src/Reflection/ReflectionClass.php b/src/Reflection/ReflectionClass.php index 2de422e4e..f7868e536 100644 --- a/src/Reflection/ReflectionClass.php +++ b/src/Reflection/ReflectionClass.php @@ -886,14 +886,10 @@ static function (ReflectionClass $ancestor) use ($filter): array { ), ...array_map( function (ReflectionClass $trait) use ($filter) { - return array_map(fn (ReflectionProperty $property): ReflectionProperty => ReflectionProperty::createFromNode( - $this->reflector, - $property->getAst(), - $property->getPositionInAst(), - $property->getDeclaringClass(), - $this, - $property->isPromoted(), - ), $trait->getProperties($filter)); + return array_map( + fn (ReflectionProperty $property): ReflectionProperty => ReflectionProperty::withImplementingClass($property, $this), + $trait->getProperties($filter), + ); }, $this->getTraits(), ), diff --git a/src/Reflection/ReflectionProperty.php b/src/Reflection/ReflectionProperty.php index 125e67ce5..f6788e40e 100644 --- a/src/Reflection/ReflectionProperty.php +++ b/src/Reflection/ReflectionProperty.php @@ -8,7 +8,6 @@ use Error; use OutOfBoundsException; use PhpParser\Node; -use PhpParser\Node\NullableType; use PhpParser\Node\Stmt\Property as PropertyNode; use ReflectionException; use ReflectionProperty as CoreReflectionProperty; @@ -21,13 +20,14 @@ use Roave\BetterReflection\Reflection\Exception\NoObjectProvided; use Roave\BetterReflection\Reflection\Exception\NotAnObject; use Roave\BetterReflection\Reflection\Exception\ObjectNotInstanceOfClass; -use Roave\BetterReflection\Reflection\Exception\Uncloneable; use Roave\BetterReflection\Reflection\StringCast\ReflectionPropertyStringCast; use Roave\BetterReflection\Reflector\Exception\IdentifierNotFound; use Roave\BetterReflection\Reflector\Reflector; use Roave\BetterReflection\Util\CalculateReflectionColumn; use Roave\BetterReflection\Util\ClassExistenceChecker; +use Roave\BetterReflection\Util\Exception\NoNodePosition; use Roave\BetterReflection\Util\GetLastDocComment; +use RuntimeException; use function assert; use function func_num_args; @@ -45,17 +45,80 @@ class ReflectionProperty */ public const IS_READONLY = 128; + /** @var non-empty-string */ + private string $name; + + private int $modifiers = 0; + + private ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType|null $type; + + private Node\Expr|null $default; + + private string $docComment; + + /** @var list */ + private array $attributes; + + /** @var positive-int|null */ + private int|null $startLine; + + /** @var positive-int|null */ + private int|null $endLine; + + /** @var positive-int|null */ + private int|null $startColumn; + + /** @var positive-int|null */ + private int|null $endColumn; + private CompiledValue|null $compiledDefaultValue = null; private function __construct( private Reflector $reflector, - private PropertyNode $node, - private int $positionInNode, + PropertyNode $node, + int $positionInNode, private ReflectionClass $declaringClass, private ReflectionClass $implementingClass, private bool $isPromoted, private bool $declaredAtCompileTime, ) { + $name = $node->props[$positionInNode]->name->name; + assert($name !== ''); + + $this->name = $name; + $this->type = $this->createType($node); + $this->default = $node->props[$positionInNode]->default; + $this->docComment = GetLastDocComment::forNode($node); + $this->attributes = ReflectionAttributeHelper::createAttributes($reflector, $this, $node->attrGroups); + + $this->computeModifiers($node); + + $startLine = null; + if ($node->hasAttribute('startLine')) { + $startLine = $node->getStartLine(); + assert($startLine > 0); + } + + $endLine = null; + if ($node->hasAttribute('endLine')) { + $endLine = $node->getEndLine(); + assert($endLine > 0); + } + + $this->startLine = $startLine; + $this->endLine = $endLine; + + try { + $this->startColumn = CalculateReflectionColumn::getStartColumn($declaringClass->getLocatedSource()->getSource(), $node); + } catch (NoNodePosition) { + $this->startColumn = null; + } + + try { + $this->endColumn = CalculateReflectionColumn::getEndColumn($declaringClass->getLocatedSource()->getSource(), $node); + } catch (NoNodePosition) { + $this->endColumn = null; + } } /** @@ -92,6 +155,15 @@ public static function createFromInstance(object $instance, string $propertyName return $property; } + /** @internal */ + public static function withImplementingClass(self $property, ReflectionClass $implementingClass): self + { + $clone = clone $property; + $clone->implementingClass = $implementingClass; + + return $clone; + } + public function __toString(): string { return ReflectionPropertyStringCast::toString($this); @@ -139,21 +211,17 @@ public function isDefault(): bool */ public function getModifiers(): int { - $val = $this->isStatic() ? CoreReflectionProperty::IS_STATIC : 0; - $val += $this->isPublic() ? CoreReflectionProperty::IS_PUBLIC : 0; - $val += $this->isProtected() ? CoreReflectionProperty::IS_PROTECTED : 0; - $val += $this->isPrivate() ? CoreReflectionProperty::IS_PRIVATE : 0; - $val += $this->isReadOnly() ? self::IS_READONLY : 0; - - return $val; + return $this->modifiers; } /** * Get the name of the property. + * + * @return non-empty-string */ public function getName(): string { - return $this->node->props[$this->positionInNode]->name->name; + return $this->name; } /** @@ -161,7 +229,7 @@ public function getName(): string */ public function isPrivate(): bool { - return $this->node->isPrivate(); + return ($this->modifiers & CoreReflectionProperty::IS_PRIVATE) === CoreReflectionProperty::IS_PRIVATE; } /** @@ -169,7 +237,7 @@ public function isPrivate(): bool */ public function isProtected(): bool { - return $this->node->isProtected(); + return ($this->modifiers & CoreReflectionProperty::IS_PROTECTED) === CoreReflectionProperty::IS_PROTECTED; } /** @@ -177,7 +245,7 @@ public function isProtected(): bool */ public function isPublic(): bool { - return $this->node->isPublic(); + return ($this->modifiers & CoreReflectionProperty::IS_PUBLIC) === CoreReflectionProperty::IS_PUBLIC; } /** @@ -185,7 +253,7 @@ public function isPublic(): bool */ public function isStatic(): bool { - return $this->node->isStatic(); + return ($this->modifiers & CoreReflectionProperty::IS_STATIC) === CoreReflectionProperty::IS_STATIC; } public function isPromoted(): bool @@ -216,7 +284,7 @@ public function isInitialized(object|null $object = null): bool public function isReadOnly(): bool { - return $this->node->isReadonly(); + return ($this->modifiers & self::IS_READONLY) === self::IS_READONLY; } public function getDeclaringClass(): ReflectionClass @@ -231,12 +299,12 @@ public function getImplementingClass(): ReflectionClass public function getDocComment(): string { - return GetLastDocComment::forNode($this->node); + return $this->docComment; } public function hasDefaultValue(): bool { - return ! $this->hasType() || $this->node->props[$this->positionInNode]->default !== null; + return ! $this->hasType() || $this->default !== null; } /** @@ -247,15 +315,13 @@ public function hasDefaultValue(): bool */ public function getDefaultValue(): string|int|float|bool|array|null { - $defaultValueNode = $this->node->props[$this->positionInNode]->default; - - if ($defaultValueNode === null) { + if ($this->default === null) { return null; } if ($this->compiledDefaultValue === null) { $this->compiledDefaultValue = (new CompileNodeToValue())->__invoke( - $defaultValueNode, + $this->default, new CompilerContext( $this->reflector, $this, @@ -276,44 +342,68 @@ public function isDeprecated(): bool /** * Get the line number that this property starts on. + * + * @return positive-int + * + * @throws RuntimeException */ public function getStartLine(): int { - return $this->node->getStartLine(); + if ($this->startLine === null) { + throw new RuntimeException('Start line missing'); + } + + return $this->startLine; } /** * Get the line number that this property ends on. + * + * @return positive-int + * + * @throws RuntimeException */ public function getEndLine(): int { - return $this->node->getEndLine(); + if ($this->endLine === null) { + throw new RuntimeException('End line missing'); + } + + return $this->endLine; } + /** + * @return positive-int + * + * @throws RuntimeException + */ public function getStartColumn(): int { - return CalculateReflectionColumn::getStartColumn($this->declaringClass->getLocatedSource()->getSource(), $this->node); - } + if ($this->startColumn === null) { + throw new RuntimeException('Start column missing'); + } - public function getEndColumn(): int - { - return CalculateReflectionColumn::getEndColumn($this->declaringClass->getLocatedSource()->getSource(), $this->node); + return $this->startColumn; } - public function getAst(): PropertyNode + /** + * @return positive-int + * + * @throws RuntimeException + */ + public function getEndColumn(): int { - return $this->node; - } + if ($this->endColumn === null) { + throw new RuntimeException('End column missing'); + } - public function getPositionInAst(): int - { - return $this->positionInNode; + return $this->endColumn; } /** @return list */ public function getAttributes(): array { - return ReflectionAttributeHelper::createAttributes($this->reflector, $this, $this->node->attrGroups); + return $this->attributes; } /** @return list */ @@ -332,16 +422,6 @@ public function getAttributesByInstance(string $className): array return ReflectionAttributeHelper::filterAttributesByInstance($this->getAttributes(), $className); } - /** - * {@inheritdoc} - * - * @throws Uncloneable - */ - public function __clone() - { - throw Uncloneable::fromClass(self::class); - } - /** * @throws ClassDoesNotExist * @throws NoObjectProvided @@ -411,11 +491,20 @@ public function setValue(mixed $object, mixed $value = null): void */ public function allowsNull(): bool { - if (! $this->hasType()) { - return true; + return $this->type === null || $this->type->allowsNull(); + } + + private function createType(PropertyNode $node): ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType|null + { + $type = $node->type; + + if ($type === null) { + return null; } - return $this->node->type instanceof NullableType; + assert($type instanceof Node\Identifier || $type instanceof Node\Name || $type instanceof Node\NullableType || $type instanceof Node\UnionType || $type instanceof Node\IntersectionType); + + return ReflectionType::createFromNode($this->reflector, $this, $type); } /** @@ -426,15 +515,7 @@ public function allowsNull(): bool */ public function getType(): ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType|null { - $type = $this->node->type; - - if ($type === null) { - return null; - } - - assert($type instanceof Node\Identifier || $type instanceof Node\Name || $type instanceof Node\NullableType || $type instanceof Node\UnionType || $type instanceof Node\IntersectionType); - - return ReflectionType::createFromNode($this->reflector, $this, $type); + return $this->type; } /** @@ -444,7 +525,7 @@ public function getType(): ReflectionNamedType|ReflectionUnionType|ReflectionInt */ public function hasType(): bool { - return $this->node->type !== null; + return $this->type !== null; } /** @@ -484,4 +565,23 @@ private function assertObject(mixed $object): object return $object; } + + private function computeModifiers(PropertyNode $node): void + { + if ($node->isStatic()) { + $this->modifiers += CoreReflectionProperty::IS_STATIC; + } + + if ($node->isReadonly()) { + $this->modifiers += self::IS_READONLY; + } + + if ($node->isPrivate()) { + $this->modifiers += CoreReflectionProperty::IS_PRIVATE; + } elseif ($node->isProtected()) { + $this->modifiers += CoreReflectionProperty::IS_PROTECTED; + } else { + $this->modifiers += CoreReflectionProperty::IS_PUBLIC; + } + } } diff --git a/test/unit/Reflection/ReflectionClassTest.php b/test/unit/Reflection/ReflectionClassTest.php index f6d3782ed..2d4f79d87 100644 --- a/test/unit/Reflection/ReflectionClassTest.php +++ b/test/unit/Reflection/ReflectionClassTest.php @@ -560,7 +560,6 @@ public function testGetPropertiesForPureEnum(): void $property = $properties['name']; - self::assertSame(0, $property->getPositionInAst()); self::assertSame('name', $property->getName()); self::assertTrue($property->isPublic()); self::assertTrue($property->isReadOnly()); @@ -613,7 +612,6 @@ public function testGetPropertiesForBackedEnum(string $className, array $propert $property = $properties[$propertyName]; - self::assertSame(0, $property->getPositionInAst(), $fullPropertyName); self::assertSame($propertyName, $property->getName(), $fullPropertyName); self::assertTrue($property->isPublic(), $fullPropertyName); self::assertTrue($property->isReadOnly(), $fullPropertyName); @@ -733,13 +731,11 @@ public function testGetImmediateProperties(): void self::assertSame(Qux::class, $fProperty->getDeclaringClass()->getName()); self::assertFalse($fProperty->isPromoted()); - self::assertSame(0, $fProperty->getPositionInAst()); $gProperty = $classInfo->getProperty('g'); self::assertSame(Qux::class, $gProperty->getDeclaringClass()->getName()); self::assertTrue($gProperty->isPromoted()); - self::assertSame(0, $gProperty->getPositionInAst()); } public function testGetProperty(): void diff --git a/test/unit/Reflection/ReflectionObjectTest.php b/test/unit/Reflection/ReflectionObjectTest.php index 324d34929..23a528a29 100644 --- a/test/unit/Reflection/ReflectionObjectTest.php +++ b/test/unit/Reflection/ReflectionObjectTest.php @@ -124,7 +124,6 @@ public function testReflectionWorksWithDynamicallyDeclaredMembers(): void self::assertFalse($propInfo->isDefault()); self::assertTrue($propInfo->isPublic()); self::assertSame('huzzah', $propInfo->getDefaultValue()); - self::assertSame(0, $propInfo->getPositionInAst()); self::assertFalse($propInfo->isPromoted()); } @@ -189,7 +188,6 @@ public function testRuntimePropertyCannotBePromoted(): void self::assertInstanceOf(ReflectionProperty::class, $propertyInfo); self::assertFalse($propertyInfo->isPromoted()); - self::assertSame(0, $propertyInfo->getPositionInAst()); } public function testGetDefaultPropertiesShouldIgnoreRuntimeProperty(): void diff --git a/test/unit/Reflection/ReflectionPropertyTest.php b/test/unit/Reflection/ReflectionPropertyTest.php index 1f9f9c16c..f07bea63d 100644 --- a/test/unit/Reflection/ReflectionPropertyTest.php +++ b/test/unit/Reflection/ReflectionPropertyTest.php @@ -17,11 +17,11 @@ use Roave\BetterReflection\Reflection\Exception\NoObjectProvided; use Roave\BetterReflection\Reflection\Exception\NotAnObject; use Roave\BetterReflection\Reflection\Exception\ObjectNotInstanceOfClass; -use Roave\BetterReflection\Reflection\Exception\Uncloneable; use Roave\BetterReflection\Reflection\ReflectionProperty; use Roave\BetterReflection\Reflector\DefaultReflector; use Roave\BetterReflection\Reflector\Reflector; use Roave\BetterReflection\SourceLocator\Ast\Locator; +use Roave\BetterReflection\SourceLocator\Type\AggregateSourceLocator; use Roave\BetterReflection\SourceLocator\Type\ComposerSourceLocator; use Roave\BetterReflection\SourceLocator\Type\SingleFileSourceLocator; use Roave\BetterReflection\SourceLocator\Type\StringSourceLocator; @@ -36,7 +36,9 @@ use Roave\BetterReflectionTest\Fixture\Php74PropertyTypeDeclarations; use Roave\BetterReflectionTest\Fixture\PropertyGetSet; use Roave\BetterReflectionTest\Fixture\StaticPropertyGetSet; +use Roave\BetterReflectionTest\Fixture\StringEnum; use Roave\BetterReflectionTest\Fixture\TraitStaticPropertyGetSet; +use RuntimeException; use stdClass; use TraitWithProperty; @@ -59,10 +61,10 @@ public function setUp(): void public function testCreateFromName(): void { - $property = ReflectionProperty::createFromName(ReflectionProperty::class, 'node'); + $property = ReflectionProperty::createFromName(ReflectionProperty::class, 'name'); self::assertInstanceOf(ReflectionProperty::class, $property); - self::assertSame('node', $property->getName()); + self::assertSame('name', $property->getName()); } public function testCreateFromNameThrowsExceptionWhenPropertyDoesNotExist(): void @@ -251,15 +253,6 @@ public function testPropertyDefaultValue(string $propertyName, bool $hasDefaultV self::assertSame($defaultValue, $property->getDefaultValue()); } - public function testCannotClone(): void - { - $classInfo = $this->reflector->reflectClass(ExampleClass::class); - $publicProp = $classInfo->getProperty('publicProperty'); - - $this->expectException(Uncloneable::class); - clone $publicProp; - } - /** * @param non-empty-string $php * @@ -310,39 +303,60 @@ public function testGetStartColumnAndEndColumn(string $php, int $startColumn, in self::assertEquals($endColumn, $constantReflection->getEndColumn()); } - /** @return list */ - public function getAstProvider(): array + public function testGetStartLineThrowsExceptionForMagicallyAddedEnumProperty(): void { - return [ - ['a', 0], - ['b', 1], - ['c', 0], - ['d', 1], - ]; + $reflector = new DefaultReflector(new AggregateSourceLocator([ + new SingleFileSourceLocator(__DIR__ . '/../Fixture/Enums.php', $this->astLocator), + BetterReflectionSingleton::instance()->sourceLocator(), + ])); + + $classReflection = $reflector->reflectClass(StringEnum::class); + $propertyReflection = $classReflection->getProperty('name'); + + self::expectException(RuntimeException::class); + $propertyReflection->getStartLine(); } - /** @dataProvider getAstProvider */ - public function testGetAst(string $propertyName, int $positionInAst): void + public function testGetEndLineThrowsExceptionForMagicallyAddedEnumProperty(): void { - $php = <<<'PHP' -astLocator), + BetterReflectionSingleton::instance()->sourceLocator(), + ])); - $classReflection = (new DefaultReflector(new StringSourceLocator($php, $this->astLocator)))->reflectClass('Foo'); - $propertyReflection = $classReflection->getProperty($propertyName); + $classReflection = $reflector->reflectClass(StringEnum::class); + $propertyReflection = $classReflection->getProperty('name'); + + self::expectException(RuntimeException::class); + $propertyReflection->getEndLine(); + } + + public function testGetStartColumnThrowsExceptionForMagicallyAddedEnumProperty(): void + { + $reflector = new DefaultReflector(new AggregateSourceLocator([ + new SingleFileSourceLocator(__DIR__ . '/../Fixture/Enums.php', $this->astLocator), + BetterReflectionSingleton::instance()->sourceLocator(), + ])); + + $classReflection = $reflector->reflectClass(StringEnum::class); + $propertyReflection = $classReflection->getProperty('name'); + + self::expectException(RuntimeException::class); + $propertyReflection->getStartColumn(); + } + + public function testGetEndColumnThrowsExceptionForMagicallyAddedEnumProperty(): void + { + $reflector = new DefaultReflector(new AggregateSourceLocator([ + new SingleFileSourceLocator(__DIR__ . '/../Fixture/Enums.php', $this->astLocator), + BetterReflectionSingleton::instance()->sourceLocator(), + ])); - $ast = $propertyReflection->getAst(); + $classReflection = $reflector->reflectClass(StringEnum::class); + $propertyReflection = $classReflection->getProperty('name'); - self::assertInstanceOf(Property::class, $ast); - self::assertSame($positionInAst, $propertyReflection->getPositionInAst()); - self::assertSame($propertyName, $ast->props[$positionInAst]->name->name); + self::expectException(RuntimeException::class); + $propertyReflection->getEndColumn(); } public function testGetDeclaringAndImplementingClassWithPropertyFromTrait(): void