Skip to content

Fix UniqueIterable comparing the whole iterable instead of its items - #817

Open
CorvusSharp wants to merge 3 commits into
yiisoft:masterfrom
CorvusSharp:fix-unique-iterable-comparison
Open

Fix UniqueIterable comparing the whole iterable instead of its items#817
CorvusSharp wants to merge 3 commits into
yiisoft:masterfrom
CorvusSharp:fix-unique-iterable-comparison

Conversation

@CorvusSharp

@CorvusSharp CorvusSharp commented Sep 11, 2026

Copy link
Copy Markdown
Q A
Is bugfix? ✔️
New feature?
Breaks BC?

UniqueIterableHandler never compares the items of the iterable. Inside the loop the
stack is filled with $value, the whole iterable, instead of $item:

if ($value instanceof Stringable) {
    $stack[] = (string) $value;
} elseif ($value instanceof DateTimeInterface) {
    $stack[] = $value->getTimestamp();
} else {
    $stack[] = $value;
}

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 result is that the rule reports the opposite of the truth for most inputs:

Value Current Expected
['a', 'a'] valid not unique
['a', 'b', 'c'] not unique valid
[1, 1] valid not unique
[1, 2, 3, 4] not unique valid

Any iterable with three or more items is reported as containing duplicates, and a real
duplicate in a pair passes.

There is a second, smaller defect in the same place: the check runs before the current
item is pushed, so the last item never takes part in it. Even with the right variable,
a duplicate formed by the final item would be missed.

The existing tests do not catch any of this because of how the data sets are shaped:
every case in dataValidationPassed has exactly two items, and every duplicate case in
dataValidationFailed has three or more. Both then pass for the wrong reason.

The fix

  • push $item into the stack instead of $value;
  • run the uniqueness check once, after the loop, so every item takes part in it.

The order of the Stringable and DateTimeInterface branches is left as it was.
Swapping them would change how an object implementing both interfaces is compared, which
is unrelated to this bug.

Order of errors

Moving the check out of the loop makes the order explicit and keeps per item errors
ahead of it: an item with a disallowed value or a type different from its predecessor is
reported wherever it occurs, and uniqueness is only reported when every item is valid.
Two data sets pin this down: [1, 1, []] reports the disallowed item and [1, 1, 'a']
reports the type mismatch, not the duplicate.

As a side effect array_unique now runs once instead of once per item.

Tests

Thirteen data sets added:

  • five in dataValidationPassed: unique lists of three and four items — strings,
    integers, floats, Stringable, DateTimeInterface;
  • six in dataValidationFailed: two equal items — strings, integers, floats, booleans,
    Stringable, DateTimeInterface;
  • two in dataValidationFailed for the order of errors described above.

The eleven covering the comparison itself fail on the current handler and pass with the
fix. The full suite stays green: 1945 tests, 4018 assertions. Psalm reports no issues.

About the failing rector job

It fails on master too, so it is not related to this change. rector.php skips
Rector\Php81\Rector\ClassMethod\NewInInitializerRector, and that rule no longer exists
in Rector, so the run stops with "These rules from $rectorConfig->skip() do not exist".
Reproduced locally on a clean checkout of master; the last green run of that workflow
on master was in April. Removing the stale entry also uncovers two files the current
Rector wants to rewrite and one more skip that is never registered, so it looks like
separate housekeeping rather than something to fold in here. Happy to send that as its
own pull request if you want it.

Copilot AI lite review requested due to automatic review settings September 11, 2026 05:25
@CorvusSharp
CorvusSharp force-pushed the fix-unique-iterable-comparison branch from 5750daa to a57e9bd Compare September 11, 2026 05:25
@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.35%. Comparing base (a3e0ef2) to head (ddc1854).

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #817      +/-   ##
============================================
+ Coverage     96.29%   96.35%   +0.05%     
+ Complexity     1153     1152       -1     
============================================
  Files           124      124              
  Lines          3540     3540              
============================================
+ Hits           3409     3411       +2     
+ Misses          131      129       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The duplicate-check ordering changes documented error precedence and requires resolution or updated contract and tests.

Pull request overview

Fixes UniqueIterable to compare individual items correctly and adds regression coverage and changelog documentation.

Changes:

  • Corrected item handling and duplicate-check timing.
  • Added regression tests.
  • Updated CHANGELOG.md.
File summaries
File Summary
tests/Rule/UniqueIterableTest.php Adds regression coverage.
src/Rule/UniqueIterableHandler.php Compares iterable items correctly.
CHANGELOG.md Documents the bug fix.
Review details

Suppressed comments (1)

src/Rule/UniqueIterableHandler.php:68

  • Moving this check below the push changes the documented error precedence: once [1, 1] is processed, the handler returns the uniqueness error immediately, so [1, 1, []] (or [1, 1, 'x']) now reports a duplicate instead of the later invalid-value/type error that the old in-loop ordering returned. The PR description says later item errors should still win; either defer a duplicate while the iterable is validated, or update the stated contract and add coverage for the new precedence.
            if (count($stack) !== count(array_unique($stack, flags: SORT_REGULAR))) {
  • 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.

@CorvusSharp
CorvusSharp force-pushed the fix-unique-iterable-comparison branch from a57e9bd to 6b15ce6 Compare September 11, 2026 05:30
@CorvusSharp
CorvusSharp force-pushed the fix-unique-iterable-comparison branch from 6b15ce6 to 4e0c53a Compare September 11, 2026 05:30
@CorvusSharp

CorvusSharp commented Sep 11, 2026

Copy link
Copy Markdown
Author

Good catch on the error order, that was a real inconsistency between the code and what I
wrote in the description.

Fixed by running the uniqueness check once after the loop instead of inside it. Per item
errors now always come first: an item with a disallowed value or a type different from
its predecessor is reported wherever it occurs, and uniqueness is reported only when
every item is valid. Two data sets pin it down, [1, 1, []] reports the disallowed item
and [1, 1, 'a'] reports the type mismatch rather than the duplicate.

As a side effect array_unique now runs once rather than once per item.

@vjik
vjik requested review from a team and a balanced review from Copilot September 11, 2026 06:20
@vjik vjik added the status:code review The pull request needs review. label Sep 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused fix matches the documented behavior and is covered by passing regression tests.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:code review The pull request needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants