Report ConsoleExecuteReturnIntRector when it rewrites a return - #1064
Open
dchaudhari7177 wants to merge 1 commit into
Open
Report ConsoleExecuteReturnIntRector when it rewrites a return#1064dchaudhari7177 wants to merge 1 commit into
dchaudhari7177 wants to merge 1 commit into
Conversation
The rule tracks $this->hasChanged, and refactor() returns the node only when that flag is set -- but only refactorReturnTypeDeclaration() ever set it. Every other mutation was made in place and not declared: setReturnTo0InsteadOfNull() rewrites `return null` to `return 0`, `return false` to `return 1`, the null side of a `??` or of a ternary, and wraps a non-int expression in `(int)`; processReturn0ToMethod() appends `return 0;` to the method. So a command whose execute() already carries `: int` gets its body rewritten while refactor() returns null. That is the empty `Applied rules:` in rectorphp/rector#9897: the diff is printed with no rule attributed to it, and which of the two happens is left to how the caller treats an undeclared in-place mutation -- on 2.4.5 it was dropped, on 2.6.6 it is printed. setReturnTo0InsteadOfNull() now reports whether it rewrote anything, the caller raises hasChanged from it, and the append raises it too. No mutation is added or removed; only the declaration of the ones already being made. Fixture: a command already returning int whose `return null` still needs rewriting -- the shape that had no coverage, since every existing fixture with a null return also lacks the return type and so was carried by refactorReturnTypeDeclaration().
Member
|
Thanks! Looks great, just one issue in PHPStan |
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.
The other half of rectorphp/rector#9897. The unreachable-
returnitself is rectorphp/rector-src#8476 (TerminatedNodeAnalyzer); this is the "no rule listed underApplied rules:" half, which lives here.The bug
refactor()returns the node only when$this->hasChangedis set, and onlyrefactorReturnTypeDeclaration()ever sets it. Everything else mutates the AST in place and says nothing:refactorReturnTypeDeclaration()— adds: intsetReturnTo0InsteadOfNull()—return null→return 0return false→return 1??or of a ternary(int)processReturn0ToMethod()— appendsreturn 0;So for a command whose
execute()already carries: int, the body is rewritten andrefactor()returnsnull. That is the emptyApplied rules:in #9897 — a printed diff with no rule attributed to it.It is not only a reporting problem. What happens to an undeclared in-place mutation is the caller's choice, and it has changed: the reporter says the same input produced no diff at all on 2.4.5 and a diff on 2.6.6. A rule that does not declare its own changes is at the mercy of that.
The fix
setReturnTo0InsteadOfNull()now returns whether it rewrote anything, the caller raiseshasChangedfrom it, and the append raises it too.No mutation is added or removed — only the declaration of the ones already being made. The one restructuring is the ternary branch, which was already computing exactly this boolean and throwing it away:
Fixture, and its honest limit
return_null_with_int_return_type.php.inc: a command already returningintwhosereturn nullstill needs rewriting. Every existing fixture with a null return also lacks the return type, sorefactorReturnTypeDeclaration()sethasChangedfor them and this path had no coverage at all.To be straight about what it does and does not prove: a
.php.incfixture asserts the printed output, not which rule was attributed to it. On rector 2.6.x the mutation is printed either way, so this fixture very likely passes with and without the production change. It is here to pin the shape — and it is the case that would start failing on any version that goes back to dropping undeclared mutations, which is what 2.4.5 did.Verification gap
I could not run the suite.
composer installhere resolvesphpunit/phpunit ^13.2, which needs PHP ≥ 8.4.1, and this machine has 8.3.6 (tomasvotruba/type-coveragewants^8.4too). So the fixture and the change are for CI to confirm. What I did check locally is that both changed files passphp -l, and I traced everyreturn;insetReturnTo0InsteadOfNull()to make sure each one that mutates now returnstrueand the single non-mutating fall-through returnsfalse.Note
This PR was drafted with AI assistance. I understand the change and can defend or revise it.