[JonHanna]

Fixes #2241

Catches the exception caused by this edge-case rather than trying to
predict it, then falls back to second code path (the exception happens
in an optimised path). Also changed that second path to remove a tests
being made on each loop.

[leppie]

try/catch is pretty nasty (on the CLR). I generally debug with exceptions thrown in all cases (unlike our JVM friends). Is there not a way to handle this cheaper?

[stephentoub]

cc: @VSadov

[JonHanna]

If you can think of a good way of catching it, that's cheap in the common case, I'd be all for it. I'd be against complex checking (I started thinking about ways to detect but gave up when all I could think of was going to hit into reflection) since this case isn't hit 99.9% of the time, and ideally shouldn't hurt the 99.9% that isn't affected. That 99.9% might suffer a try, but won't suffer a catch, and there are implicit try all over the place with enumerables anyway.

[leppie]

An easier way might be to tackle it earlier. IOW, get the conversion to do the double cast (I am not familiar with this code) as it does in the non-optimized case.

[mikedn]

I don't think the issue should be fixed in the Buffer class, it's not its fault that Cast fed it an array of the wrong type.

[nguerrera]

Also changed that second path to remove a tests being made on each loop.

I would prefer that the this bug fix PR did not contain separate optimization. I believe the changes should be discussed separately.

[nguerrera]

The problem is of course not limited to LINQ:

var x = new int[] { 1, 2, 3 };
var y = (uint[])(object)x;
var z = new uint[3];
y.CopyTo(z, 0); // throws ArrayTypeMismatchException

My opinion is that we should either continue to ignore this edge case (notice how the C# type system doesn't model it, hence the intermediate cast to object) or do the work at a lower level to make the copy succeed. I'd much prefer the latter, but we need to discuss the ramifications.

I don't want to set a precedent that would have us scrutinizing every use of CopyTo for a potential need to handle ArrayTypeMismatchException.

[stephentoub]

Thanks, Nick. I left a similar opinion at https://github.com/dotnet/corefx/issues/2241.

[JonHanna]

I don't think the issue should be fixed in the Buffer class, it's not its fault that Cast fed it an array of the wrong type.

You know at first I thought changing Cast was best avoided, because people are presumably benefiting from the optimisation it contains. I also thought catching the problematic cases would be hard. Considering it further though:

1. Any shift from value-type => value-type that the optimisation manages are going to be a problem.
2. Any reference-type => reference-type that it manages should be fine.
3. There's no Nullable => Nullable cast that the optimisation would try to do. So the only nullable types that the optimisation might happen for are going to be the OK reference types. So we can therefore block it when the type isn't nullable.
4. Detecting whether a type parameter is nullable is easy.
5. As a bonus we get a micro-opt on the optimisation not being tested for if the type isn't nullable.

Everyone's a winner?

[nguerrera]

@stephentoub Ah, I missed that and the discussion about how/whether to address at lower level is more developed there too. :)

@hackcraft I think we need to come to a conclusion on the general issue in #2241 before changing LINQ for this.

[JonHanna]

The general issue of arrays having broken covariance goes back to 2002, or indeed to 1995 since it was copied from Java's. I don't think a generalised solution to that is possible. The general advice is "don't do anything that causes arrays' covariance to cause problems", and Linq broke that in the optimisation to Cast.

[nguerrera]

This is really separate from the main problem with the general covariance case -- e.g. that you can't write a Foo into an object[] that is really a converted string[] array.

That one is fundamental as memory safety would be violated if the check weren't enforced. Nothing of the sort can be said about why CopyTo was failing here in the primitive/enum array conversion case.

[stephentoub]

@hackcraft, thanks for pushing ahead with this (and all the other LINQ work you've been doing), but I'm going to close out this PR until the broader issue in #2241 has been decided on.

[JonHanna]

Probably for the best. It looks like there might be a bigger win available there that would fix the issue this fixes and more besides, and if not this can always be returned to.

[stephentoub]

Thanks, @hackcraft.