Skip to content

Fix ISXB-1790: ArrayHelpers.Merge(IEqualityComparer) used wrong Equal… #2377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
K-Tone wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix ISXB-1790: ArrayHelpers.Merge(IEqualityComparer) used wrong Equal… #2377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions Assets/Tests/InputSystem/Utilities/ArrayHelperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Globalization;
using NUnit.Framework;
using Unity.Collections;
Expand Down Expand Up @@ -118,6 +119,35 @@ public void Utilities_IndexOfReference__IsUsingReferenceEqualsAndConstrainedBySt
Assert.AreEqual(2, arr.IndexOfReference(arr[2], 1, 3));
}

[Test]
[Category("Utilities")]
public void Utilities_HaveDuplicateReferences_DetectsDuplicatesInFullRange()
{
var withDup = new object[] { new object(), new object(), new object() };
withDup[2] = withDup[0]; // duplicate at 0 and 2
Assert.That(withDup.HaveDuplicateReferences(0, 3), Is.True);

var noDup = new object[] { new object(), new object(), new object() };
Assert.That(noDup.HaveDuplicateReferences(0, 3), Is.False);

// Regression test for ISXB-1792: inner loop was "n < count - i" so later pairs were never checked
var dupAtEnd = new object[] { new object(), new object(), new object(), new object() };
dupAtEnd[3] = dupAtEnd[2]; // duplicate at 2 and 3
Assert.That(dupAtEnd.HaveDuplicateReferences(0, 4), Is.True);
}

[Test]
[Category("Utilities")]
public void Utilities_MergeWithComparer_UsesComparerToDeduplicate()
{
// Regression test for ISXB-1790: Merge(IEqualityComparer) was calling comparer.Equals(secondValue)
// instead of comparer.Equals(x, secondValue), so it compared against the comparer instance.
var first = new[] { "a", "b" };
var second = new[] { "A", "c" }; // "A" equals "a" with case-insensitive comparer
var merged = ArrayHelpers.Merge(first, second, StringComparer.OrdinalIgnoreCase);
Assert.That(merged, Is.EqualTo(new[] { "a", "b", "c" }));
}

[Test]
[Category("Utilities")]
public void Utilities_IndexOfPredicate__IsUsingPredicateForEqualityAndConstraintedByStartIndexAndCount()
Expand Down
4 changes: 2 additions & 2 deletions Packages/com.unity.inputsystem/InputSystem/Utilities/ArrayHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public static bool HaveDuplicateReferences<TFirst>(this TFirst[] first, int inde
for (var i = 0; i < count; ++i)
Copy link
Contributor

@u-pr u-pr bot Mar 12, 2026

Choose a reason for hiding this comment

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

low
Consider adding a where TFirst : class constraint to this method. Given the name HaveDuplicateReferences and the use of ReferenceEquals, this utility is clearly intended for reference types. Without a type constraint, passing value types to this method would cause boxing allocations on every comparison, which could significantly impact performance in hot paths.

🤖 Helpful? 👍/👎

Copy link
Collaborator

@ekcoh ekcoh Mar 13, 2026

Choose a reason for hiding this comment

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

Agree, the generic constraint is kind of required already. @K-Tone

{
var element = first[i];
for (var n = i + 1; n < count - i; ++n)
for (var n = i + 1; n < count; ++n)
Copy link
Contributor

@u-pr u-pr bot Mar 12, 2026

Choose a reason for hiding this comment

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

high
Have you considered that the index parameter is completely ignored in both the outer and inner loops?

While fixing the inner loop bound to n < count solves the issue when index == 0, it will still fail to check the correct range when index > 0. Since the method is designed to check the range [index, index + count), the iteration bounds should account for the index offset:

for (var i = index; i < index + count; ++i)
{
    var element = first[i];
    for (var n = i + 1; n < index + count; ++n)
    {
        if (ReferenceEquals(element, first[n]))
            return true;
    }
}

Consider adding a test case with a non-zero index to ensure it correctly scans the sub-range.

🤖 Helpful? 👍/👎

Copy link
Collaborator

@ekcoh ekcoh Mar 13, 2026

Choose a reason for hiding this comment

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

@K-Tone This looks like another bug that we should fix here.

{
if (ReferenceEquals(element, first[n]))
return true;
Expand Down Expand Up @@ -552,7 +552,7 @@ public static TValue[] Merge<TValue>(TValue[] first, TValue[] second, IEqualityC
for (var i = 0; i < second.Length; ++i)
{
var secondValue = second[i];
if (!merged.Exists(x => comparer.Equals(secondValue)))
if (!merged.Exists(x => comparer.Equals(x, secondValue)))
Copy link
Contributor

@u-pr u-pr bot Mar 12, 2026

Choose a reason for hiding this comment

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

medium
The call to merged.Exists with a lambda expression (x => comparer.Equals(x, secondValue)) captures comparer and secondValue, which results in a heap allocation for a closure in every iteration of the loop. To minimize GC pressure in the Input System, have you considered using a manual for loop instead?

var alreadyExists = false;
for (var j = 0; j < merged.Count; ++j)
{
    if (comparer.Equals(merged[j], secondValue))
    {
        alreadyExists = true;
        break;
    }
}
if (!alreadyExists)
    merged.Add(secondValue);

🤖 Helpful? 👍/👎

Copy link
Collaborator

@ekcoh ekcoh Mar 13, 2026

Choose a reason for hiding this comment

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

We should use a manual for loop here, should be addressed.

{
merged.Add(secondValue);
}
Expand Down
Loading