From 322cff6f436379a2d54475cb6f457f1d6d37785d Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sat, 19 Nov 2022 15:54:02 +0100 Subject: [PATCH 1/9] Declaring more precise types and purity boundaries on `ext-reflection` symbols in `.phpstub` files Also: * added PHP 8.2 stubs * refined types to make impossible scenarios more clear (like `ReflectionIntersectionType#allowsNull()`) This is a first attempt at refining these types: the structure of these stubs is quite confusing to me, so I don't know if this approach is correct, and if the stubs are merged together, or if entire symbols need to be completely re-declared for each PHP version. --- src/Psalm/Config.php | 10 + stubs/Php80.phpstub | 113 ++++++++- stubs/Php81.phpstub | 28 +-- stubs/Php82.phpstub | 7 + stubs/Reflection.phpstub | 504 +++++++++++++++++++++++++++++++++++++-- 5 files changed, 620 insertions(+), 42 deletions(-) create mode 100644 stubs/Php82.phpstub diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php index ee172a4300a..6967ef44445 100644 --- a/src/Psalm/Config.php +++ b/src/Psalm/Config.php @@ -2085,6 +2085,16 @@ public function visitPreloadedStubFiles(Codebase $codebase, ?Progress $progress $core_generic_files[] = $stringable_path; } + if (PHP_VERSION_ID < 8_02_00 && $codebase->analysis_php_version_id >= 8_02_00) { + $stringable_path = dirname(__DIR__, 2) . '/stubs/Php82.phpstub'; + + if (!file_exists($stringable_path)) { + throw new UnexpectedValueException('Cannot locate PHP 8.2 classes'); + } + + $core_generic_files[] = $stringable_path; + } + $stub_files = array_merge($core_generic_files, $this->preloaded_stub_files); if (!$stub_files) { diff --git a/stubs/Php80.phpstub b/stubs/Php80.phpstub index 50d9d78e4be..5d3c9fcf4cd 100644 --- a/stubs/Php80.phpstub +++ b/stubs/Php80.phpstub @@ -17,14 +17,24 @@ class ReflectionAttribute { } + /** + * @return non-empty-string + * + * @psalm-pure + */ public function getName() : string { } + /** + * @psalm-pure + * @return int-mask-of + */ public function getTarget() : int { } + /** @psalm-pure */ public function isRepeated() : bool { } @@ -48,13 +58,101 @@ class ReflectionAttribute } } +/** + * @template-covariant T as object + * + * @property-read class-string $name + */ +class ReflectionClass implements Reflector { + /** + * @return non-empty-string|false + * @psalm-pure + */ + public function getFileName(): string|false {} + + /** + * @return positive-int|false + * @psalm-pure + */ + public function getStartLine(): int|false {} + + /** + * @return positive-int|false + * @psalm-pure + */ + public function getEndLine(): int|false {} + + /** + * @return non-empty-string|false + * @psalm-pure + */ + public function getDocComment(): string|false {} + + /** + * @param ReflectionClass|class-string $class + * + * @psalm-pure + */ + public function isSubclassOf(self|string $class): bool {} + + /** + * @param self|class-string $interface + * + * @psalm-pure + */ + public function implementsInterface(self|string $interface): bool {} + + /** + * @return non-empty-string|false + * + * @psalm-pure + */ + public function getExtensionName(): string|false {} +} + +/** @psalm-immutable */ class ReflectionClassConstant { public const IS_PUBLIC = 1; public const IS_PROTECTED = 2; public const IS_PRIVATE = 4; + + /** @return non-empty-string|false */ + public function getDocComment(): string|false {} +} + +abstract class ReflectionFunctionAbstract implements Reflector +{ + /** + * @return non-empty-string|false + * + * @psalm-pure + */ + public function getDocComment(): string|false {} + + /** + * @return positive-int|false + * + * @psalm-pure + */ + public function getStartLine(): int|false {} + + /** + * @return positive-int|false + * + * @psalm-pure + */ + public function getEndLine(): int|false {} + + /** + * @return non-empty-string|false + * + * @psalm-pure + */ + public function getFileName(): string|false {} } +/** @psalm-immutable */ class Attribute { public int $flags; @@ -76,11 +174,20 @@ class Attribute } } -class ReflectionUnionType extends ReflectionType { +class ReflectionProperty implements Reflector +{ /** - * @return non-empty-list + * @return non-empty-string|false + * + * @psalm-pure */ - public function getTypes() {} + public function getDocComment(): string|false {} +} + +/** @psalm-immutable */ +class ReflectionUnionType extends ReflectionType { + /** @return non-empty-list */ + public function getTypes(): array {} } class UnhandledMatchError extends Error {} diff --git a/stubs/Php81.phpstub b/stubs/Php81.phpstub index 694e4e9ce72..984fef8609c 100644 --- a/stubs/Php81.phpstub +++ b/stubs/Php81.phpstub @@ -26,6 +26,12 @@ namespace { public static function tryFrom(string|int $value): ?static; } + class ReflectionClass implements Reflector { + /** @psalm-pure */ + public function isEnum(): bool {} + } + + /** @psalm-immutable */ class ReflectionEnum extends ReflectionClass implements Reflector { public function getBackingType(): ?ReflectionType; @@ -36,32 +42,26 @@ namespace { public function isBacked(): bool; } + /** @psalm-immutable */ class ReflectionEnumUnitCase extends ReflectionClassConstant implements Reflector { - /** - * @psalm-pure - */ public function getEnum(): ReflectionEnum; - - /** - * @psalm-pure - */ public function getValue(): UnitEnum; } + /** @psalm-immutable */ class ReflectionEnumBackedCase extends ReflectionEnumUnitCase implements Reflector { - /** - * @psalm-pure - */ public function getBackingValue(): int|string; } + /** @psalm-immutable */ class ReflectionIntersectionType extends ReflectionType { - /** - * @return non-empty-list - */ - public function getTypes() {} + /** @return non-empty-list */ + public function getTypes(): array {} + + /** @return false */ + public function allowsNull(): bool {} } } diff --git a/stubs/Php82.phpstub b/stubs/Php82.phpstub new file mode 100644 index 00000000000..fd153f3e5c0 --- /dev/null +++ b/stubs/Php82.phpstub @@ -0,0 +1,7 @@ +|interface-string|trait-string|enum-string $argument + * @psalm-pure */ public function __construct($argument) {} /** * @return class-string + * @psalm-pure */ public function getName(): string {} + + /** @psalm-pure */ + public function isInternal(): bool {} + + /** @psalm-pure */ + public function isUserDefined(): bool {} + + /** @psalm-pure */ + public function isInstantiable(): bool {} + + /** @psalm-pure */ + public function isCloneable(): bool {} + + /** + * @return non-empty-string|false + * @psalm-pure + */ + public function getFileName() {} + + /** + * @return positive-int|false + * @psalm-pure + */ + public function getStartLine() {} + + /** + * @return positive-int|false + * @psalm-pure + */ + public function getEndLine() {} + + /** + * @return non-empty-string|false + * @psalm-pure + */ + public function getDocComment() {} + + /** @psalm-pure */ + public function getConstructor(): ?ReflectionMethod {} + + /** @psalm-pure */ + public function hasMethod(string $name): bool {} + + /** + * @param non-empty-string $name + * + * @psalm-pure + * @throws ReflectionException + */ + public function getMethod(string $name): ReflectionMethod {} + + /** + * @param int-mask-of|null $filter + * @return list + * @psalm-pure + */ + public function getMethods(?int $filter = null): array {} + + /** + * @param non-empty-string $name + * + * @psalm-pure + * @throws ReflectionException + */ + public function hasProperty(string $name): bool {} + + /** + * @param non-empty-string $name + * + * @psalm-pure + * @throws ReflectionException + */ + public function getProperty(): ReflectionProperty {} + + /** + * @param int-mask-of|null $filter + * @return list + * + * @psalm-pure + */ + public function getProperties(?int $filter = null): array {} + + /** + * @param non-empty-string $name + * + * @psalm-pure + */ + public function hasConstant(string $name): bool {} + + /** + * @param non-empty-string $name + * @return mixed + * + * @psalm-pure + * @throws ReflectionException + */ + public function getConstant(string $name) {} + + /** + * @param non-empty-string $name + * @return ReflectionClassConstant|false + * + * @psalm-pure + * @throws ReflectionException + */ + public function getReflectionConstant(string $name) {} + + /** + * @param int-mask-of|null $filter + * @return array + * + * @psalm-pure + */ + public function getConstants(?int $filter = null): array {} + + /** + * @param int-mask-of|null $filter + * @return array + * + * @psalm-pure + */ + public function getReflectionConstants(?int $filter = null): array {} + + /** + * @return array + * + * @psalm-pure + */ + public function getInterfaces(): array {} + + /** + * @return list + * + * @psalm-pure + */ + public function getInterfaceNames(): array {} + + /** @psalm-pure */ + public function isInterface(): bool {} + + /** + * @return array + * + * @psalm-pure + */ + public function getTraits(): array {} + + /** + * @return list + * + * @psalm-pure + */ + public function getTraitAliases(): array {} + + /** @psalm-pure */ + public function isTrait(): bool {} + + /** @psalm-pure */ + public function isAbstract(): bool {} + + /** @psalm-pure */ + public function isFinal(): bool {} + + /** + * @return int-mask-of + * @psalm-pure + */ + public function getModifiers(): bool {} + + /** + * @template TCheckedObject of object + * + * @param TCheckedObject $object + * + * @return (TCheckedObject is T ? true : bool) + * + * @psalm-pure + */ + public function isInstance(object $object): bool {} /** * @param mixed ...$args @@ -41,9 +222,66 @@ class ReflectionClass implements Reflector { */ public function newInstanceWithoutConstructor(): object {} + /** @psalm-pure */ + public function getParentClass(): ?ReflectionClass {} + + /** + * @param ReflectionClass|class-string $class + * + * @psalm-pure + */ + public function isSubclassOf($class): bool {} + + /** @return array */ + public function getStaticProperties(): array {} + + /** + * @return array + * + * @psalm-pure + */ + public function getDefaultProperties(): array {} + + /** @psalm-pure */ + public function isIterateable(): bool {} + + /** @psalm-pure */ + public function isIterable(): bool {} + + /** + * @param self|class-string $interface + * + * @psalm-pure + */ + public function implementsInterface($interface): bool {} + + /** @psalm-pure */ + public function getExtension(): ?ReflectionExtension {} + + /** + * @return non-empty-string|false + * + * @psalm-pure + */ + public function getExtensionName() {} + + /** @psalm-pure */ + public function inNamespace(): bool {} + + /** @psalm-pure */ + public function getNamespaceName(): string {} + + /** + * @return non-empty-string + * + * @psalm-pure + */ + public function getShortName(): string {} + /** * @return ?array * @psalm-ignore-nullable-return + * @psalm-pure */ public function getTraitNames(): array {} @@ -51,13 +289,138 @@ class ReflectionClass implements Reflector { * @since 8.0 * @template TClass as object * @param class-string|null $name - * @return ($name is null ? array> : array>) + * @return ($name is null ? list> : list>) + * @psalm-pure */ public function getAttributes(?string $name = null, int $flags = 0): array {} } -class ReflectionFunction implements Reflector +abstract class ReflectionFunctionAbstract implements Reflector { + /** @psalm-pure */ + public function inNamespace(): bool {} + + /** @psalm-pure */ + public function isClosure(): bool {} + + /** @psalm-pure */ + public function isDeprecated(): bool {} + + /** @psalm-pure */ + public function isInternal(): bool {} + + /** @psalm-pure */ + public function isUserDefined(): bool {} + + public function getClosureThis(): ?object {} + + /** @psalm-pure */ + public function getClosureScopeClass(): ?ReflectionClass {} + + /** @psalm-pure */ + public function getClosureCalledClass(): ?ReflectionClass {} + + /** + * @return non-empty-string|false + * + * @psalm-pure + */ + public function getDocComment() {} + + /** + * @return positive-int|false + * + * @psalm-pure + */ + public function getStartLine() {} + + /** + * @return positive-int|false + * + * @psalm-pure + */ + public function getEndLine() {} + + /** @psalm-pure */ + public function getExtension(): ?ReflectionExtension {} + + /** + * @return non-empty-string + * + * @psalm-pure + */ + public function getExtensionName(): string {} + + /** + * @return non-empty-string|false + * + * @psalm-pure + */ + public function getFileName() {} + + /** + * @return non-empty-string + * + * @psalm-pure + */ + public function getName(): string {} + + /** @psalm-pure */ + public function getNamespaceName(): string {} + + /** + * @return positive-int|0 + * + * @psalm-pure + */ + public function getNumberOfParameters(): int {} + + /** + * @return positive-int|0 + * + * @psalm-pure + */ + public function getNumberOfRequiredParameters(): int {} + + /** + * @return list + * + * @psalm-pure + */ + public function getParameters(): array {} + + /** + * @psalm-assert-if-true ReflectionType $this->getReturnType() + * + * @psalm-pure + */ + public function hasReturnType(): bool {} + + /** @psalm-pure */ + public function getReturnType(): ?ReflectionType {} + + /** + * @return non-empty-string + * + * @psalm-pure + */ + public function getShortName(): string {} + + /** @psalm-pure */ + public function returnsReference(): bool {} + + /** @psalm-pure */ + public function isGenerator(): bool {} + + /** @psalm-pure */ + public function isVariadic(): bool {} + + /** @psalm-pure */ + public function isDisabled(): bool {} + + /** @psalm-pure */ + public function getClosure(): Closure {} + /** * @since 8.0 * @template TClass as object @@ -67,10 +430,23 @@ class ReflectionFunction implements Reflector public function getAttributes(?string $name = null, int $flags = 0): array {} } +class ReflectionFunction extends ReflectionFunctionAbstract +{ + /** @psalm-pure */ + public function __construct(callable $function) {} + + /** + * @return non-empty-string + * + * @psalm-pure + */ + public function __toString(): string {} +} + class ReflectionProperty implements Reflector { /** - * @var string + * @var non-empty-string * @readonly */ public $name; @@ -81,6 +457,13 @@ class ReflectionProperty implements Reflector */ public $class; + /** + * @return non-empty-string + * + * @psalm-pure + */ + public function getName(): string {} + /** * @since 8.0 * @template TClass as object @@ -92,32 +475,74 @@ class ReflectionProperty implements Reflector /** * @since 7.4 * @psalm-assert-if-true ReflectionType $this->getType() + * @psalm-pure */ public function hasType() : bool {} /** * @since 7.4 - * @psalm-mutation-free + * @psalm-pure */ public function getType() : ?ReflectionType {} + /** @psalm-pure */ + public function isPublic(): bool {} + + /** @psalm-pure */ + public function isPrivate(): bool {} + + /** @psalm-pure */ + public function isProtected(): bool {} + + /** @psalm-pure */ + public function isStatic(): bool {} + + /** @psalm-pure */ + public function isDefault(): bool {} + /** - * @since 8.0 - */ + * @return int-mask-of + * @psalm-pure + */ + public function getModifiers(): int {} + + /** @psalm-pure */ + public function getDeclaringClass(): ReflectionClass {} + + /** + * @return non-empty-string|false + * + * @psalm-pure + */ + public function getDocComment() {} + + /** + * @since 8.0 + * @psalm-pure + */ public function hasDefaultValue(): bool {} /** - * @since 8.0 - */ + * @return mixed + * + * @psalm-pure + */ + public function getDefaultValue() {} + + /** + * @since 8.0 + * @psalm-pure + */ public function isPromoted(): bool {} /** - * @since 8.1 - */ + * @since 8.1 + * @psalm-pure + */ public function isReadOnly(): bool {} } -class ReflectionMethod implements Reflector +class ReflectionMethod extends ReflectionFunctionAbstract { /** * @var string @@ -131,21 +556,33 @@ class ReflectionMethod implements Reflector */ public $class; + /** @psalm-pure */ + public function isStatic(): bool {} + + /** @psalm-pure */ + public function isConstructor(): bool {} + + /** @psalm-pure */ + public function isDestructor(): bool {} + /** - * @since 8.0 - * @template TClass as object - * @param class-string|null $name - * @return ($name is null ? array> : array>) + * @return int-mask-of + * @psalm-pure */ - public function getAttributes(?string $name = null, int $flags = 0): array {} + public function getModifiers(): bool {} - public function isStatic(): bool {} + /** @psalm-pure */ + public function getDeclaringClass(): ReflectionClass {} + + /** @psalm-pure */ + public function getPrototype(): ReflectionMethod {} } +/** @psalm-immutable */ class ReflectionClassConstant implements Reflector { /** - * @var string + * @var non-empty-string * @readonly */ public $name; @@ -170,17 +607,27 @@ class ReflectionClassConstant implements Reflector * @return ($name is null ? array> : array>) */ public function getAttributes(?string $name = null, int $flags = 0): array {} + + /** @return non-empty-string */ + public function getName(): string {} + + /** @return int-mask-of */ + public function getModifiers(): int {} + + /** @return non-empty-string|false */ + public function getDocComment() {} } -/** - * @psalm-immutable - */ +/** @psalm-immutable */ class ReflectionParameter implements Reflector { /** - * @var string + * @var non-empty-string * @readonly */ public $name; + + /** @return non-empty-string */ + public function getName(): string {} /** * @psalm-assert-if-true ReflectionType $this->getType() @@ -203,15 +650,22 @@ class ReflectionParameter implements Reflector { public function isPromoted(): bool {} } -/** - * @psalm-immutable - */ +/** @psalm-immutable */ +abstract class ReflectionType +{ +} + +/** @psalm-immutable */ class ReflectionNamedType extends ReflectionType { + /** @return non-empty-string */ public function getName(): string {} /** * @psalm-assert-if-false class-string|'self'|'static' $this->getName() */ public function isBuiltin(): bool {} + + /** @return non-empty-string */ + public function __toString(): string {} } From d5cccbade209819c3a350ec96d89150bcb7bc7bb Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 6 Dec 2022 11:12:01 +0100 Subject: [PATCH 2/9] Marking `ReflectionProperty#$name` as `string` rather than `non-empty-string` Because @weirdan is a party pooper (they poop at the parties) Ref: https://www.youtube.com/watch?v=gjwofYhUJEM Ref: https://github.com/vimeo/psalm/pull/8722#discussion_r1027151708 --- stubs/Reflection.phpstub | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stubs/Reflection.phpstub b/stubs/Reflection.phpstub index cc6b8477002..166e2a67768 100644 --- a/stubs/Reflection.phpstub +++ b/stubs/Reflection.phpstub @@ -446,7 +446,7 @@ class ReflectionFunction extends ReflectionFunctionAbstract class ReflectionProperty implements Reflector { /** - * @var non-empty-string + * @var string * @readonly */ public $name; From d9a0cc531113c75ea98dba04cbeccc01f4ae7ba4 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 6 Dec 2022 11:19:16 +0100 Subject: [PATCH 3/9] Prevent usage of callable objects in `ReflectionFunction::__construct()` As per @weirdan's feedback, we can prevent the usage of `object` instances that implement `__invoke()`, as well as `array` callables, by declaring the ctor argument of `ReflectionFunction` to be either a real `Closure`, or a `callable-string`. While this may not be 100% of scenarios, it is a healthy way to identify errors in userland. Ref: https://github.com/vimeo/psalm/pull/8722#discussion_r1027151421 --- stubs/Reflection.phpstub | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/stubs/Reflection.phpstub b/stubs/Reflection.phpstub index 166e2a67768..478dfdf8039 100644 --- a/stubs/Reflection.phpstub +++ b/stubs/Reflection.phpstub @@ -432,7 +432,11 @@ abstract class ReflectionFunctionAbstract implements Reflector class ReflectionFunction extends ReflectionFunctionAbstract { - /** @psalm-pure */ + /** + * @param callable-string|Closure $function + * + * @psalm-pure + */ public function __construct(callable $function) {} /** From 79a1a8b26ce5f8fc9589fd779a3ec1da939b07c8 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 6 Dec 2022 11:21:09 +0100 Subject: [PATCH 4/9] Removed templated parameters from `ReflectionClass#isInstance()` These templates were leading to false positives: assuming an `object` is given as input, the inferred return type would always have been `true`, which is obviously not valid. Removing them is the healthier alternative, for now. Ref: https://github.com/vimeo/psalm/pull/8722#discussion_r1027102713 --- stubs/Reflection.phpstub | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/stubs/Reflection.phpstub b/stubs/Reflection.phpstub index 478dfdf8039..4d464ebc707 100644 --- a/stubs/Reflection.phpstub +++ b/stubs/Reflection.phpstub @@ -192,15 +192,7 @@ class ReflectionClass implements Reflector { */ public function getModifiers(): bool {} - /** - * @template TCheckedObject of object - * - * @param TCheckedObject $object - * - * @return (TCheckedObject is T ? true : bool) - * - * @psalm-pure - */ + /** @psalm-pure */ public function isInstance(object $object): bool {} /** From 93c5df6bfc1bf5c28b0696bfe3d3cbc6092edcb6 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 6 Dec 2022 18:26:50 +0100 Subject: [PATCH 5/9] Refine `ReflectionUnionType` and `ReflectionIntersectionType` for PHP 8.1 and PHP 8.2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * in PHP 8.0, `ReflectionUnionType` is composed on `ReflectionNamedType`s * in PHP 8.1, `ReflectionIntersectionType` is composed of `ReflectionNamedType`s * in PHP 8.2, `ReflectionUnionType` is composed of `ReflectionIntersectionType|ReflectionNamedType`s Slight variations for each PHP version. As per local testing, this doesn't work yet. ## Local testing setup: I did some digging to make sure that the stubs work as expected. Here's what I did to validate this patch locally (since I don't think it can really be fully automated) ## Create a dummy file to verify used symbols ```php */ function getAClass(): \ReflectionClass { throw new \Exception('irrelevant'); } function getAnUnionType(): \ReflectionUnionType { throw new \Exception('irrelevant'); } function getAnIntersectionType(): \ReflectionIntersectionType { throw new \Exception('irrelevant'); } // verifying that `getName()` is stubbed in all versions: this should always be a `class-string<\stdClass>` $name = getAClass()->getName(); // union types should appear starting with PHP 8.0. Starting with PHP 8.2, they allow for intersections. $unionTypes = getAnUnionType()->getTypes(); // intersection types should appear starting with PHP 8.1 $intersectionTypes = getAnIntersectionType()->getTypes(); $results = [$name, $unionTypes, $intersectionTypes]; /** @psalm-trace $results */ // tracing this will show us the differences between versions return $results; ``` ## Run the script against various `vimeo/psalm` versions ```sh docker run --rm -ti -v $(pwd):/app -w /app php:7.4 ./psalm --php-version=7.4 --no-cache reflection-test.php | grep Trace docker run --rm -ti -v $(pwd):/app -w /app php:8.0 ./psalm --php-version=8.0 --no-cache reflection-test.php | grep Trace docker run --rm -ti -v $(pwd):/app -w /app php:8.1 ./psalm --php-version=8.1 --no-cache reflection-test.php | grep Trace docker run --rm -ti -v $(pwd):/app -w /app php:8.2.0RC7-cli ./psalm --php-version=8.2 --no-cache reflection-test.php | grep Trace ``` ## Evaluate output ``` ❯ docker run --rm -ti -v $(pwd):/app -w /app php:7.4 ./psalm --php-version=7.4 --no-cache reflection-test.php | grep Trace ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string, mixed, mixed} (see https://psalm.dev/224) ❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.0 ./psalm --php-version=8.0 --no-cache reflection-test.php | grep Trace ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string, non-empty-list, mixed} (see https://psalm.dev/224) ❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.1 ./psalm --php-version=8.1 --no-cache reflection-test.php | grep Trace ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string, non-empty-list, non-empty-list} (see https://psalm.dev/224) psalm on  feature/#8720-improve-types-and-purity-for-reflection-symbols [!?] via 🐘 v8.1.13 via ❄️ impure (nix-shell) took 4s ❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.2.0RC7-cli ./psalm --php-version=8.2 --no-cache reflection-test.php | grep Trace ERROR: Trace - reflection-test.php:20:1 - $results: list{class-string, non-empty-list, non-empty-list} (see https://psalm.dev/224) ``` --- stubs/Php81.phpstub | 2 +- stubs/Php82.phpstub | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/stubs/Php81.phpstub b/stubs/Php81.phpstub index 984fef8609c..547ee0fa3d0 100644 --- a/stubs/Php81.phpstub +++ b/stubs/Php81.phpstub @@ -57,7 +57,7 @@ namespace { /** @psalm-immutable */ class ReflectionIntersectionType extends ReflectionType { - /** @return non-empty-list */ + /** @return non-empty-list */ public function getTypes(): array {} /** @return false */ diff --git a/stubs/Php82.phpstub b/stubs/Php82.phpstub index fd153f3e5c0..5ac5ee5f316 100644 --- a/stubs/Php82.phpstub +++ b/stubs/Php82.phpstub @@ -4,4 +4,10 @@ namespace { /** @psalm-pure */ public function isReadOnly(): bool {} } + + /** @psalm-immutable */ + class ReflectionUnionType extends ReflectionType { + /** @return non-empty-list */ + public function getTypes(): array {} + } } From ed2cde1b939a1f56b89757b376c2811d16b18ff5 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Wed, 7 Dec 2022 14:22:15 +0100 Subject: [PATCH 6/9] Mark `Reflection(Method|Property)#setAccessible()` as pure starting from PHP 8.1 onwards This will highlight unused code. Ref: https://github.com/php/php-src/pull/5412 Ref: https://wiki.php.net/rfc/make-reflection-setaccessible-no-op Ref: https://github.com/php/php-src/pull/5411 Example https://3v4l.org/PNeeZ ```php getValue(new Foo)); //var_dump((new ReflectionMethod(Foo::class, 'taz'))->invoke(new Foo)); ``` Produces (starting from PHP 8.1): ``` string(3) "baz" string(3) "waz" ``` --- stubs/Php81.phpstub | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/stubs/Php81.phpstub b/stubs/Php81.phpstub index 547ee0fa3d0..06d6d0debc3 100644 --- a/stubs/Php81.phpstub +++ b/stubs/Php81.phpstub @@ -31,6 +31,26 @@ namespace { public function isEnum(): bool {} } + class ReflectionProperty implements Reflector + { + /** + * Starting from PHP 8.1, this method is pure, and has no effect. + * + * @psalm-pure + */ + public function setAccessible(bool $accessible): void {} + } + + class ReflectionMethod extends ReflectionFunctionAbstract + { + /** + * Starting from PHP 8.1, this method is pure, and has no effect. + * + * @psalm-pure + */ + public function setAccessible(bool $accessible): void {} + } + /** @psalm-immutable */ class ReflectionEnum extends ReflectionClass implements Reflector { From 30a49633a533600a2aab5bd5681f3ab30623861b Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Wed, 7 Dec 2022 15:44:38 +0100 Subject: [PATCH 7/9] Corrected `AttributeTest` expectation: `ReflectionAttribute`s always come in a `list` --- tests/AttributeTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/AttributeTest.php b/tests/AttributeTest.php index 717a61a6ed7..0544fd1b0df 100644 --- a/tests/AttributeTest.php +++ b/tests/AttributeTest.php @@ -125,7 +125,7 @@ function foo(string $s) : void { $b = $a->getAttributes(); ', 'assertions' => [ - '$b' => 'array>', + '$b' => 'list>', ], 'ignored_issues' => [], 'php_version' => '8.0' From 042305107e826c1d4f14491ab2e3ed23b0824202 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Wed, 7 Dec 2022 15:48:59 +0100 Subject: [PATCH 8/9] Always load preloaded stub files when the analysis version is compatible Before this change, preloaded stubs would only be loaded when running on a PHP version lower than the one that is in the stubs. With this change, the analysis PHP version (defined via config, input parameter, or inferred from the runtime) becomes authoritative. Since the PHP-version-specific stubs are not just polyfills, but actually type refinements on top of the PHP core symbols at hand, this change always loads them, so that it is possible to get more precise type inference downstream. This will likely lead to downstream breakages, because the stubs do indeed provide better type resolution, but indeed formalizes the idea that these stubs will provide better value for finding problems in analyzed code. --- src/Psalm/Config.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php index 6967ef44445..87860d0fb35 100644 --- a/src/Psalm/Config.php +++ b/src/Psalm/Config.php @@ -2065,7 +2065,7 @@ public function visitPreloadedStubFiles(Codebase $codebase, ?Progress $progress $core_generic_files = []; - if (PHP_VERSION_ID < 8_00_00 && $codebase->analysis_php_version_id >= 8_00_00) { + if ($codebase->analysis_php_version_id >= 8_00_00) { $stringable_path = dirname(__DIR__, 2) . '/stubs/Php80.phpstub'; if (!file_exists($stringable_path)) { @@ -2075,7 +2075,7 @@ public function visitPreloadedStubFiles(Codebase $codebase, ?Progress $progress $core_generic_files[] = $stringable_path; } - if (PHP_VERSION_ID < 8_01_00 && $codebase->analysis_php_version_id >= 8_01_00) { + if ($codebase->analysis_php_version_id >= 8_01_00) { $stringable_path = dirname(__DIR__, 2) . '/stubs/Php81.phpstub'; if (!file_exists($stringable_path)) { @@ -2085,7 +2085,7 @@ public function visitPreloadedStubFiles(Codebase $codebase, ?Progress $progress $core_generic_files[] = $stringable_path; } - if (PHP_VERSION_ID < 8_02_00 && $codebase->analysis_php_version_id >= 8_02_00) { + if ($codebase->analysis_php_version_id >= 8_02_00) { $stringable_path = dirname(__DIR__, 2) . '/stubs/Php82.phpstub'; if (!file_exists($stringable_path)) { From 68d88c546b286a5fbdabe4728f6be9dd1db0dbe0 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Thu, 8 Dec 2022 18:22:51 +0100 Subject: [PATCH 9/9] Load PHP-version-specific stubs based on analysis PHP version, and only when visiting stub files While `visitPreloadedStubFiles` seemed to work at first, it led to overriding symbols declared by PHP itself too eagerly. By only loading PHP-version-specific stubs in `visitStubFiles`, we ensure that the reflection-declared symbols take priority, and that our stubs overlay on top of that, without actually replacing the symbol entirely, but rather merging with its definition. This fixes current test failures too, and works with the code examples from https://github.com/vimeo/psalm/pull/8722#issuecomment-1339711882 --- src/Psalm/Config.php | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php index 87860d0fb35..a8a5da7101a 100644 --- a/src/Psalm/Config.php +++ b/src/Psalm/Config.php @@ -2065,7 +2065,7 @@ public function visitPreloadedStubFiles(Codebase $codebase, ?Progress $progress $core_generic_files = []; - if ($codebase->analysis_php_version_id >= 8_00_00) { + if (PHP_VERSION_ID < 8_00_00 && $codebase->analysis_php_version_id >= 8_00_00) { $stringable_path = dirname(__DIR__, 2) . '/stubs/Php80.phpstub'; if (!file_exists($stringable_path)) { @@ -2075,7 +2075,7 @@ public function visitPreloadedStubFiles(Codebase $codebase, ?Progress $progress $core_generic_files[] = $stringable_path; } - if ($codebase->analysis_php_version_id >= 8_01_00) { + if (PHP_VERSION_ID < 8_01_00 && $codebase->analysis_php_version_id >= 8_01_00) { $stringable_path = dirname(__DIR__, 2) . '/stubs/Php81.phpstub'; if (!file_exists($stringable_path)) { @@ -2085,7 +2085,7 @@ public function visitPreloadedStubFiles(Codebase $codebase, ?Progress $progress $core_generic_files[] = $stringable_path; } - if ($codebase->analysis_php_version_id >= 8_02_00) { + if (PHP_VERSION_ID < 8_02_00 && $codebase->analysis_php_version_id >= 8_02_00) { $stringable_path = dirname(__DIR__, 2) . '/stubs/Php82.phpstub'; if (!file_exists($stringable_path)) { @@ -2135,16 +2135,21 @@ public function visitStubFiles(Codebase $codebase, ?Progress $progress = null): $dir_lvl_2 . DIRECTORY_SEPARATOR . 'stubs' . DIRECTORY_SEPARATOR . 'SPL.phpstub', ]; - if (PHP_VERSION_ID >= 8_00_00 && $codebase->analysis_php_version_id >= 8_00_00) { + if ($codebase->analysis_php_version_id >= 8_00_00) { $stringable_path = $dir_lvl_2 . DIRECTORY_SEPARATOR . 'stubs' . DIRECTORY_SEPARATOR . 'Php80.phpstub'; $this->internal_stubs[] = $stringable_path; } - if (PHP_VERSION_ID >= 8_01_00 && $codebase->analysis_php_version_id >= 8_01_00) { + if ($codebase->analysis_php_version_id >= 8_01_00) { $stringable_path = $dir_lvl_2 . DIRECTORY_SEPARATOR . 'stubs' . DIRECTORY_SEPARATOR . 'Php81.phpstub'; $this->internal_stubs[] = $stringable_path; } + if ($codebase->analysis_php_version_id >= 8_02_00) { + $stringable_path = $dir_lvl_2 . DIRECTORY_SEPARATOR . 'stubs' . DIRECTORY_SEPARATOR . 'Php82.phpstub'; + $this->internal_stubs[] = $stringable_path; + } + foreach ($this->php_extensions as $ext => $enabled) { if ($enabled) { $this->internal_stubs[] = $dir_lvl_2 . DIRECTORY_SEPARATOR . "stubs"