-
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
[Regression] Unexpected unbound variable error with closure #35152
Comments
Confirmed regression from 8.0 to 9.0 - another thing to come out of the funcletizer rewrite, most likely. Reproawait using var context = new BlogContext();
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();
int v = 1;
Expression<Func<Blog, object>> f = x => v;
var q = await context.Set<Blog>().OrderBy(f).ToListAsync();
public class BlogContext : DbContext
{
public DbSet<Blog> Blogs { get; set; }
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
=> optionsBuilder
.UseSqlServer("Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false")
.LogTo(Console.WriteLine, LogLevel.Information)
.EnableSensitiveDataLogging();
}
public class Blog
{
public int Id { get; set; }
public string Name { get; set; }
} |
The issue isn't the Expression variable, it's the convert-to-Object node that's added inside; the same error happens with: var v = 1;
var q = await context.Set<Blog>().OrderBy(x => (object)v).ToListAsync(); This means that it's possible to work around this simply by having the lambda return the exact type, so no up-cast is needed: Expression<Func<Dummy, int>> f = x => v; The problem is that when we process the body of a lambda in the funcletizer, we incorrectly leave the state as EvaluatableWithCapturedVariables, after we've already evaluated the body inside (EDIT: the actual bug was in VisitUnary, not VisitLambda - see below); this causes the whole lambda to be evaluated again (technically the wrapping Quote node), and the attempt to run the LINQ interpreter over the ParameterExpression inside causes the exception. The correct state after evaluating something is NotEvaluatable, so that we don't attempt to re-evaluate something that already has been evaluated. Note that the issue no longer reproes after recent switch to QueryParameterExpression in 10.0 (#35101), but for the reasons aren't material to this issue: the evaluated node is now a QueryParameterExpression rather than a ParameterExpression, and the LINQ interpreter surprisingly doesn't error on that unknown node type. But the bug is still there and could probably manifest in other ways. |
@roji I noticed that the issue was with conversion and applied the exact workaround you proposed. |
Upon a deeper look, VisitLambda turned out to be fine - the problem was in VisitUnary's EvaluateOperand, which left the unary's state as evaluatable instead of setting it to NoEvaluatable (after having evaluated the operand). |
Fixes dotnet#35152 (cherry picked from commit 3ba88c4)
Fixes dotnet#35152 (cherry picked from commit 3ba88c4)
Fixes dotnet#35152 (cherry picked from commit 3ba88c4)
Using EFCore 9 the following code (which uses a closure in an order-by condition)
throws at runtime with the following error:
Stacktrace
The same code completes successfully using EFCore 8.
Moreover, if the boxing is moved out of the expression like below
the queryable is interpreted successfully.
The database provider is not relevant, because the exception is thrown at interpretation time (nevertheless, I have tested with both InMemory and PostgreSQL).
I have created a gist to reproduce the issue
Gist output
The text was updated successfully, but these errors were encountered: