[Php70] add a rector to pass only variables as arguments by reference by Lctrs · Pull Request #2546 · rectorphp/rector

Lctrs · 2020-01-03T17:00:35Z

Still WIP.

Creating it to gather first reviews.

TomasVotruba · 2020-01-04T12:46:44Z

Hi, is this ready for review or do you plan more work first?

Lctrs · 2020-01-04T13:31:00Z

I'm planning to handle a few more cases and simplify code a little bit, but main structure/flow is here.

TomasVotruba

Great job with covergin with test 👍
I've left few mostly readability comments.

Have you use bin/rector create command?

Lctrs · 2020-01-04T19:26:32Z

Yes I have used the create command.

Lctrs · 2020-01-06T16:15:29Z

@TomasVotruba Ready for review. Build failure is unrelated I guess.

Lctrs · 2020-01-06T16:45:35Z

@TomasVotruba Finally still WIP. Tried it on a real project. Got errors.

TomasVotruba · 2020-01-06T17:12:24Z

No troubles. Just ping me when you need me

TomasVotruba · 2020-01-06T17:12:40Z

CI should pass completely now. Just fixed the master

Lctrs · 2020-01-06T17:14:41Z

@TomasVotruba Any idea what leaveNode() may only return an array if the parent structure is an array means ?

TomasVotruba · 2020-01-06T17:57:52Z

That happens when you get node, but return an array of nodes.

Lctrs · 2020-01-06T18:18:42Z

How/Where can this happen ? I don't return an array in my rector.

TomasVotruba · 2020-01-06T19:26:29Z

There can be like dozens of cases where this can happen.

I'd have to see the test case.

Lctrs · 2020-01-07T13:33:08Z

@TomasVotruba It was caused by a return statement. When I add something like return reset(bar()); for example, the node was inserted between the return and the reset, hence the error. I handled this case on line 100-101 in my rector, but maybe this can be generalized ?

Otherwise, it's finally ready for review. It worked well in one of my project. I tried to be as extensive as possible on test.

Lctrs · 2020-01-07T13:33:31Z

Only the PHAR compilation fails ATM. Will take a look at it.

Lctrs · 2020-01-07T14:13:18Z

@TomasVotruba All good 🎉

TomasVotruba

Thank you for a great job!

I've added few notes. Mostly about decopuling Reflection from Rector

TomasVotruba · 2020-01-07T14:42:21Z

+        }
+    }
+
+    private function processArgument(Expr $expr, Scope $scope): VariableAssignPair


This looks like VariableAssignPairFactory.

process/refactor method should change nodes in some way

TomasVotruba · 2020-01-07T14:43:11Z

+    private function createCountedValueName(?string $valueName, Scope $scope): string
+    {
+        $countedValueName = $valueName ?? self::DEFAULT_VARIABLE_NAME;
+
+        // make sure variable name is unique
+        if (! $scope->hasVariableType($countedValueName)->yes()) {
+            return $countedValueName;
+        }
+
+        // we need to add number suffix until the variable is unique
+        $i = 2;
+        $countedValueNamePart = $countedValueName;
+        while ($scope->hasVariableType($countedValueName)->yes()) {
+            $countedValueName = $countedValueNamePart . $i;
+            ++$i;
+        }
+
+        return $countedValueName;
+    }
+}


This would be great if abstracted in AbstractRector, as this logic is used in multiple places now

@TomasVotruba How would you handle things like suffix ? (ForRepeatedCountToOwnVariableRector uses a suffix)

What do you mean? Suffix of what?

I'd just move this to protected method to abstract class, so it can be re-used in multiple Rector rules.

TomasVotruba · 2020-01-07T14:46:24Z

+
+?>
+-----
+<?php
+
+namespace Rector\Php70\Tests\Rector\FuncCall\NonVariableToVariableOnFunctionCallRector\Fixture;
+
+function propertyFetch()
+{
+    reset((new \stdClass())->dummy);
+}
+
+?>


This can be removed, as nothing changes

TomasVotruba · 2020-01-07T14:48:28Z

+    public function refactor(Node $node): ?Node
+    {
+        $scope = $node->getAttribute(AttributeKey::SCOPE);
+        if (! $scope instanceof MutatingScope) {


Scope is good enough as interface

Scope doesn't contains the method assignExpression sadly. See (upcoming) explanation below.

Ah, I see. I'm affraid that this will go in mess like "which one to use when", since instanceof Scope is used everywhere now.

Will see if I can do something with the ScopeFactory instead.

But a simple rule could be if you need to mutate scope, go with MutatingScope, otherwise use Scope.

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.

Furthermore, MutatingScope seems to be the only available implementation of Scope.

TomasVotruba · 2020-01-07T14:49:22Z

+            $this->addNodeBeforeNode($replacements->assign(), $current instanceof Return_ ? $current : $node);
+            $node->args[$key]->value = $replacements->variable();
+
+            $scope = $scope->assignExpression($replacements->variable(), $scope->getType($replacements->variable()));


I never used this before. What is it for? When should we use it?

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 createCountedValueName() returns a truly unique variable name.

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()));

assignExpression() always returns a MutatingScope. No need to add a phpDoc IMHO.

I meant it to have instanceof Scope above and here make the annotation for variable on the right:

/** @var MutatingScope $scope */ $whatever = **$scope**->assignExpression(...);

Lctrs · 2020-01-07T16:27:22Z

@TomasVotruba New version with most of your comments adressed. Only things missing is extracting createCountedValueName() to AbstractRector. I don't know yet how to handle properly things such as suffix, etc. If you have any idea...

Lctrs · 2020-01-08T10:07:10Z

@TomasVotruba Should be good.

Lctrs · 2020-01-09T19:06:36Z

@TomasVotruba FYI, I have a working version locally that uses the core reflection API (well, and also roave/better-reflection for one case). So no more Broker use. Just need to split it into more functions, Should be up tomorrow.

TomasVotruba · 2020-01-09T20:29:41Z

All rigth. Maybe look at PHPStan static reflection there is since 0.12.4

Ping me when ready for review

Lctrs · 2020-01-09T22:24:09Z

Should I understand that you prefer to use the Reflection API from PHPStan ?

TomasVotruba · 2020-01-09T22:27:39Z

Some of these commits: https://github.com/phpstan/phpstan-src/search?q=static+reflection&type=Commits

Lctrs · 2020-01-09T23:23:58Z

Yeah but if we use a reflection provider from PHPStan, we'll get objects implementing the reflection API from PHPStan, not from the core. Or maybe I'm missing something ?

TomasVotruba · 2020-01-09T23:35:28Z

It's just for inspiration, I don't personally use it

Lctrs · 2020-01-10T14:49:22Z

@TomasVotruba WDYT ? Yeah I know that roave/better-reflection is required as dev-master, but due to version conflict, I had no other choice.

TomasVotruba · 2020-01-10T14:52:17Z

Yeah I know that roave/better-reflection is required as dev-master, but due to version conflict, I had no other choice.

That would disable composer require rector/rector, not possible.

I think we should first finish the rule, then add more dependencies. This PR is already mixing many various problems. One step at a time.

Lctrs · 2020-01-10T14:56:53Z

Okay, I reverted my last commit for now, I think I covered all your remarks. Maybe I'm missing something ?

TomasVotruba · 2020-01-11T10:04:56Z

I'm looking at it

TomasVotruba · 2020-01-11T10:08:26Z

All looks good! It's like reading a well written story.

Thanks for this aweseome feature. I think you're the most skilled first contritbutor Rector ever had 🙇‍♂️

rectorphp/rector-src@dd4064e cleanup php-version bound tests, now under one version no need to separate (#2546)

TomasVotruba reviewed Jan 4, 2020

View reviewed changes

Lctrs marked this pull request as ready for review January 6, 2020 16:14

Lctrs changed the title ~~WIP: [Php70] add a rector to pass only variables as arguments by reference~~ [Php70] add a rector to pass only variables as arguments by reference Jan 6, 2020

TomasVotruba reviewed Jan 7, 2020

View reviewed changes

Comment thread ...0/tests/Rector/FuncCall/NonVariableToVariableOnFunctionCallRector/Fixture/func_calls.php.inc

[Php70] add a rector to pass only variables as arguments by reference

85e73f9

TomasVotruba self-requested a review January 11, 2020 10:07

TomasVotruba approved these changes Jan 11, 2020

View reviewed changes

TomasVotruba merged commit 2e83fe7 into rectorphp:master Jan 11, 2020

Lctrs deleted the argument_by_reference branch January 11, 2020 12:16

TomasVotruba added a commit that referenced this pull request Jun 21, 2022

Updated Rector to commit dd4064e

7547ec3

rectorphp/rector-src@dd4064e cleanup php-version bound tests, now under one version no need to separate (#2546)

Uh oh!

Conversation

Lctrs commented Jan 3, 2020

Uh oh!

TomasVotruba commented Jan 4, 2020

Uh oh!

Lctrs commented Jan 4, 2020

Uh oh!

TomasVotruba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lctrs commented Jan 4, 2020

Uh oh!

Lctrs commented Jan 6, 2020

Uh oh!

Lctrs commented Jan 6, 2020

Uh oh!

TomasVotruba commented Jan 6, 2020

Uh oh!

TomasVotruba commented Jan 6, 2020

Uh oh!

Lctrs commented Jan 6, 2020

Uh oh!

TomasVotruba commented Jan 6, 2020

Uh oh!

Lctrs commented Jan 6, 2020

Uh oh!

TomasVotruba commented Jan 6, 2020

Uh oh!

Lctrs commented Jan 7, 2020

Uh oh!

Lctrs commented Jan 7, 2020

Uh oh!

Lctrs commented Jan 7, 2020

Uh oh!

TomasVotruba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomasVotruba Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

TomasVotruba Jan 7, 2020 •

edited

Loading

TomasVotruba commented Jan 10, 2020 •

edited

Loading

TomasVotruba commented Jan 11, 2020 •

edited

Loading