Generic.Formatting.DisallowMultipleStatements false positive for FOR loop with no body

[gmponos]

Hi,

When I run phpcs against this file https://github.com/doctrine/coding-standard/pull/208/files#diff-9023c633b2425ded52916bf44f88ede1R17

The error I get are:

  7 | ERROR | [x] Useless semicolon. (SlevomatCodingStandard.PHP.UselessSemicolon.UselessSemicolon)
 10 | ERROR | [x] Useless semicolon. (SlevomatCodingStandard.PHP.UselessSemicolon.UselessSemicolon)
 10 | ERROR | [x] Each PHP statement must be on a line by itself (Generic.Formatting.DisallowMultipleStatements.SameLine)
 15 | ERROR | [x] Useless semicolon. (SlevomatCodingStandard.PHP.UselessSemicolon.UselessSemicolon)
 17 | ERROR | [x] Whitespace found before first semicolon of FOR loop
    |       |     (Squiz.ControlStructures.ForLoopDeclaration.SpacingBeforeFirst)
 17 | ERROR | [x] Space found before semicolon; expected "0;" but found "0 ;" (Squiz.WhiteSpace.SemicolonSpacing.Incorrect)
 17 | ERROR | [x] Each PHP statement must be on a line by itself (Generic.Formatting.DisallowMultipleStatements.SameLine)
 26 | ERROR | [x] Each PHP statement must be on a line by itself (Generic.Formatting.DisallowMultipleStatements.SameLine)
 26 | ERROR | [x] Useless semicolon. (SlevomatCodingStandard.PHP.UselessSemicolon.UselessSemicolon)

I care mainly for 17 | ERROR | [x] Each PHP statement must be on a line by itself (Generic.Formatting.DisallowMultipleStatements.SameLine)

The autofix is this:

https://github.com/doctrine/coding-standard/pull/208/files#diff-df7e22f20fb934baed4dd64e4ff1cd75R17

which seems wrong.

[gsherwood]

The sample code to test this is:

for ($i = 0 ; $i < 10; $i++);
{
}

The sniff auto-fixes this to be:

for ($i = 0 ; $i < 10;
$i++);
{
}

[gsherwood]

The reason this happens is because the semicolon at the end of the FOR declaration means that the bracket statement below has nothing to do with the FOR statement at all. So PHPCS is not finding brackets and is not finding a valid FOR declaration as part of tokenizing. Technically, I think this is just an empty FOR declaration.

So the sniff sees 2 statements on a line - $i = 0 and $i < 10 and wants them on a line by themselves, hence the newline during fixing. If this was a standard FOR declaration, the sniff would have ignored it.

The fix here would have to be in the tokenizer itself, to allow FOR declarations to be valid even when they have no body. Then somehow let the sniffs know that this is a valid construct.

I'm not sure if it is worth the effort trying to get this right for FOR declarations, and everything else developers could write. In this specific case, it's good that the sniff generates an error so it can at least be manually reviewed.

A workaround might be to have the sniff ignore multiple statements if they are contained within parenthesis, but I haven't thought of the full implications of a change like that.

[jrfnl]

is not finding a valid FOR declaration as part of tokenizing. Technically, I think this is just an empty FOR declaration.

Technically, it is a perfectly valid for statement with the semi-colon in place.

https://3v4l.org/9OJLd

[gsherwood]

Technically, it is a perfectly valid for statement with the semi-colon in place.

I didn't mean to imply that it isn't. I just mean't it isn't tokenizing it as a FOR declaration.

[jrfnl]

I just mean't it isn't tokenizing it as a FOR declaration.

Sorry, this is confusing me. What do you mean ?
AFAICS it tokenizes as T_FOR with correct parenthesis_owner, parenthesis_opener and parenthesis_closer indexes set, but (again correctly) without the scope_opener and scope_closer set as it is a control structure without a body.

[gsherwood]

Sorry, this is confusing me. What do you mean ?

I meant that it's not part of the scope map.

I've gotten some more time to look at the code now and I can see that the sniff is trying to ignore FOR statements but is confused in this specific case because it never expects to see one end with a semicolon. It looks to me like it would need to perform the same check on the previous semicolon it found to confirm that it too is not part of a FOR statement.

[gmponos]

Do you have room for a sniff that detects for-loop without a body and maybe return a warning (as it is valid). Users can raise this to error.

Asking because this happened as a bug for me. Had a semicolon missed and I could not understand why the for-loop is not entering the body.

[gsherwood]

Do you have room for a sniff that detects for-loop without a body and maybe return a warning (as it is valid). Users can raise this to error.

Yeah, I would include that due to its simplicity. It would likely just look for T_FOR without a scope opener, indicating that the FOR has no body and may be an error.

[gsherwood]

I've committed a fix for this issue, which will be released in 3.5.6. Thanks for reporting it.