Rector removes lines in switch-case that is not dead

[escopecz]

Subject	Details
Rector version	https://github.com/rectorphp/rector-prefixed/releases/tag/v0.6.13
PHP version	7.2.26
Full Command	mautic (3.x) $ bin/rector process app/bundles/AssetBundle --set dead-code --dry-run1
Demo link	https://getrector.org/demo/4db04e11-6f12-407c-9b4d-97133acff941
rector.yaml	No config

Current Behaviour

Rector works for Mautic!!! 🎉 I tried to run Rector on AssetBundle and noticed following removed lines which I don't think is correct.

We use a static method to convert upload size to bytes in Mautic, but it was borrowed from Symfony:

Rector suggests to remove these lines:

         switch (strtolower(substr($size, -1))) {
             case 't':
-                $max *= 1024;
                 // no break
             case 'g':
-                $max *= 1024;
                 // no break
             case 'm':
-                $max *= 1024;
                 // no break
             case 'k':
                 $max *= 1024;
    ----------- end diff -----------

Minimal PHP Code Causing Issue

<?php declare(strict_types=1);

final class DemoFile
{
    public static function convertSizeToBytes($size)
    {
        if ('' === $size) {
            return PHP_INT_MAX;
        }

        $max = ltrim($size, '+');
        if (0 === strpos($max, '0x')) {
            $max = intval($max, 16);
        } elseif (0 === strpos($max, '0')) {
            $max = intval($max, 8);
        } else {
            $max = intval($max);
        }

        switch (strtolower(substr($size, -1))) {
            case 't':
                $max *= 1024;
                // no break
            case 'g':
                $max *= 1024;
                // no break
            case 'm':
                $max *= 1024;
                // no break
            case 'k':
                $max *= 1024;
        }

        return $max;
    }
}

Expected Behaviour

I don't think it should remove the lines.

[TomasVotruba]

Rector works for Mautic!!! tada

That's great 👏 🎉

Ah, at first it looked wrong...

On second look, I think it's correct. All 4 items return the bottom value.

[escopecz]

I see it now too. Rector is clever!

[TomasVotruba]

Yea, Rector is based on pure distilled human lazyness :D

[escopecz]

Getting back to this. It is a bug. Since there are no breaks in the switch case then Rector's change breaks the behavior. Consider calling DemoFile::convertSizeToBytes('1K'). The result is 1024. If you call DemoFile::convertSizeToBytes('1M') then in the modified version it's still 1024 instead of 1048576 that it used to be before the change and which is also the right result.

[TomasVotruba]

I see. it's comulative.

Could you send failing test case?

What is the rule to separate this from duplication? Modifying the same value?

[TomasVotruba]

Any ideas?

[escopecz]

TBH, I don't use switches that much. Trying to avoid them with better design. This was new behaviour to me:

The switch statement executes line by line (actually, statement by statement). In the beginning, no code is executed. Only when a case statement is found whose expression evaluates to a value that matches the value of the switch expression does PHP begin to execute the statements. PHP continues to execute the statements until the end of the switch block, or the first time it sees a break statement. If you don't write a break statement at the end of a case's statement list, PHP will go on executing the statements of the following case

source: https://www.php.net/manual/en/control-structures.switch.php

So I don't think Rector should squash case statements if they do not contain break. Does it make sense?

[TomasVotruba]

So I don't think Rector should squash case statements if they do not contain break. Does it make sense?

👍 Yes, that's what I was looking for. Simple and clear. I'll fix it