Skip to content

Fix IList.Contains to work with multidimensional arrays #119583

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

Closed
wants to merge 3 commits into from
Closed

Fix IList.Contains to work with multidimensional arrays #119583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a74a430
Fix IList.Contains to work with multidimensional arrays
stephentoub Sep 11, 2025
8b83939
Update src/libraries/System.Linq/tests/OfTypeTests.cs
stephentoub Sep 11, 2025
e443b70
Update src/libraries/System.Runtime/tests/System.Runtime.Tests/System…
stephentoub Sep 11, 2025
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 src/libraries/System.Linq/tests/OfTypeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,5 +228,35 @@ public void MultipleIterations()
Assert.Equal(i + 1, count);
}
}

[Fact]
public void MultiDimArray_OfType_Succeeds()
{
var array = new string[3, 4];
for (int i = 0; i < 3; i++)
{
for (int j = 0; j < 4; j++)
{
array[i, j] = $"{i}{j}";
}
}

// ToArray
var result = array.OfType<string>().ToArray();
Assert.Equal(12, result.Length);
for (int i = 0; i < 3; i++)
{
for (int j = 0; j < 4; j++)
{
Assert.Equal($"{i}{j}", result[i * 4 + j]);
}
}

// Contains
foreach (string s in array)
{
Assert.True(array.OfType<string>().Contains(s));
}
}
}
}
17 changes: 16 additions & 1 deletion src/libraries/System.Private.CoreLib/src/System/Array.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1039,7 +1039,22 @@ int IList.Add(object? value)

bool IList.Contains(object? value)
{
return IndexOf(this, value) >= this.GetLowerBound(0);
// IndexOf only works for single-dimensional arrays.
Copy link
Member

@jkotas jkotas Sep 11, 2025

Choose a reason for hiding this comment

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

Should this be fixed in IndexOf instead?

Copy link
Member

@eiriktsarpalis eiriktsarpalis Sep 11, 2025

Choose a reason for hiding this comment

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

What would the output of IndexOf be in the context of MD arrays?

Copy link
Member Author

@stephentoub stephentoub Sep 11, 2025

Choose a reason for hiding this comment

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

I considered that, but wasn't sure what semantics we would want for IndexOf in terms of the actual index it computes.

My intent is to backport this change to 10. If we're ok at this point defining what those semantics would be, I could do that instead.

Copy link
Member

@eiriktsarpalis eiriktsarpalis Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I suppose the type does implement IList so an MD array index is technically speaking well defined.

Copy link
Member Author

@stephentoub stephentoub Sep 11, 2025

Choose a reason for hiding this comment

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

I suppose the type does implement IList so an MD array index is technically speaking well defined.

You mean because of the indexer? That throws for MD arrays, too.

Copy link
Member Author

@stephentoub stephentoub Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My thinking was that for 10 we fix Contains in order to fix the LINQ issue, and then for 11 if desired we can look at declaring the ordering and making the other members work... doing so will unblock some other optimizations we previously tried in LINQ but had to back out because the incoming IList wasn't actually usable, and just in general remove unexpected surprises.

Copy link
Member Author

@stephentoub stephentoub Sep 11, 2025

Choose a reason for hiding this comment

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

You mean because of the indexer? That throws for MD arrays, too

Ah, ok. It is weird that IList.Count does not throw, but the indexer throws.

Agreed :)

Copy link
Member

@tannergooding tannergooding Sep 11, 2025

Choose a reason for hiding this comment

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

That won't work for all MD arrays as they can have more than Array.MaxLength items and therefore have values outside the bounds of int.MaxValue

Copy link
Member Author

@stephentoub stephentoub Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That won't work for all MD arrays

What is the "that" in this statement? IndexOf? That's a good point, too, in addition to ordering, you might have indices too large to return.

Copy link
Member

@tannergooding tannergooding Sep 11, 2025

Choose a reason for hiding this comment

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

Any of the IList APIs that deal with indexes risk not working with MD arrays, so while we could update them to work in some cases (and likely almost any real world case), it is important to note (at least in the impl, potentially in the documentation) that some MD arrays are "incompatible" with those APIs and so we need to have the fallback paths regardless.

if (Rank == 1)
{
return IndexOf(this, value) >= GetLowerBound(0);
Copy link
Member

@jkotas jkotas Sep 11, 2025

Choose a reason for hiding this comment

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

This has integer overflow bug.

using System.Collections;

IList a = Array.CreateInstance(typeof(int), [5], [Int32.MinValue]);
Console.WriteLine(a.Contains(42));

prints true, but it should print false.

Copy link
Member

@tannergooding tannergooding Sep 11, 2025

Choose a reason for hiding this comment

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

Looks like we return GetLowerBound(0) - 1 in the case that it isn't found, so this should likely be the following, which indicates it was found regardless of length (provided it is Rank == 1)

Suggested change
return IndexOf(this, value) >= GetLowerBound(0);
return IndexOf(this, value) != (GetLowerBound(0) - 1);

Copy link
Member Author

@stephentoub stephentoub Sep 11, 2025

Choose a reason for hiding this comment

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

Shall I fix that in this PR as well? I didn't introduce this line, I just wrapped it in the if.

Copy link
Member

@jkotas jkotas Sep 11, 2025

Choose a reason for hiding this comment

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

I think so. It is part of end-to-end of making IList.Contains work correctly for non-SZ arrays.

}

// For multi-dimensional arrays, fall back to enumeration.
foreach (object? element in this)
{
if (Equals(element, value))
{
return true;
}
}

return false;
}

void IList.Clear()
Expand Down
25 changes: 23 additions & 2 deletions src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public static void CastAs_IListOfT()
[Fact]
public static void Construction()
{
// Check a number of the simple APIs on Array for dimensions up to 4.
// Check a number of the simple APIs on Array for dimensions up to 4, first with value types, then reference types.
Array array = new int[] { 1, 2, 3 };
VerifyArray(array, typeof(int), new int[] { 3 }, new int[1]);

Expand All @@ -93,6 +93,18 @@ public static void Construction()

array = new int[2, 3, 4, 5];
VerifyArray(array, typeof(int), new int[] { 2, 3, 4, 5 }, new int[4]);

array = new string[] { "1", "2", "3" };
VerifyArray(array, typeof(string), new int[] { 3 }, new int[1]);

array = new string[,] { { "1", "2", "3" }, { "4", null, "6" } };
VerifyArray(array, typeof(string), new int[] { 2, 3 }, new int[2]);

array = new string[2, 3, 4];
VerifyArray(array, typeof(string), new int[] { 2, 3, 4 }, new int[3]);

array = new string[2, 3, 4, 5];
VerifyArray(array, typeof(string), new int[] { 2, 3, 4, 5 }, new int[4]);
}

[Fact]
Expand Down Expand Up @@ -4682,10 +4694,19 @@ private static void VerifyArrayAsIList(Array array)
}
else
{
Assert.Throws<RankException>(() => iList.Contains(null));
Assert.Throws<RankException>(() => iList.IndexOf(null));
AssertExtensions.Throws<ArgumentException>(null, () => iList[0]);
AssertExtensions.Throws<ArgumentException>(null, () => iList[0] = 1);

bool containsNull = false;
foreach (object? obj in array)
{
containsNull |= obj is null;
Assert.True(iList.Contains(obj), $"{obj} not found in {string.Join(",", array.Cast<object?>())}");
}

Assert.False(iList.Contains(new object()));
Assert.Equal(containsNull, iList.Contains(null));
}
}

Expand Down