CSHARP-5959: Replace switch by method name in SerializerFinderVisitMethodCall with lookup table with MethodInfo as a key#1961
Conversation
…thodCall with lookup table with MethodInfo as a key
| } | ||
| } | ||
|
|
||
| // TODO: merge this and next methods. |
There was a problem hiding this comment.
I'll create a ticket for this, as these 2 methods looks like doing the same stuff.
There was a problem hiding this comment.
Pull request overview
This PR refactors LINQ3 serializer deduction for MethodCallExpression nodes by replacing a large name-based switch with a MethodInfo-keyed lookup table (plus a small dynamic fallback) to improve maintainability and correctness around overload resolution.
Changes:
- Replaced
switch (node.Method.Name)inSerializerFinderVisitor.VisitMethodCallwith aMethodInfo -> deducerregistry (including caching and generic-method normalization). - Added new
StringMethodreflection entries forstring.Equalsinstance/static variants and updated registrations accordingly. - Introduced
EnumerableMethod.IsContainsMethod(MethodInfo)to support dynamic detection ofContainsmethods.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderVisitMethodCall.cs |
Major refactor: method serializer deduction now uses a MethodInfo lookup table with dynamic fallback and caching. |
src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderHelperMethods.cs |
Adds a TODO comment around helper-method duplication. |
src/MongoDB.Driver/Linq/Linq3Implementation/Reflection/StringMethod.cs |
Adds reflection MethodInfos for string.Equals (instance + static) and exposes them. |
src/MongoDB.Driver/Linq/Linq3Implementation/Reflection/EnumerableOrQueryableMethod.cs |
Removes the AnyOverloads set (now uses Any/AnyWithPredicate sets directly). |
src/MongoDB.Driver/Linq/Linq3Implementation/Reflection/EnumerableMethod.cs |
Adds IsContainsMethod(MethodInfo) to support dynamic Contains deduction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (var i = 1; i <= expression.Arguments.Count; i++) | ||
| { | ||
| var argumentExpression = expression.Arguments[i]; | ||
| if (visitor.IsNotKnown(argumentExpression)) | ||
| { | ||
| var itemSerializer = tupleSerializer.GetItemSerializer(i); | ||
| if (i == 8) |
| } | ||
|
|
||
| void DeduceCompareOrCompareToMethodSerializers() | ||
| // DeduceAddToSetMethodSerializers |
There was a problem hiding this comment.
I've left such comments so for now, for review purposes. So it would be easier to find where the implementation came from. I'll remove them before merging.
|
|
||
| void DeduceCompareOrCompareToMethodSerializers() | ||
| // DeduceAddToSetMethodSerializers | ||
| private static void DeduceAddToSetMethodSerializers(SerializerFinderVisitor visitor, MethodCallExpression expression) |
There was a problem hiding this comment.
I've moved all deducers from local method to static-class level methods, because codecov is struggling to show the code coverage for local methods. As the result I can see which deducers are not covered by tests. Probably I need to create more test cases to improve code coverage.
| var method = node.Method; | ||
| var arguments = node.Arguments; | ||
| var methodInfo = node.Method.IsGenericMethod && !node.Method.ContainsGenericParameters ? node.Method.GetGenericMethodDefinition() : node.Method; | ||
| var deducer = MethodCallSerializerDeducers.GetSerializerDeducer(methodInfo); |
There was a problem hiding this comment.
I've decided to move all deducers implementation into private static class simply to clear the "entry" point, to have this method on the very top and visible.
| RegisterSerializerDeducers(EnumerableOrQueryableMethod.ElementAtOverloads, (visitor, expression) => visitor.DeduceItemAndCollectionSerializers(expression, expression.Arguments[0])); | ||
| RegisterSerializerDeducers(EnumerableOrQueryableMethod.Except, DeduceExceptMethodSerializers); | ||
| // TODO: Definitely wrong registration copy-pasted from prev code, investigate before merge to main | ||
| // RegisterSerializerResolver(EnumerableMethod.First, DeduceSetWindowFieldsMethodSerializers); |
There was a problem hiding this comment.
I'm about to investigate this problem. Will fix or remove the registration before merging the PR.
sanych-sun
added
the
improvement
The idea of the PR is to replace the switch by string method's name and checking of methodInfo, with the dictionary of deducers by method info. Most of the use-cases is covered by "static" registration, however there are number of use-cases where we duck-typing the method call (see CreateDynamicSerializerDeducer method), in such case we do dynamic registration as we go: so the next translation of the method will use "previously registered deducer" and do not need to go through all checks.