Fix remaining falsy index guards and cache the unary lookup - #81
Merged
Conversation
NoIsNull still skipped a call that opens a file. findPrevious() returns index 0 for the open tag, and `if (!$possibleCastIndex)` treats that as not found, so `<?php is_null($x);` went unreported while the same call one line lower was caught. Two further guards in the same file had the same shape. The two that already compared against false are left alone. isUnaryOperator() rebuilt its prefix lookup on every call, and since the sniff registers T_MINUS that now happens for every subtraction in a file. The set is constant, so it is built once.
There was a problem hiding this comment.
Pull request overview
This PR addresses two follow-ups from the review of #80 by fixing remaining “falsy zero” index guards in NoIsNullSniff and improving performance in ImplicitCastSpacingSniff by caching the unary-prefix lookup.
Changes:
- Replace
if (!$index)-style guards with strict=== falsechecks inNoIsNullSniffso token index0(open tag) is not misinterpreted as “not found”. - Cache the
isUnaryOperator()unary-prefix token set behind astaticso it’s built once instead of on every call. - Keep behavior unchanged aside from the intended fixes (no functional expansion beyond the guard correctness and cached lookup).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| PhpCollective/Sniffs/WhiteSpace/ImplicitCastSpacingSniff.php | Caches unary-prefix token set in isUnaryOperator() to avoid rebuilding it for every operator occurrence. |
| PhpCollective/Sniffs/PHP/NoIsNullSniff.php | Fixes incorrect “falsy index” guards by using === false checks for findPrevious() results. |
💡 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.
Both points from Copilot's review of #80.
NoIsNullskipped calls at the top of a filefindPrevious()returns index0for the open tag, andif (!$possibleCastIndex)reads that as "nothing found". #80 fixed the same shape on the sniff's first guard; this one sits further down the same method, with two more like it. The two guards in that file that already compared againstfalseare untouched.The unary lookup is built once
isUnaryOperator()rebuilt its prefix set on every call. That was cheap while the sniff only saw!and@, but it now registersT_MINUS, so it runs for every subtraction in a file. The set is constant, so it moves behind astatic.