-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
First-class span support causes translation failure with Contains #35100
Comments
@jamesgurung I pasted both your csproj and Program.cs exactly as is with .NET SDK 9.0.100, and the program ran fine... Printing the result of ToQueryString() gives the following: SELECT "b"."Id"
FROM "Blogs" AS "b"
WHERE "b"."Id" IN (
SELECT "l"."value"
FROM json_each(@__list_0) AS "l"
) Can you please double-check and confirm? |
Hi @roji, thanks for getting back so quickly! I'm consistently getting the error in Visual Studio 2022 17.13.0 Preview 1.0, but I've just checked and it runs fine on the command line with |
Hmm, am not sure about that - it wouldn't make sense for VS to be the cause of a bug like that... @ajcvickers feel like trying to repro this? |
Is this related to this: dotnet/runtime#109757? |
@pinkfloydx33 definitely seems related - let's see what happens there. If the change is intended on the .NET side, we could look at reacting to it here in EF. |
I was trying to build
On the I will say that if I hover of the
Note that the Also, whether or not it has any part of this, the VS Installer for 17.13 preview 1 still installs Somehow or other, at least in some instances when the Edit to add: Tried in 17.12. and it looks fine there even when |
Thanks for diving into it @ChrisJollyAU... That's all sounds quite odd indeed. It feels like we should wait and see if the change (and all its consequences) are indeed intentional and desired (in dotnet/runtime#109757); we can react in EF e.g. by normalizing MemoryExtensions.Reverse() to Enumerable.Reverse() at some point in the query pipeline (though there seem to be deeper consequences around generics, as that issue shows). But it seems prudent to first see what's said on the runtime side I think... |
@jamesgurung Can you try with VS 17.12? Even with LangVersion on preview? Would be nice for extra confirmation it only affects the preview branch of Visual Studio. @roji I primarily use the preview branch of VS and haven't encountered this anywhere during the various 17.12 previews. Might just need to make a note of this as a known issue for 17.13 for now. |
I've attached a simple console app to repro this issue ConsoleApp2.zip |
@ilGianfri Tried in both VS 17.12 and VS 17.13p1 and had no errors |
That's strange, I get this |
@ilGianfri Yeah, I get that if I update the project to target .Net 9 (your attached is targeting net 8). I only get that when using 17.13p1. If you also get it on 17.12 please try a full clean and rebuild so that there are no artifacts from the roslyn version used by 17.13p1 This is something you would only pick up from 17.13p1 (further details coming up) |
I tried to rebuild it using 17.12 and I don't get the crash, I get the crash if I rebuild it using using 17.13p1 on both .NET 8 and .NET 9 |
@roji Been doing more research on this. It isn't a runtime issue but part of something with the roslyn compiler. This is related to First class span types. For roslyn it was introduced in 17.13p1 (see https://github.com/dotnet/roslyn/blob/main/docs/Language%20Feature%20Status.md). There is already a breaking changes documentation on this already https://github.com/dotnet/roslyn/blob/main/docs/compilers/CSharp/Compiler%20Breaking%20Changes%20-%20DotNet%2010.md . Note that some things for From the c# language proposal https://github.com/dotnet/csharplang/blob/main/proposals/first-class-span-types.md it is planned for C# 14. By the looks of it on VS 17.12 langversion For the current So directly compiled the below line with
This is fine on directly compiled code, however efcore uses some dynamic compilation. When running you are running on .Net 9 which would use a version of roslyn that doesn't know of first class spans (aka equivalent to VS 17.12). Thus when it compiles while running the method signature it creates would be on the |
Thanks for this thorough explanation! I'm not exactly following everything, but based on your recommendation will place this issue in 10 for follow-up later... I'm also proposing we switch the project's As far as you can understand, is there potential a change needed in the LINQ interpreter to react to these changes? |
@roji As far as I can tell, for .Net/EF Core 9 GA, there is nothing needed to be done Once efcore starts switching to the Quick summary:
|
Thanks. Just about this:
Looking at the original error message and stack traces, this isn't about compilation in runtime: it's EF running the LINQ interpreter, which is part of the .NET runtime (nothing to do with Roslyn or compilation). In other words, it's maybe the runtime that needs to react to the new C# 14 feature, by making sure that the LINQ interpreter works correctly with the LINQ trees that the compiler now produces (different than the previous ones). In other words, if my understanding here is correct, then it should be possible to create a minimal console program which uses the LINQ interpreter directly (like EF does), showing a simple LINQ construct that used to work and now fails with the VS etc. |
@roji Initially it does look like part of Linq but if you go deeper into EF Core you end up in the Specifically this is the code
Note the Edit to add: The test app that @ilGianfri did, is a nice simple one to start playing around on |
Yeah, the naming is confusing, but note
We prefer to interpret here rather than compile, since in this particular case we know that the resulting lambda will only ever be invoked once (that's the Invoke() just afterwards). The overhead of actually compiling is far too great for this one-off invocation (see #29814). |
@roji Yeah I had started thinking the compilation part was starting to go down the wrong path. Busy debugging even more (even better now that I sorted my compilation errors out).
TO DO
|
Can Expressions even handle these cases generally? |
Yeah, I'm just noting here that the OP's stack trace shows the following error:
This is clearly the interpreter having trouble with the LINQ expression tree that's now getting generated by compiler. I'd repro this outside of EF (just a console program doing |
@roji There is also the fact that we are doing that Lambda call (with the added Convert) at a part where we probably shouldn't do it. In the example above, when we get to In C# 14, it gets wrapped in a On a different note, have made a lot of progress in updating ef core to handle/behave nicely with first class spans. I do have a noticeable change to ask about. I've found that I end up inlining a lot more arrays into the sql query and not using it as a parameter. This only occurs on plain arrays (so something like Without using the Thoughts on leaving it like that or making |
Yep you are right. Calling Unfortunately, there is no easy way to find all occurrences that need to be updated, unless each query is covered by an integration test. Otherwise I have to wait for runtime crashes and fix them on a case-by-case basis, or I have to find all occurrencies where I have a linq expression with such a condition (array + contains) in advance. But AFAIK there is not an easy way to reliably find and fix all occurencies automatically. E.g. when searching for |
Set |
Nice, that is a useful trick to narrow it down to the most relevant usages. Thanks. 👍 |
Reopening to consider backporting to 9.0.x and 8.0.x. |
Fixes dotnet#35100 (cherry picked from commit 6c0106b)
Fixes dotnet#35100 (cherry picked from commit 6c0106b)
Fixes dotnet#35100 (cherry picked from commit 6c0106b)
Fixes dotnet#35100 (cherry picked from commit 6c0106b)
Backported to 8.0 and 9.0. |
@roji This has probably popped up only now as 9.0.20x SDK is linked to VS 17.13 (whereas the previously working 9.0.10x was VS 12.12). From https://github.com/dotnet/roslyn/blob/main/docs/compilers/CSharp/Compiler%20Breaking%20Changes%20-%20DotNet%2010.md#spant-and-readonlyspant-overloads-are-applicable-in-more-scenarios-in-c-14-and-newer it mentions it was introduced in VS 17.13 (and obviously together with the .20x SDK and in conjunction as the preview feature but still there). Those still on VS 17.12 wouldn't have hit this even with |
Thanks for the additional info @ChrisJollyAU. We discussed this as a team, and given how simple/low-risk the fix (or rather workaround) is, we decided it's worth making EF 8.0 and 9.0 forward-compatible with C# 14 by backporting it. |
True. Honestly, I'm more surprised/intrigued that you can turn a preview c# 14 feature on in a 9.0 stable release. PS. Bit of extra reading I will link https://github.com/dotnet/csharplang/blob/main/meetings/2025/LDM-2025-01-06.md#ignoring-ref-structs-in-expressions. Was a proposal to ignore ref structs in expression trees. LDM decided not to make any changes |
I admit that I am too...
Yeah, I aware of this discussion... There have been several instances where first-class spans have caused trouble recently around expression trees and LINQ (see also this, which is a result of #35547). |
Fixes dotnet#35100 (cherry picked from commit 6c0106b)
Since installing .NET 9, I have come across a bug in production where setting
<LangVersion>preview</LangVersion>
results in certain LINQ queries causing anInvalidOperationException
. This did not happen with .NET 9 RC2, however now that I've installed the .NET 9.0.100 SDK it always occurs even if I roll back EF Core.Minimal repro:
Project.csproj
Program.cs
Exception
Inner Exception
Version
EF Core version: 9.0.0
Database provider: Microsoft.EntityFrameworkCore.Sqlite
Target framework: .NET 9.0
Operating system: Windows 11
IDE: Visual Studio 2022 17.13 Preview 1
The text was updated successfully, but these errors were encountered: