empty_loop_body must honor comments

[mvorisek]

Bug report

empty_loop_body must honor comments and keep curly brackets around the body. This is critical as commented code (which can be uncommented later) inside loop body has significantly other meaning tham code placed below the loop.

Code snippet that reproduces the problem

    1) data/src/Reference/HasOne.php (empty_loop_body, braces, no_singleline_whitespace_before_semicolons)
-        foreach ($fieldPropsRefl as $fieldPropRefl) {
-            // TODO before merge - uncomment!
+        foreach ($fieldPropsRefl as $fieldPropRefl);
+        // TODO before merge - uncomment!
 //            unset($this->{$fieldPropRefl->getName()});
-        }
     }

[SpacePossum]

I see nothing critical here, the comment is not removed. I would reconfigure the rule to add the braces so these won't be removed, or remove the rule all together if you want mixed styles.

[mvorisek]

@SpacePossum let's question this rule from the other side - why is it any good that the loop body is placed after the loop?

[julienfalque]

I tend to agree comments should be considered part of the loop and prevent the braces from being removed, e.g. to make things explicit:

while (foo()) {
    // no-op
}

[SpacePossum]

So configure it with braces? I don't understand why you want ; one time and { the other time.

For example, from the other side:

while (foo()) ;
    // no-op

should this be fixed because we assume this comment is a body?
so to ?

while (foo()) {
    // no-op
}

who about this than?

while (foo()) ;
    // $a is always > 1 here
    ++$a;

adding all these exceptions and details are just to harmful for the code base IMHO. As such I won't add this, review it and will vote 👎 on it, if there is a majority for the change and a reviewer amongst the members than we can reopen here

[mvorisek]

The typical usecase is:

while (foo()) {
    // log(++$a);
}

whis is currently converted to:

while (foo());
// log(++$a);

which completely changes the meaning of the code/comment. I think this rule should apply strictly only when there is nothing inside the loop body, even if only a comment

[julienfalque]

@SpacePossum Those are valid reasons :) To be honest, I don't feel like implementing it myself as well, empty loops that contain actual comments (i.e. not commented out code) are very rare, though I'd be ok if one would contribute it.

[mvorisek]

If we can all agree that the reasons are valid, I might contribute the fix, should be easy :)

[mvorisek]

please reaopen this issue

[SpacePossum]

why? if you want multi line body for comments config { }, if not configure ; , if you don't care don't use the rule

[mvorisek]

Because this rule "basically scriples" meaning of a code. If something is inside loop body, it is meant to be evaluated per iteration, not after the loop/once.

[SpacePossum]

so configure the rule to use { } and it will be inside the loop

[mvorisek]

I use default config. @julienfalque was also thinks this should be fixed. @SpacePossum, why you defend moving comments outside the body after the loop, is there any ratonale?

[SpacePossum]

I didn't set the default and I certainly never ever told you to you use, so if you use it than why ask me? I cannot reason why you are you use this configuration

[mvorisek]

Thanks. The rule itself and even both configs are ok. The only issue I have is that it should be considered non-empty when there is a comment inside. This is the only change I ask for, therefore, I kindly ask you to reopen this issue so someone can fix that.

[SpacePossum]

I'm not interested in that exception in the core of this project, so I won't reopen this. The tool accepts 3rd party rules so would advice that, such rule can have configuration and defaults of its own.