CSHARP-6024: Support ReadOnlySet Serialization by papafe · Pull Request #1987 · mongodb/mongo-csharp-driver

papafe · 2026-05-05T13:39:02Z

No description provided.

papafe · 2026-05-05T13:40:07Z

                return value;
            }

+            // last resort: try a constructor that takes a HashSet<TItem> (e.g. ReadOnlySet<T>(ISet<T>))


I've put this as the last possibility in the method so that we do not cause issues with preexisting code.

Copilot

Pull request overview

Adds support for deserializing set-shaped IEnumerable<T> implementers (including .NET 9+ ReadOnlySet<T>) by introducing a HashSet<T>-based constructor fallback in EnumerableInterfaceImplementerSerializer, along with new unit tests covering constructor selection and ReadOnlySet<T> roundtripping.

Changes:

Updated EnumerableInterfaceImplementerSerializer<TValue, TItem>.FinalizeResult to add a last-resort HashSet<TItem> constructor fallback (enabling ISet<T>-constructor-only targets).
Added tests to verify: (1) set-ctor fallback behavior, (2) duplicate-collapsing behavior for set-shaped targets, (3) preference for IEnumerable<T> ctor when available.
Added a .NET 9+ roundtrip test for System.Collections.ObjectModel.ReadOnlySet<T>.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
tests/MongoDB.Bson.Tests/Serialization/Serializers/EnumerableInterfaceImplementerSerializerTests.cs	Adds regression/behavior tests for set-constructor fallback and .NET 9+ `ReadOnlySet<T>` roundtrip.
src/MongoDB.Bson/Serialization/Serializers/EnumerableInterfaceImplementerSerializer.cs	Implements `HashSet<TItem>`-based constructor fallback in `FinalizeResult` to support set-shaped targets like `ReadOnlySet<T>`.

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

+            // last resort: try a constructor that takes a HashSet<TItem> (e.g. ReadOnlySet<T>(ISet<T>))
+            // Note: collapses duplicate elements, which is correct for set-shaped targets.


sanych-sun · 2026-05-05T17:26:47Z


+            // last resort: try a constructor that takes a HashSet<TItem> (e.g. ReadOnlySet<T>(ISet<T>))
+            // Note: collapses duplicate elements, which is correct for set-shaped targets.
+            var setCtor = FindSingleArgumentConstructor(typeof(HashSet<TItem>));


Should it be ISet<TItem>?

Agreed, fixed.

I think the change from HashSet<TItem> to ISet<TItem> went in the wrong direction.

Is the intent of this fallback not to "find any single-argument constructor that a HashSet can satisfy"?

In which case the right probe is therefore typeof(HashSet<TItem>), not typeof(ISet<TItem>). The difference matters because HashSet<T> implements IReadOnlySet<T> (added in .NET 5), but ISet<T> does not. A type with a constructor taking IReadOnlySet<T> would work with HashSet<TItem> as the probe but silently fall through to the exception with ISet<TItem>. No such type exists in the BCL today, but the HashSet<TItem> probe is strictly more correct as the intent.

@papafe @sanych-sun any thoughts on my comment above?

Yep, I think we should try to find a constructor for the actual type we're using.

I think we should decide on #1987 (comment) comment first, because it might change the way we approach the problem.

sanych-sun · 2026-05-05T17:38:21Z

+
+        // exercises the HashSet<TItem> ctor fallback in FinalizeResult: target type with
+        // only an ISet<TItem> constructor — same shape as ReadOnlySet<T>
+        public class SetOnlyCtorEnumerable<T> : IEnumerable<T>


Do we really need this mock as we have net10.0 target for tests and can exercise with the ReadOnlySet<> itself?

Agreed, removed this one.

adelinowona

FinalizeResult currently seems to do reflection on every deserialization call to figure out how to construct TValue, but the answer is fixed for any given TValue/TItem pair. A static readonly field on the generic class could compute this once per concrete type thus saving us a reflection cost. @sanych-sun WDYT?

sanych-sun · 2026-06-04T16:31:35Z

About FinalizeResult: the logic there is based on method's parameter real type, not on class generic type parameter. I'm not sure if we can assume that all instances of this class always get accumulator of this exact type. Worth extra investigation.

adelinowona · 2026-06-11T20:49:22Z

@sanych-sun Regarding FinalizeResult: I did some investigation aided by claude. Traced the call chain: FinalizeResult is protected abstract and the only caller is EnumerableSerializerBase.Deserialize, which always passes it the result of CreateAccumulator() directly. Since EnumerableInterfaceImplementerSerializer<TValue, TItem> is sealed and doesn't override Deserialize, the accumulator reaching FinalizeResult is always new List<TItem>() — so the static field assumption seems safe.

sanych-sun · 2026-06-11T20:52:38Z

@adelinowona In our codebase - yes, but we are talking about changes in public base class, there might be another implementations out of our control.

adelinowona · 2026-06-11T21:52:42Z

@sanych-sun yep, my proposed change is entirely contained within EnumerableInterfaceImplementerSerializer<TValue, TItem> — the sealed class. The static field will be a private member there, and FinalizeResult is already overridden there. Nothing in the public base classes changes. External subclasses of the base would be completely unaffected.

papafe requested a review from adelinowona May 5, 2026 13:39

papafe requested a review from a team as a code owner May 5, 2026 13:39

Copilot AI review requested due to automatic review settings May 5, 2026 13:39

papafe added the bug Fixes issues or unintended behavior. label May 5, 2026

Copilot started reviewing on behalf of papafe May 5, 2026 13:39 View session

papafe commented May 5, 2026

View reviewed changes

Copilot AI reviewed May 5, 2026

View reviewed changes

sanych-sun requested changes May 5, 2026

View reviewed changes

sanych-sun added feature Adds new user-facing functionality. and removed bug Fixes issues or unintended behavior. labels May 5, 2026

papafe added 3 commits May 19, 2026 11:56

CSHARP-6024: Support ReadOnlySet Serialization

4b187e2

Small fix.

9e967db

Small fixes following PR

5fb2db6

papafe force-pushed the csharp6024 branch from 037abf6 to 5fb2db6 Compare May 19, 2026 09:56

papafe requested a review from sanych-sun May 19, 2026 10:07

sanych-sun requested changes May 28, 2026

View reviewed changes

Comment thread src/MongoDB.Bson/Serialization/Serializers/EnumerableInterfaceImplementerSerializer.cs

papafe requested a review from sanych-sun June 2, 2026 13:54

adelinowona reviewed Jun 3, 2026

View reviewed changes

Corrected ctor finding

5fbd4f0

papafe requested a review from adelinowona June 8, 2026 13:07

		// last resort: try a constructor that takes a HashSet<TItem> (e.g. ReadOnlySet<T>(ISet<T>))
		// Note: collapses duplicate elements, which is correct for set-shaped targets.

Conversation

papafe commented May 5, 2026

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!

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!

Uh oh!

adelinowona left a comment

Choose a reason for hiding this comment

Uh oh!

sanych-sun commented Jun 4, 2026

Uh oh!

adelinowona commented Jun 11, 2026

Uh oh!

sanych-sun commented Jun 11, 2026

Uh oh!

adelinowona commented Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants