djoos / Symfony-coding-standard

Development repository for the Symfony coding standard
MIT License
402 stars 102 forks source link

Pre-increment, pre-decrement is forced by the Symfony.ControlStructure.UnaryOperators.Invalid test #94

Closed redthor closed 5 years ago

redthor commented 7 years ago

If we write:

for ($x = 0; $x < $y; $x++) {}

the UnaryOperatorsSniff will fail with:

Place unary operators adjacent to the affected variable

To quieten the error we need to use pre-increment:

for ($x = 0; $x < $y; ++$x) {}

I'm unsure that this is what the standard is asking us? The post-increment is still 'adjacent' to the variable.

Note that the example code in #66 fails as it includes $a = $b++; at line 22.

Ref: #78

wickedOne commented 7 years ago

Place unary operators (!, --, ...) adjacent to the affected variable; https://symfony.com/doc/current/contributing/code/standards.html#structure

so it's my understanding that indeed is what the standard is asking of us.

what would be the argument against it? i.e. isn't it more consequent to enforce for ($x = 0; $x < $y; ++$x) {} if you enforce $a = ++$b?

as for the example code in #66: it's meant to "break all the rules" so line 22 actually is intended to trigger that specific error as stated on line 21

razbakov commented 7 years ago

@wickedOne

Wrong implementation

The rule is Place unary operators (!, --, ...) adjacent to the affected variable

Adjacent means near

It is just a huge misunderstanding of what is meant by Place unary operators (!, --, ...) adjacent to the affected variable;.

In Drupal it is formulated in other words, which makes more sense:

Unary operators (operators that operate on only one value), such as ++, should not have a space between the operator and the variable or number they are operating on. See reference.

See definition of adjacent:

The implementation doesn't work correctly.

Actual result:

// good
$a = !$b;
// good
$a = ! $b;
// good
$a = ++$b;
// good
$a = ++ $b;
// bad
$a = $b++;
// good
$a = $b ++;

Expected result:

// good
$a = !$b;
// bad
$a = ! $b;
// good
$a = ++$b;
// bad
$a = ++ $b;
// good
$a = $b++;
// bad
$a = $b ++;

Pre-increment, pre-decrement shouldn't be forced.

I agree, that you have to use pre-increment when it is possible because it is faster, but it shouldn't be forced. This is also how code optimisers work.

Different behaviour

In some cases you just can't force to use pre-increment, because pre-increment and post-increment have a different behaviour:

Pre-increment - Increments $a by one, then returns $a. Post-increment - Returns $a, then increments $a by one.

Confusion with changes in Symfony

I found one change in Symfony, where they changed all their code to pre-increments.

But Symfony didn't specify it as a standard.

And pre-increments are also not used everywhere, for example see file Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php

$nextIndex = is_int($index) ? $index++ : call_user_func($index, $choice, $key, $value);

Please see also a code example in Symfony documentation

for ($i = 0; $i < $times; $i++) {
    $greetings[] = 'Hi';
}
redthor commented 7 years ago

Can we get another look at this sometime?

razbakov commented 6 years ago

Might be solved by #125. I have to check all mentioned cases.

djoos commented 5 years ago

As far as I'm aware this is covered by #125. Closing off this issue, but happy to reopen if you still experience issues!