Cover return by reference in ImplicitCastSpacing - #83
Merged
Conversation
The reference marker added in #82 only matched in front of a variable or a variadic ellipsis, so `function & name()` slipped through - the name is not a variable. That form is checked first now, keyed on the preceding function, closure or arrow-function keyword. Found while retiring psr2r-sniffer's UnaryOperatorSpacing against this rule: return by reference was the one construct it still reported that this sniff did not.
There was a problem hiding this comment.
Pull request overview
This PR closes a gap in PhpCollective.WhiteSpace.ImplicitCastSpacing by correctly detecting return-by-reference declarations (e.g., function & getItems()), which previously slipped through because the sniff’s reference detection relied on the token after & being a variable/ellipsis.
Changes:
- Extend reference-operator detection to treat
&immediately followingfunction/closure/fnas a reference marker (return-by-reference) before applying the existing “variable/ellipsis on the right” heuristic. - Add a new fixture case covering
protected function & byRefReturn(): arrayand its fixed formprotected function &byRefReturn(): array. - Update the unit test expectations to account for the additional fixable error.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| PhpCollective/Sniffs/WhiteSpace/ImplicitCastSpacingSniff.php | Adds an early return-by-reference detection path keyed off the preceding function token to correctly classify & in function & name() signatures. |
| tests/_data/ImplicitCastSpacing/before.php | Adds a failing fixture for return-by-reference spacing (function & byRefReturn). |
| tests/_data/ImplicitCastSpacing/after.php | Adds the corrected fixture output (function &byRefReturn). |
| tests/PhpCollective/Sniffs/WhiteSpace/ImplicitCastSpacingSniffTest.php | Updates expected fixable error counts from 8 to 9 to include the new case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 reference marker added in #82 required a variable or a variadic ellipsis on the right, so a return-by-reference declaration slipped through - the thing on the right is a function name:
That form is now checked first, keyed on the preceding
function/function () use/fnkeyword rather than on what follows.Found while retiring
psr2r-sniffer'sUnaryOperatorSpacingin favour of this sniff. Comparing the two against the same inputs, return by reference was the one construct the PSR2R sniff still reported that this one did not - removing it there would have quietly dropped the check.