[NodeAnalyzer] Detect always terminated stmts with statements before the return - #8260
Merged
Merged
Conversation
…the return TerminatedNodeAnalyzer bailed out whenever the second-to-last statement of a try/catch, if/else or switch branch was not itself a terminator. That guard was added in #6837 to keep a break inside a switch case from being treated as always terminated, but it also rejected every branch doing real work before its return. Move the check to the Switch_ branch, where a break/continue actually escapes to the next statement, so the generic path only looks at the last statement.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TerminatedNodeAnalyzerbailed out whenever the second-to-last statement of atry/catch,if/elseorswitchbranch was not itself a terminator:That guard came from #6837, to stop a
breakinside a switch case being read as always terminated. It is too broad: any branch that does real work before itsreturntrips it.This moves the check to the
Switch_branch, where abreak/continuegenuinely escapes to the statement after the switch. The generic path now only looks at the last statement, as intended.Effect
RemoveUnreachableStatementRectornow sees through branches that do work before returning:try { echo 'try'; return something(); } catch (Exception $e) { echo 'catch'; return null; } - -echo 'never executed';Bare-return branches already worked; only the ones with preceding statements were missed.
Downstream
This also fixes
ConsoleExecuteReturnIntRectorin rector-symfony appending a second, unreachablereturn 0;:Notes
#6837'sskip_switch_break_not_unreachable.php.incstill passes.breakbelonging to an inner loop still counts as escaping. That stays conservative (skip rather than remove) and matches the previous behaviour;skip_switch_case_nested_loop_break.php.inclocks it in.isTerminatedNode()params narrowed toStmt, astype-perfectrequires once the generic call site is gone.