-
Notifications
You must be signed in to change notification settings - Fork 5k
Re-revert "Make mutable generic collection interfaces implement read-only collection interfaces (#95830)" #115802
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?
Re-revert "Make mutable generic collection interfaces implement read-only collection interfaces (#95830)" #115802
Conversation
…only collection interfaces (dotnet#95830)" This reverts commit a2bd583.
Tagging subscribers to this area: @dotnet/area-system-collections |
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 re-applies the change that makes all mutable collection/list/dictionary/set interfaces extend their read-only counterparts via default interface method implementations. It updates LINQ fast-paths, core runtime dispatch, dynamic and collection classes, and adapts existing tests to exercise the new read-only APIs.
- LINQ: Switch
IList<T>
/ICollection<T>
checks toIReadOnlyList<T>
/IReadOnlyCollection<T>
in performance-sensitive methods. - Core and runtime: Simplify array generic interface method binding and add
IReadOnlyDictionary
toExpandoObject
. - Tests: Introduce
CollectionAsserts
helpers and wrap read-only interface assertions under#if !NETFRAMEWORK
.
Reviewed Changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/libraries/System.Linq/src/System/Linq/*.cs | Replace mutable interface checks with their read-only counterparts. |
src/libraries/System.Linq.Expressions/src/System/Dynamic/ExpandoObject.cs | Add IReadOnlyDictionary<string, object?> implementation and members. |
src/libraries/Common/tests/System/Collections/CollectionAsserts.cs | New test helpers to assert both mutable and read-only interface behavior. |
src/coreclr/vm/array.cpp | Simplify generic array interface dispatch; remove slot-based lookup. |
Comments suppressed due to low confidence (1)
src/libraries/Common/tests/System/Collections/CollectionAsserts.cs:13
- [nitpick] Many helper methods repeat the pattern of asserting on both mutable and read-only interfaces; consider consolidating the
#if !NETFRAMEWORK
block into a single helper or method attribute to reduce duplication.
internal static class CollectionAsserts
src/libraries/System.Linq.Expressions/src/System/Dynamic/ExpandoObject.cs
Show resolved
Hide resolved
I do not see how we can merge this before the MSVC fixes are released. |
Completes #31001
This PR reverts #101644, which reverted #95830, and makes all built-in mutable collection/list/dictionary/set interfaces extend their relative counterparts, using DIMs (default interface method implementations) to implement them by default.
We have been working with the MSVC team, which is currently preparing a fix for this for C++/CLI, so that the change will not cause breaks in WPF applications. The fix will be ingested into a future MSVC update, and we're coordinating with them to ensure the timeline is aligned so it gets ingested in time for .NET 10, leaving enough time to validate things. In the meantime, we're opening this PR to merge the actual runtime/BCL changes earlier, so that we can get additional validation in other scenarios, and to have more time for the broader community to provide feedback as well.
cc. @richlander @tannergooding @stephentoub @jkotas @AaronRobinsonMSFT