Add @param-out support

[staabm]

trafficstars

requires https://github.com/phpstan/phpdoc-parser/pull/150 refs https://github.com/phpstan/phpstan/issues/6426#issuecomment-1073791565

see https://psalm.dev/docs/annotating_code/supported_annotations/#param-out-psalm-param-out

just put togehter the basic building blocks and tests:

• [x] wire ResolvedPhpDocBlock
• [x] wire ResolvedDocNodeResolver
• [x] validate param-out phpdoc tags and tests
• [x] initial NodeScopeResolverTests
• [x] implement param-out type specification

I had the idea of adding param-out-type specification of the expressions right before the return-type of FuntionLikes is handled - I have not yet an idea where this place is though and whether thats a good idea :)

would be great to get a hint, where/how the actual param-out logic should be implemented.

[ondrejmirtes]

In NodeScopeResolver there are places that are interested in PassedByReference - @param-out should be handled there.

Also - require your version of phpdoc-parser with the support added in composer.json, so that the pipeline can actually get green.

Also, you have to guard any call to new method on PhpDocNode with method_exists so that Rector can do its downgrading job. See: https://github.com/phpstan/phpstan-src/pull/1799#issuecomment-1269682926

[staabm]

thanks for the tips.

I tried testing the PR with the following code

<?php

use function PHPStan\Testing\assertType;

class X {
	/**
	 * @param-out string $s
	 */
	function addFoo(?string &$s): void
	{
		if ($s === null) {
			$s = "hello";
		}
		$s .= "foo";
	}
}

function foo1(?string $s) {
	assertType('string|null', $s);
	$x = new X();
	$x->addFoo($s);
	assertType('string', $s);
}

but I currently struggle with php-doc resolving. 2 problems arise:

• ~~in NodeScopeResolver->getParameterOutTypes(..) the phpdoc /** @param-out string $s */ gets always resolved to an empty-block from these lines~~ https://github.com/phpstan/phpstan-src/blob/49cd1319bf95d65a488fd51c543d93eeac049f8b/src/Type/FileTypeMapper.php#L111-L113
• ~~FunctionReflection does not yet contain a getDocComment method (like MethodReflection has)... it seems this is a missing part I need to implement with this PR?~~

[staabm]

I should write comments about my problems more often... sorry/thanks for rubberducking.. found the reason for doc-block resolving problem because I had mixed up caller and callee side of things - 48a2cf8.

I think the PR works now as expected for basic method examples. open todo is for regular functions. I would implement a getDocComment method on FunctionReflection now, to get it working... if we agree on this

[staabm]

~~for some reason the PR as is failling after rebase, because I am running into~~ -> fixed by re-adding a recently added use-statement which got lost after manual merge conflict resolving

https://github.com/phpstan/phpstan-src/blob/f097ca90572d73f88ddbed6eb296eebfc5e72e7f/src/Analyser/NodeScopeResolver.php#L531-L533

[ondrejmirtes]

You updated a lot irrelevant deps. You need to run just composer update phpstan/phpdoc-parser instead.

[ondrejmirtes]

And now you get relevant build errors finally 😊

[staabm]

I had a look into the psalm test-suite for some more advanced use-cases... I will push them as a followup after we got the basic support landed

[ondrejmirtes]

Two more ideas:

• Support for @param-out in stubs. Should be tested in a similar fashion to this: https://github.com/phpstan/phpstan-src/blob/1.9.x/tests/PHPStan/Analyser/ConditionalReturnTypeFromMethodStubTest.php
• Support for conditional tags in @param-out.

[ondrejmirtes]

The stubs support needs to be implemented and fixed in multiple places. Essentially look at all the usages of StubPhpDocProvider::findMethodPhpDoc() and findFunctionPhpDoc():

• Functions coming from PHP
• Userland functions
• Methods coming from PHP
• Userland methods

[ondrejmirtes]

BTW this should also be taken advantage of in internal PHP function stubs (preg_match maybe?), feel free to look at Psalm stubs for these 😊

[ondrejmirtes]

Also https://github.com/phpstan/phpstan-src/pull/1674

[staabm]

I think it mostly works for basic cases. major case not working atm is phpdocs which should get inherited.

[ondrejmirtes]

Please do this too: https://github.com/phpstan/phpstan-src/pull/1804#issuecomment-1279928326 This part can give us valuable feedback about this feature working :)

[staabm]

alright: I included all psalm stubs which contain a param-out and picked a few tests out of the psalm codebase for those.

now we need to work out if the expectations are right and the remaining bugs :)

[staabm]

some errors I see regarding functions happen, because phpstan decides to use a function-variant from e.g. php8-stubs or function-map - which don't contain information about param-out.

should/can param-out information be merged from phpdoc stubs into the variants of other signature sources?

[staabm]

after sleeping a night over it, I came to the conclusion that the problems we see stems from the fact that I have implemented getOutType(): ?Type on ParameterReflectionWithPhpDocs and therefore parameter-signatures taken from php8-stubs or the resources/functionMap do not deliver this information.

I see 2 possible solutions: A) implement getOutType(): ?Type on ParameterReflection B) implement getParameterOutTypes(): Type[] on FunctionReflection|ExtendedMethodReflection

for both solutions I would drop getOutType(): ?Type from ParameterReflectionWithPhpDocs.. wdyt?

[ondrejmirtes]

1. We can't, it'd be a BC break
2. That's not as nice

NativeMethodReflection already has ParametersAcceptorWithPhpDocs[].

NativeFunctionReflection has only ParametersAcceptor, but that can/should be adjusted to ParametersAcceptorWithPhpDocs. Please do that.

[staabm]

ok - I think we arrived at a point, where all newly added param-out tests pass.

next step: work thru regressions

[staabm]

the remaining 5 errors look a bit suprising:

There were 5 failures:

1) PHPStan\Analyser\LegacyNodeScopeResolverTest::testAssignInIf with data set #11 (PHPStan\Analyser\MutatingScope Object (...), 'matches', PHPStan\TrinaryLogic Object (...), 'mixed')
Type of variable $matches does not match the expected one.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'mixed'
+'(0 is 256 ? array<array{string, int<-1, max>}> : (0 is 512 ? array<string|null> : (0 is 768 ? array<array{null, -1}|array{string, int<0, max>}> : array<string>)))'

C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser\LegacyNodeScopeResolverTest.php:923
C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser\LegacyNodeScopeResolverTest.php:753

2) PHPStan\Analyser\LegacyNodeScopeResolverTest::testAssignInIf with data set #18 (PHPStan\Analyser\MutatingScope Object (...), 'matches2', PHPStan\TrinaryLogic Object (...), 'mixed')
Type of variable $matches2 does not match the expected one.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'mixed'
+'(0 is 256 ? array<array{string, int<-1, max>}> : (0 is 512 ? array<string|null> : (0 is 768 ? array<array{null, -1}|array{string, int<0, max>}> : array<string>)))'

C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser\LegacyNodeScopeResolverTest.php:923
C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser\LegacyNodeScopeResolverTest.php:753

3) PHPStan\Analyser\LegacyNodeScopeResolverTest::testAssignInIf with data set #20 (PHPStan\Analyser\MutatingScope Object (...), 'matches3', PHPStan\TrinaryLogic Object (...), 'mixed')
Type of variable $matches3 does not match the expected one.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'mixed'
+'(0 is 256 ? array<array{string, int<-1, max>}> : (0 is 512 ? array<string|null> : (0 is 768 ? array<array{null, -1}|array{string, int<0, max>}> : array<string>)))'

C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser\LegacyNodeScopeResolverTest.php:923
C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser\LegacyNodeScopeResolverTest.php:753

4) PHPStan\Analyser\LegacyNodeScopeResolverTest::testAssignInIf with data set #21 (PHPStan\Analyser\MutatingScope Object (...), 'matches4', PHPStan\TrinaryLogic Object (...), 'mixed')
Type of variable $matches4 does not match the expected one.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'mixed'
+'(0 is 256 ? array<array{string, int<-1, max>}> : (0 is 512 ? array<string|null> : (0 is 768 ? array<array{null, -1}|array{string, int<0, max>}> : array<string>)))'

C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser\LegacyNodeScopeResolverTest.php:923
C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Analyser\LegacyNodeScopeResolverTest.php:753

5) PHPStan\Analyser\LegacyNodeScopeResolverTest::testAssignInIf with data set #30 (PHPStan\Analyser\MutatingScope Object (...), 'ternaryMatches', PHPStan\TrinaryLogic Object (...), 'mixed')
Type of variable $ternaryMatches does not match the expected one.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'mixed'
+'(0 is 256 ? array<array{string, int<-1, max>}> : (0 is 512 ? array<string|null> : (0 is 768 ? array<array{null, -1}|array{string, int<0, max>}> : array<string>)))'

to me it looks more or less like an unresolved conditional return type. maybe thats kind of the way how the LegacyNodeScopeResolverTest works, and I just need to update the expected values?

[phpstan-bot]

This pull request has been marked as ready for review.

[staabm]

to me it looks more or less like an unresolved conditional return type. maybe thats kind of the way how the LegacyNodeScopeResolverTest works, and I just need to update the expected values?

verified this theses by putting the code in question into a regular param-out test. so, I just adapt the test expectations with 503a1d2

[ondrejmirtes]

I fixed the problem with LegacyNodeScopeResolverTest, you can rebase and adjust those asserts again. https://github.com/phpstan/phpstan-src/commit/a83fdeb3f2eca8f081ea2bdd449434ebf2a9586a

[staabm]

you can rebase and adjust those asserts again

thank you. done

[staabm]

fixed and rebased. thank you

[ondrejmirtes]

Thank you!

[staabm]

btw: I opened a new issue on php-src, to add/discuss @param-out directly into php-src-stubs

https://github.com/php/php-src/issues/9897

[ondrejmirtes]

@staabm Could you describe and open another issue for flag constants added to the stubs too? Like all the valid constants for $flags on json_encode (https://www.php.net/manual/en/function.json-encode.php) could be there.

[staabm]

open another issue for flag constants added to the stubs too?

here we go: https://github.com/php/php-src/issues/9900