-
-
Notifications
You must be signed in to change notification settings - Fork 739
[Php70] add a rector to pass only variables as arguments by reference #2546
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,3 +8,4 @@ services: | |
| exclude: | ||
| - '../src/Rector/**/*Rector.php' | ||
| - '../src/Exception/*' | ||
| - '../src/ValueObject/*' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,177 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Rector\Php70\Rector\FuncCall; | ||
|
|
||
| use function lcfirst; | ||
| use PhpParser\Node; | ||
| use PhpParser\Node\Expr; | ||
| use PhpParser\Node\Expr\ArrayDimFetch; | ||
| use PhpParser\Node\Expr\Assign; | ||
| use PhpParser\Node\Expr\AssignOp; | ||
| use PhpParser\Node\Expr\AssignRef; | ||
| use PhpParser\Node\Expr\FuncCall; | ||
| use PhpParser\Node\Expr\MethodCall; | ||
| use PhpParser\Node\Expr\New_; | ||
| use PhpParser\Node\Expr\PropertyFetch; | ||
| use PhpParser\Node\Expr\StaticCall; | ||
| use PhpParser\Node\Expr\StaticPropertyFetch; | ||
| use PhpParser\Node\Expr\Variable; | ||
| use PhpParser\Node\Name; | ||
| use PhpParser\Node\Stmt\Return_; | ||
| use PHPStan\Analyser\MutatingScope; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Reflection\ParameterReflection; | ||
| use Rector\CodingStyle\Naming\ClassNaming; | ||
| use Rector\NodeTypeResolver\Node\AttributeKey; | ||
| use Rector\Php70\Reflection\CallReflection; | ||
| use Rector\Php70\ValueObject\VariableAssignPair; | ||
| use Rector\Rector\AbstractRector; | ||
| use Rector\RectorDefinition\CodeSample; | ||
| use Rector\RectorDefinition\RectorDefinition; | ||
|
|
||
| /** | ||
| * @see https://www.php.net/manual/en/migration70.incompatible.php | ||
| * | ||
| * @see \Rector\Php70\Tests\Rector\FuncCall\NonVariableToVariableOnFunctionCallRector\NonVariableToVariableOnFunctionCallRectorTest | ||
| */ | ||
| final class NonVariableToVariableOnFunctionCallRector extends AbstractRector | ||
| { | ||
| /** | ||
| * @var string | ||
| */ | ||
| private const DEFAULT_VARIABLE_NAME = 'tmp'; | ||
|
|
||
| /** | ||
| * @var CallReflection | ||
| */ | ||
| private $callReflection; | ||
|
|
||
| /** | ||
| * @var ClassNaming | ||
| */ | ||
| private $classNaming; | ||
|
|
||
| public function __construct(CallReflection $callReflection, ClassNaming $classNaming) | ||
| { | ||
| $this->callReflection = $callReflection; | ||
| $this->classNaming = $classNaming; | ||
| } | ||
|
|
||
| public function getDefinition(): RectorDefinition | ||
| { | ||
| return new RectorDefinition( | ||
| 'Transform non variable like arguments to variable where a function or method expects an argument passed by reference', | ||
| [new CodeSample('reset(a());', '$a = a(); reset($a);')] | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @return string[] | ||
| */ | ||
| public function getNodeTypes(): array | ||
| { | ||
| return [FuncCall::class, MethodCall::class, StaticCall::class]; | ||
| } | ||
|
|
||
| /** | ||
| * @param FuncCall|MethodCall|StaticCall $node | ||
| */ | ||
| public function refactor(Node $node): ?Node | ||
| { | ||
| $scope = $node->getAttribute(AttributeKey::SCOPE); | ||
| if (! $scope instanceof MutatingScope) { | ||
| return null; | ||
| } | ||
|
|
||
| $arguments = $this->getNonVariableArguments($node, $scope); | ||
|
|
||
| if ($arguments === []) { | ||
| return null; | ||
| } | ||
|
|
||
| foreach ($arguments as $key => $argument) { | ||
| $replacements = $this->getReplacementsFor($argument, $scope); | ||
|
|
||
| $current = $node->getAttribute(AttributeKey::CURRENT_STATEMENT); | ||
| $this->addNodeBeforeNode($replacements->assign(), $current instanceof Return_ ? $current : $node); | ||
| $node->args[$key]->value = $replacements->variable(); | ||
|
|
||
| $scope = $scope->assignExpression($replacements->variable(), $scope->getType($replacements->variable())); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I never used this before. What is it for? When should we use it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It returns a new Scope from the previous Scope + the assigned expression (for example, if you pass a variable to it, the new scope will be aware of this variable). I did this so that Scope is aware of newly created variables, so that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, thanks! Maybe it would be more consistent to add type here: /** @var MutatingScope $scope */
$scope = $scope->assignExpression($replacements->variable(), $scope->getType($replacements->variable()));
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant it to have /** @var MutatingScope $scope */
$whatever = **$scope**->assignExpression(...); |
||
| } | ||
|
|
||
| $node->setAttribute(AttributeKey::SCOPE, $scope); | ||
|
|
||
| return $node; | ||
| } | ||
|
|
||
| /** | ||
| * @param FuncCall|MethodCall|StaticCall $node | ||
| * | ||
| * @return array<int, Expr> | ||
| */ | ||
| private function getNonVariableArguments(Node $node, Scope $scope): array | ||
| { | ||
| $arguments = []; | ||
|
|
||
| /** @var ParameterReflection $parameter */ | ||
| foreach ($this->callReflection->getParameterReflections($node, $scope) as $key => $parameter) { | ||
| // omitted optional parameter | ||
| if (! isset($node->args[$key])) { | ||
| continue; | ||
| } | ||
|
|
||
| if ($parameter->passedByReference()->no()) { | ||
| continue; | ||
| } | ||
|
|
||
| $argument = $node->args[$key]->value; | ||
|
|
||
| if ($this->isVariableLikeNode($argument)) { | ||
| continue; | ||
| } | ||
|
|
||
| $arguments[$key] = $argument; | ||
| } | ||
|
|
||
| return $arguments; | ||
| } | ||
|
|
||
| private function getReplacementsFor(Expr $expr, Scope $scope): VariableAssignPair | ||
| { | ||
| if ( | ||
| ( | ||
| $expr instanceof Assign | ||
| || $expr instanceof AssignRef | ||
| || $expr instanceof AssignOp | ||
| ) | ||
| && $this->isVariableLikeNode($expr->var) | ||
| ) { | ||
| return new VariableAssignPair($expr->var, $expr); | ||
| } | ||
|
|
||
| $variable = new Variable($this->getVariableNameFor($expr, $scope)); | ||
|
|
||
| return new VariableAssignPair($variable, new Assign($variable, $expr)); | ||
| } | ||
|
|
||
| private function isVariableLikeNode(Node $node): bool | ||
| { | ||
| return $node instanceof Variable | ||
| || $node instanceof ArrayDimFetch | ||
| || $node instanceof PropertyFetch | ||
| || $node instanceof StaticPropertyFetch; | ||
| } | ||
|
|
||
| private function getVariableNameFor(Expr $expr, Scope $scope): string | ||
| { | ||
| if ($expr instanceof New_ && $expr->class instanceof Name) { | ||
| $name = $this->classNaming->getShortName($expr->class); | ||
| } else { | ||
| $name = $this->getName($expr); | ||
| } | ||
|
|
||
| return lcfirst($this->createCountedValueName($name ?? self::DEFAULT_VARIABLE_NAME, $scope)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Rector\Php70\Reflection; | ||
|
|
||
| use PhpParser\Node; | ||
| use PhpParser\Node\Arg; | ||
| use PhpParser\Node\Expr; | ||
| use PhpParser\Node\Expr\FuncCall; | ||
| use PhpParser\Node\Expr\MethodCall; | ||
| use PhpParser\Node\Expr\StaticCall; | ||
| use PhpParser\Node\Name; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Broker\Broker; | ||
| use PHPStan\Broker\FunctionNotFoundException; | ||
| use PHPStan\Reflection\ParameterReflection; | ||
| use PHPStan\Reflection\ParametersAcceptor; | ||
| use PHPStan\Reflection\ParametersAcceptorSelector; | ||
| use Rector\NodeTypeResolver\NodeTypeResolver; | ||
| use Rector\PhpParser\Node\Resolver\NameResolver; | ||
|
|
||
| final class CallReflection | ||
| { | ||
| /** | ||
| * @var Broker | ||
| */ | ||
| private $broker; | ||
|
|
||
| /** | ||
| * @var NodeTypeResolver | ||
| */ | ||
| private $nodeTypeResolver; | ||
|
|
||
| /** | ||
| * @var NameResolver | ||
| */ | ||
| private $nameResolver; | ||
|
|
||
| public function __construct(Broker $broker, NodeTypeResolver $nodeTypeResolver, NameResolver $nameResolver) | ||
| { | ||
| $this->broker = $broker; | ||
| $this->nodeTypeResolver = $nodeTypeResolver; | ||
| $this->nameResolver = $nameResolver; | ||
| } | ||
|
|
||
| /** | ||
| * @param FuncCall|MethodCall|StaticCall $node | ||
| * | ||
| * @return array<int, ParameterReflection> | ||
| */ | ||
| public function getParameterReflections(Node $node, Scope $scope): array | ||
| { | ||
| if ($node instanceof MethodCall || $node instanceof StaticCall) { | ||
| $classType = $this->nodeTypeResolver->getObjectType( | ||
| $node instanceof MethodCall ? $node->var : $node->class | ||
| ); | ||
| $methodName = $this->nameResolver->getName($node->name); | ||
|
|
||
| if ($methodName === null || ! $classType->hasMethod($methodName)->yes()) { | ||
| return []; | ||
| } | ||
|
|
||
| return ParametersAcceptorSelector::selectSingle( | ||
| $classType->getMethod($methodName, $scope)->getVariants() | ||
| )->getParameters(); | ||
| } | ||
|
|
||
| if ($node->name instanceof Expr) { | ||
| return $this->getParametersForExpr($node->name, $scope); | ||
| } | ||
|
|
||
| return $this->getParametersForName($node->name, $node->args, $scope); | ||
| } | ||
|
|
||
| /** | ||
| * @return array<int, ParameterReflection> | ||
| */ | ||
| private function getParametersForExpr(Expr $expr, Scope $scope): array | ||
| { | ||
| $type = $scope->getType($expr); | ||
|
|
||
| if (! $type instanceof ParametersAcceptor) { | ||
| return []; | ||
| } | ||
|
|
||
| return $type->getParameters(); | ||
| } | ||
|
|
||
| /** | ||
| * @param Arg[] $args | ||
| * | ||
| * @return array<int, ParameterReflection> | ||
| */ | ||
| private function getParametersForName(Name $name, array $args, Scope $scope): array | ||
| { | ||
| try { | ||
| return ParametersAcceptorSelector::selectFromArgs( | ||
| $scope, | ||
| $args, | ||
| $this->broker->getFunction($name, $scope)->getVariants() | ||
| )->getParameters(); | ||
| } catch (FunctionNotFoundException $functionNotFoundException) { | ||
| return []; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scopeis good enough as interfaceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scopedoesn't contains the methodassignExpressionsadly. See (upcoming) explanation below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. I'm affraid that this will go in mess like "which one to use when", since
instanceof Scopeis used everywhere now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will see if I can do something with the
ScopeFactoryinstead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But a simple rule could be if you need to mutate scope, go with
MutatingScope, otherwise useScope.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might, but classes with more public method than interfaces they implement always create crappy code. It's PHPStan design flaw we cannot cover here I'm affraid.
Let's keep it your way for now, not that important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore,
MutatingScopeseems to be the only available implementation ofScope.