bug: BinaryOperatorSpacesFixer - Fix align of operator with function declaration

[VincentLanglet]

Hi @SpacePossum, I found another bug with the BinaryOperatorSpacesFixer.
Seems like removing the ++$this->deepestLevel is solving the issue without any other test failing.

NB: The code

'<?php
                    $c                 = [$d]                 = $e[1];
                    function A(){}[$a] = $a[$c];
                    $b                 = 1;
                ',
                '<?php
                    $c = [$d] = $e[1];
                    function A(){}[$a] = $a[$c];
                    $b = 1;
                ',

is not a bug introduce by my PR.

Prior to my fix the $c = was ignored and not align with the following lines.
The fact the second = has a weird alignment is another bug which exist already and should be fixed in another PR.

'<?php
                    $c          = [$d]          = $e[1];
                    $bbbbb = 1;
                ',
                '<?php
                    $c = [$d] = $e[1];
                    $bbbbb = 1;
                ',

is a reproducer of the bug, which was already failing without my PR.

[coveralls]

Coverage decreased (-0.0004%) to 92.879% when pulling ecdf39d on VincentLanglet:bugfix/functionKind into aa976c5 on FriendsOfPHP:master.

[julienfalque]

Thank you @VincentLanglet.

[SpacePossum]

the placement of $e[1] is this really better than before?

[VincentLanglet]

No. But I tried to explain that this is not related.

$c = [$d] = $e[1];
function A(){}[$a] = $a[$c];
$b = 1;

Was fixed to

$c = [$d] = $e[1];
function A(){}[$a] = $a[$c];
$b                 = 1;

because the function definition was blocking the fix. But the bug was already here and you can reproduce it with

$c = [$d] = $e[1];
$aLongVariableName = 1;

which was already solved to

$c                 = [$d]                 = $e[1];
$aLongVariableName = 1;

BEFORE my bugfix.

I just showed an existing bug. I'll try to take time to fix it.

[SpacePossum]

Fair enough, I think we shouldn't merge utest that prove an issue though. There are a bunch of issues reported on this rule and PR's with tests to prove these issues. I think going slow on merging changes on this rule might be good, because of the number of open issues, that is all.

[SpacePossum]

hmm testing it again, your PR fixes your issue but the fix does introduce this side effect of:

-                    $c                 = [$d] = $e[1];
+                    $c = [$d] = $e[1];
                     function A(){}[$a] = $a[$c];
                     $b                 = 1;

We can talk about if it is good or bad, but the code that was added was designed to not align the = here. It seems to me you might not agree with the behavior of the rule, but that doesn't make it a bug, others added this test as expected/wanted behavior.

[VincentLanglet]

This was introduced by #2475
and the test is called testAlignArrayDestruction.

I clearly don't understand what the test was looking for. align is supposed to align all the following = of the same scope. And we're in the same scope here. So yes, they need to be aligned and the behavior is not supposed to be discussed here.

The only discussion is "what about the next = ?".
And I think it should be

$c                 = [$d] = $e[1];
function A(){}[$a] = $a[$c];
$b                 = 1;

and we should ignore the same = of the same line.

[SpacePossum]

I clearly don't understand what the test was looking for.

I think that is the issue here, a misunderstanding of what the code was doing.

the behavior is not supposed to be discussed here.

Sure, this PR is already merged, so the discussion is too late.

All I'm saying is, that looking at the code, the code changes seems minimal, yet the changes done by the rule by looking into a bit by testing and running on some repo's seem big. Combined with changing an existing utest that was put it place to prevent that, I just wonder if this was a good thing to do. The example you give here

$c                 = [$d] = $e[1];
function A(){}[$a] = $a[$c];
$b                 = 1;

if the input was:

$c = [$d] = $e[1];
function A(){}[$a] = $a[$c];
$b = 1;

than the fix is not correct with the current intend of the rule. So you might not want to discuss this intend or design or what not here, but my point is not about discussing it, it is about if this breaks the current behavior other people put in place (not me btw.)

End of the day, I don't use align CS and if people want to change this, so be it, but I'm not convinced by the utests this is a fix for everyone.

[VincentLanglet]

I proposed a fix for this situation #6450

I'm currently using the align on several projects and my current understanding is the following:
"If two identical binary operator are on the following lines and in the same scope, the need to be aligned".

I cannot believe that this behavior is correct

$c = [$d] = $e[1];
function A(){}[$a] = $a[$c];
$bbbb              = 1;

it should be treated the same way than

$c     = [$d] = $e[1];
$d[$a] = $a[$c];
$bbbb  = 1;

The main issue was the fact we ended with

$c                 = [$d]                 = $e[1];

and this is fixed by #6450