Skip to content

[CodingStyle][EarlyReturn] Fix infinite if else on BinarySwitchToIfElseRector+RemoveAlwaysElseRector#5057

Merged
samsonasik merged 4 commits intomainfrom
fix-infinite-else
Sep 21, 2023
Merged

[CodingStyle][EarlyReturn] Fix infinite if else on BinarySwitchToIfElseRector+RemoveAlwaysElseRector#5057
samsonasik merged 4 commits intomainfrom
fix-infinite-else

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik
Copy link
Copy Markdown
Member Author

@jlherren this should fix rectorphp/rector#8220, the remove always else need on second run :)

@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba I am merging it ;)

@samsonasik samsonasik merged commit 69055b5 into main Sep 21, 2023
@samsonasik samsonasik deleted the fix-infinite-else branch September 21, 2023 12:13
@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented Sep 21, 2023

Thanks 👍

I still think the switch to if/else is kind of pointless with match. I recall it was my idea to add this rule to avoid switched back in PHP 7.4 :) but now match seem like much better logical choice, that this rule would skip.

@samsonasik
Copy link
Copy Markdown
Member Author

switch to match is part of upgrade, while that can cause a different logic, as need default value:

$message = match ($statusCode) {
    200, 300 => null,
    400 => 'not found',
    500 => 'server error',
    default => 'unknown status code',
};

this code doesn't have default value to compare so mostly use if else.

@samsonasik
Copy link
Copy Markdown
Member Author

Without default value, it will cause UnhandledMatchError, which if else is better for this case https://3v4l.org/52bgD

BinarySwitchToIfElse is to make if elseif works and the transformation keep working as expected.

@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented Sep 21, 2023

Else is the default here, so it can be upgraded to match as well:

        switch (mt_rand()) {
            case 1:
                return 10;
            case 2:
                return 20;
        }
        return 30;

return match(mt_rand()) {
    1 => 10,
    2 => 20,
    default => 30,
};

The switch should remain untouched, untill PHP 8.0 comes and offer better option. Whether manual or Rector is less important

@samsonasik
Copy link
Copy Markdown
Member Author

I think that depends, if no last return, this rule still make sense, see https://getrector.com/demo/0412d7c9-b99a-4299-a945-7f735cbc9fd9

@TomasVotruba
Copy link
Copy Markdown
Member

I think that depends

Indeed, that's why PHPStan and developer should handle these.

In switch() cases I've seen in most projects there is another default value under it/above it or implicit null after the switch.

@samsonasik
Copy link
Copy Markdown
Member Author

switch use case doesn't necessarily before return, it can be on middle of multiple statement, something like this assign is vaild https://getrector.com/demo/c23ff74a-0f2d-4306-8c9f-f6bb09d7cbdb

which change to if elseif is more valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Rector hangs indefinitely

3 participants