Skip to content

Conversation

cincuranet
Copy link
Contributor

Fixes #36496

…T>) in parameters for primitive collections.
@cincuranet cincuranet changed the title Fix handling of readonly fields using abstract class (i.e. FrozenSet<T>) in parameters for primitive collections. Fix handling of readonly fields using abstract classes (i.e. FrozenSet<T>) in parameters for primitive collections. Sep 12, 2025
@cincuranet cincuranet marked this pull request as ready for review September 12, 2025 10:49
@cincuranet cincuranet requested a review from a team as a code owner September 12, 2025 10:49
Copy link
Member

@roji roji left a 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.

Comment on lines +92 to +94
// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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)
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants