Bug Report
| Subject |
Details |
| Rector version |
2.6.6 |
After upgrading from 2.4.5 to 2.6.6 I noticed that ConsoleExecuteReturnIntRector appends an unreachable return Command::SUCCESS;
Diffs are printed with no rule listed under Applied rules:, which is probably why they stayed unnoticed before: on 2.4.5 the same input and config produce no diff at all.
Minimal PHP Code Causing Issue
Expected Behaviour
ConsoleExecuteReturnIntRector: no change. execute() already returns int on every path, both the try and the catch end with a return, so the appended statement is unreachable and PHPStan reports deadCode.unreachable on it. TerminatedNodeAnalyzer::isTerminatedInLastStmts() only accepts a Return_ or an exit/throw expression as the last statement of a block, so a trailing comment (parsed as Nop) or a while (true) loop at the end of the try block makes it look like the method falls through.
jfyi: guess at the cause: both rules(this one and #9898) mutate the AST in place without reporting it. processReturn0ToMethod() appends the statement without setting $this->hasChanged, and refactorNew() checks $replacedNode !== $currentNode while processArgs() mutates the argument and returns the very same instance, so in both cases refactor() ends up returning null. On 2.4.5 such undeclared mutations seem to be dropped, on 2.6.6 they are printed, which would explain both the empty Applied rules: and why these rules looked fine before.
Bug Report
After upgrading from 2.4.5 to 2.6.6 I noticed that
ConsoleExecuteReturnIntRectorappends an unreachablereturn Command::SUCCESS;Diffs are printed with no rule listed under
Applied rules:, which is probably why they stayed unnoticed before: on 2.4.5 the same input and config produce no diff at all.Minimal PHP Code Causing Issue
ConsoleExecuteReturnIntRector: https://getrector.com/demo/45fd46a3-698b-4809-9a91-ecf334b831b5Expected Behaviour
ConsoleExecuteReturnIntRector: no change.execute()already returnsinton every path, both thetryand thecatchend with areturn, so the appended statement is unreachable and PHPStan reportsdeadCode.unreachableon it.TerminatedNodeAnalyzer::isTerminatedInLastStmts()only accepts aReturn_or anexit/throwexpression as the last statement of a block, so a trailing comment (parsed asNop) or awhile (true)loop at the end of thetryblock makes it look like the method falls through.jfyi: guess at the cause: both rules(this one and #9898) mutate the AST in place without reporting it.
processReturn0ToMethod()appends the statement without setting$this->hasChanged, andrefactorNew()checks$replacedNode !== $currentNodewhileprocessArgs()mutates the argument and returns the very same instance, so in both casesrefactor()ends up returningnull. On 2.4.5 such undeclared mutations seem to be dropped, on 2.6.6 they are printed, which would explain both the emptyApplied rules:and why these rules looked fine before.