Skip to content

Add @param-out support #1804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Oct 21, 2022
Merged

Add @param-out support #1804

merged 20 commits into from
Oct 21, 2022

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Oct 8, 2022

requires phpstan/phpdoc-parser#150
refs phpstan/phpstan#6426 (comment)

closes phpstan/phpstan#7231
closes phpstan/phpstan#6871
closes phpstan/phpstan#6186
closes phpstan/phpstan#4372

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

just put togehter the basic building blocks and tests:

  •  wire ResolvedPhpDocBlock
  •  wire ResolvedDocNodeResolver
  •  validate param-out phpdoc tags and tests
  • initial NodeScopeResolverTests
  • 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
Copy link
Member

ondrejmirtes commented Oct 8, 2022

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

@staabm
Copy link
Contributor Author

staabm commented Oct 9, 2022

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
    if (!isset($this->inProcess[$fileName][$nameScopeKey])) { // wrong $fileName due to traits
    return ResolvedPhpDocBlock::createEmpty();
    }
  • 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
Copy link
Contributor Author

staabm commented Oct 9, 2022

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 staabm marked this pull request as ready for review October 9, 2022 09:30
@clxmstaab clxmstaab force-pushed the param-out branch 4 times, most recently from ba182dd to 53c3b12 Compare October 12, 2022 07:35
@staabm
Copy link
Contributor Author

staabm commented Oct 12, 2022

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

if (!$methodReflection instanceof ExtendedMethodReflection) {
throw new ShouldNotHappenException();
}

@clxmstaab clxmstaab force-pushed the param-out branch 2 times, most recently from e274a59 to 09342ee Compare October 12, 2022 07:44
@staabm staabm force-pushed the param-out branch 3 times, most recently from 2c3136e to 2df5ad2 Compare October 12, 2022 19:31
@ondrejmirtes
Copy link
Member

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

@staabm staabm force-pushed the param-out branch 2 times, most recently from f43fc38 to a7347f5 Compare October 12, 2022 19:42
@ondrejmirtes
Copy link
Member

And now you get relevant build errors finally 😊

@staabm staabm force-pushed the param-out branch 2 times, most recently from 5823ae9 to 3cdc61b Compare October 12, 2022 20:13
@staabm
Copy link
Contributor Author

staabm commented Oct 12, 2022

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

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please test a scenario where @param-out on a method is inherited from a parent:

  1. When the parameter names match.
  2. When the parameters are renamed (but it should still be applied based on parameter order).
  3. When the @param-out is overriden for the same parameter in the child class.

$docComment = $reflection->getDocComment();

if ($docComment !== null) {
$resolvedPhpDoc = $this->fileTypeMapper->getResolvedPhpDoc(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FunctionReflection|ExtendedMethodReflection should already have a method for @param-out tags. But to be honest, it should probably be an information for a specific parameter on ParameterReflectionWithPhpDocs to be really clean. Something like public function getOutType(): ?Type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't definitely parse PHPDoc here again. That's a job for PhpClassReflectionExtension, BetterReflectionProvider::getCustomFunction() and NativeFunctionReflectionProvider.

test.php Outdated

addValue($foo["a"]);

\PHPStan\Testing\assertType('int', $foo["a"]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file shouldn't be commited.

@staabm
Copy link
Contributor Author

staabm commented Oct 21, 2022

fixed and rebased. thank you

@ondrejmirtes ondrejmirtes changed the title Add @param-out support Add @param-out support Oct 21, 2022
@ondrejmirtes ondrejmirtes merged commit 73bb9eb into phpstan:1.9.x Oct 21, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@staabm
Copy link
Contributor Author

staabm commented Nov 7, 2022

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

php/php-src#9897

@ondrejmirtes
Copy link
Member

@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
Copy link
Contributor Author

staabm commented Nov 7, 2022

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

here we go: php/php-src#9900

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants