CSHARP-5572: Implement new KnownSerializerFinder #1700

rstam · 2025-05-27T18:33:09Z

No description provided.

Szmiglo · 2025-07-23T09:02:43Z

hi @rstam, what's the status of this effort?

rstam · 2025-07-23T15:47:16Z

hi @rstam, what's the status of this effort?

It is progressing well, but it is a large effort. I work on this whenever I'm not working on some smaller higher priority task.

adelinowona · 2025-10-16T06:42:22Z

src/MongoDB.Driver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerMap.cs

+            }
+        }
+
+        if (_map.TryGetValue(node, out var existingSerializer))


I think we should move the duplicate check to the beginning of the AddSerializer method to implement fail-fast behavior. This would avoid unnecessary serializer transformation work when a serializer already exists for the node.

However, I notice the current implementation places this check at the end, likely to preserve information about the conflicting serializer for the exception. While this debugging information could be helpful, I question just how useful it is. Knowing where a duplicate attempt happened seems more useful knowing what the conflicting serializer would have been.

I lean toward the fail-fast approach since the conflicting serializer details seem like a "nice-to-have" rather than essential debugging information.

If we move the error check further up then we have to remove information about the redundant serializer from the error message.

Also, I don't think we need to worry about failing fast because we actually never expect to hit this error. This check is here to help us find bugs. In the absence of bugs this exception will never be thrown.

adelinowona · 2025-10-16T07:00:35Z

...ngoDB.Driver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitor.cs

+    private bool _isMakingProgress = true;
+    private readonly KnownSerializerMap _knownSerializers;
+    private int _oldKnownSerializersCount = 0;
+    private int _pass = 0;


Seems like this is never used?

adelinowona · 2025-10-16T07:11:12Z

...ngoDB.Driver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitor.cs

+        var newKnownSerializersCount = _knownSerializers.Count;
+        if (newKnownSerializersCount == _oldKnownSerializersCount)
+        {
+            if (_useDefaultSerializerForConstants)


Could we maybe make use of a state enum as such:

private enum DiscoveryPhase { InferringFromContext, // Not using defaults UsingDefaultSerializers, // Last attempt with defaults Complete // No more progress possible }

to replace the use of all these booleans used in keeping track of the state of the visitor?

It's somewhat reasonable, but I think the booleans work better here.

There are only 2 booleans.

One issue is that if we ever had more states the order of the enum values could get very confusing and maybe even no order is possible.

sanych-sun

Partial review.

sanych-sun · 2025-11-25T19:31:13Z

src/MongoDB.Driver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinder.cs

+
+namespace MongoDB.Driver.Linq.Linq3Implementation.KnownSerializerFinders;
+
+internal static class KnownSerializerFinder


Term Known in the class name suggests there might be unknown serializers. But as far as I remember from our meetings currently we make sure each expression MUST have serializer assigned. May be we should rename it to SerializerFinder?

sanych-sun · 2025-11-25T20:18:53Z

src/MongoDB.Driver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerMap.cs

+
+    public bool IsKnown(Expression node)
+    {
+        return node != null && _map.ContainsKey(node);


Is there any real use cases when we pass node=null? Sounds like we should throw instead.

sanych-sun · 2025-11-25T20:51:41Z

...iver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitMethodCall.cs

+                DeduceUnknownMethodSerializer();
+            }
+
+            bool IsCollectionContainsMethod(out Expression collectionExpression, out Expression itemExpression)


We already have similar detection in at least 2 places:
ContainsMethodToAggregationExpressionTranslator,
ContainsMethodToFilterTranslator

Now it's going to be 3rd. Should we consolidate this into some helper method?

sanych-sun · 2025-11-25T20:54:37Z

...iver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitMethodCall.cs

+                DeduceUnknownMethodSerializer();
+            }
+
+            bool IsContainsValueInstanceMethod(out Expression collectionExpression, out Expression valueExpression)


We have very similar logic in:
ContainsValueMethodToFilterTranslator,
ContainsValueMethodToAggregationExpressionTranslator

Now it's going to be 3rd. Should we consolidate this into some helper method?

sanych-sun · 2025-11-25T21:00:59Z

...iver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitMethodCall.cs

+                DeduceUnknownMethodSerializer();
+            }
+
+            static bool IsSetEqualsMethod(MethodInfo method)


Similar checks are done in:
SetEqualsMethodToAggregationExpressionTranslator,

Should we consolidate into some helper method?

sanych-sun · 2025-11-25T21:26:19Z

...Driver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderHelperMethods.cs

+            _ when collectionType.IsArray => ArraySerializer.Create(itemSerializer),
+            _ when collectionType.IsConstructedGenericType && collectionType.GetGenericTypeDefinition() == typeof(IEnumerable<>) => IEnumerableSerializer.Create(itemSerializer),
+            _ when collectionType.IsConstructedGenericType && collectionType.GetGenericTypeDefinition() == typeof(IOrderedEnumerable<>) => IOrderedEnumerableSerializer.Create(itemSerializer),
+            _ when collectionType.IsConstructedGenericType && collectionType.GetGenericTypeDefinition() == typeof(IQueryable<>) => IQueryableSerializer.Create(itemSerializer),


Same for IQueryable: it implements IEnumerable

sanych-sun · 2025-11-25T22:06:05Z

...ver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitConditional.cs

+
+        return node;
+
+        void DeduceConditionaSerializers()


Typo: "ConditionaL"

sanych-sun · 2025-11-25T22:09:53Z

...DB.Driver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitIndex.cs

+        bool IsDictionaryIndexer()
+        {
+            return
+                indexer.DeclaringType.Name.Contains("Dictionary") &&


Are we really need to validate the class name? Is there any better way to do that? For example Hashtable or SortedList implements IDictionary, but it has no Dictionary in the name.

sanych-sun · 2025-11-25T22:18:25Z

...B.Driver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitMember.cs

+
+        bool IsTupleOrValueTuple(Type type)
+        {
+            return


For net6 and higher there is a better way to check: both Tuple and ValueTuple implements ITuple interface. I suppose we can have #if def here and use interface approach for modern targets, when have something like the proposed code for net472.

sanych-sun · 2025-11-25T22:22:25Z

...DB.Driver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitUnary.cs

+                    throw new ExpressionNotSupportedException(expression, because: "target type is not an enum");
+                }
+
+                // if (sourceSerializer is IHasRepresentationSerializer sourceHasRepresentationSerializer &&


Do we need this commented code?

Copilot

Pull request overview

This PR implements a new KnownSerializerFinder system to replace the previous approach of dynamically determining serializers during expression translation. The new system pre-analyzes expressions to build a map of known serializers for each expression node, enabling more reliable and consistent serializer resolution throughout the translation process.

Key Changes:

Introduced a comprehensive KnownSerializerFinder infrastructure that pre-computes serializers for expression nodes
Updated TranslationContext to accept and use the pre-computed serializer map
Refactored multiple translators to leverage the new known serializer system
Added several new serializer types to support the enhanced type inference

Reviewed changes

Copilot reviewed 98 out of 99 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`KnownSerializerFinder*.cs`	New visitor-based system for discovering and mapping serializers to expression nodes
`TranslationContext.cs`	Extended to store and provide access to known serializers map
`LinqProviderAdapter.cs`	Updated all translation entry points to use the new KnownSerializerFinder
`*Serializer.cs`	Added new serializer types (Polymorphic, Upcasting, Unknowable, etc.) to support enhanced type inference
Test files	Updated test contexts to use new TranslationContext.Create signatures

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-25T22:23:36Z

...Driver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitListInit.cs

+        DeduceSerialiers();
+        base.VisitListInit(node);
+        DeduceSerialiers();
+
+        return node;
+
+        void DeduceSerialiers()


Corrected spelling of 'DeduceSerialiers' to 'DeduceSerializers'.

Suggested change

DeduceSerialiers();

base.VisitListInit(node);

DeduceSerialiers();

return node;

void DeduceSerialiers()

DeduceSerializers();

base.VisitListInit(node);

DeduceSerializers();

return node;

void DeduceSerializers()

Copilot · 2025-11-25T22:23:36Z

...ver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitConditional.cs

+        DeduceConditionaSerializers();
+        base.VisitConditional(node);
+        DeduceConditionaSerializers();
+
+        return node;
+
+        void DeduceConditionaSerializers()


Corrected spelling of 'DeduceConditionaSerializers' to 'DeduceConditionalSerializers'.

Suggested change

DeduceConditionaSerializers();

base.VisitConditional(node);

DeduceConditionaSerializers();

return node;

void DeduceConditionaSerializers()

DeduceConditionalSerializers();

base.VisitConditional(node);

DeduceConditionalSerializers();

return node;

void DeduceConditionalSerializers()

Copilot · 2025-11-25T22:23:37Z

...DB.Driver/Linq/Linq3Implementation/KnownSerializerFinders/KnownSerializerFinderVisitUnary.cs

+                    return sourceSerializer;
+                }
+
+                // handle conversionsn to BsonValue before any others


Corrected spelling of 'conversionsn' to 'conversions'.

Suggested change

// handle conversionsn to BsonValue before any others

// handle conversion to BsonValue before any others

Copilot · 2025-11-25T22:23:37Z

src/MongoDB.Driver/Linq/Linq3Implementation/KnownSerializerFinders/UnknownSerializerFinder.cs

+        _knownSerializers = knownSerializers;
+    }
+
+    public Expression ExpressionWithUnknownSerialier => _expressionWithUnknownSerializer;


Corrected spelling of 'ExpressionWithUnknownSerialier' to 'ExpressionWithUnknownSerializer'.

Suggested change

public Expression ExpressionWithUnknownSerialier => _expressionWithUnknownSerializer;

public Expression ExpressionWithUnknownSerializer => _expressionWithUnknownSerializer;

rstam requested review from adelinowona and sanych-sun May 27, 2025 18:33

rstam added bug Fixes issues or unintended behavior. improvement Optimizations or refactoring (no new features or fixes). labels May 27, 2025

rstam changed the title ~~CSHARP-5572: Implement new KnownSerializerFinder.~~ CSHARP-5572: Implement new KnownSerializerFinder May 27, 2025

rstam force-pushed the csharp5572 branch from 89c78f2 to bd9bd3e Compare August 21, 2025 15:24

rstam requested review from a team and removed request for a team August 21, 2025 15:25

rstam force-pushed the csharp5572 branch from bd9bd3e to 8228c21 Compare September 30, 2025 01:13

rstam force-pushed the csharp5572 branch 2 times, most recently from 86fbb24 to 56a464c Compare October 13, 2025 15:15

adelinowona reviewed Oct 16, 2025

View reviewed changes

rstam force-pushed the csharp5572 branch from 56a464c to 32aae48 Compare October 28, 2025 18:30

rstam and others added 13 commits October 29, 2025 11:42

CSHARP-5572: Implement new KnownSerializerFinder.

defc892

stash

5963931

Work in progress.

1beb847

Work in progress.

fbaf046

Work in progress.

4d572d5

Work in progress.

bd1abbb

Work in progress.

e91d9be

Work in progress.

dd956e6

Work in progress.

36a7753

Work in progress.

e0898a3

Work in progress.

69705b6

Zero tests failing! But more refactoring to do.

35bd8b9

CSHARP-5572: Fixed some issues with KeyValuePair.

f2124f1

rstam added 8 commits October 29, 2025 11:42

Use KeyValuePairSerializer.

363b72d

stash

728bdf8

Fix issues caused by rebasing on main.

e9339e9

CSHARP-5572: Refactoring ready for review.

0d53f9c

CSHARP=5572: Fix SelectMany after rebasing on main.

9358835

stash

27a540e

CSHARP-5572: Refactoring.

8f4b081

CSHARP-5572: Minor refactoring.

7f2a64c

rstam force-pushed the csharp5572 branch from 32aae48 to 7f2a64c Compare October 31, 2025 15:58

sanych-sun marked this pull request as ready for review November 25, 2025 22:22

sanych-sun requested a review from a team as a code owner November 25, 2025 22:22

Copilot AI review requested due to automatic review settings November 25, 2025 22:22

sanych-sun requested changes Nov 25, 2025

View reviewed changes

Copilot AI reviewed Nov 25, 2025

View reviewed changes


		namespace MongoDB.Driver.Linq.Linq3Implementation.KnownSerializerFinders;

		internal static class KnownSerializerFinder

	// handle conversionsn to BsonValue before any others
	// handle conversion to BsonValue before any others

	public Expression ExpressionWithUnknownSerialier => _expressionWithUnknownSerializer;
	public Expression ExpressionWithUnknownSerializer => _expressionWithUnknownSerializer;

CSHARP-5572: Implement new KnownSerializerFinder #1700

Are you sure you want to change the base?

CSHARP-5572: Implement new KnownSerializerFinder #1700

Uh oh!

Conversation

rstam commented May 27, 2025

Uh oh!

Szmiglo commented Jul 23, 2025

Uh oh!

rstam commented Jul 23, 2025

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!

sanych-sun left a comment

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!

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants