-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Fix handling of readonly fields using abstract classes (i.e. FrozenSet<T>) in parameters for primitive collections. #36757
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?
Conversation
…T>) in parameters for primitive 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.
Thanks, LGTM aside from below nits.
// For collections that are abstract (i.e. FrozenSet<T>) the compiler uses some "internal" | ||
// implementation and for readonly fields the compiler adds explicit cast. We need to | ||
// unwrap here. |
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.
// For collections that are abstract (i.e. FrozenSet<T>) the compiler uses some "internal" | |
// implementation and for readonly fields the compiler adds explicit cast. We need to | |
// unwrap here. | |
// For collections that are abstract (i.e. FrozenSet<T>), some internal type is used | |
// and for readonly fields the compiler adds explicit cast. We need to unwrap here. |
if (expression is UnaryExpression { NodeType: ExpressionType.Convert } convertExpression | ||
&& convertExpression.Type.GetGenericTypeDefinition() == typeof(IEnumerable<>)) | ||
&& convertExpression.Type.GetGenericTypeImplementations(typeof(IEnumerable<>)) is not null) |
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.
Given that below we match on non-generic IEnumerable, should we match for this here as well instead of the generic variant?
using System.Collections.Immutable; | ||
|
||
namespace Microsoft.EntityFrameworkCore.Query; | ||
|
||
public abstract class PrimitiveCollectionsQueryTestBase<TFixture>(TFixture fixture) : QueryTestBase<TFixture>(fixture) | ||
where TFixture : PrimitiveCollectionsQueryTestBase<TFixture>.PrimitiveCollectionsQueryFixtureBase, new() | ||
{ | ||
// List<T> is a regular class |
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.
👍
Fixes #36496