Add golden test for reflection #2679

schlndh · 2023-10-13T10:20:23Z

I was looking into some reflection issues (as you asked me here). However, it is hard to do these sort of "global" fixes without knowing what the effect will be. Of course some things will be caught by unit and integration tests, but it's never going to cover "everything".

So I came up with this proof-of-concept golden test for reflection. The idea is to get "all" the standard classes, methods, properties and functions, get their reflection and dump it into a file (in some human-readable format). Then whenever a change is made to the reflection code (or when the stubs are updated), we'll have an idea what the effect is by looking at what changed in the test file. Based on that we should accept the correct changes and reject the incorrect ones.

Before I go any further with it, I'd like to know your opinion on this idea.

Here are some things that will still need to be addressed:

Generate reflection data for all relevant PHP versions + make it easy to do locally.
Figure out how to deal with things that rely on native reflection. The first thing is that the data will have to be generated in the same environment in which the GH action runs (right now I just generated it with my PHP installation). But then we might get test failures during local development.

Here is an example of it's usefulness in my WIP attempt to fix the parameter names for multi-variant functions: schlndh/phpstan-src@feature-reflectionGoldenTest...schlndh:phpstan-src:fix-paramNamesFromSignatureMap

ondrejmirtes · 2023-10-13T11:01:05Z

This is an interesting idea! I've been meaning to do the same thing about rules and type inference tests. Right now RuleTestCase runs only the rule under test. But we could take advantage of the fact that we have literally thousands of interesting code snippets in the test suite. And we could run all the rules on all the code snippets and store the results somewhere. Then we could detect and measure the impact of every change on this dataset. And the fact that the output changes isn't always bad but sometimes good!

That said, we need some less obtrusive way than adding 100k LoC to the codebase.

I have an idea: We could use GitHub Actions in a similar way to the issue bot. A new workflow could do:

Checkout the base commit of a push or a PR.
Run this golden test and save the output somewhere.
Checkout the HEAD commit of a push or a PR.
Re-run this golden test, save the output.
Diff the outputs from 2) and 5) and print it.

That way we don't need to store huge amounts of information in the repo, and still get the same benefit.

schlndh · 2023-10-13T11:26:59Z

@ondrejmirtes Ok, thanks for the feedback. The dump is currently about 2 MB in size, and we'd need a separate one for each relevant PHP version. So it would be a lot of extra data.

The idea to use Github Actions looks promising. I'll try to look into it.

phpstan-bot · 2023-10-20T11:09:25Z

This pull request has been marked as ready for review.

schlndh · 2023-10-20T11:11:41Z

I moved it to a Github Action. Here is a PR that showcases how it works when something changes in the reflection code: schlndh#1

ondrejmirtes

If the workflow checks out the base commit and dumps the reflection data before the new commits, we don't need to have tests/PHPStan/Reflection/data/golden/phpSymbols.php commited in the repository, right?

schlndh · 2023-10-29T08:45:35Z

@ondrejmirtes The base commit needs access to the symbols, so they have to come from somewhere. I suppose that the symbols could somehow be generated dynamically (e.g. function map, phpstorm/php-8 stubs, ...). It would be more complicated, but on the other hand it would auto-update itself to new PHP versions etc (and we wouldn't need the giant file with the symbols).

Which do you prefer?

ondrejmirtes · 2023-10-29T08:48:33Z

Sorry, I'd need more time to actually understand what the 500-LoC test does in order to get why we need that phpSymbols.php file beforehand. But in my mind and my expectations from this feature the workflow wouldn't need anything like that.

We can dump the state by running the base commit and compare with the dumped state from the new commit. I don't get why we need an extra pre-existing huge file that exists on the base commit too. We could just generate it during the workflow.

schlndh · 2023-10-29T09:11:57Z

It basically works like this:

ReflectionProviderGoldenTest:dumpOutput is called on the base commit. It goes through a list of symbols and generates and reflection dump for each of those. It saves the symbols and their corresponding dumps into a test file (e.g. reflection-8.2.test). The test file looks like this:

FUNCTION strval
-----
Variants: 1
    /**
     * @param bool|float|int|resource|string|null $var
     * @return string
     */
    function strval(bool|float|int|resource|string|null $var): string
-----
FUNCTION substr
-----
Variants: 1
    /**
     * @param string $string
     * @param int $start
     * @param int $length
     * @return (string|false)
     */
    function substr(string $string, int $start, int $length): (string|false)

ReflectionProviderGoldenTest::test is called on the head commit. The data provider parses the test file generated from the base commit. The result is a list of symbols and their expected reflection dumps. It then generates a new dump for each symbol and compares it to the old one. This step no longer needs phpSymbols.php - it only uses the test file.

So the process needs a list of symbols as an input, otherwise we wouldn't know what to include in the test file generated from the base commit. Right now, that list of symbols comes from phpSymbols.php (for simplicity), but it could be generated dynamically (e.g. by parsing the stubs/functionMap).

ondrejmirtes · 2023-10-30T13:11:40Z

Yes, please, use the following sources to compile a list of functions and methods:

https://github.com/phpstan/phpstan-src/blob/1.11.x/resources/functionMap.php + delta files with corresponding PHP version
https://github.com/phpstan/php-8-stubs/blob/main/Php8StubsMap.php
https://github.com/JetBrains/phpstorm-stubs/blob/master/PhpStormStubsMap.php
And for the extra bit of craziness, please enable some obscure PHP extensions that are not covered by any of the above sources in setup-php (https://github.com/shivammathur/setup-php#heavy_plus_sign-php-extension-support), and get the list with get_declared_classes and get_defined_functions.

This reverts commit ebcaa2ade4a773e9c3f39db388531650f349ed88.

These symbols fail to reflect on PHP 7.3/7.4 CLASS Random\IntervalBoundary CLASS Relay\KeyType METHOD Random\IntervalBoundary::cases METHOD Random\Randomizer::getFloat PROPERTY Random\IntervalBoundary::$name

ondrejmirtes

Alright, this is my first actual review of your code. And please fix the build :)

One idea I have that you might or might not like is that changes shouldn't fail the build. They're not always wrong! Instead, I'd like to have the build stay green but have some output in the workflow through $GITHUB_STEP_SUMMARY like issue bot does.

So this might not be actually a job for PHPUnit test but for a simple script that does the job.

If you're worried that we might miss some changes this way, I have another feature planned that would address this - the issue bot and this workflow should comment on a PR when they have something to say. I don't expect you to implement here, it's just that I expect you to have some concerns about my proposal and this is my way to address them in the future :)

Anyway, I'm really excited about this and I'm looking forward to the additional safety and changes this will allows us to do!

.github/workflows/reflection-golden-test.yml

ondrejmirtes · 2023-11-06T09:48:30Z

tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php

+
+		$parts = explode('-----', $contents);
+
+		for ($i = 1; $i + 1 < count($parts); $i += 2) {


I'd prefer if this file was just JSON so that we don't need to do this parsing.

This format is certainly less robust (e.g. ----- could sneak in somewhere). But it is more readable and it is easily editable by hand (which could be useful if you want to "pin" the expected changes during local development). So given the fact that it's easy enough to generate and parse this custom format by hand, I'd say it's worth it to keep it.

ondrejmirtes · 2023-11-06T09:49:59Z

tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php

+				throw $e;
+			}
+
+			$result = "Generating symbol description failed:\n"


I don't think we need such fancy exception handling if something fails :)

Ok, less work for me. It will make it a bit more involved do debug these things, but it's not the main purpose of the test anyway.

These symbols fail on PHP 7.4 (7.3 also fails, though I haven't checked which symbols - presumably it's the same ones):

CLASS Random\IntervalBoundary CLASS Relay\KeyType METHOD Random\IntervalBoundary::cases METHOD Random\Randomizer::getFloat PROPERTY Random\IntervalBoundary::$name

It seems like a better-reflection bug (or maybe it's a feature). There are probably going to be bugs like that from time to time, so IMO it's better not to break the whole test because of it. There are two distinct error messages:

----- CLASS Random\IntervalBoundary ----- Generating symbol description failed: PHPStan\BetterReflection\Reflector\Exception\IdentifierNotFound: PHPStan\BetterReflection\Reflection\ReflectionClass "UnitEnum" could not be found in the located source in /app/vendor/ondrejmirtes/better-reflection/src/Reflector/Exception/IdentifierNotFound.php:31 Stack trace: #0 /app/vendor/ondrejmirtes/better-reflection/src/Reflector/DefaultReflector.php(40): PHPStan\BetterReflection\Reflector\Exception\IdentifierNotFound::fromIdentifier(Object(PHPStan\BetterReflection\Identifier\Identifier)) #1 /app/vendor/ondrejmirtes/better-reflection/src/Reflection/ReflectionClass.php(1291): PHPStan\BetterReflection\Reflector\DefaultReflector->reflectClass('UnitEnum') #2 /app/vendor/ondrejmirtes/better-reflection/src/Reflection/ReflectionClass.php(1654): PHPStan\BetterReflection\Reflection\ReflectionClass->addEnumInterfaces(Array) #3 /app/vendor/ondrejmirtes/better-reflection/src/Reflection/ReflectionClass.php(1477): PHPStan\BetterReflection\Reflection\ReflectionClass->getCurrentClassImplementedInterfacesIndexedByName() #4 /app/vendor/ondrejmirtes/better-reflection/src/Reflection/Adapter/ReflectionClass.php(392): PHPStan\BetterReflection\Reflection\ReflectionClass->getInterfaces() #5 /app/src/Reflection/ClassReflection.php(879): PHPStan\BetterReflection\Reflection\Adapter\ReflectionClass->getInterfaces() #6 /app/tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php(245): PHPStan\Reflection\ClassReflection->getImmediateInterfaces() #7 /app/tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php(79): PHPStan\Reflection\ReflectionProviderGoldenTest::generateClassDescription('Random\\Interval...') #8 /app/tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php(112): PHPStan\Reflection\ReflectionProviderGoldenTest::generateSymbolDescription('CLASS Random\\In...') #9 /app/tests/generate-reflection-test.php(10): PHPStan\Reflection\ReflectionProviderGoldenTest::dumpOutput() #10 {main} ----- METHOD Random\Randomizer::getFloat ----- Generating symbol description failed: LogicException: This enum case does not have a value in /app/vendor/ondrejmirtes/better-reflection/src/Reflection/ReflectionEnumCase.php:111 Stack trace: #0 /app/vendor/ondrejmirtes/better-reflection/src/Reflection/Adapter/ReflectionClassConstant.php(100): PHPStan\BetterReflection\Reflection\ReflectionEnumCase->getValueExpression() #1 /app/src/Reflection/InitializerExprTypeResolver.php(1891): PHPStan\BetterReflection\Reflection\Adapter\ReflectionClassConstant->getValueExpression() #2 /app/src/Reflection/InitializerExprTypeResolver.php(1946): PHPStan\Reflection\InitializerExprTypeResolver->getClassConstFetchTypeByReflection(Object(PhpParser\Node\Name\FullyQualified), 'ClosedOpen', Object(PHPStan\Reflection\ClassReflection), Object(Closure)) #3 /app/src/Reflection/InitializerExprTypeResolver.php(174): PHPStan\Reflection\InitializerExprTypeResolver->getClassConstFetchType(Object(PhpParser\Node\Name\FullyQualified), 'ClosedOpen', 'Random\\Randomiz...', Object(Closure)) #4 /app/src/Reflection/Php/PhpParameterReflection.php(114): PHPStan\Reflection\InitializerExprTypeResolver->getType(Object(PhpParser\Node\Expr\ClassConstFetch), Object(PHPStan\Reflection\InitializerExprContext)) #5 /app/tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php(374): PHPStan\Reflection\Php\PhpParameterReflection->getDefaultValue() #6 /app/tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php(307): PHPStan\Reflection\ReflectionProviderGoldenTest::generateVariantsDescription('getFloat', Array) #7 /app/tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php(81): PHPStan\Reflection\ReflectionProviderGoldenTest::generateClassMethodDescription('Random\\Randomiz...') #8 /app/tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php(112): PHPStan\Reflection\ReflectionProviderGoldenTest::generateSymbolDescription('METHOD Random\\R...') #9 /app/tests/generate-reflection-test.php(10): PHPStan\Reflection\ReflectionProviderGoldenTest::dumpOutput() #10 {main}

It's because it's unrealistic for these classes to exist on these versions. PHPStan already has that information, you might filter that out when generating the symbol list.

PhpStormStubsSourceStubber has isPresentClass method that respects the $phpVersion passed into the constructor.

If you want to utilize such PHPStan/BetterReflection internals in the generator script, you can do that by creating new ContainerFactory and then getting the container by calling ->create.

It's because it's unrealistic for these classes to exist on these versions.

Yeah, I expected as much. But I'd prefer to keep the symbols "from the future" (or past) in there (even if they generate an exception as is the case here). That will prevent us from accidentally introducing these symbols into older PHP versions. E.g. I currently have a dump for PHP 7.4 generated and this is in there:

FUNCTION str_starts_with ----- Has side-effects: Maybe NOT BUILTIN Variants: 1 /** * @param string|null $haystack * @param string|null $needle * @return bool */ function str_starts_with(string|null $haystack, string|null $needle): bool

I haven't checked how it got there (maybe a polyfill - since it says NOT BUILTIN?), but ideally it shouldn't be there.

ondrejmirtes · 2023-11-06T09:53:15Z

tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php

+
+			$result .= $variantIdent . "/**\n";
+			$result .= implode('', $paramsPhpDoc);
+			$result .= $variantIdent . ' * @return ' . $variant->getReturnType()->describe($verbosityLevel) . "\n";


For printing PHPDocs please use Type::toPhpDocNode() and put it into Printer from phpdoc-parser: https://phpstan.github.io/phpdoc-parser/PHPStan.PhpDocParser.Printer.Printer.html

Otherwise you're gonna generate invalid PHPDocs.

Otherwise you're gonna generate invalid PHPDocs.

I don't think that's relevant. The "PHP" code is not valid either.

I want to have as much detail as possible in a human readable form. I tried using the Printer for return type, but some detail is lost. Here are some examples:

603) PHPStan\Reflection\ReflectionProviderGoldenTest::test with data set "METHOD WeakReference::get" ('METHOD WeakReference::get', 'Has side-effects: Maybe\nVisi...t|null') Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ Visibility: public Variants: 1 /** - * @return T of object (class WeakReference, parameter)|null + * @return T|null */ function get(): object|null' 598) PHPStan\Reflection\ReflectionProviderGoldenTest::test with data set "METHOD Vtiful\Kernel\Format::underline" ('METHOD Vtiful\Kernel\Format::underline', 'Is internal: Yes\nHas side-ef...Format') Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ Variants: 1 /** * @param int $style - * @return static(Vtiful\Kernel\Format) + * @return static */ function underline(int $style): Vtiful\Kernel\Format' 584) PHPStan\Reflection\ReflectionProviderGoldenTest::test with data set "METHOD SplTempFileObject::getType" ('METHOD SplTempFileObject::getType', 'Has side-effects: Maybe\nVisi... mixed') Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ Visibility: public Variants: 1 /** - * @return (string|false) + * @return string|false */ function getType(): mixed' 582) PHPStan\Reflection\ReflectionProviderGoldenTest::test with data set "METHOD SplTempFileObject::getRealPath" ('METHOD SplTempFileObject::getRealPath', 'Has side-effects: Maybe\nVisi... mixed') Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ Visibility: public Variants: 1 /** - * @return (string|false) + * @return string|false */ function getRealPath(): mixed'

Alright, understood.

schlndh · 2023-11-06T17:52:34Z

@ondrejmirtes Thanks for the review. I kept running into issues and I finally ran out of time over the weekend, so I wasn't able to get it to get it fully ready before you looked at it, and I also didn't manage to write a summary of the changes. But you probably noticed that the phpSymbols.php are gone. Instead I now scrape the symbols dynamically from the sources you suggested. As a result there are now many more symbols tested - roughly ~24k up from ~9k.

Regarding the "changes shouldn't fail the build.": If you don't want to complicate it with the bot comments right from the start then IMO it's best to have it fail for now. It's certainly annoying that there will be some "failures" due to fixes, but IDK how often you change stuff that could trigger a change in reflection data? So perhaps it would be enough for now to add an explanation somewhere that the test indicates a change, not necessarily a wrong change?

Also note, that the failure is "non-persistent" in the main branches - i.e. next commit will "fix" it (unless it introduces a change of its own).

As for the bot comments: that seems like a good idea. If nothing else, it gives us a place to explain what's going on properly. I assume that the bot could comment on commits made directly to the main branches as well.

As for PHPUnit: Couldn't the bot just link to the phpunit output in the workflow results (IDK if the github action can get a link)? I like (ab)using phpunit for this, because it works well for local development. E.g. I can run/debug a single symbol in phpstorm.

BTW: Here is a result of an dummy PR made on top of this branch (unfortunately, I chose a bit too big of a change) https://github.com/schlndh/phpstan-src/actions/runs/6774232790/job/18410952267

ondrejmirtes · 2023-11-08T22:39:37Z

Yeah, totally fine, I agree with everything 👍 So please make the build pass and we can continue.

schlndh · 2023-11-09T06:38:19Z

I'd consider the build to be passing already. There are 7 failures with the build: 6 of them are the new reflection golden test, which is expected because the test doesn't exist in the base commit. It should disappear once the base commit contains the test.

The remaining failure is PHPStan (7.4, windows-latest) which runs out of memory - since I'm not even touching src/ this PR can hardly cause higher memory requirements. So it's probably just the few extra files being checked that push it over the edge.

ondrejmirtes · 2023-11-09T07:20:38Z

Alright, let's try it out and see what happens. Please try to make the planned changes to reflection you wanted :)

ondrejmirtes · 2023-11-15T12:34:53Z

I'm sorry, I can't stand the PR build failures (and on the branches too after merging), and contributors are also confused about it.

I'm gonna make the workflow always green. When you change something on purpose you can still check the test results.

Thanks for understanding.

schlndh force-pushed the feature-reflectionGoldenTest branch from 53b9ab9 to ebcaa2a Compare October 20, 2023 10:10

schlndh marked this pull request as ready for review October 20, 2023 11:09

ondrejmirtes requested changes Oct 29, 2023

View reviewed changes

schlndh added 13 commits November 3, 2023 11:36

WIP: add golden test for reflection

7a17162

keep dump for reflection golden test out of repository

75762d2

downgrade match to switch for PHP < 8

4c50737

Revert "downgrade match to switch for PHP < 8"

c65c55f

This reverts commit ebcaa2ade4a773e9c3f39db388531650f349ed88.

use switch to prevent phpstan from complaining about downgraded source

2dac312

WIP: add github action for reflection golden test

766128f

fix concurrency group

4b4569f

generate php symbols dynamically from function maps

9fb6f05

scrape symbols also from php-8-stubs

95d6868

scrape symbols also from phpstorm stubs

9a5f1d4

scrape symbols also from reflection

4483a58

make sure that empty classes are checked as well - e.g. stdClass

299b634

include few exotic extensions to discover more symbols

b3791cf

schlndh force-pushed the feature-reflectionGoldenTest branch from 83724d4 to b3791cf Compare November 3, 2023 14:29

schlndh added 4 commits November 3, 2023 15:41

fix reflection golden test

339d822

fix symbol dump download

a9105ff

catch reflection exceptions

ebc5ea3

These symbols fail to reflect on PHP 7.3/7.4 CLASS Random\IntervalBoundary CLASS Relay\KeyType METHOD Random\IntervalBoundary::cases METHOD Random\Randomizer::getFloat PROPERTY Random\IntervalBoundary::$name

filter exception stack trace to keep it same in dump and test

9c4f33e

schlndh force-pushed the feature-reflectionGoldenTest branch from f341cdd to 9c4f33e Compare November 4, 2023 09:27

ondrejmirtes requested changes Nov 6, 2023

View reviewed changes

schlndh added 2 commits November 6, 2023 17:58

don't put stack trace to reflection golden test

0065094

remove useless PHP reinstall

69f7811

ondrejmirtes merged commit 62107a9 into phpstan:1.10.x Nov 9, 2023

schlndh deleted the feature-reflectionGoldenTest branch November 15, 2023 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add golden test for reflection #2679

Add golden test for reflection #2679

schlndh commented Oct 13, 2023

ondrejmirtes commented Oct 13, 2023

schlndh commented Oct 13, 2023

phpstan-bot commented Oct 20, 2023

schlndh commented Oct 20, 2023

ondrejmirtes left a comment

schlndh commented Oct 29, 2023

ondrejmirtes commented Oct 29, 2023

schlndh commented Oct 29, 2023 •

edited

Loading

ondrejmirtes commented Oct 30, 2023 •

edited

Loading

ondrejmirtes left a comment

ondrejmirtes Nov 6, 2023

schlndh Nov 6, 2023

ondrejmirtes Nov 6, 2023

schlndh Nov 6, 2023

ondrejmirtes Nov 6, 2023

schlndh Nov 6, 2023

ondrejmirtes Nov 6, 2023

schlndh Nov 6, 2023

ondrejmirtes Nov 6, 2023

schlndh commented Nov 6, 2023

ondrejmirtes commented Nov 8, 2023

schlndh commented Nov 9, 2023

ondrejmirtes commented Nov 9, 2023

ondrejmirtes commented Nov 15, 2023


		$parts = explode('-----', $contents);

		for ($i = 1; $i + 1 < count($parts); $i += 2) {

Add golden test for reflection #2679

Add golden test for reflection #2679

Conversation

schlndh commented Oct 13, 2023

ondrejmirtes commented Oct 13, 2023

schlndh commented Oct 13, 2023

phpstan-bot commented Oct 20, 2023

schlndh commented Oct 20, 2023

ondrejmirtes left a comment

Choose a reason for hiding this comment

schlndh commented Oct 29, 2023

ondrejmirtes commented Oct 29, 2023

schlndh commented Oct 29, 2023 • edited Loading

ondrejmirtes commented Oct 30, 2023 • edited Loading

ondrejmirtes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlndh commented Nov 6, 2023

ondrejmirtes commented Nov 8, 2023

schlndh commented Nov 9, 2023

ondrejmirtes commented Nov 9, 2023

ondrejmirtes commented Nov 15, 2023

schlndh commented Oct 29, 2023 •

edited

Loading

ondrejmirtes commented Oct 30, 2023 •

edited

Loading