Fix UniqueIterable comparing the whole iterable instead of its items - #816
Closed
CorvusSharp wants to merge 2 commits into
Closed
Fix UniqueIterable comparing the whole iterable instead of its items#816CorvusSharp wants to merge 2 commits into
CorvusSharp wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #816 +/- ##
============================================
+ Coverage 96.29% 96.35% +0.05%
+ Complexity 1154 1153 -1
============================================
Files 124 124
Lines 3540 3540
============================================
+ Hits 3409 3411 +2
+ Misses 131 129 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved issues were identified, and the fix includes regression coverage.
Pull request overview
Fixes UniqueIterableHandler to compare normalized items correctly and detect duplicates after each item is added.
Changes:
- Correct uniqueness validation logic.
- Add regression coverage.
- Document the bug fix.
File summaries
| File | Description |
|---|---|
tests/Rule/UniqueIterableTest.php |
Adds regression coverage for unique and duplicate values. |
src/Rule/UniqueIterableHandler.php |
Corrects uniqueness validation logic. |
CHANGELOG.md |
Documents the bug fix. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
UniqueIterableHandlernever compares the items of the iterable. Inside the loop thestack is filled with
$value, the whole iterable, instead of$item:So the stack holds N copies of one and the same iterable, and the uniqueness check fires
as soon as it holds two of them, regardless of the content. The check also runs before
the current item is pushed, so it is always one iteration behind.
The result is that the rule reports the opposite of the truth for most inputs:
['a', 'a']['a', 'b', 'c'][1, 1][1, 2, 3, 4]Any iterable with three or more items is reported as containing duplicates, and a real
duplicate in a pair passes.
The existing tests do not catch it because of how the data sets are shaped: every case in
dataValidationPassedhas exactly two items, and every duplicate case indataValidationFailedhas three or more. Both then pass for the wrong reason.The fix
$iteminto the stack instead of$value;Two things are deliberately left as they were, to keep the change limited to the bug:
StringableandDateTimeInterfacebranches. Swapping them wouldchange how an object implementing both interfaces is compared, which is unrelated to
this bug;
precedence: a later item with a disallowed value or a different type is still reported
before a duplicate found earlier would be, and the rule still returns as soon as the
duplicate is complete.
Tests
Eleven data sets added, covering both sides of the failure for every supported item type:
dataValidationPassed: unique lists of three and four items — strings,integers, floats,
Stringable,DateTimeInterface;dataValidationFailed: two equal items — strings, integers, floats, booleans,Stringable,DateTimeInterface.All eleven fail on the current handler and pass with the fix. The full suite stays green:
1943 tests, 4014 assertions. Psalm reports no issues.