Add fix for .NET 10 first-class span support#2345
Conversation
…d resolution (MemoryExtensions vs Enumerable) resulting in breakage.
|
@dotnet-policy-service agree company="Microsoft" |
roji
left a comment
There was a problem hiding this comment.
Thanks @ericsampson, the PR generally looks quite good.
However, the bar for any changes to EF6 is very high, and as very few users have been complaining about this issue, we weren't planning on fixing this problem at this time. I'll bring this up internally and we'll see what to do.
|
Note also the build failures. |
@roji I totally understand. I was hoping to just reduce the "activation energy" by providing a working PR for you folks to consider rather than expecting someone from MSFT to do it, as it would likely not meet prioritization or at least not anytime soon. The build issue should be addressed. Last time the build queue was taking so long to pick up the job that I had to just walk away and let it get picked up overnight, so that's why I didn't fix the failure immediately. Looks like the fix is now in the same queue waiting game state :) Cheers |
The UnitTests project was designed for .NET Framework and has extensive dependencies on Framework-specific infrastructure. Rather than exclude hundreds of incompatible tests, whitelist only the Span-related tests needed to verify the MemoryExtensions rewrite logic on .NET 10+.
|
@roji CI tests are passing now :) |
roji
left a comment
There was a problem hiding this comment.
Thanks, this looks almost ready (good work!). A couple of small fixes and I'll merge it.
Just to clearly set expectations; for now, although we'll be merging this PR, we won't be releasing a new version of EF6 in the immediate future. In principle, new versions of EF6 are only being released for security fixes, which this isn't. Based on feedback we can reevaluate.
|
Thanks @roji addressed the feedback; I have one suggestion for y'all to consider: What about publishing this to NuGet as
Thoughts? |
PR feedback
|
@ericsampson @roji Just to be clear, this PR is to enable EF6 to run under .NET 10 without making any code changes when .Contains is used in LINQ queries? |
@ErikEJ EF6 as-is works in .NET 10 IF you set the langversion to <=13. This PR will allow EF6 using .Contains to work on .NET 10 with C# 14+ (as would typically happen in an app upgrade scenario where the langversion isn't pinned to a specific numbered version). I think I've stated that correctly :) |
|
@roji should I merge this in? Or do you want to. What do you think of the idea of publishing this to NuGet as |
|
My team has hit this issue and is preventing our ability to upgrade to C#14 which we would like to do. It is a pretty big blocker for us. |
|
This is blocking us from upgrading to C# 14. Luckily we were alerted to this problem with hundreds of test failures in the CI pipeline from simply switching from C# 13 to C# 14. This is a potential landmine for any projects using EF6 and .NET 10 together without such test coverage. |
|
@ericsampson I just merged this - thanks again for the contribution. We'll discuss internally about when a new version can be released with this fix. @bkzoller you don't have to use C# 14 when upgrading to .NET 10 - it's perfectly fine to upgrade to .NET 10 but keep your C# language at 13. If there's some C# 14 feature you absolutely must have, as a workaround you can also modify your queries which use Contains over an array to e.g. do it over a List instead. |
|
Thanks @roji, appreciate your reviews and suggestions. Cheers! |
|
@roji @ericsampson not sure who to tag here, but this merge is useless without a new release on nuget. What are the plans for releasing this? |
|
@PaulVrugt unfortunately this will take a bit more, as we have a lot of things going on and this requires some non-trivial work. |
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Port of #2345: Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
…some fixes for byte[].Contains
See:
This PR is as direct of a port of @roji's original .EF Core fix as I could make it, with minor adjustments as required by EF6.
I added unit tests that demonstrated failure before the fix and success on all cases afterwards.
Thanks for the help hopefully getting this into EF6!
Cheers,
Eric