Fix IList.Contains to work with multidimensional arrays #119583

stephentoub · 2025-09-11T15:05:31Z

It currently fails because it uses IndexOf, but there's no fundamental reason this shouldn't work with multidimensional arrays, and it breaks expectations that it doesn't work. We can just fall back to enumerating if the Rank isn't 1.

Fixes #119569

We need to fix the issue in 10, and fixing Array itself seems like the right answer. If there are objections to backporting this fix, please let me know. This is taking something that previously always threw an exception and now makes it work. (There are probably a couple more array interface implementations we could fix subsequently to work with multidimensional arrays, as well, e.g. CopyTo, a problem we've bumped into from LINQ as well in the past.)

It currently fails because it uses IndexOf, but there's no fundamental reason this shouldn't work with multidimensional arrays, and it breaks expectations that it doesn't work. We can just fall back to enumerating if the Rank isn't 1.

Copilot

Pull Request Overview

This PR fixes the IList.Contains method to work with multidimensional arrays. Previously, Contains always failed for multidimensional arrays because it relied on IndexOf, which only supports single-dimensional arrays. The fix implements a fallback enumeration approach for multidimensional arrays while maintaining the efficient IndexOf-based implementation for single-dimensional arrays.

Key changes:

Modified Array.IList.Contains to check array rank and use enumeration for multidimensional arrays
Added comprehensive test coverage for both value and reference type multidimensional arrays
Added LINQ integration test to verify the fix works with existing LINQ operations

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/libraries/System.Private.CoreLib/src/System/Array.cs	Core fix implementing rank check and enumeration fallback for multidimensional arrays
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs	Enhanced test coverage for multidimensional arrays and IList.Contains behavior
src/libraries/System.Linq/tests/OfTypeTests.cs	Integration test verifying LINQ operations work with multidimensional arrays

eiriktsarpalis

Thanks

dotnet-policy-service · 2025-09-11T15:08:43Z

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

src/libraries/System.Linq/tests/OfTypeTests.cs

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs

…/ArrayTests.cs

jkotas · 2025-09-11T15:30:39Z

src/libraries/System.Private.CoreLib/src/System/Array.cs

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


Should this be fixed in IndexOf instead?

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

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.

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

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.

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.

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 :)

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

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.

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.

jkotas · 2025-09-11T16:12:01Z

We need to fix the issue in 10

Why do we need to fix this issue in 10? Was there some change in Linq that exposed this bug?

jkotas · 2025-09-11T16:17:08Z

Ok, I read the issue - this was exposed by #119569 (comment) .

Have we looked around for other IList implementations with the same problem?

The safe fix for .NET 10 would be to revert the optimization.

stephentoub · 2025-09-11T16:17:30Z

Was there some change in Linq that exposed this bug?

Yes, OfType().Contains() uses IList.Contains now. I'd rather fix Array's broken implementation than revert the optimization.

stephentoub · 2025-09-11T16:18:55Z

Have we looked around for other IList implementations with the same problem?

It's rarely used in LINQ, which is mostly based on IEnumerable<T>; the only two operators that work with the non-generic IEnumerable are Cast and OfType. I searched for other IList uses that would be problematic and didn't see any.

skyoxZ · 2025-09-11T16:32:59Z

runtime/src/libraries/System.Linq/src/System/Linq/OfType.SpeedOpt.cs

Lines 178 to 188 in 3823740

    
           public override bool Contains(TResult value) 
        
           { 
        
               // Avoid checking for IList when size-optimized because it keeps IList 
        
               // implementations which may otherwise be trimmed. Since List<T> implements 
        
               // IList and List<T> is popular, this could potentially be a lot of code. 
        
               if (!IsSizeOptimized && 
        
                   !typeof(TResult).IsValueType && // don't box TResult 
        
                   _source is IList list) 
        
               { 
        
                   return list.Contains(value); 
        
               }

This optimization seems problematic. The code below correctly outputs True in .NET 9 but now it outputs False.

using System;
using System.Collections.Generic;
using System.Linq;

List<D> source = new List<D> { new D(1) };
D d2 = new(2);
Console.WriteLine(source.OfType<S>().Contains(d2));


class S : IEquatable<S>
{
    public bool Equals(S? other) => other is not null;

    public override bool Equals(object? obj) => obj is S;

    public override int GetHashCode() => 0;
}

class D : S, IEquatable<D>
{
    public D(int x) => X = x;

    public int X { get; }

    public bool Equals(D? other) => other is not null && X == other.X;

    public override bool Equals(object? obj) => obj is D d && Equals(d);

    public override int GetHashCode() => X;
}

tannergooding · 2025-09-11T16:39:06Z

That example looks to violate the implementation requirements of GetHashCode and Equals, no?

jkotas · 2025-09-11T16:50:47Z

src/libraries/System.Private.CoreLib/src/System/Array.cs

+            // IndexOf only works for single-dimensional arrays.
+            if (Rank == 1)
+            {
+                return IndexOf(this, value) >= GetLowerBound(0);


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.

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);

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

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

skyoxZ · 2025-09-11T17:01:05Z

That example looks to violate the implementation requirements of GetHashCode and Equals, no?

https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-object-equals#notes-for-inheritors

https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-object-gethashcode

https://learn.microsoft.com/en-us/dotnet/api/system.object.gethashcode?view=net-9.0#notes-to-inheritors

Violate what?

tannergooding · 2025-09-11T17:10:02Z

Violate what?

The issue here is in:

Console.WriteLine(((S)source[0]).Equals(d2));
vs
Console.WriteLine(source[0].Equals(d2));

You've defined two different IEquatable<T> implementations for the same hierarchy and not unified them, which means that x.Equals(y) will return true for two objects that return different hash codes (1 vs 2) and violates the requirements defined.

For IEquatable<T> in particular that is https://learn.microsoft.com/en-us/dotnet/api/system.iequatable-1?view=net-9.0#notes-to-implementers, which basically boils down to Equals(object), IEquatable<T>.Equals(T) and GetHashCode() all being part of a unified thing and needing to be self consistent between eachother; otherwise bugs like you've listed can and will occur.

skyoxZ · 2025-09-11T18:04:21Z

@tannergooding you are right. What about this example:

using System;
using System.Collections.Generic;
using System.Linq;

List<S> source = new List<S> { new S() };
Console.WriteLine(source.OfType<D>().Contains(new D(1)));

class S
{
    public bool Equals(S? other) => other is not null;

    public override bool Equals(object? obj) => obj is S;

    public override int GetHashCode() => 0;
}

class D : S
{
    public D(int x) => X = x;

    public int X { get; }

    public bool Equals(D? other) => other is not null && X == other.X;

    public override bool Equals(object? obj) => obj is D d && Equals(d);

    public override int GetHashCode() => X;
}

tannergooding · 2025-09-11T18:14:03Z

That correctly reports false. source.OfType<D>() returns 0 items as it only contains a single item of type S and so the contains check fails.

Contains will only use object.Equals or IEquatable<T>.Equals for comparison. It won't use an arbitrary user-defined method named Equals. (excepting a custom IEqualityComparer being passed in, but that then must follow the same rules/guidance).

When implementing either object.Equals or IEquatable<T>.Equals, they are required to be consistent and that includes correctly having the right virtual declarations for any of the APIs that might be used here so they can be overridden by a derived type to remain correct.

You can write all kinds of incorrect code and invalid implementations that violate this, but the bug for that is on the type author not on LINQ or the built-in collection types.

skyoxZ · 2025-09-11T18:23:10Z

That correctly reports false. source.OfType() returns 0 items as it only contains a single item of type S and so the contains check fails.

The output is True in .NET 10.

tannergooding · 2025-09-11T18:37:16Z

The output is True in .NET 10.

I see.

That looks like a bug in OfTypeIterator.Contains then, yes. It's directly grabbing the underlying list and not filtering based on whether the items are of type TResult, which could cause items to unnecessarily be returned.

CC. @stephentoub

stephentoub · 2025-09-11T20:30:31Z

Replaced by #119600

stephentoub · 2025-09-11T22:44:27Z

@tannergooding you are right. What about this example:

using System;
using System.Collections.Generic;
using System.Linq;

List<S> source = new List<S> { new S() };
Console.WriteLine(source.OfType<D>().Contains(new D(1)));

class S
{
    public bool Equals(S? other) => other is not null;

    public override bool Equals(object? obj) => obj is S;

    public override int GetHashCode() => 0;
}

class D : S
{
    public D(int x) => X = x;

    public int X { get; }

    public bool Equals(D? other) => other is not null && X == other.X;

    public override bool Equals(object? obj) => obj is D d && Equals(d);

    public override int GetHashCode() => X;
}

That example is also problematic. x.Equals(y) != y.Equals(x)

stephentoub added this to the 10.0.0 milestone Sep 11, 2025

stephentoub requested review from Copilot and jkotas September 11, 2025 15:05

Copilot AI reviewed Sep 11, 2025

View reviewed changes

github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 11, 2025

dotnet-policy-service bot assigned stephentoub Sep 11, 2025

eiriktsarpalis approved these changes Sep 11, 2025

View reviewed changes

stephentoub added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 11, 2025

stephentoub commented Sep 11, 2025

View reviewed changes

src/libraries/System.Linq/tests/OfTypeTests.cs Outdated Show resolved Hide resolved

Update src/libraries/System.Linq/tests/OfTypeTests.cs

8b83939

stephentoub commented Sep 11, 2025

View reviewed changes

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs Outdated Show resolved Hide resolved

Update src/libraries/System.Runtime/tests/System.Runtime.Tests/System…

e443b70

…/ArrayTests.cs

jkotas reviewed Sep 11, 2025

View reviewed changes

tannergooding approved these changes Sep 11, 2025

View reviewed changes

jkotas reviewed Sep 11, 2025

View reviewed changes

stephentoub closed this Sep 11, 2025

github-actions bot locked and limited conversation to collaborators Oct 12, 2025

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

Fix IList.Contains to work with multidimensional arrays #119583

Fix IList.Contains to work with multidimensional arrays #119583

Uh oh!

Conversation

stephentoub commented Sep 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

eiriktsarpalis left a comment

Choose a reason for hiding this comment

Uh oh!

dotnet-policy-service bot commented Sep 11, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eiriktsarpalis Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Sep 11, 2025

Uh oh!

jkotas commented Sep 11, 2025

Uh oh!

stephentoub commented Sep 11, 2025

Uh oh!

stephentoub commented Sep 11, 2025

Uh oh!

skyoxZ commented Sep 11, 2025

Uh oh!

tannergooding commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skyoxZ commented Sep 11, 2025

Uh oh!

tannergooding commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skyoxZ commented Sep 11, 2025

Uh oh!

tannergooding commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skyoxZ commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented Sep 11, 2025

Uh oh!

eiriktsarpalis Sep 11, 2025 •

edited

Loading

stephentoub Sep 11, 2025 •

edited

Loading

stephentoub Sep 11, 2025 •

edited

Loading

tannergooding commented Sep 11, 2025 •

edited

Loading

tannergooding commented Sep 11, 2025 •

edited

Loading

tannergooding commented Sep 11, 2025 •

edited

Loading

skyoxZ commented Sep 11, 2025 •

edited

Loading