rectorphp / rector

Instant Upgrades and Automated Refactoring of any PHP 5.3+ code
https://getrector.com
MIT License
8.71k stars 687 forks source link

Incorrect behavior of RemoveUnusedVariableInCatchRector #8131

Closed marmichalski closed 1 year ago

marmichalski commented 1 year ago

Bug Report

Subject Details
Rector version last dev-main
Installed as composer dependency

Minimal PHP Code Causing Issue

See https://getrector.com/demo/18be2f38-6db9-4d3b-a4da-db861dc424b4

<?php

final class DemoFile
{
    public function run()
    {
        $triesLeft = 3;

        do {
            try {
                return 'method that might throw';
            } catch (\RuntimeException $failure) {
                continue;
            }
        } while ($triesLeft --> 0);

        throw $failure;
    }
}

Responsible rules

Expected Behavior

Rector should not remove variable from the catch.

samsonasik commented 1 year ago

Hi, I think you can skip via $rectorConfig->skip() for that as that already out of current StmtsAware scope.

marmichalski commented 1 year ago

Hi, I think you can skip via $rectorConfig->skip() for that as that already out of current StmtsAware scope.

I do, but I would not call that a solution, considering it used to work few versions back. 😸 It also works correctly without do-while/while block wrap: https://getrector.com/demo/144958f1-1988-4bc9-9548-e0f6a1b652b7

samsonasik commented 1 year ago

You can refactor to add counter check inside do than throw that

https://getrector.com/demo/1bfc069c-acb2-43c2-81d0-c33afd544c55

that's the variable will always on scope area and exists ;)

marmichalski commented 1 year ago

You can refactor to add counter check inside do than throw that

https://getrector.com/demo/1bfc069c-acb2-43c2-81d0-c33afd544c55

that's the variable will always on scope area and exists ;)

I can do many things to work around rector limitations. Happy to close this issue, as by the looks of it, it is not considered a bug.

samsonasik commented 1 year ago

Ok, I am closing it, thank you for understanding ;)

kunicmarko20 commented 7 months ago

Hey @samsonasik how is this not considered a bug?

samsonasik commented 7 months ago

as answered, because the usage is out of scope variable.