Skip to content
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

Implement call_user_func() DynamicFunctionReturnTypeExtension #2479

Merged
merged 17 commits into from
Jun 29, 2023

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jun 22, 2023

submitting the non-controversial parts of #756 as a separate PR

refs phpstan/phpstan#5942

closes phpstan/phpstan#7057

return null;
}

$calledOnType = $scope->getType($args[0]->value);
Copy link
Member

Choose a reason for hiding this comment

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

Still wrong. See: https://3v4l.org/F67Rj

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a test, which shows it works If I got your point.

at this point of execution named args are already re-ordered to positional ones

Copy link
Member

Choose a reason for hiding this comment

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

Your logic isn't correct because I don't see the handling of call_user_func that would be necessary for PHPStan to understand it.

I added a rule to prove my point. You can see the rule doesn't see the signature correctly and so can't the dynamic return type extension.

The problem is that MutatingScope (before it calls your extension) already calls ArgumentsNormalizer which mangles the arguments so they're wrong. Because it doesn't know what to do with them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I have an idea now what you are talking about.

Do you expect this PR to still contain this new rule when finished, or do you think I should/can adjust the core in a way, that the rewritten FuncCall will be finally checked by the pre-existing CallToFunctionParametersRule ?

Copy link
Member

Choose a reason for hiding this comment

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

I've got an idea:

  1. It should not be an extension, just a piece of code in MutatingScope before extensions are called. Before the normalizer is used.
  2. That way we can find the first/callback arg and remove it from args. And create a new FuncCall with the rest.
  3. We can call $this->getType on the FuncCall and return that.
  4. The new rule should just do the same thing as in 2) and pass the new FuncCall to FunctionCallParametersCheck.

Copy link
Contributor Author

@staabm staabm Jun 24, 2023

Choose a reason for hiding this comment

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

thanks.

I had to re-calibrate my expectations for this PR, as it turned from a return-type improving into a more overall goal :).
it seems to work as you described now.

@staabm staabm marked this pull request as ready for review June 23, 2023 08:33
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

call_user_func(i: 'foo', callback: $f);
call_user_func($f, i: 1);
call_user_func(i: 1, callback: $f);
call_user_func($f, j: 1);
Copy link
Contributor Author

@staabm staabm Jun 24, 2023

Choose a reason for hiding this comment

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

this line cannot be re-ordered by ArgumentsNormalizer::reorderCallUserFuncArguments because the param names cannot be matched with the parameter acceptor.

therefore the "plain" FuncCall is validated instead and thats the reason why the error for this line is reported by CallToFunctionParametersRule instead of CallUserFuncRule

@staabm
Copy link
Contributor Author

staabm commented Jun 24, 2023

one jobs CI errors we see is

Error: Parameter #1 $persisted of callable passed to call_user_func() expects array<E of Nextras\Orm\Entity\IEntity>, array<Nextras\Orm\Entity\IEntity> given.
Error: Parameter #2 $removed of callable passed to call_user_func() expects array<E of Nextras\Orm\Entity\IEntity>, array<Nextras\Orm\Entity\IEntity> given.
 ------ ---------------------------------------------------------------------- 
  Line   src/Repository/Repository.php                                         
 ------ ---------------------------------------------------------------------- 
  621    Parameter #1 $persisted of callable passed to call_user_func()        
         expects array<E of Nextras\Orm\Entity\IEntity>,                       
         array<Nextras\Orm\Entity\IEntity> given.                              
         💡 Type Nextras\Orm\Entity\IEntity is not always the same as E. It    
            breaks the contract for some argument types, typically subtypes.   
  621    Parameter #2 $removed of callable passed to call_user_func() expects  
         array<E of Nextras\Orm\Entity\IEntity>,                               
         array<Nextras\Orm\Entity\IEntity> given.                              
         💡 Type Nextras\Orm\Entity\IEntity is not always the same as E. It    
            breaks the contract for some argument types, typically subtypes.   
 ------ ---------------------------------------------------------------------- 

I cannot judge whether this is something expected or not.

@staabm staabm marked this pull request as ready for review June 24, 2023 08:38
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes
Copy link
Member

Pushed some changes, feel free to check them out. Now please contribute the same thing for call_user_func_array :) Thank you.

@ondrejmirtes ondrejmirtes merged commit 9b2ff16 into phpstan:1.10.x Jun 29, 2023
116 checks passed
@staabm
Copy link
Contributor Author

staabm commented Jun 30, 2023

Thank you

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