Collection bug #2450
Collection bug #2450
Conversation
| Assign(passedDestination, destExpression), | ||
| assignNewExpression, | ||
| Call(destination, clearMethod), | ||
| ToType(mapExpr, createInstance.Type) |
There was a problem hiding this comment.
Stupid question. Wouldn't this be sufficient not to cast the result back? Not sure why anything else would be needed. The problem wasn't that collection was a List when passed down it's cause it was casted as such instead of ICollection.
There was a problem hiding this comment.
No that's where I started with, just removing this piece. The problem is our execution plan includes two paths for deciding what concrete type to create.
| return Constant(null, typeof(string)); | ||
| } | ||
|
|
||
| if (type.IsInterface()) |
There was a problem hiding this comment.
Is this really necessary? CollectionMapper returns ICollection it's just casts to List instead of left as instance type.
There was a problem hiding this comment.
This code is called in a few places so I can't assume what the mappers do.
There was a problem hiding this comment.
I guess it's not that bad :) What about ISet? Maybe move arrays here too? Or move this code there.
There was a problem hiding this comment.
I'll get another PR for that for now lgtm
|
Here's the before/after execution plan: |
|
Ok looks good enough to me. Also I don't feel like debugging Expressions atm. :) |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This fixes the newly introduced issue of casting collection types. I didn't think forcing everyone to use
UseDestinationValuewas a good solution, this change centers around the interfaces and the concrete types are only used for instantiation.