-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-5793: Support Enumerable Contains and SequenceEqual with null comparer #1828
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.
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
base: main
Are you sure you want to change the base?
Conversation
src/MongoDB.Driver/Linq/Linq3Implementation/Misc/ClrCompatExpressionRewriter.cs
Outdated
Show resolved
Hide resolved
| results.Select(x => x.Id).Should().Equal(2, 3); | ||
| } | ||
|
|
||
| [Fact] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new tests are fine.
I would also add these new tests:
[Fact]
public void Enumerable_Contains_with_null_comparer_should_work()
{
var collection = Fixture.Collection;
var names = new[] { "Two", "Three" };
var queryable = collection.AsQueryable().Where((C x) => names.Contains(x.Name, null));
var results = queryable.ToArray();
results.Select(x => x.Id).Should().Equal(2, 3);
}
[Fact]
public void Enumerable_SequenceEqual_with_null_comparer_work()
{
var collection = Fixture.Collection;
var ratings = new[] { 1, 9, 6 };
var queryable = collection.AsQueryable().Where((C x) => ratings.SequenceEqual(x.Ratings, null));
var results = queryable.ToArray();
results.Select(x => x.Id).Should().Equal(3);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
If you agree with my requested changes I would change the description of CSHARP-5793 to: Support Enumerable Contains and SequenceEqual with null comparer It's a more general fix, because not only does it address the C# 14 issue but it also adds support for |
| var arguments = expression.Arguments; | ||
|
|
||
| if (method.IsOneOf(EnumerableMethod.Contains, QueryableMethod.Contains)) | ||
| if (method.IsOneOf(EnumerableMethod.Contains, QueryableMethod.Contains) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct.
We want to go into the then clause for the WithComparer methods and throw an exception if the comparer is not null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I suggest creating static readonly fields __containsMethods and __containsWithComparerMethods to shorten the code and avoid extra array creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to do that? That's a behavioral change and a custom message - it should be fine with the current message it already displays today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the cached arrays and also did the same thing in the ClrCompat visitor too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave the custom/alternate error message for cases where a non-null comparer is passed for now.
|
|
||
| if (method.IsOneOf(EnumerableMethod.SequenceEqual, QueryableMethod.SequenceEqual)) | ||
| if (method.IsOneOf(EnumerableMethod.SequenceEqual, QueryableMethod.SequenceEqual) | ||
| || method.IsOneOf(EnumerableMethod.SequenceEqualWithComparer, QueryableMethod.SequenceEqualWithComparer) && arguments[2] is ConstantExpression { Value: null }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for handling Contains and SequenceEqual methods when they are called with a null comparer argument. This scenario arises in .NET 10 with C# 14 when using non-IEquatable types, where the compiler automatically uses 3-argument overloads of MemoryExtensions.Contains and MemoryExtensions.SequenceEqual with null as the comparer.
Key Changes:
- Modified translators to treat 3-arg Contains/SequenceEqual with null comparer the same as 2-arg versions
- Added QueryableMethod reflection definitions for ContainsWithComparer and SequenceEqualWithComparer
- Refactored method matching in ClrCompatExpressionRewriter to use static arrays for better maintainability
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp5749Tests.cs | Added test methods for enum-based Contains/SequenceEqual and null comparer scenarios; updated test fixture data model with Day and Days properties |
| src/MongoDB.Driver/Linq/Linq3Implementation/Translators/ExpressionToAggregationExpressionTranslators/MethodTranslators/SequenceEqualMethodToAggregationExpressionTranslator.cs | Added static method arrays and logic to handle 3-arg SequenceEqual with null comparer |
| src/MongoDB.Driver/Linq/Linq3Implementation/Translators/ExpressionToAggregationExpressionTranslators/MethodTranslators/ContainsMethodToAggregationExpressionTranslator.cs | Added static method arrays and logic to handle 3-arg Contains with null comparer |
| src/MongoDB.Driver/Linq/Linq3Implementation/Reflection/QueryableMethod.cs | Added ContainsWithComparer and SequenceEqualWithComparer method reflection definitions |
| src/MongoDB.Driver/Linq/Linq3Implementation/Misc/ClrCompatExpressionRewriter.cs | Refactored to use static readonly MethodInfo arrays instead of inline method checks for better performance and readability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ressionTranslators/MethodTranslators/SequenceEqualMethodToAggregationExpressionTranslator.cs
Outdated
Show resolved
Hide resolved
...onExpressionTranslators/MethodTranslators/ContainsMethodToAggregationExpressionTranslator.cs
Outdated
Show resolved
Hide resolved
tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp5749Tests.cs
Outdated
Show resolved
Hide resolved
…sionToAggregationExpressionTranslators/MethodTranslators/SequenceEqualMethodToAggregationExpressionTranslator.cs Co-authored-by: Copilot <[email protected]>
…sionToAggregationExpressionTranslators/MethodTranslators/ContainsMethodToAggregationExpressionTranslator.cs Co-authored-by: Copilot <[email protected]>
…p5749Tests.cs Co-authored-by: Copilot <[email protected]>
Handles the scenario where, when targetting .NET 10 and using C# 14 with a non-IEquatable type will use the 3-arg versions of
MemoryExtensions.ContainsandMemoryExtensions.SequenceEqualspassingnullas the comparer.Implementation
Now just rewrites the 2 arg versions and the 3 arg version when it is null and writes that back to the Enumerable 2 args version as our C# Provider does not handle the 3-arg versions at all.
Did this for
ContainsandSequenceEqualsafter confirming with Shay that EF probably should have the null/3-arg version remapped for SequenceEquals too.Notes
Unlike the other 2-arg versions we can't test these directly without switching to .NET 10. The tests will for now not exercise that path but will do so when we switch to .NET 10 and C# in the future.
They have been tested with an external project. We can't add that yet due to CI limitations.
Fixes CSHARP-5749.