bug: Fix indentation of comment on switch case

[julienfalque]

Fixes #6470 (comment), #6572 and #7179.

[julienfalque]

This creates a conflict with PSR-12 rule about the // no break comment. Should the comment remain indented as part of the case body? Or should it be indented like the following case/default? Or should we just dismiss this PR?

[coveralls]

Coverage increased (+0.01%) to 92.888% when pulling 36e7bcf on julienfalque:fix-indentation-switch-case into 3fe97e6 on FriendsOfPHP:master.

[mvorisek]

This creates a conflict with PSR-12 rule about the // no break comment. Should the comment remain indented as part of the case body? Or should it be indented like the following case/default? Or should we just dismiss this PR?

There are 3 states of comment indentation:

a) "for case body"
b) "for next case"
c) invalid - not a) or b)

so the fixer should work like following (in order):

a) state is invalid if the next token (excluding other comments) is case or default and the previous code always terminate - of course, we cannot detect never return type from user functions, but we can detect break, return, exit, throw statements

c) should be always fixed to a)

when a) state is not valid, fix to b)

some usecases:

switch ($foo) {
    case 1:
        // no break - comment for case body
    case 2:
        if ($a > $b) {
            return '1 or 2';
        }
        // no break - comment for case body
    case 3:
    // comment for next case
    case 4:
        if ($a > $b) {
            return '1 or 2';
        }
    // comment for next case
    case 5:
    case 6:
        // no break - comment for case body
    // comment for next case
    default:
        return '2 or default';
}

[julienfalque]

Thinking about this again. Currently this is supported for else/elseif blocks but only to make tests from braces pass. I think there is no reliable way to say whether a comment is intended to be part of the body of its block or if it's related to the next one. See #5115 for an exemple where a comment before a else is intended to be part of the if body but considered to be related to the elseif.

Instead of extending this hazardous behavior to case/default, I propose to drop it entirely except in braces for BC.

[mvorisek]

Yes,

if comment is not the last statement/token or if it is inside curly braces (where it is always part of the body block),

then simply both levels of comment indentation can be allowed/unfixed + zero indentation for comments. If different, it should still be fixed as if it was part of the body block.

[julienfalque]

@SpacePossum @keradus @kubawerlos What is your opinion about this?

[keradus]

@SpacePossum and myself on holidays, maybe @kubawerlos or @localheinz ?

[TeLiXj]

hi! any update on this?

[leofeyer]

I think there is no reliable way to say whether a comment is intended to be part of the body of its block or if it's related to the next one.

IMHO there is, if the comment is preceded by a new line:

switch (true) {
    case 'foo':
        // inner block comment

    case 'bar':
        break;

    // outer block comment
    case 'baz':
        break;
}

If there are no empty lines at all, there is no reliable way indeed.

[leofeyer]

Also, if a comment comes directly after the break; statement, it can be assumed that it is intended for the outer block.

[Wirone]

@leofeyer new line before comment can't determine indentation:

switch (true) {
    case 'foo':
        $foo = [];

        // Some complex logic
        foreach ($foo as $f) {
        }

        // Some comment
        break;

    // outer block comment
    case 'baz':
        break;
}

But yeah, when comment is after break; then it's related to next case.

[julienfalque]

#7624 introduced a new option to control this behavior for else/elseif cases. I'll update this PR to extend this option to cases.

[github-actions]

Since this pull request has not had any activity within the last 90 days, I have marked it as stale.

The purpose of this action is to enforce backlog review once in a while. This is mostly for maintainers and helps with keeping repository in good condition, because stale issues and PRs can accumulate over time and make it harder for others to find relevant information. It is also possible that some changes has been made to the repo already, and issue or PR became outdated, but wasn't closed for some reason. This action helps with periodic review and closing of such stale items in automated way.

You may let maintainers handle this or verify current relevancy by yourself, to help with re-triage. Any activity will remove stale label so it won't be automatically closed at this point.

I will close it if no further activity occurs within the next 14 days.

Please keep your branch up-to-date by rebasing it when main branch is ahead of it, thanks in advance!