Declaring more precise types and purity boundaries on `ext-reflection` symbols in `.phpstub` files #8722

Ocramius · 2022-11-19T14:54:58Z

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.

Fixes #8720
Fixes #6378

src/Psalm/Config.php

stubs/Php80.phpstub

stubs/Php81.phpstub

stubs/Php82.phpstub

stubs/Reflection.phpstub

Ocramius · 2022-11-19T15:03:14Z

@kukulich you are the reflection wizard, these days: can you glance over this, if you have time and want to? (not binding, I just think your experience here is extremely valuable)

stubs/Reflection.phpstub

Ocramius · 2022-11-23T16:23:55Z

Besides feedback provided so far, is this the right way of implementing this patch, with multiple stubs for the same class, each stub "decorating" previous PHP version stubs?

If that's the case, then I can gladly apply the provided feedback, get the build green, and then we can move this forward.

orklah · 2022-11-23T17:40:42Z

To be honest, this is pretty much uncharted territories, The versioned stubs are somewhat recent (we never needed those before PHP 8) and we mainly built them empirically when issues are raised.

So yeah, if it works as is, I, for one, will gladly merge this but I'm not perfectly sure it will. For example, I'm not completely sure we can have two loaded stubs of the same class (especially if there are conflicting methods). If it's causing an issue, we may have to resort to having exclusive stubs loaded for only one PHP version.

By the way, sorry to bring this only now, we have an extension folder in which the reflection stubs should probably be extracted (I kinda forgot this since it's something new from Psalm 5 that was added some time ago). If we have to have versioned stubs, this will be easier to have a Reflection71, Reflection72... in there

Note: many reflection-related suppressions depend on vimeo/psalm#8722 Also, we preserved many assertions on `class-string`, since removing input validation would be a BC break.

…` 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.

@weirdan

…-string` Because @weirdan is a party pooper (they poop at the parties) Ref: https://www.youtube.com/watch?v=gjwofYhUJEM Ref: vimeo#8722 (comment)

@weirdan

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: vimeo#8722 (comment)

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: vimeo#8722 (comment)

Ocramius · 2022-12-06T16:35:00Z

I did some more local testing: PHP 8.2 stubs are somehow not picked up correctly.

That will take some time to debug :D

Ocramius · 2022-12-06T17:17:35Z

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)

TL;DR: ✔️ works as expected in my own test expectations

Create a dummy file to verify used symbols

<?php

namespace Testing;

/** @return \ReflectionClass<\stdClass> */
function getAClass(): \ReflectionClass
{
    throw new \Exception('irrelevant');
}

function getAnUnionType(): \ReflectionUnionType
{
    throw new \Exception('irrelevant');
}

function getAnIntersectionType(): \ReflectionIntersectionType
{
    throw new \Exception('irrelevant');
}

function getAProperty(): \ReflectionProperty
{
    throw new \Exception('irrelevant');
}

function getAMethod(): \ReflectionMethod
{
    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
$unionTypes = getAnUnionType()->getTypes();
// intersection types should appear starting with PHP 8.1, and change slightly with PHP 8.2
$intersectionTypes = getAnIntersectionType()->getTypes();

getAProperty()->setAccessible(true); // should be marked as unused expression starting from PHP 8.1
getAMethod()->setAccessible(true); // should be marked as unused expression starting from PHP 8.1

$results = [$name, $unionTypes, $intersectionTypes, \ReflectionMethod::IS_PUBLIC];

/** @psalm-trace $results */ // tracing this will show us the differences between versions
return $results;

Run the script against various `vimeo/psalm` versions

docker run --rm -ti -v $(pwd):/app -w /app php:7.4 ./psalm --php-version=7.4 --no-cache reflection-test.php

docker run --rm -ti -v $(pwd):/app -w /app php:8.0 ./psalm --php-version=8.0 --no-cache reflection-test.php

docker run --rm -ti -v $(pwd):/app -w /app php:8.1 ./psalm --php-version=8.1 --no-cache reflection-test.php

docker run --rm -ti -v $(pwd):/app -w /app php:8.2.0RC7-cli ./psalm --php-version=8.2 --no-cache reflection-test.php

Evaluate output

❯ docker run --rm -ti -v $(pwd):/app -w /app php:7.4 ./psalm --php-version=7.4 --no-cache reflection-test.php
Target PHP version: 7.4 (set by CLI argument) Extensions enabled: dom, simplexml (unsupported extensions: ctype, json, libxml, mbstring, tokenizer)
Scanning files...
Analyzing files...

░

To whom it may concern: Psalm cannot detect unused classes, methods and properties
when analyzing individual files and folders. Run on the full project to enable
complete unused code detection.

ERROR: UndefinedClass - reflection-test.php:11:28 - Class, interface or enum named ReflectionUnionType does not exist (see https://psalm.dev/019)
function getAnUnionType(): \ReflectionUnionType


ERROR: UndefinedClass - reflection-test.php:16:35 - Class, interface or enum named ReflectionIntersectionType does not exist (see https://psalm.dev/019)
function getAnIntersectionType(): \ReflectionIntersectionType


ERROR: MixedAssignment - reflection-test.php:34:1 - Unable to determine the type that $unionTypes is being assigned to (see https://psalm.dev/032)
$unionTypes = getAnUnionType()->getTypes();


ERROR: UndefinedClass - reflection-test.php:34:15 - Class, interface or enum named ReflectionUnionType does not exist (see https://psalm.dev/019)
$unionTypes = getAnUnionType()->getTypes();


ERROR: MixedAssignment - reflection-test.php:36:1 - Unable to determine the type that $intersectionTypes is being assigned to (see https://psalm.dev/032)
$intersectionTypes = getAnIntersectionType()->getTypes();


ERROR: UndefinedClass - reflection-test.php:36:22 - Class, interface or enum named ReflectionIntersectionType does not exist (see https://psalm.dev/019)
$intersectionTypes = getAnIntersectionType()->getTypes();


ERROR: Trace - reflection-test.php:44:1 - $results: list{class-string<stdClass>, mixed, mixed, 1} (see https://psalm.dev/224)
/** @psalm-trace $results */ // tracing this will show us the differences between versions
return $results;


------------------------------
7 errors found
------------------------------

❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.0 ./psalm --php-version=8.0 --no-cache reflection-test.php
Target PHP version: 8.0 (set by CLI argument) Extensions enabled: dom, simplexml (unsupported extensions: ctype, json, libxml, mbstring, tokenizer)
Scanning files...
Analyzing files...

░

To whom it may concern: Psalm cannot detect unused classes, methods and properties
when analyzing individual files and folders. Run on the full project to enable
complete unused code detection.

ERROR: UndefinedClass - reflection-test.php:16:35 - Class, interface or enum named ReflectionIntersectionType does not exist (see https://psalm.dev/019)
function getAnIntersectionType(): \ReflectionIntersectionType


ERROR: MixedAssignment - reflection-test.php:36:1 - Unable to determine the type that $intersectionTypes is being assigned to (see https://psalm.dev/032)
$intersectionTypes = getAnIntersectionType()->getTypes();


ERROR: UndefinedClass - reflection-test.php:36:22 - Class, interface or enum named ReflectionIntersectionType does not exist (see https://psalm.dev/019)
$intersectionTypes = getAnIntersectionType()->getTypes();


ERROR: Trace - reflection-test.php:44:1 - $results: list{class-string<stdClass>, non-empty-list<ReflectionNamedType>, mixed, 1} (see https://psalm.dev/224)
/** @psalm-trace $results */ // tracing this will show us the differences between versions
return $results;


------------------------------
4 errors found
------------------------------
❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.1 ./psalm --php-version=8.1 --no-cache reflection-test.php
Target PHP version: 8.1 (set by CLI argument) Extensions enabled: dom, simplexml (unsupported extensions: ctype, json, libxml, mbstring, tokenizer)
Scanning files...
Analyzing files...

░

To whom it may concern: Psalm cannot detect unused classes, methods and properties
when analyzing individual files and folders. Run on the full project to enable
complete unused code detection.

ERROR: UnusedMethodCall - reflection-test.php:38:17 - The call to ReflectionProperty::setAccessible is not used (see https://psalm.dev/209)
getAProperty()->setAccessible(true); // should be marked as unused expression starting from PHP 8.1


ERROR: UnusedMethodCall - reflection-test.php:39:15 - The call to ReflectionMethod::setAccessible is not used (see https://psalm.dev/209)
getAMethod()->setAccessible(true); // should be marked as unused expression starting from PHP 8.1


ERROR: Trace - reflection-test.php:44:1 - $results: list{class-string<stdClass>, non-empty-list<ReflectionNamedType>, non-empty-list<ReflectionNamedType>, 1} (see https://psalm.dev/224)
/** @psalm-trace $results */ // tracing this will show us the differences between versions
return $results;


------------------------------
3 errors found
------------------------------
❯ docker run --rm -ti -v $(pwd):/app -w /app php:8.2.0RC7-cli ./psalm --php-version=8.2 --no-cache reflection-test.php
Target PHP version: 8.2 (set by CLI argument) Extensions enabled: dom, simplexml (unsupported extensions: ctype, json, libxml, mbstring, tokenizer)
Scanning files...
Analyzing files...

░

To whom it may concern: Psalm cannot detect unused classes, methods and properties
when analyzing individual files and folders. Run on the full project to enable
complete unused code detection.

ERROR: UnusedMethodCall - reflection-test.php:38:17 - The call to ReflectionProperty::setAccessible is not used (see https://psalm.dev/209)
getAProperty()->setAccessible(true); // should be marked as unused expression starting from PHP 8.1


ERROR: UnusedMethodCall - reflection-test.php:39:15 - The call to ReflectionMethod::setAccessible is not used (see https://psalm.dev/209)
getAMethod()->setAccessible(true); // should be marked as unused expression starting from PHP 8.1


ERROR: Trace - reflection-test.php:44:1 - $results: list{class-string<stdClass>, non-empty-list<ReflectionIntersectionType|ReflectionNamedType>, non-empty-list<ReflectionNamedType>, 1} (see https://psalm.dev/224)
/** @psalm-trace $results */ // tracing this will show us the differences between versions
return $results;


------------------------------
3 errors found
------------------------------

Note: output updated as per 68d88c5

@return

… 8.1 and PHP 8.2 * 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 <?php namespace Testing; /** @return \ReflectionClass<\stdClass> */ 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<stdClass>, 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<stdClass>, non-empty-list<ReflectionNamedType>, 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<stdClass>, non-empty-list<ReflectionNamedType>, non-empty-list<ReflectionNamedType>} (see https://psalm.dev/224) psalm on  feature/vimeo#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<stdClass>, non-empty-list<ReflectionNamedType>, non-empty-list<ReflectionNamedType>} (see https://psalm.dev/224) ```

…rom PHP 8.1 onwards This will highlight unused code. Ref: php/php-src#5412 Ref: https://wiki.php.net/rfc/make-reflection-setaccessible-no-op Ref: php/php-src#5411 Example https://3v4l.org/PNeeZ ```php <?php class Foo { private $bar = 'baz'; private function taz() { return 'waz'; } } //var_dump((new ReflectionProperty(Foo::class, 'bar'))->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" ```

…come in a `list`

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.

stubs/Reflection.phpstub

orklah · 2022-12-07T15:17:54Z

Thanks for your work on this! I'll find some time this week end to look at what's going on to see if I can help :)

src/Psalm/Config.php

This massively reduces the internal complexity of the `TypeGenerator`, since we convert `ReflectionType` symbols **directly** to our `@internal` data structures, ready to be rendered. Before this, we would cast the whole `ReflectionType` to a `string` that fits our need, and then we would go back to parsing that string, with a substantial overhead (especially considering the newly introduced validation rules around DNF types). Note that this introduces new Psalm violations that were added to `psalm-baseline.xml`, but which are solved by my work @ vimeo/psalm#8722 Signed-off-by: Marco Pivetta <ocramius@gmail.com>

…ly 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 vimeo#8722 (comment)

Ocramius · 2022-12-08T17:38:27Z

@orklah I think actual coding work on this is complete: what is missing is validation / decision making, but otherwise we are green :)

Hopefully that means not cutting too much into your time anymore 👍

orklah · 2022-12-11T21:23:23Z

Thanks Marco!

I checked that and I was surprised that Stringable was not flagged as an error here: https://psalm.dev/r/3d60a7e0ea?phpVersion=7.1

So we already have global namespace pollution and it didn't cause much issue, I'm not worried about Reflection classes leaking then.

I'd be interested in the result of running Psalm against BetterReflection in different php versions for verification but otherwise, I'm good! We can merge when ready :)

psalm-github-bot · 2022-12-11T21:23:25Z

I found these snippets:

https://psalm.dev/r/3d60a7e0ea

<?php

class A implements Stringable{}

Psalm output (using commit ef02ded):

ERROR: UnimplementedInterfaceMethod - 3:7 - Method __tostring is not defined on class A

Ocramius · 2022-12-11T23:38:36Z

The two most aggressive reflection codebases I know of are:

laminas/laminas-code (very inheritance-heavy)
roave/better-reflection, which is providing full polyfills for core reflection

I will check both against this patch and report back 👍

Ocramius · 2022-12-12T09:47:36Z

I ran a report over roave/better-reflection and laminas/laminas-code (attached):

laminas-laminas-code-report.txt

roave-betterreflection-report.txt

Overall just MissingImmutableAnnotation and a few less specific types downstream :)

orklah · 2022-12-12T20:27:40Z

Seems great! Thanks a lot Marco!

Ocramius · 2022-12-12T20:28:54Z

🎉

As per PHP 8.1, this call has no effect anymore. Ref: vimeo/psalm#8722 Ref: https://wiki.php.net/rfc/make-reflection-setaccessible-no-op

Ocramius commented Nov 19, 2022

View reviewed changes

stubs/Reflection.phpstub Show resolved Hide resolved

Ocramius mentioned this pull request Nov 19, 2022

Drop PHP 7.4 and PHP 8.0 support laminas/laminas-code#154

Merged

2 tasks

weirdan added the release:feature The PR will be included in 'Features' section of the release notes label Nov 19, 2022

weirdan reviewed Nov 19, 2022

View reviewed changes

stubs/Reflection.phpstub Outdated Show resolved Hide resolved

kukulich reviewed Nov 21, 2022

View reviewed changes

stubs/Reflection.phpstub Show resolved Hide resolved

Ocramius added 2 commits December 6, 2022 11:08

Marking ReflectionProperty#$name as string rather than `non-empty…

d5cccba

…-string` Because @weirdan is a party pooper (they poop at the parties) Ref: https://www.youtube.com/watch?v=gjwofYhUJEM Ref: vimeo#8722 (comment)

Ocramius force-pushed the feature/#8720-improve-types-and-purity-for-reflection-symbols branch from e4eb00b to d5cccba Compare December 6, 2022 10:12

Ocramius added 2 commits December 6, 2022 11:19

Ocramius added 4 commits December 6, 2022 18:26

Corrected AttributeTest expectation: ReflectionAttributes always …

30a4963

…come in a `list`

Ocramius commented Dec 7, 2022

View reviewed changes

stubs/Reflection.phpstub Show resolved Hide resolved

Ocramius commented Dec 7, 2022

View reviewed changes

src/Psalm/Config.php Outdated Show resolved Hide resolved

orklah merged commit 72e7386 into vimeo:master Dec 12, 2022

Ocramius deleted the feature/#8720-improve-types-and-purity-for-reflection-symbols branch December 12, 2022 20:28

VincentLanglet mentioned this pull request Dec 18, 2022

Fix ReflectionClass stub #8930

Merged

orklah mentioned this pull request Jan 9, 2023

Fix CallMap for Reflection(Union|Intersection)Type::getTypes() #9084

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Declaring more precise types and purity boundaries on `ext-reflection` symbols in `.phpstub` files #8722

Declaring more precise types and purity boundaries on `ext-reflection` symbols in `.phpstub` files #8722

Ocramius commented Nov 19, 2022 •

edited by weirdan

Ocramius commented Nov 19, 2022

Ocramius commented Nov 23, 2022

orklah commented Nov 23, 2022

Ocramius commented Dec 6, 2022

Ocramius commented Dec 6, 2022 •

edited

orklah commented Dec 7, 2022

Ocramius commented Dec 8, 2022

orklah commented Dec 11, 2022

psalm-github-bot bot commented Dec 11, 2022

Ocramius commented Dec 11, 2022

Ocramius commented Dec 12, 2022

orklah commented Dec 12, 2022

Ocramius commented Dec 12, 2022

Declaring more precise types and purity boundaries on ext-reflection symbols in .phpstub files #8722

Declaring more precise types and purity boundaries on ext-reflection symbols in .phpstub files #8722

Conversation

Ocramius commented Nov 19, 2022 • edited by weirdan

Ocramius commented Nov 19, 2022

Ocramius commented Nov 23, 2022

orklah commented Nov 23, 2022

Ocramius commented Dec 6, 2022

Ocramius commented Dec 6, 2022 • edited

Create a dummy file to verify used symbols

Run the script against various vimeo/psalm versions

Evaluate output

orklah commented Dec 7, 2022

Ocramius commented Dec 8, 2022

orklah commented Dec 11, 2022

psalm-github-bot bot commented Dec 11, 2022

Ocramius commented Dec 11, 2022

Ocramius commented Dec 12, 2022

orklah commented Dec 12, 2022

Ocramius commented Dec 12, 2022

Declaring more precise types and purity boundaries on `ext-reflection` symbols in `.phpstub` files #8722

Declaring more precise types and purity boundaries on `ext-reflection` symbols in `.phpstub` files #8722

Ocramius commented Nov 19, 2022 •

edited by weirdan

Ocramius commented Dec 6, 2022 •

edited

Run the script against various `vimeo/psalm` versions