Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SimplifyUselessVariableRector missing opportunity #8262

Closed
staabm opened this issue Oct 13, 2023 · 5 comments
Closed

SimplifyUselessVariableRector missing opportunity #8262

staabm opened this issue Oct 13, 2023 · 5 comments
Labels

Comments

@staabm
Copy link
Contributor

staabm commented Oct 13, 2023

Bug Report

I expected SimplifyUselessVariableRector to cleanup this code

Subject Details
Rector version e.g. v0.11.5 (invoke vendor/bin/rector --version)

Minimal PHP Code Causing Issue

https://getrector.com/demo/b2fd9fbe-1209-4cbb-a317-fdd49ad2f2b1

Expected Behaviour

<?php

function apcInstalled(): bool
{
    $apcInstalled = false;
-   $apcInstalled = false;
    if (rand(0,1)) {
        return true;
    }

-    return $apcInstalled;
+    return false;
}
@staabm staabm added the bug label Oct 13, 2023
@samsonasik
Copy link
Member

Since variable may be used in next call, the SideEffectNodeDetector and by-ref param may need to be used to verify.

@staabm
Copy link
Contributor Author

staabm commented Oct 13, 2023

Alternativly maybe there should be a rector which tests all return expression whether they just return a constant type and replace the return expression with said constant?

@samsonasik
Copy link
Member

On method call or user defined function. it clearly should be skipped, as variable can be used via:

global $var;

++$var;

which change behavior, On native function, there is PureFunctionDetector service, that consumed by SideEffectNodeDetector

@samsonasik
Copy link
Member

samsonasik commented Oct 13, 2023

@samsonasik
Copy link
Member

I checked the code, it seems the rule is designed to cover Assign in Expression, then use after that, which your use case is out of scope.

we had the MoveVariableDeclarationNearReferenceRector, and due to magical, removed at

on that reasoning, I am closing it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants