Remove duplicate rule coverage and add unary minus spacing - #80
Merged
Conversation
Three constructs were reported by more than one rule at once. In each case the narrower rule is silenced and the one with the widest coverage kept, so every input still gets flagged, just once: - long casts: SlevomatCodingStandard.PHP.TypeCast survives. It is the only one of the three that rewrites (double) to (float) and copes with inner spaces like ( boolean ). PSR12.Keywords.ShortFormTypeKeywords is excluded, and so is the cast half of PhpCollective.PHP.ShortCast, whose DoubleNotInvalid check stays because nothing else flags a double negation used as a bool cast. - incrementer spacing: Generic.WhiteSpace.IncrementDecrementSpacing survives, since it also handles targets like $b[0] ++. The matching pair of codes on PhpCollective.WhiteSpace.ImplicitCastSpacing is excluded. - sizeof(): PhpCollective.PHP.RemoveFunctionAlias survives, because unlike Generic.PHP.ForbiddenFunctions it can fix the call rather than only report it. That last one only holds once the alias sniff stops skipping a call that opens a file. Its guard read `if (!$previous)`, and findPrevious() returns index 0 for the open tag, so `<?php sizeof($x);` was silently ignored. Four sniffs in the same family carried the same falsy-zero guard and are corrected together. ImplicitCastSpacing gains unary minus, which nothing covered. It is deliberately conservative: a minus counts as unary only when what precedes it cannot end a value, so an unrecognized predecessor is read as subtraction and left alone. The reverse test would have to enumerate every value-producing token, and anything forgotten - __LINE__, true, null, a qualified constant - would be "fixed" into __LINE__ -1. `- -$i` is also left alone, since closing that gap would turn a double negation into a decrement.
There was a problem hiding this comment.
Pull request overview
This PR reduces duplicate reporting across sniffs in the PhpCollectiveStrict standard, fixes a falsy-index bug in several sniffs’ token lookups, and extends ImplicitCastSpacing to also enforce spacing for unary minus without overlapping subtraction formatting.
Changes:
- Silences narrower/duplicate rule coverage in
PhpCollective/ruleset.xml(long casts, increment/decrement spacing,sizeof()aliasing) while keeping broader rules enabled. - Fixes
findPrevious()index handling (0vsfalse) in multiple sniffs so top-of-file constructs aren’t skipped. - Adds unary-minus support to
ImplicitCastSpacingand updates fixtures/docs accordingly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/_data/ImplicitCastSpacing/before.tokens.php | Adds token fixture coverage for unary minus and related cases. |
| tests/_data/ImplicitCastSpacing/before.php | Updates “before” fixture to include unary minus spacing violations and binary-minus non-violations. |
| tests/_data/ImplicitCastSpacing/after.php | Updates “after” fixture to show expected unary minus spacing fixes. |
| PhpCollective/Sniffs/WhiteSpace/ImplicitCastSpacingSniff.php | Adds T_MINUS handling with unary/binary disambiguation logic. |
| PhpCollective/Sniffs/PHP/ShortCastSniff.php | Fixes findPrevious() falsy-index guard (0 vs false). |
| PhpCollective/Sniffs/PHP/RemoveFunctionAliasSniff.php | Fixes findPrevious() falsy-index guard (0 vs false) to avoid skipping at file start. |
| PhpCollective/Sniffs/PHP/NoIsNullSniff.php | Fixes one falsy-index guard in is_null detection logic. |
| PhpCollective/Sniffs/PHP/DisallowFunctionsSniff.php | Fixes findPrevious() falsy-index guards in forbidden-function checks. |
| PhpCollective/Sniffs/Internal/DisallowFunctionsSniff.php | Fixes findPrevious() falsy-index guard in internal forbidden-function check. |
| PhpCollective/ruleset.xml | Removes/adjusts overlapping rule coverage and documents rationale. |
| docs/sniffs.md | Updates published sniff counts and PSR12 list after silencing an overlapping sniff. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+90
to
+91
|
|
||
| $unaryPrefixes = Tokens::$operators |
Comment on lines
41
to
44
| $tokenContent = $tokens[$stackPtr]['content']; | ||
| $previous = $phpcsFile->findPrevious(T_WHITESPACE, ($stackPtr - 1), null, true); | ||
| if (!$previous || in_array($tokens[$previous]['code'], $wrongTokens)) { | ||
| if ($previous === false || in_array($tokens[$previous]['code'], $wrongTokens)) { | ||
| return; |
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.
Three constructs were each reported by more than one rule. In every case the rule with the widest coverage is kept and the narrower ones silenced, so nothing stops being detected - it is just reported once.
SlevomatCodingStandard.PHP.TypeCastPSR12.Keywords.ShortFormTypeKeywords,PhpCollective.PHP.ShortCast.LongInvalid(double)to(float)and handles( boolean )Generic.WhiteSpace.IncrementDecrementSpacingImplicitCastSpacing.WhitespaceBeforeVariable/.WhitespaceAfterVariable$b[0] ++sizeof()PhpCollective.PHP.RemoveFunctionAliassizeofentry onGeneric.PHP.ForbiddenFunctionsShortCastkeeps itsDoubleNotInvalidcheck - nothing else flags!!used as a bool cast.Before, one
(boolean)cast produced three errors; now one. Coverage went up rather than down:(double)was previously reported by only one of the three, and it is the one that stayed.A falsy-zero guard that made the sizeof swap unsafe
RemoveFunctionAliasskipped any call that opens a file:findPrevious()returns index0for the open tag, and!0is true - so<?php sizeof($x);was silently ignored while<?php\n$y = 1;\nsizeof($x);was caught. Dropping theForbiddenFunctionsentry would have made that case unreported entirely.Four sniffs in the same family carried the identical guard and are corrected together:
NoIsNull,DisallowFunctions(both copies) andShortCast.Unary minus
Nothing covered
- $a, the one rulepsr2r-snifferhas that this standard lacked.ImplicitCastSpacingalready owns this shape for!and@, so it gainsT_MINUSrather than a new sniff that would overlap it.The detection is deliberately one-directional: a minus counts as unary only when the preceding token cannot end a value. Testing the other way round would mean enumerating every value-producing token, and anything missed becomes a false positive that phpcbf acts on -
__LINE__ - 1rewritten to__LINE__ -1. Failing to binary leaves unknown input untouched.Two cases the fixture pins down:
- -$countkeeps its space. Closing it produces--$count, a decrement.yield - $v,fn () => - $v,<?= - $v ?>are detected; open tags and arrow tokens count as expression-introducers.