Transform `void` return to `null` after call #2778

herndlm · 2023-11-26T13:02:06Z

Closes phpstan/phpstan#6720

What I meant with question marks yesterday

I'm not a 100% sure about this yet, we can see that it turns off many reports and e.g. UsageOfVoidMatchExpressionRule seems to be not doing anything anymore I guess
On the other hand it is how PHP behaves
I noticed that Generator void returns are not handled

Also thought of: only transform void in union/intersection types, but that does feel inconsistent..

WDYT?

tests/PHPStan/Analyser/data/reflection-type.php

tests/PHPStan/Rules/Methods/CallMethodsRuleTest.php

tests/PHPStan/Analyser/data/reflection-type.php

ondrejmirtes · 2023-11-26T14:28:52Z

I think the problem with existing rules might not be that hard to fix.

Most of the failures are from the error that's supposed to be reported here:

phpstan-src/src/Rules/FunctionCallParametersCheck.php

Lines 233 to 241 in 3c7860c

    
           if ( 
        
           	!$funcCall instanceof Node\Expr\New_ 
        
           	&& !$scope->isInFirstLevelStatement() 
        
           	&& $scope->getType($funcCall)->isVoid()->yes() 
        
           ) { 
        
           	$errors[] = RuleErrorBuilder::message($messages[7]) 
        
           		->identifier(sprintf('%s.void', $nodeType)) 
        
           		->line($funcCall->getLine()) 
        
           		->build();

We could for example clone the node and add an attribute like 'keepVoid' => true that will not transform void into null while resolving the type in MutatingScope.

herndlm · 2023-11-27T08:17:38Z

I think the problem with existing rules might not be that hard to fix.

Most of the failures are from the error that's supposed to be reported here:

phpstan-src/src/Rules/FunctionCallParametersCheck.php

Lines 233 to 241 in 3c7860c

if (

!$funcCall instanceof Node\Expr\New_

&& !$scope->isInFirstLevelStatement()

&& $scope->getType($funcCall)->isVoid()->yes()

) {

$errors[] = RuleErrorBuilder::message($messages[7])

->identifier(sprintf('%s.void', $nodeType))

->line($funcCall->getLine())

->build();

We could for example clone the node and add an attribute like 'keepVoid' => true that will not transform void into null while resolving the type in MutatingScope.

I had a similar thought. Basically, we'd need to either a) keep the information somewhere that this was a void before, either e.g. in the type or the node as you said or b) modify the void type to behave more like null, since it's a special return-only type it always means that it must have came from a return. I guess the "remember in node" approach is the cleanest and one that makes most sense :) I also quickly checked psalm yesterday and looks like it's transforming void to null too basically.

herndlm · 2023-11-28T16:52:29Z

hmm, this keeps being tricky, I tried to clone the node in rules and add an attribute as you suggested. problem: there were cases where the type was already resolved and it would get it from the internal cache via key again. this could also be circumvented I guess? it's a bit unfortunate hmm.
I'd like to have a wasVoid() method on the type that tells me this, but feels like way too much overhead just for this.. :)

ondrejmirtes · 2023-11-28T17:38:04Z

@herndlm Change the cache key based on the attribute in MutatingScope::getNodeKey().

staabm · 2023-11-29T16:41:44Z

src/Analyser/MutatingScope.php

@@ -674,6 +674,10 @@ private function getNodeKey(Expr $node): string
 			$key .= '/*' . $node->getAttribute('startFilePos') . '*/';
 		}

+		if ($node->hasAttribute('keepVoid')) {


would be great if we could put this string into a class-constant somewhere.

that way it would ease navigation with the IDE and make dependent parts obvious

yeah, I wanted to check first if all is fine and then ask about how to properly refactor this thing ☺️ I hate that it is sprinkled x times around the code base now..

Could be a new method on Scope, just don't start it with getType, I'd hate to confuse autocompletion with this niche use case

still need to do this refactor, missed the comment. currently working on it

looks like I'm prone to commenting race conditions today.. :) did something, not sure about the name of course. this is now in Scope after all and should be stable. WDYT?

herndlm · 2023-11-29T17:03:03Z

got it working, this is looking not bad, did only quickly go through the failures, the ones that worry me

Method call return value that should be used, but is not from simplify/simplify, no clue what that is yet.
the Parameter #2 .. of class .. expects Closure(): void, Closure(): null given. of PMMP, could be a sign that more adaptions are needed hmm

ondrejmirtes · 2023-11-29T17:37:15Z

Closure(): void, Closure(): null given.

Stop early in the callback:

if ($type instanceof CallableType || $type instanceof ClosureType) {
    return $type;
}

About simplify - don't worry about it, they'll have to fix their code.

staabm · 2023-11-29T19:06:49Z

symplify/symplify is abandoned. parts of it moved to symplify/phpstan-rules

ondrejmirtes · 2023-11-29T19:41:50Z

I've update the tests in phpstan/phpstan to symplify/phpstan-rules, you can try again :)

herndlm · 2023-11-30T11:37:11Z

sorry, still trying to recreate one of the failures of PMMP to properly test the fix..
currently e.g.

class Bar
{

	/** @phpstan-param \Closure(): void $handler */
	public function doFoo(\Closure $handler): void
	{
		\PHPStan\dumpType($handler()); // void
		\PHPStan\dumpType($this->handler()); // null
	}

	public function doBar(): void
	{
		$handler = $this->handler(...);
		\PHPStan\dumpType($handler); // Closure(): void
		$this->doFoo($handler);
	}

	private function handler(): void
	{
	}

}

is working just fine in an AnalyserIntegrationTest which I don't understand. I tried to replicate the CI failure I saw via https://github.com/pmmp/PocketMine-MP/blob/0984aa670d5c04e4714853dbf738f51bfa4f39e1/src/network/mcpe/handler/SessionStartPacketHandler.php#L39 and https://github.com/pmmp/PocketMine-MP/blob/0984aa670d5c04e4714853dbf738f51bfa4f39e1/src/network/mcpe/NetworkSession.php#L201

but I can of course just try to adapt the traverse callback 😅

ondrejmirtes · 2023-11-30T13:04:42Z

This passes on 1.10.x but fails on your PR: https://phpstan.org/r/f8d2decc-2d58-4ce4-a2b3-d811700abe9f

ondrejmirtes · 2023-11-30T13:05:54Z

I'd say we need to do the same keepVoid thing for inferring the return type of arrow function in MutatingScope around here:

phpstan-src/src/Analyser/MutatingScope.php

Lines 1230 to 1233 in 7e7cc9c

    
           $returnType = $arrowScope->getType($node->expr); 
        
           if ($node->returnType !== null) { 
        
           	$returnType = TypehintHelper::decideType($this->getFunctionType($node->returnType, false, false), $returnType); 
        
           }

herndlm · 2023-11-30T13:34:52Z

bingo! thx again :)

what do you think about the previous code example I pasted, it looks like those closure returns are not transformed to null yet. I guess that could be fixed?

ondrejmirtes · 2023-11-30T13:36:16Z

it looks like those closure returns are not transformed to null

That's okay and actually correct behaviour. We don't want to transform void to null in a return type there.

Your TypeTransformer callback only transforms standalone void + void in union and intersection 👍

herndlm · 2023-11-30T14:17:54Z

sorry for the many force pushes, should be good now. do you not think we should refactor it somehow? e.g. put the 'keepVoid' in some constant, or maybe even have some static helper methods to add / preserve it or so.

phpstan-bot · 2023-11-30T15:01:02Z

This pull request has been marked as ready for review.

ondrejmirtes · 2023-11-30T15:04:30Z

Please see #2778 (comment)

src/Analyser/MutatingScope.php

…oach

staabm · 2023-12-11T13:30:05Z

@herndlm this PR here will render the PR at #2494 obsolete, right?

ondrejmirtes · 2023-12-11T14:57:41Z

Thank you!

ondrejmirtes · 2023-12-12T15:37:52Z

@herndlm Can you please look at this bug? https://github.com/WordPress/WordPress-Coding-Standards/pull/2416/files

We should allow returning null from void method when it's typed in a PHPDoc. Here's the code: https://github.com/WordPress/WordPress-Coding-Standards/blob/develop/WordPress/Sniffs/WhiteSpace/ObjectOperatorSpacingSniff.php

herndlm · 2023-12-12T17:15:42Z

I dislike those WP void unions.. but sure. https://phpstan.org/r/12a90bd8-64db-4620-855c-23579d0085f6 seems to be a simple reproducer too.

staabm · 2024-01-14T18:01:41Z

src/Analyser/MutatingScope.php

@@ -1236,7 +1242,7 @@ private function resolveType(string $exprString, Expr $node): Type
 						new VoidType(),
 					]);
 				} else {
-					$returnType = $arrowScope->getType($node->expr);
+					$returnType = $arrowScope->getKeepVoidType($node->expr);


does it make sense to handle a "void" return type of a arrow function? can arrow functions have a void return type?

Try to change it and see what fails. I think it's valid, at least for a throw expr or calling never function.

just found #2778 (comment) which answers this question, I guess

hmm locally I did not get a test failure after changing it. lets see #2873

herndlm commented Nov 26, 2023

View reviewed changes

tests/PHPStan/Analyser/data/reflection-type.php Outdated Show resolved Hide resolved

herndlm commented Nov 26, 2023

View reviewed changes

tests/PHPStan/Analyser/data/reflection-type.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Nov 26, 2023

View reviewed changes

tests/PHPStan/Rules/Methods/CallMethodsRuleTest.php Outdated Show resolved Hide resolved

herndlm commented Nov 26, 2023

View reviewed changes

tests/PHPStan/Analyser/data/reflection-type.php Outdated Show resolved Hide resolved

herndlm force-pushed the transform-void-return-to-null branch from b51abd9 to 7dbe5a8 Compare November 29, 2023 16:39

staabm reviewed Nov 29, 2023

View reviewed changes

herndlm mentioned this pull request Nov 30, 2023

void vs. null szepeviktor/phpstan-wordpress#176

Closed

herndlm force-pushed the transform-void-return-to-null branch 4 times, most recently from 42d4d9f to edf2ead Compare November 30, 2023 14:17

herndlm marked this pull request as ready for review November 30, 2023 15:00

herndlm requested a review from ondrejmirtes December 9, 2023 20:58

ondrejmirtes requested changes Dec 10, 2023

View reviewed changes

src/Analyser/MutatingScope.php Show resolved Hide resolved

src/Analyser/MutatingScope.php Show resolved Hide resolved

herndlm commented Dec 10, 2023

View reviewed changes

src/Analyser/MutatingScope.php Outdated Show resolved Hide resolved

herndlm and others added 7 commits December 10, 2023 16:32

Transform void return to null after call

6d286c1

Revert test change that looked wrong from the beginning

8cae3b7

Properly revert the unwanted test change..

d75e324

Keep other rules working via 'keepVoid' => true node attribute appr…

2a24487

…oach

Fix for Closure returns

100c78c

Refactor keepVoid solution to not leak out of Scope

14b2f71

Only add Mutatingscope cache key if keepVoid is true

14871cd

herndlm force-pushed the transform-void-return-to-null branch from 23bfd2e to 14871cd Compare December 10, 2023 15:33

herndlm requested a review from ondrejmirtes December 10, 2023 15:39

ondrejmirtes mentioned this pull request Dec 11, 2023

feat: treat void as null for null coalescing operator #2494

Closed

ondrejmirtes merged commit 1b4b134 into phpstan:1.10.x Dec 11, 2023
420 of 424 checks passed

ondrejmirtes mentioned this pull request Dec 11, 2023

Return type void is adopted by variables phpstan/phpstan#6720

Closed

herndlm deleted the transform-void-return-to-null branch December 11, 2023 15:44

ondrejmirtes mentioned this pull request Dec 11, 2023

ForbidUnusedMatchResultRule needs fixing after PHPStan change shipmonk-rnd/phpstan-rules#186

Closed

herndlm mentioned this pull request Dec 12, 2023

Allow returning null from a PHPDoc void union phpstan/phpstan#10291

Closed

gehrisandro mentioned this pull request Dec 18, 2023

Incorrect behavior of TernaryFalseExpressionToIfRector rectorphp/rector#8367

Closed

staabm reviewed Jan 14, 2024

View reviewed changes

staabm mentioned this pull request Jan 14, 2024

Simplify arrow-function return type inference #2873

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transform `void` return to `null` after call #2778

Transform `void` return to `null` after call #2778

herndlm commented Nov 26, 2023 •

edited

ondrejmirtes commented Nov 26, 2023

herndlm commented Nov 27, 2023

herndlm commented Nov 28, 2023 •

edited

ondrejmirtes commented Nov 28, 2023

staabm Nov 29, 2023

herndlm Nov 29, 2023 •

edited

ondrejmirtes Nov 30, 2023

herndlm Nov 30, 2023

herndlm Nov 30, 2023

herndlm commented Nov 29, 2023 •

edited

ondrejmirtes commented Nov 29, 2023

staabm commented Nov 29, 2023

ondrejmirtes commented Nov 29, 2023

herndlm commented Nov 30, 2023 •

edited

ondrejmirtes commented Nov 30, 2023

ondrejmirtes commented Nov 30, 2023

herndlm commented Nov 30, 2023

ondrejmirtes commented Nov 30, 2023

herndlm commented Nov 30, 2023

phpstan-bot commented Nov 30, 2023

ondrejmirtes commented Nov 30, 2023

staabm commented Dec 11, 2023

ondrejmirtes commented Dec 11, 2023

ondrejmirtes commented Dec 12, 2023

herndlm commented Dec 12, 2023

staabm Jan 14, 2024

ondrejmirtes Jan 14, 2024

staabm Jan 14, 2024

staabm Jan 14, 2024

Transform void return to null after call #2778

Transform void return to null after call #2778

Conversation

herndlm commented Nov 26, 2023 • edited

ondrejmirtes commented Nov 26, 2023

herndlm commented Nov 27, 2023

herndlm commented Nov 28, 2023 • edited

ondrejmirtes commented Nov 28, 2023

Choose a reason for hiding this comment

herndlm Nov 29, 2023 • edited

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

herndlm commented Nov 29, 2023 • edited

ondrejmirtes commented Nov 29, 2023

staabm commented Nov 29, 2023

ondrejmirtes commented Nov 29, 2023

herndlm commented Nov 30, 2023 • edited

ondrejmirtes commented Nov 30, 2023

ondrejmirtes commented Nov 30, 2023

herndlm commented Nov 30, 2023

ondrejmirtes commented Nov 30, 2023

herndlm commented Nov 30, 2023

phpstan-bot commented Nov 30, 2023

ondrejmirtes commented Nov 30, 2023

staabm commented Dec 11, 2023

ondrejmirtes commented Dec 11, 2023

ondrejmirtes commented Dec 12, 2023

herndlm commented Dec 12, 2023

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

Transform `void` return to `null` after call #2778

Transform `void` return to `null` after call #2778

herndlm commented Nov 26, 2023 •

edited

herndlm commented Nov 28, 2023 •

edited

herndlm Nov 29, 2023 •

edited

herndlm commented Nov 29, 2023 •

edited

herndlm commented Nov 30, 2023 •

edited