Contains on generic IList/HashSet/ImmutableHashSet will throw exception

[andtii]

Describe what is not working as expected.

When using the interface IList instead of List and doing Contains you will get the exception

Exception message:
Operation is not valid due to the current state of the object.
Stack trace:

Steps to reproduce

This code will throw the exception since its an IList we use Contains against
  var rolesId = new List<Guid> {
                new Guid("cb6b6945-15d7-4596-b3cb-52ab3560600e"),
                new Guid("b5f963cd-a24a-45be-bc9f-60f30ac1af89"),
            } as IList<Guid>;

            var res = _dbSet
            .Where(r => rolesId.Contains(r.Id));

This code works since its a List
  var rolesId = new List<Guid> {
                new Guid("cb6b6945-15d7-4596-b3cb-52ab3560600e"),
                new Guid("b5f963cd-a24a-45be-bc9f-60f30ac1af89"),
            };

            var res = _dbSet
            .Where(r => rolesId.Contains(r.Id));

Further technical details

EF Core version: 3.0.0-preview8.19405.11
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10
IDE: Visual Studio 2019 16.3.0 Preview 2.0

[smitpatel]

Our current code indeed work on IList rather than List.
https://github.com/aspnet/EntityFrameworkCore/blob/cd072b6bd2bcab1d692377de3431c30405bc0c68/src/EFCore.Relational/Query/Internal/ContainsTranslator.cs#L33-L34
Would need investigation to understand why it is not translated.

[ajcvickers]

@andtii Can you post the stack trace?

[smitpatel]

So IList<T> does not implement IList but implements ICollection<T> hence we fail to translate.

[smitpatel]

And List implements IList<T> and IList both. I guess better match to check for IList.Contains

[alexmurari]

@smitpatel

This would be an viable solution?

else if (method.DeclaringType.GetInterfaces().Any(t => t == typeof(IList) || (t.IsGenericType && t.GetGenericTypeDefinition() == typeof(IList<>)))
                && string.Equals(method.Name, nameof(IList.Contains)))

I think that checking for either the generic and non-generic IList interfaces will do the job.

The only difference is the Contains method declaration:

• In the non-generic version, it's declared on itself (IList).
• In the generic version, it's declared on the ICollection<> interface.

This would make the nameof(IList.Contains) not accurately right since the method can come from different declaring types but shares the same name (hopefully forever 😄) and the nameof output is the name only: Contains.

[smitpatel]

else if (method.DeclaringType.GetInterfaces().Any(t => t == typeof(IList) || (t.IsGenericType && t.GetGenericTypeDefinition() == typeof(ICollection<>)))
                && string.Equals(method.Name, nameof(IList.Contains)))

[smitpatel]

Would use ICollection<> rather than IList<>.
ICollection.Contains kicks in for any collection which is generic on type.
IList.Contains kicks in for any collection which does not have generic constraint (like Array).
Name check part is fine.

[andreichuk]

@smitpatel the latest code (3.1 preview) still fails when Contains method of HashSet<>, ImmutableHashSet<> collections is used in LINQ.

I changed Translate method of Microsoft.EntityFrameworkCore.Query.Internal.ContainsTranslator class to support types that are IList or ICollection<> or implement those interfaces. Please check the code below:

public virtual SqlExpression Translate(SqlExpression instance, MethodInfo method, IReadOnlyList<SqlExpression> arguments)
{
    if (method.IsGenericMethod
        && method.GetGenericMethodDefinition().Equals(EnumerableMethods.Contains))
    {
        return _sqlExpressionFactory.In(arguments[1], arguments[0], false);
    }

    // early return
    if (string.Equals(method.Name, nameof(IList.Contains)) == false)
    {
        return null;
    }

    // check if it is or has IList interface
    if (typeof(IList).IsAssignableFrom(method.DeclaringType))
    {
        return _sqlExpressionFactory.In(arguments[0], instance, false);
    }

    // check if it is ICollection<> interface
    if (method.DeclaringType.IsGenericType
        && method.DeclaringType.GetGenericTypeDefinition() == typeof(ICollection<>))
    {
        return _sqlExpressionFactory.In(arguments[0], instance, false);
    }

    // check if it has ICollection<> interface
    var hasICollectionInterface =
        method.DeclaringType.GetInterfaces()
            .Where(t => t.IsGenericType)
            .Select(t => t.GetGenericTypeDefinition())
            .Any(t => t == typeof(ICollection<>));

    if (hasICollectionInterface)
    {
        return _sqlExpressionFactory.In(arguments[0], instance, false);
    }

    return null;
}

[smitpatel]

@roji - Can you look into above? We want to make sure that ContainsTranslator handles all generic collection types. (no need to put a lot of effort for exhaustive search, just the ones mentioned in this thread should be good enough)

[roji]

Sure @smitpatel, will do that tomorrow.

[roji]

@smitpatel ended up simply implementing what you originally proposed: #17342 (comment). Now any one-argument method names Contains on any type which implements ICollection<> will be matched.

Continuing to track via this issue, but tell me if you prefer a new one.