Skip to content
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

Consider removing support for collections of primitives typed as object array/list used in parameters to queries #35332

Open
maumar opened this issue Dec 14, 2024 · 4 comments

Comments

@maumar
Copy link
Contributor

maumar commented Dec 14, 2024

Currently it's possible to do something like this:

        var orderIds = new object[] { 10248, 10249 };
        return AssertQuery(
            async,
            ss => ss.Set<Order>()
                .Where(o => orderIds.Contains(o.OrderID)));

This generates type mapping with a comparer being ListOfReferenceTypesComparer typed as object, but it's element comparer is DefaultValueComparer<int>. This makes it harder to reason about types of things involved (type argument of the element comparer is not compatible with type argument of the collection comparer), but doesn't really buy us much - if the collection contains non-int it will blow up anyway.

        var orderIds = new object[] { 10248, 10249, "Foo" };

We should consider removing this to make our code cleaner.

@roji
Copy link
Member

roji commented Dec 14, 2024

This generates type mapping with a comparer being ListOfReferenceTypesComparer typed as object, but it's element comparer is DefaultValueComparer. This makes it harder to reason about types of things involved (type argument of the element comparer is not compatible with type argument of the collection comparer)

I can indeed see that the list comparer type is ListOfReferenceTypesComparer<object[], object>, whereas the element comparer type (as inferred from o.OrderID) is DefaultValueComparer<int>. And I can indeed see a case for making the element type mapping here typed as DefaultValueComparer<object> to more cleanly represent things - though would that help us in any concrete way?

In other words, it would be good if you could provide a bit more context on what exact trouble this is causing you?

but doesn't really buy us much - if the collection contains non-int it will blow up anyway.

For mixing ints/strings you're right, but there are other cases where e.g. inheritance is involved, and so a list of heterogeneous CLR types may need to be used. A case that comes to mind is the spatial types:

var spatialThings = new object[] { new Point(...), new Polygon(...) };

All these are subclasses of NetTopologySuite's Geometry, so if you have a Geometry property on an entity type, it could make sense to do Contains like this. It's true that I'd use a Geometry[] rather than an object[] in this case, but that's still a heterogeneous list with all the same kind of trouble, I think?

Regardless, we generally try to translate stuff that works in regular LINQ, so if the user for some reason has a collection typed as List<object>, ideally we translate that even if the user could type it more narrowly/correctly.

As a side note, when pasting test snippets, it would be great if you could paste the name of the test (Where_array_of_object_contains_over_value_type), or just the whole test including the method signature - in this case it's very short. This allows jumping directly to it without having to search for the code.

@roji
Copy link
Member

roji commented Dec 14, 2024

In other words, it would be good if you could provide a bit more context on what exact trouble this is causing you?

I see now, this is about #35326 (would be good to link in the issue to give the context!)

@AndriySvyryd
Copy link
Member

@roji This is specifically about object[], Geometry[] would still be supported

@roji
Copy link
Member

roji commented Dec 18, 2024

@AndriySvyryd I don't quite understand... The idea of removing object[] support is to remove the need for the hack introduced here; that hack is introduced to handle the mismatch between the type of the element value comparer and the list value comparer. AFAICT, the same mismatch can occur in other, non-object[] scenarios (anywhere where polymorphism is involved, no?). And even regardless, I'd simply look at fixing the root cause (at least at first), rather then introducing a hack and then considering a breaking change (removing object[] support) to obviate the hack...

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

No branches or pull requests

3 participants