Skip to content

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.

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
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