Automatic use of ConvertChecked method when "Check arithmetic overflow" is activated in project's build settings

[Vyzzuvazzadth]

When Check for arithmetic overflow/underflow is activated in a project's build settings (Project > Properties > Build > Advanced...) Entity Framework (or the compiler?) automatically wraps some types (for example short) inside a ConvertChecked method call, which unfortunately has to be evaluated client-side.

This causes a query with a column filter of type short/smallint to retrieve much more data than necessary.

We use Check for arithmetic overflow/underflow in all of our projects and we also try to accomplish as much as possible on the SQL server. To enforce that, we generally activate client evaluation exceptions for queries, which produces the following exception message when client evaluation occurs:

System.InvalidOperationException: 
'Warning as error exception for warning 'Microsoft.EntityFrameworkCore.Query.QueryClientEvaluationWarning': 
The LINQ expression 'where (ConvertChecked([c].VersionYear) == ConvertChecked(__filter_VersionYear_1))' could not be translated and will be evaluated locally. 

In the above example, the VersionYear field is defined as smallint in the database and as short in the table model property. filter.VersionYear (translated to the parameter __filter_VersionYear_1) is the value the result set is to be filtered by and is also of type short.

Steps to reproduce

Follow the instructions for a test project here: https://docs.microsoft.com/en-us/ef/core/get-started/full-dotnet/new-db

Before adding the migration and updating the database, add a simple short property to the Blog class

public class Blog
{
	public int BlogId { get; set; }
	public string Url { get; set; }
	public short Rating { get; set; } // add this

	public List<Post> Posts { get; set; }
}

Now finish the tutorial but don't execute the program yet.

In program.cs, replace the code within the using statement for the BloggingContext with the following code:

db.Blogs.Add(new Blog { Url = "http://blogs.msdn.com/adonet", Rating = 3 }); // added Rating = 3
var count = db.SaveChanges();
Console.WriteLine("{0} records saved to database", count);

Console.WriteLine();
Console.WriteLine("All blogs in database:");
foreach (var blog in db.Blogs.Where(b => b.Rating >= 3)) // added where clause
{
	Console.WriteLine(" - {0}, {1}", blog.Url, blog.Rating); // added rating
}
Console.ReadKey(); // added for convenience

Before running the program, configure the warnings for the context to throw an exception on client-side evaluation:

protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
{
	optionsBuilder
		.UseSqlServer(@"Server=.\ALSOFT;Database=ShortTest;Trusted_Connection=True;")
		.ConfigureWarnings(warnings => warnings.Throw(RelationalEventId.QueryClientEvaluationWarning)); // add this line
}

Now run the program. It will run through without any hiccups.

After closing the program, go to Project > Properties > Build > Advanced... and check the option Check for arithmetic overflow/underflow.

Save and run the program again.

This time, program execution will throw a System.InvalidOperationException saying ConvertChecked could not be translated and will be evaluated locally.

Further technical details

EF Core version: 2.0.1
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10 (1709)
IDE: Visual Studio 2017 15.5.5

Closing

Int data types work as intended without the added ConvertChecked method. There might be other data types with the same behavior as short, such as byte or float (I unfortunately don't have time to test them all).

I'm pretty sure that's by design. However, it's very unfortunate that we have to disable the Check for arithmetic overflow/underflow option to be able to sensibly enforce server-side evaluation for EF Core. Catching arithmetic overflows contributes to a better maintainability of the application, which we don't want to give up on.

EF 6.x never had that problem as we're still using that version for another project, also with the Check for arithmetic overflow/underflow option activated.

We also tried circumnavigating the ConvertChecked method usage by wrapping for example ToList() or Count() method calls with the unchecked keyword but to no avail.

Attachments

Full test project for convenience's sake.

DBTestEFCore.zip

[anpete]

@Vyzzuvazzadth This look like a bug - thanks for the detailed analysis!

Note for fix: It looks like we are missing handling of ConvertChecked nodes in SqlTranslatingExpressionVisitor. We should review all usages of ExpressionType.Convert.

[anpete]

@ajcvickers reminded me that this behavior is currently by-design because we don't want to silently remove the checking.

You should be able to use unchecked to suppress it, though.

Make sure you're using unchecked around the actual LINQ expression at the time it is being constructed.

[Vyzzuvazzadth]

@anpete Thank you for your swift reply!

It seems my mistake was to encapsulate the .ToList() and .Count() LINQ method calls inside an unchecked block instead of query construction methods like .Where() for example.

My assumption was that anything inside of the call stack within the unchecked block would be affected, so I used

unchecked
{
    result = this.CreateQuery(filter).ToList();
}

instead of

private IQueryable<T> CreateQuery(QueryFilter filter)
{
    unchecked
    {
        return this._context.Table.Where(a => a.Year == filter.Year)
    }
}

(simplified example for clarity's sake)

The second example works on my end with the option Check for arithmetic overflow/underflow activated. As in no usage of ConvertChecked and therefore no client evaluation.

I'm closing this issue since my problem has been resolved and you seem to have received enough information to look into this matter for future versions.

Thanks again and have a good one!

[MarcoLoetscher]

I think this issue should be reopened, because to use unchecked is just a workaround but not a final solution. EF 6. x also works without this workaround.

[ajcvickers]

@MarcoLoetscher @Vyzzuvazzadth I'm re-opening this so we can discuss. The question is when using overflow checking in a query, is it important that the semantics of overflow checking are preserved?

• If overflow checking must be preserved, then we must client-eval because we cannot translate the overflow checking to SQL.
• If overflow checking can always be ignored, then we can strip it our before doing SQL translation, like EF6 does.
• If sometimes it's okay to ignore it, but sometimes the semantics must be preserved, then the C# language features seem the most appropriate--preserve it if it is being used (client eval) and translate if it's not being used, using checked/unchecked in the language to control whether it is used or not.

[MarcoLoetscher]

It would be best if I could configure the behaviour on the DbContext.

[MarcoLoetscher]

Client evaluation is almost never a good idea. You have already made bad experiences with the missing GroupBy translation in the current EF Core version.
Can't you check for arithmetic overflow/underflow and throw an OverflowException if necessary before the query is executed from the database?

[Vyzzuvazzadth]

I suppose I was a bit hasty with immediately closing this issue. My mistake.

Anyway, @MarcoLoetscher raises some good points.
I think it's best to have the following options available during the construction of the DbContext:

• Turn the checking for arithmetic overflow/underflow on or off explicitly for all queries in that DbContext instance or apply the project setting (this affects the usage of ConvertChecked).
• Throw an OverflowException if applicable either before or after query execution instead of using ConvertChecked (depending on the first option).

This way, we'd have full control over how EF Core behaves and also still have the option to use checked/unchecked for specific cases if needed.

[ajcvickers]

Decision from triage: we will ignore (not remove) the checked convert nodes. This means for anything evaluated on the server there will be no overflow checking, but for anything evaluated on the client it will still be used. We will stop forcing client evaluation of the query because we cannot translate a convert node.

[smitpatel]

Run a visitor to convert convertchecked nodes to convert nodes.