CSHARP-5959: Replace switch by method name in SerializerFinderVisitMethodCall with lookup table with MethodInfo as a key by sanych-sun · Pull Request #1961 · mongodb/mongo-csharp-driver

sanych-sun · 2026-04-22T21:09:57Z

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.

sanych-sun · 2026-04-22T21:10:47Z

        }
    }

+    // TODO: merge this and next methods.


I'll create a ticket for this, as these 2 methods looks like doing the same stuff.

Copilot

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) in SerializerFinderVisitor.VisitMethodCall with a MethodInfo -> deducer registry (including caching and generic-method normalization).
Added new StringMethod reflection entries for string.Equals instance/static variants and updated registrations accordingly.
Introduced EnumerableMethod.IsContainsMethod(MethodInfo) to support dynamic detection of Contains methods.

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 `MethodInfo`s 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.

sanych-sun · 2026-04-22T21:16:15Z

        }
-
-        void DeduceCompareOrCompareToMethodSerializers()
+        // DeduceAddToSetMethodSerializers


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.

sanych-sun · 2026-04-22T21:18:10Z

-
-        void DeduceCompareOrCompareToMethodSerializers()
+        // DeduceAddToSetMethodSerializers
+        private static void DeduceAddToSetMethodSerializers(SerializerFinderVisitor visitor, MethodCallExpression expression)


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.

sanych-sun · 2026-04-22T21:19:39Z

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


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.

sanych-sun · 2026-04-22T21:24:48Z

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


I'm about to investigate this problem. Will fix or remove the registration before merging the PR.

Don't forget about this!

adelinowona · 2026-04-24T21:29:28Z

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


Don't forget about this!

adelinowona · 2026-04-24T21:43:22Z

+    private static class MethodCallSerializerDeducers
+    {
+        private static readonly Dictionary<MethodInfo, Action<SerializerFinderVisitor, MethodCallExpression>> __serializerResolvers = new();
+        private static readonly ReaderWriterLockSlim __serializerResolversLock = new();


Why do we not use a ConcurrentDictionary instead of the whole read-lock / upgrade-to-write / recheck dance in GetSerializerDeducer?

Will do. Somehow I was thinking having Dictionary<> for "mostly readonly" access might be better, but apparently ConcurrentDictionary also handles this use case better then combination of Dictionary<> and ReadWriteLockSlim.
Thank you for the comment!

Done, however instead of simply use the GetOrAdd method, I've decided to use TryGetValue, then manually call CreateDynamicSerializerDeducer and if it returns not-null then use GetOrAdd. We need this to avoid caching of null values in the serializer deducers dictionary.

ajcvickers · 2026-04-27T09:59:35Z

Looks like a reasonable change to me.

damieng · 2026-04-27T12:08:39Z

        public static IReadOnlyMethodInfoSet ReverseOverloads => __reverseOverloads;

        // public methods
+        public static bool IsContainsMethod(MethodInfo method)


Is this going to also match String.Contains? A lot of providers typically special case excluding String.Contains as we don't want to handle looking for chars in a string in the same way we look for types in a containable list.

Nope, String.Contains has it's pre-registered deducer (see like 216).

damieng · 2026-04-27T12:10:44Z

        [TestHelpers.MakeLambda((MyModel model) => model.Value.Equals(1f)), typeof(BooleanSerializer)],
        [TestHelpers.MakeLambda((MyModel model) => model.Value.Equals(model.Other)), typeof(BooleanSerializer)],
-        [TestHelpers.MakeLambda((MyModel model) => float.Parse("42")), typeof(SingleSerializer)],
+        [TestHelpers.MakeLambda((MyModel model) => float.Parse(model.StringValue)), typeof(SingleSerializer)],


42 probably should have fractial parts for the float/decimal/double tests originally?

We need non-constant here, so the PartialEvaluator will not replace this with ConstantExpression.

adelinowona · 2026-04-27T19:33:54Z

+            static void DeduceToArrayMethodSerializers(SerializerFinderVisitor visitor, MethodCallExpression expression)
            {
-                DeduceUnknownMethodSerializer();
+                var sourceExpression = expression.Method.IsStatic ? expression.Arguments[0] : expression.Object;
+                visitor.DeduceCollectionAndCollectionSerializers(expression, sourceExpression);
            }
-        }

-        bool IsDictionaryContainsKeyExpression(out Expression keyExpression)
-        {
-            if (DictionaryMethod.IsContainsKeyMethod(method))
+            static void DeduceToListSerializers(SerializerFinderVisitor visitor, MethodCallExpression expression)
            {
-                keyExpression = arguments[0];
-                return true;
+                if (visitor.IsNotKnown(expression))
+                {
+                    var source = expression.Method.IsStatic ? expression.Arguments[0] : expression.Object;
+                    if (visitor.IsKnown(source, out var sourceSerializer))
+                    {
+                        var sourceItemSerializer = ArraySerializerHelper.GetItemSerializer(sourceSerializer);
+                        var resultSerializer = ListSerializer.Create(sourceItemSerializer);
+                        visitor.AddNodeSerializer(expression, resultSerializer);
+                    }
+                }
            }


Why are DeduceToArray and DeduceToList so different? Shouldn't they have similar bodies?

It's an interesting question, but it's out of this PR scope. I'll double-check if the method was migrated/moved properly and can create a separate ticket for the investigation if you want.

adelinowona · 2026-04-27T19:58:31Z

+                "ToList" => DeduceToListSerializers,
+                "ToString" => (visitor, expression) => visitor.DeduceSerializer(expression, StringSerializer.Instance),


why don't these have guards like the others?

Do you mean checking if node is known already? Because DeduceSerializer is doing that for us + we do not create any new serializers here, but passing pre-created one.

adelinowona · 2026-05-04T19:29:19Z

            {
-                DeduceUnknownMethodSerializer();
+                return null;


I believe the old code was caching this null case. We don't need to do that anymore?

…thodCall with lookup table with MethodInfo as a key

Copilot AI review requested due to automatic review settings April 22, 2026 21:09

sanych-sun requested a review from a team as a code owner April 22, 2026 21:09

sanych-sun requested a review from ajcvickers April 22, 2026 21:09

Copilot started reviewing on behalf of sanych-sun April 22, 2026 21:10 View session

sanych-sun commented Apr 22, 2026

View reviewed changes

sanych-sun requested a review from adelinowona April 22, 2026 21:11

Copilot AI reviewed Apr 22, 2026

View reviewed changes

sanych-sun commented Apr 22, 2026

View reviewed changes

sanych-sun added the improvement Optimizations or refactoring (no new features or fixes). label Apr 22, 2026

sanych-sun marked this pull request as draft April 22, 2026 22:33

adelinowona reviewed Apr 24, 2026

View reviewed changes

damieng reviewed Apr 27, 2026

View reviewed changes

adelinowona requested changes Apr 27, 2026

View reviewed changes

sanych-sun requested a review from adelinowona April 28, 2026 20:44

adelinowona reviewed May 4, 2026

View reviewed changes

adelinowona mentioned this pull request May 19, 2026

WIP: Add LINQ translation benchmark suite #2004

Draft

5 tasks

sanych-sun added 6 commits May 20, 2026 11:13

tmp

3f9bf1e

CSHARP-5959: Replace switch by method name in SerializerFinderVisitMe…

5308000

…thodCall with lookup table with MethodInfo as a key

Fix wrong rebase

b3ae703

pr

29c46fe

pr

146f78a

Lookup performance optimization

54973d0

sanych-sun force-pushed the csharp5959 branch from 0abfc30 to 54973d0 Compare May 20, 2026 19:24

		"ToList" => DeduceToListSerializers,
		"ToString" => (visitor, expression) => visitor.DeduceSerializer(expression, StringSerializer.Instance),

Conversation

sanych-sun commented Apr 22, 2026 • 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

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!

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!

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!

ajcvickers commented Apr 27, 2026

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanych-sun Apr 28, 2026 • 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sanych-sun commented Apr 22, 2026 •

edited

Loading

sanych-sun Apr 28, 2026 •

edited

Loading